Closed Bug 1423425 Opened 7 years ago Closed 6 years ago

Document how to implement WebExtensions APIs

Categories

(WebExtensions :: General, enhancement)

Product:
WebExtensions
Component:
General
Version:
56 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

Details

Attachments

(4 files)

Bug 1423425 Let sphinx-js read .js files
59 bytes, text/x-review-board-request
erik
: review+
Details
Bug 1423425 Make some extensions comments jsdoc-compatible
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1423425 Initial draft of WebExtensions API documentation
59 bytes, text/x-review-board-request
bsilverberg
: review+
Details
Bug 1423425 Stop reading the Extension.jsm module object in specialpowers
59 bytes, text/x-review-board-request
kmag
: review+
Details 
      No description provided.
Attachment #8934784 - Flags: review?(lgreco)
Attachment #8934784 - Flags: review?(bob.silverberg)
This is certainly a work in progress, the jsdoc-generated documentation in particular needs some further attention, but I think we can do that in follow-up bugs.
For the third patch at least, its probably much easier to review in its formatted form, I've put a formatted copy here:
https://aswan.github.io/gecko-docs/toolkit/components/extensions/webextensions/index.html
Attachment #8934782 - Flags: review?(erik)
Comment on attachment 8934783 [details]
Bug 1423425 Make some extensions comments jsdoc-compatible

https://reviewboard.mozilla.org/r/205662/#review211912

I took a brief look at this patch, I think that some of these changes are going to trigger new eslint validation errors, I've added comments on the ones that I've noticed (because I've already experienced in the past with similar changes which introduced new jsdoc inline comments), but a push to try (or running ./mach eslint locally) is likely to show some that I could have missed.

I also recall that `this.Extension = class extends ExtensionData { ... }` used to be necessary, iirc because of issues with the EXPORTED_SYMBOLS when importing these identifiers from the jsm, but that could have been fixed in the meantime (I briefly tried locally by creating a small jsm module and it seems to work fine, and we will also get an additional confirmation when we will push these patches to try).

::: toolkit/components/extensions/Extension.jsm:324
(Diff revision 1)
>      let id = this.id || "<unknown>";
>      return Log.repository.getLogger(LOGGER_ID_BASE + id);
>    }
>  
> -  // Report an error about the extension's manifest file.
> +  /**
> +   * Report an error about the extension's manifest file.

eslint is going to complain here because there isn't a jsdoc for the message parameter (also on line 331)

::: toolkit/components/extensions/Extension.jsm:1032
(Diff revision 1)
> -// We create one instance of this class per extension. |addonData|
> -// comes directly from bootstrap.js when initializing.
> -this.Extension = class extends ExtensionData {
> +/**
> + * We create one instance of this class per extension. |addonData|
> + * comes directly from bootstrap.js when initializing.
> + * @extends ExtensionData
> + */
> +class Extension extends ExtensionData {

`Extension` is currently used a bunch of lines before (in the `startup` method of the `BootstrapScope` class) and I think that eslint will start to complain once `Extension` is declared as a class here (and I guess that the same is going to happen for `Langpack` and `LangpackBootstrapScope`).

It seems that we can just move `BootstrapScope` and `LangpackBootstrapScope` so that they get defined after these classes, though.

::: toolkit/components/extensions/Extension.jsm:1678
(Diff revision 1)
>        let origins = this.manifest.optional_permissions.filter(perm => classifyPermission(perm).origin);
>        this._optionalOrigins = new MatchPatternSet(origins, {ignorePath: true});
>      }
>      return this._optionalOrigins;
>    }
>  };

once `class ... extends ... {}` is not part of an assignment expression, the `;` is going to become an eslint validation error (for both `Extension` and `Langpack`).

::: toolkit/components/extensions/ExtensionCommon.jsm:1555
(Diff revision 1)
> -// content process). |name| is for debugging. |register| is a function
> -// to register the listener. |register| should return an
> -// unregister function that will unregister the listener.
> + * hasListener methods. |context| is an add-on scope (either an
> + * ExtensionContext in the chrome process or ExtensionContext in a
> + * content process). |name| is for debugging. |register| is a function
> + * to register the listener. |register| should return an
> + * unregister function that will unregister the listener.
> + * @constructor

I think that eslint would start to complain about the missing jsdoc for the parameters once this is a jsdoc comment.

(Nit, it would be nice if it would be possible to link the pages from the rst files from the jsdoc comments, so that if you are looking at the API reference you will know where to look for example code snippets, but I'm not sure if it is currently possible with sphinx-js).
> Nit, it would be nice if it would be possible to link the pages from the rst files from the jsdoc comments

Sure! You can do anything in sphinx-js that you can in vanilla Sphinx. What you seek would be something like :doc:`someOtherDoc`, which would link to someOtherDoc.rst. If you want to separately set the link text, the syntax is :doc:`some other doc<someOtherDoc>`.
Comment on attachment 8934782 [details]
Bug 1423425 Let sphinx-js read .js files

https://reviewboard.mozilla.org/r/205660/#review212060

Lgtm!
Attachment #8934782 - Flags: review?(erik) → review+
Comment on attachment 8934784 [details]
Bug 1423425 Initial draft of WebExtensions API documentation

https://reviewboard.mozilla.org/r/205664/#review213198

These docs look great, Andrew, nice job! I found a few errors, made some suggestions where I think the wording could be improved, and pointed out some places where I found the docs a bit confusing. I opened issues for the things I think need to be fixed, and just posted comments for the others which are expressing my opinion.

::: toolkit/components/extensions/docs/background.rst:5
(Diff revision 1)
> +Background
> +==========
> +
> +WebExtensions run in a sandboxed environment much like regular web content.
> +But the purpose of extensions is to enhance the browser in a way that

Remove the `But`.

::: toolkit/components/extensions/docs/background.rst:18
(Diff revision 1)
> +or `ChromeOnly WebIDL features <https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#ChromeOnly>`_.
> +
> +The rest of this documentation covers how API implementations interact
> +with the implementation of WebExtensions.
> +To expose some browser feature to WebExtensions, the first step is
> +of course to design the API.  Some high-level principles for API design

Remove the `of course`.

::: toolkit/components/extensions/docs/background.rst:44
(Diff revision 1)
> +  uses a namespace called ``tabs``, so all its functions and other
> +  properties appear on the object ``browser.tabs``.
> +  For a new API that provides features via Javascript, the usual practice
> +  is to create a new namespace with a concise but descriptive name.
> +
> +- **Scopes**:

This section is good, but I feel like it could be clearer. The section is entitled `Scopes`, but then scopes are never explicitly mentioned, rather they are referred to as `environments` in the paragraphs that follow. Perhaps using just one of those terms would be clearer, or mention that scopes are environments in the desription.

You say that `Most Javascript features are available in extension pages.`, perhaps you could say that that is the default, and that a developer would have to add code to make a feature available in one of the other scopes?

::: toolkit/components/extensions/docs/background.rst:123
(Diff revision 1)
> +  leak any resources.  In particular, any action from an extension that
> +  causes a resource to be allocated within the browser should be
> +  automatically cleaned up when the extension is disabled or uninstalled.
> +  This is described in more detail in the section on :ref:`lifecycle`.
> +
> +- **Performance**: A new WebExtension API should not add any new overhead

Isn't this first bit, about lazy loading of APIs, something that the framework takes care of? Is this something an API developer needs to worry about, or just being provided as information? If the latter, maybe it's worth mentioning that the framework takes care of this?

::: toolkit/components/extensions/docs/basics.rst:98
(Diff revision 1)
> +- ``mobile/android/components/extensions``: APIs that are only supported
> +  on Firefox for Android.
> +
> +Within the appropriate extensions directory, the convention is that the
> +API schema is in a file called ``schemas/name.json`` (where *name* is
> +the name of the extension, typically the same as its namespace if it has

s/extension/API

::: toolkit/components/extensions/docs/basics.rst:114
(Diff revision 1)
> +be loaded if an extension references either ``browser.myapi`` or
> +``browser.anothernamespace.subproperty``.
> +
> +A reference to a property beneath ``browser`` only causes the API to be
> +loaded if it occurs within a scope listed in the ``scopes`` property.
> +Valid ``scopes`` are:

This list doesn't render the way I think you intended it to.

::: toolkit/components/extensions/docs/events.rst:36
(Diff revision 1)
> +This fragment defines an event that is used from an extension with
> +code such as:
> +
> +.. code-block:: js
> +
> +   browser.myapi.onSomething.addListener((param1) => {

Nit: This could just be `(param1 => {`.

::: toolkit/components/extensions/docs/events.rst:113
(Diff revision 1)
> +``getAPI()`` method as in the example above.
> +
> +Handling extra arguments to addListener()
> +-----------------------------------------
> +The standard ``addListener()`` method for events may accept optional
> +addition parameters to allows extra information to be passed when registering

s/allows/allow

::: toolkit/components/extensions/docs/events.rst:219
(Diff revision 1)
> +         myapi: {
> +           onSomething: new EventManager(context, "myapi.onSomething", fire => {
> +             const callback = async (value) => {
> +               let rv = await fire.async(value);
> +               if (rv == true) {
> +                 /* do something... */

I find this section a bit confusing. Should there be a `return` statement here, or am I misunderstanding how this works?

::: toolkit/components/extensions/docs/functions.rst:42
(Diff revision 1)
> +   ]
> +
> +The ``type`` and ``description`` properties were described above.
> +The ``name`` property is the name of the function as it appears in
> +the given namespace.  That is, the fragment above creates a function
> +callable from an extension as ``browser.myapi.dosomething()``.

The name in the schema is `add`, but here you call it `dosomething`.

::: toolkit/components/extensions/docs/functions.rst:43
(Diff revision 1)
> +
> +The ``type`` and ``description`` properties were described above.
> +The ``name`` property is the name of the function as it appears in
> +the given namespace.  That is, the fragment above creates a function
> +callable from an extension as ``browser.myapi.dosomething()``.
> +The ``parameters`` and ``returns`` properties describe the parameters

You mention `returns` here, but returns is a bit odd, as I understand it. I think we only use it for sync functions, and the values returned by resolved promises are not actually documented in the schema. I think it would be worthwhile to either document and disucss this, or just leave `returns` out, as it is unlikely that a developer will be creating a function that declares a `returns`.

::: toolkit/components/extensions/docs/functions.rst:81
(Diff revision 1)
> +When a function is declared in the API schema, a wrapper for the function
> +is automatically created and injected into appropriate extension Javascript
> +contexts.  This wrapper automatically validates arguments passed to the
> +function against the formal parameters declared in the schema and immediately
> +throws an Error if invalid arguments are passed.
> +It also processes optional arguments and inserts default values as needed.

Is it worth mentioning that any optional arguments without defaults will be passed into the function as a `null`?

::: toolkit/components/extensions/docs/lifecycle.rst:16
(Diff revision 1)
> +Extension Shutdown
> +------------------
> +APIs that allocate any resources (e.g., adding elements to the browser's
> +user interface, setting up internal event listeners, etc.) must free
> +these resources when the extension for which they are allocated is
> +shut down.  An API does this by using the ``callOnClose()``

An example might be a nice addition.

::: toolkit/components/extensions/docs/lifecycle.rst:19
(Diff revision 1)
> +user interface, setting up internal event listeners, etc.) must free
> +these resources when the extension for which they are allocated is
> +shut down.  An API does this by using the ``callOnClose()``
> +method on an `Extension <reference.html#extension-class>`_ object. 
> +
> +Extension Uninstall

This section deals with both Uninstall and Update, so perhaps the heading should be changed to reflect that.

::: toolkit/components/extensions/docs/lifecycle.rst:48
(Diff revision 1)
> +     "events": ["update", "uninstall"]
> +   }
> +
> +If these properties are present, the ``onUpdate()`` and ``onUninstall()``
> +methods will be called for the relevant ``ExtensionAPI`` instances when
> +an extension that uses the API is updated or uninstalled.

Again, an example might be helpful.

::: toolkit/components/extensions/docs/manifest.rst:52
(Diff revision 1)
> +
> +The final step is to write code to handle the new manifest entry.
> +The WebExtensions framework processes an extension's manifest when the
> +extension starts up, this happens for existing extensions when a new
> +browser session starts up and it can happen in the middle of a session
> +when an extension is first installed or enabled.

Is onManifestEntry also called when an extension is updated?

::: toolkit/components/extensions/docs/schema.rst:67
(Diff revision 1)
> +
> +- **types**: A type is a re-usable schema fragment.  A common use of types
> +  is to define in one place an object with a particular set of typed fields
> +  that is used in multiple places in an API.
> +
> +- **properties**: A property is a fixed Javascript value available to

Nit: The order of these bullet points differs from the order they are declared in the schema above.
Attachment #8934784 - Flags: review?(bob.silverberg) → review+
Comment on attachment 8934784 [details]
Bug 1423425 Initial draft of WebExtensions API documentation

https://reviewboard.mozilla.org/r/205664/#review213198

> This section is good, but I feel like it could be clearer. The section is entitled `Scopes`, but then scopes are never explicitly mentioned, rather they are referred to as `environments` in the paragraphs that follow. Perhaps using just one of those terms would be clearer, or mention that scopes are environments in the desription.
> 
> You say that `Most Javascript features are available in extension pages.`, perhaps you could say that that is the default, and that a developer would have to add code to make a feature available in one of the other scopes?

Hm, I used "Scopes" since that's the name we use in the modules json file (eg ext-toolkit.json).  But I think "Javascript environments" as used here is more understandable.  Let me think about this one.

> You say that Most Javascript features are available in extension pages., perhaps you could say that that is the default, and that a developer would have to add code to make a feature available in one of the other scopes?

Not sure I understand, this has do be explicitly declared in the modules json file.

> Isn't this first bit, about lazy loading of APIs, something that the framework takes care of? Is this something an API developer needs to worry about, or just being provided as information? If the latter, maybe it's worth mentioning that the framework takes care of this?

I think it is important that API authors understand this, there are several different things that can cause an API implementaiton to be loaded (eg, an extension with a particular manifest key is loaded, an extension touches a particular name under `browser`, etc).  My reasoning for including this here was to help API developers think about exactly when their API needs to be loaded.

> I find this section a bit confusing. Should there be a `return` statement here, or am I misunderstanding how this works?

Maybe I should have made this more specific, the point is just that the event implementation can take some action (or choose not to or whatever) based on the return value from the listener, which is stored in `rv`.
Comment on attachment 8934783 [details]
Bug 1423425 Make some extensions comments jsdoc-compatible

https://reviewboard.mozilla.org/r/205662/#review213524

::: toolkit/components/extensions/Extension.jsm:290
(Diff revision 2)
> -// primarily related to manifest parsing and localization, which is
> -// useful prior to extension installation or initialization.
> -//
> -// No functionality of this class is guaranteed to work before
> -// |loadManifest| has been called, and completed.
> -this.ExtensionData = class {
> + * This class implements the functionality of the Extension class,
> + * primarily related to manifest parsing and localization, which is
> + * useful prior to extension installation or initialization.
> + *
> + * No functionality of this class is guaranteed to work before
> + * |loadManifest| has been called, and completed.

Nit: s/|/`/g

::: toolkit/components/extensions/Extension.jsm:1020
(Diff revision 2)
>  class LangpackBootstrapScope {
>    install(data, reason) {}
>    uninstall(data, reason) {}
>  
>    startup(data, reason) {
> +    // eslint-disable-next-line no-use-before-define

Please pre-declare rather than doing this.

::: toolkit/components/extensions/Extension.jsm:1032
(Diff revision 2)
> -// comes directly from bootstrap.js when initializing.
> -this.Extension = class extends ExtensionData {
> + * We create one instance of this class per extension. |addonData|
> + * comes directly from bootstrap.js when initializing.

This isn't really a class description, just a note. If you want to make this a JSDoc comment, please make it actually describe the class.

::: toolkit/components/extensions/Extension.jsm:1133
(Diff revision 2)
>  
> +  // Some helpful properties added elsewhere:
> +  /**
> +   * An object used to map between extension-visible tab ids and
> +   * native Tab object
> +   * @var {TabManager} tabManager

This isn't a var, it's a property.

::: toolkit/components/extensions/ExtensionCommon.jsm:187
(Diff revision 2)
>  
> +/**
> + * This class contains the information we have about an individual
> + * extension.  It is never instantiated directly, instead subclasses
> + * for each type of process extend this class and add members that are
> + * relevant for that process.

Nit: @abstract

::: toolkit/components/extensions/ExtensionCommon.jsm:1549
(Diff revision 2)
> -// ExtensionContext in the chrome process or ExtensionContext in a
> -// content process). |name| is for debugging. |register| is a function
> -// to register the listener. |register| should return an
> -// unregister function that will unregister the listener.
> + * The result is an object with addListener, removeListener, and
> + * hasListener methods. |context| is an add-on scope (either an
> + * ExtensionContext in the chrome process or ExtensionContext in a
> + * content process). |name| is for debugging. |register| is a function
> + * to register the listener. |register| should return an
> + * unregister function that will unregister the listener.

Nit: s/|/`/g

::: toolkit/components/extensions/ExtensionCommon.jsm:1556
(Diff revision 2)
> -// to register the listener. |register| should return an
> -// unregister function that will unregister the listener.
> + * ExtensionContext in the chrome process or ExtensionContext in a
> + * content process). |name| is for debugging. |register| is a function
> + * to register the listener. |register| should return an
> + * unregister function that will unregister the listener.
> + * @constructor
> + * 

Nit: Trailing space.
Attachment #8934783 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8934783 [details]
Bug 1423425 Make some extensions comments jsdoc-compatible

https://reviewboard.mozilla.org/r/205662/#review213524

> Please pre-declare rather than doing this.

We have a circular issue though where `Langpack` and `LangpackBootstrapScope` refer to each other (same with `Extension` and `BootstrapScope`).  Given the choice of disabling no-use-before-define versus pre-declaring the name and filling in the implementation later, I find the first less objectionable but I could be persuaded otherwise...

> This isn't a var, it's a property.

I can't actually get this to show up in the autojs output but *shrug*
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/04017639642a
Let sphinx-js read .js files r=erik
https://hg.mozilla.org/integration/autoland/rev/d64fd69a4b88
Make some extensions comments jsdoc-compatible r=kmag
https://hg.mozilla.org/integration/autoland/rev/c3ac950b0b8a
Initial draft of WebExtensions API documentation r=bsilverberg
Attachment #8937496 - Flags: review?(kmaglione+bmo)
Comment on attachment 8937496 [details]
Bug 1423425 Stop reading the Extension.jsm module object in specialpowers

https://reviewboard.mozilla.org/r/208170/#review213968
Attachment #8937496 - Flags: review?(kmaglione+bmo) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da4fe391303b
Let sphinx-js read .js files r=erik
https://hg.mozilla.org/integration/autoland/rev/a92c9a459eeb
Stop reading the Extension.jsm module object in specialpowers r=kmag
https://hg.mozilla.org/integration/autoland/rev/7d5aebbafcf7
Make some extensions comments jsdoc-compatible r=kmag
https://hg.mozilla.org/integration/autoland/rev/4e0519ed8131
Initial draft of WebExtensions API documentation r=bsilverberg
Flags: needinfo?(aswan)
Flags: qe-verify-
Attachment #8934784 - Flags: review?(lgreco)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: