style: Renamed some of the things

[james-cnz]

An attempt to apply a consistent naming scheme to some major functions and data, to hopefully clarify the code. Particular changes are:

The terms "wrap" and "wrappers" seemed to be used to apply to two kinds of things. I have divided these into:

• proxies: things that actually polyfill parts of the Firefox API
• wrappers: things that wrap parts of the Chrome API, as an intermediate step to polyfilling parts of the Firefox API

Functions that return a new proxy or wrapper for some X are generally called "wrapX". I think "new_XProxy" / "new_XWrapper" may be clearer.

A couple of functions are declared with the intention of being bound before use. I think naming these "unbound_..." makes this clearer.

Also, changed to pass chrome as an argument to the main function, as per #139.

[james-cnz]

I guess people have different tastes in naming schemes, so I'm not sure this one will be to other people's liking. I think it does bring some consistency and clarity, though, so I think something like this would be helpful.

Also, having posted this PR, I realise "Rename some of the things" probably seems like understatement, since most major things are renamed. It seemed appropriate to me, because I originally started out renaming pretty much everything, then cut back to renaming major things.

[rpl]

@james-cnz I agree that the polyfill internals may be a bit hard to read initially, but I think that it is more related to "how abstract they are" then to "how the internal components are named", and in my opinion the changes applied in this PR are not actually making them any clearer by just changing the names used internally (e.g. the renamed components are not still able to make it immediately clear how these components work together when the extension is going to call an API method on the wrapped API object).

(Also, personally I don't really like these new_ and unbound_ prefixes applied to the names used internally, which could be just a matter of taste in the "naming conventions", but to me they looks a bit out of place in the rest of the polyfill sources).

My preferred strategy would be to actually start by adding a new section in the CONTRIBUTING.md file (which is going to be added by #147), with some details about how the polyfill internals works and which are their role.

[james-cnz]

@rpl Hopefully the PR changes look less out of place now?

The polyfill is certainly quite abstract, but I still think how the components are named could help. In particular, I had difficulty remembering that "wrapAPIs", "wrapObject", and "wrapMethod" provide polyfill for the Firefox browser, while "staticWrappers", "wrapEvent", "wrapAsyncFunction", and "wrappedSendMessage" provide only an intermediate step. I think naming could be a big help here.

[james-cnz]

I think perhaps a table would better explain what I'm trying to achieve. (And maybe something like this would be useful for documentation too?) Below is my current understanding of the role of major components, with their current names, then with the names given in the PR (as revised).

Current Naming

Thing	Polyfill side	Wrapper	Target side
WebExt API (Object)	wrapAPIs()	staticWrappers
Object	wrapObject()
Event (Object)		wrapEvent()
onMessage Listeners			onMessageWrappers
Function	wrapMethod()	wrapAsyncFunction()
SendMessage (Function)		wrappedSendMessage
Callback			makeCallback()
SendMessage Callback			wrappedSendMessageCallback

Suggested Naming (Revised)

Thing	Polyfill side	Wrapper	Target side
WebExt API (Object)	makeApiPolyfill()	apiWrappers
Object	makeObjectPolyfill()
Event (Object)		makeEventWrapper()
onMessage Listeners			onMessageListeners
Function	makeFunctionPolyfill()	makeFunctionWrapper()
SendMessage (Function)		sendMessageWrapper
Callback			makeCallback()
SendMessage Callback			sendMessageCallback

[Rob--W]

What is the difference between polyfill, wrapper and target (in particular the last one)?

The polyfill (=the library) exposes a Proxy that lazily (and recursively) expands the object if needed. Proxies themselves are also a kind of wrapper (wrapping the original API object).

[james-cnz]

@Rob--W

• "Polyfill" is the Firefox-style, Promise-based side, implemented with Proxies, providing the interface for use by the extension programmer. (I originally called this side "Proxy", but then thought that the fact it uses Proxies is probably really an implementation detail, and since this side is the interface exposed to the extension programmer, and directly used to provide polyfill, "Polyfill" is probably fitting?)
• "Wrapper" is used internally only. It is called by the "Polyfill" side, and in turn calls the underlying native "Target" API. (The terms "wrap" and "wrapper" are currently most often used within the code for this purpose, ~20 times, as opposed to various other purposes, ~11 times, I think.)
• "Target" is the native Chrome-style, callback-based side. The components listed under "Target" in the table are created in the native API style, to be called by the native API, and then in turn call back to the "Polyfill" side (I think). (I originally classified these with the Proxies/Polyfill, but I think they really belong on the side with the native API, since the interface they expose is the Chrome-style one, not the Firefox-style one, I think.)

Does this make sense?

[Rob--W]

Thanks for explaining your train of thoughts. Your distinction is not as obvious as it seems: All methods are used internally in some sense. Even wrapEvent (which you call "internally") returns an object that is directly exposed as a polyfill.

wrapAPIs, wrapObject, wrapMethod and (confusingly?) wrapAsyncFunction are the basic internal functions within the polyfill. Without exceptions, this would be simple (and probably be easier to understand).

However, the extension messaging API is somewhat complicated, and we have implemented this exception via staticWrappers, which depends on wrapEvent, onMessageWrappers, wrappedSendMessage. This last method makes use of wrappedSendMessageCallback.

Out of your list, I haven't mentioned makeCallback since it is a generic utility method (of which there are multiple others that you haven't listed, such as isThenable, pluralizeArgments.)

I don't think that changing names is going to make the code more readable. Comprehensibility of the source code can more easily be improved by giving a high-level overview of how the polyfill works, for example at the CONTRIBUTING.md page (or as done in this comment).

[james-cnz]

Thanks for explaining your train of thoughts.

It took some effort. :-)

Your distinction is not as obvious as it seems: All methods are used internally in some sense.

Yes, but some are also exposed to the extension programmer as polyfill, or to the underlying API in the native style, and others aren't. I guess perhaps my wording could use improvement, but I think there is a worthwhile distinction to be made.

Even wrapEvent (which you call "internally") returns an object that is directly exposed as a polyfill.

I don't think so. Looking at the code, the functions in wrapEvent (addListener, hasListener, and removeListener) take "target" as the first parameter, but the ones in the exposed polyfill don't, I think? But this is exactly the sort of thing I'm struggling with. If everything directly exposed as polyfill was named xPolyfill or similar, then I wouldn't have to read through the function (or check the docs) to find out whether it was directly exposed as polyfill or not. I'd be able to tell just by the name.

wrapAPIs, wrapObject, wrapMethod and (confusingly?) wrapAsyncFunction are the basic internal functions within the polyfill. Without exceptions, this would be simple (and probably be easier to understand).

However, the extension messaging API is somewhat complicated, and we have implemented this exception via staticWrappers, which depends on wrapEvent, onMessageWrappers, wrappedSendMessage. This last method makes use of wrappedSendMessageCallback.

Yes, that's true, but for explaining how the polyfill works as a whole, I think it's more useful to classify components according to the role they play, rather than whether they are basic or exceptional cases. e.g. wrapAsyncFunction() and wrappedSendMessage play a similar role. Their results are both called by the result of wrapMethod(), and in turn they call to the underlying native API, I think. Because of this, I think it's helpful to classify them together, even though wrapAsyncFunction() is for the basic case, and wrappedSendMessage is an exception.

Out of your list, I haven't mentioned makeCallback since it is a generic utility method (of which there are multiple others that you haven't listed, such as isThenable, pluralizeArgments.)

Yes, I guess makeCallback is a utility method, but again, for explaining how the polyfill works, I think it's more helpful to classify it according to the role it plays in producing something for use by the native API (in which regard it is similar to wrappedSendMessageCallback), rather than as a utility method. (The other utility methods don't play these kinds of roles, I think.)

I don't think that changing names is going to make the code more readable.

I guess it probably won't make much difference to someone who already understands the code well,
but as someone who is still struggling to do so, I think it would be a big help.

Comprehensibility of the source code can more easily be improved by giving a high-level overview of how the polyfill works, for example at the CONTRIBUTING.md page (or as done in this comment).

I think both could help. It would be useful to have an overview in the docs, sure, but I'm not going to be able to keep it all in my head while reading the code.

[Rob--W]

We decided to merge #139 to decouple the chrome global from the internal implementation and will stick to the current names for now, since there is no obvious advantage either way.

Those who have trouble with understanding the current implementation can try to increase their understanding by reading the comments in this PR thanks to you.