update Page Mods API for forward-compatibility with electrolysis (e10s)

RESOLVED FIXED in 0.8

Status

defect
P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: myk, Assigned: irakli)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

9 years ago
The Page Mods API landing in bug 546739 should be updated to be forward-compatible with electrolysis (e10s).
Reporter

Updated

9 years ago
Priority: -- → P1
Reporter

Comment 4

9 years ago
Comment on attachment 475074 [details] [diff] [review]
Implementation + Documentation + Tests

>-      onStart: function(wrappedWindow) {
>-        // this runs each time a new content document starts loading, but
>-        // before the page starts loading, so we can't interact with the
>-        // page's DOM here yet.
>-        wrappedWindow.wrappedJSObject.newExposedProperty = 1;
>-      },
>-      onReady: function(wrappedWindow) {
>-        // at this point we can work with the DOM
>-        wrappedWindow.document.body.innerHTML = "<h1>Page Mods!</h1>";
>-      }
>+      // this runs each time a new content document starts loading, but
>+      // before the page starts loading, so we can't interact with the
>+      // page's DOM here yet.
>+      contentScript: 'window.newExposedProperty = 1;'

Nit: this -> This
(The problem was in the original as well, but since you're changing it, you might as well make the fix.)


>-Note: currently, it is necessary to access the `wrappedJSObject` of window
>-and document objects in order to set properties on them that the web page
>-can access.  We are working to remove this restriction.
>+    // if you want to work with the DOM then you should set `contentScriptWhen`
>+    // to `'ready'`.

Nit: if -> If
Nit: DOM then -> DOM, then


>+    var pageMod = require("page-mod");
>+    pageMod.add(new pageMod.PageMod({
>+      include: ["*.example.com",
>+                "http://example.org/a/specific/url",
>+                "http://example.info/*"],
>+      contentScriptWhen: 'ready',
>+      contentScript: 'document.body.innerHTML = "<h1>Page Mods!</h1>";'
>+    }));
>+
>+    // You can also pass messages between content scripts and jetpack process

Nit: jetpack process -> the program.


>+    var pageMod = require("page-mod");
>+    var myPageMod = pageMod.add({
>+      include: [
>+        '*.example.com',
>+        'http://example.org/a/specific/url',
>+        'http://example.info/*'
>+      ],
>+      contentScriptWhen: 'ready',
>+      contentScript: 'new ' + function() {
>+        onMessage = function onMessage() {
>+          postMessage('My current location is: ' + window.location);
>+        }
>+      },

We should not be encouraging function serialization in content scripts, since it suggests the functions can close around the scope in which they are defined, which they can't, so this should be something like:

    contentScript: "onMessage = function onMessage() {" +
                   "  postMessage('My current location is:' " +
                   "              window.location); " +
                   "};"

Note: function serialization is fine in the tests; it's just in these user-facing docs that I want to avoid it.


>+      onOpen: function onOpen(worker, mod) {
>+        // you can handle errors that occur in the content scripts
>+        // by adding listener to the error events
>+        worker.on('error', function(error) {
>+          console.error(error.message);
>+        });
>+        worker.on('message', function(data) {
>+          console.log(data);
>+        });
>+        worker.postMessage('worker what is your location ?');

Nit: worker what is your location ? -> Worker, what is your location?


>-  Options for the page mod, with the following keys:
>+  Options for the page mod, with all the keys descripbed for [Loader] and few
>+  additionals:

Loader is a low-level API, while PageMod is a high-level one, so these docs should document the Loader-provided options rather than making users read them separately in the docs for the low-level module.


>+  @prop [onOpen] {function}
>+    Functions to call when a matching page is open.
>+    Function will be called with two arguments:

The word "open" may be confusing in this context, given its existing connotations.  Let's use "attach" instead, i.e. onAttach.

Functions to call when a matching page is open. -> A function to call when the page mod attaches content scripts to a matching page.


>-  @prop [onReady] {function,array}
>-    Functions to call when a matching page's DOM is ready.
>+    1. [Worker] object for that window that can be used for communication with
>+    a content scripts.
>+    [Worker]:https://jetpack.mozillalabs.com/sdk/latest/docs/#module/jetpack-core/content/worker
>+    2. `this` `PageMod`.

The Worker API should be described as a part of this PageMod API instead of referencing the docs for the low-level Worker module.

Also, `pageMod` should be the first argument to the function, with `worker` being the second argument, for consistency with other event listeners (per bug 593921).


> <api name="add">
> @function
> Register a page mod, activating it for the pages to which it applies.
>-@param page mod {PageMod} the page mod to add
>+@param pageMod {PageMod,Object}
>+The page mod to add or an options to create new pageMod and add it.

The page mod to add or an options to create new pageMod and add it.
->
The page mod to add, or options for a page mod to create and then add.


>+const Rules = EventEmitter.resolve({ toString: null }).compose({
>+  add: function() Array.slice(arguments).forEach(function onAdd(rule) {
>+    if (this._has(rule)) return;
>+    // registering rule to the rules registry
>+    if (!(rule in RULES))
>+      RULES[rule] = URLRule(rule);
>+    this._add(rule);
>+    this._emit('add', rule);
>+  }.bind(this)),
>+  remove: function() Array.slice(arguments).forEach(function onRemove(rule) {
>+    if (!this._has(rule)) return;
>+    this._remove(rule);
>+    this._emit('remove', rule);
>+  }.bind(this)),
>+}, List);

Nit: it would be a bit easier to see the composition if List were first in the list of traits from which to compose the Rules constructor (provided order is unimportant).


>+    // exception is thrown if `hostname` is accseed on 'about:*' urls in FF 3.*

Nit: accseed -> accessed
Attachment #475074 - Flags: review?(myk) → review-
Reporter

Comment 6

9 years ago
Comment on attachment 475387 [details] [diff] [review]
Addressing review commets

>+    1. An object implementing [web worker] interface, that can be used
>+    for communication with a content scripts (See examples section for more
>+    details).
>+    [web worker]:http://www.w3.org/TR/workers/#worker

These docs are intended for addon developers, whereas the W3 specification is intended for specification implementers. These are very different audiences, and the specification is not appropriate for our audience, so we should not be directing developers to it.

In the interests of getting this in for 0.8, however, r=myk, and we'll fix this in a followup bug.
Attachment #475387 - Flags: review?(myk) → review+
Reporter

Updated

9 years ago
Keywords: checkin-needed
Reporter

Comment 7

9 years ago
Fixed by changeset https://hg.mozilla.org/labs/jetpack-sdk/rev/4954c5f61bbe.

The docs issue has been filed as bug 596470.
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Reporter

Comment 8

9 years ago
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.