546739 - PageMods JEP

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Daniel Buchner [:dbuc]]

PageMods API functionality needs implementation per the agreed upon spec at https://wiki.mozilla.org/Labs/Jetpack/Reboot/JEP/107

Comment 1

[Nickolay_Ponomarev]

Since I was approached about finishing up my version of the JEP for 0.5, I thought I'd link to it <https://wiki.mozilla.org/User:Asqueella/JEP_107> and to  some discussion on this topic: http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/09deffcc11fa00ea/2ce4c1ed979cb8da

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Nickolay pointed me to the current version of his implementation <http://bitbucket.org/nickolay/jetpack-packages/src/tip/packages/page-mods/>, which doesn't yet take into account the discussion on an E10S-compatible API, and here's a review:


page-mod.js:

> // XXX disable debug output before adding to the SDK
> // Perhaps provide a 'debug' switch somewhere?
> function DEBUG_LOG(str) {
>   console.debug(str);
> }

I think it's best to just call console.debug directly when you think it's appropriate, and then we can separately figure out how to enable/disable debug output as appropriate on debug/release builds.


>   if (typeof opts.include == "string") {
>     this._includeRules = [parseURLRule(opts.include)];
>   } else {
>     this._includeRules = [parseURLRule(rule) for each (rule in opts.include)];
>   }
...
> PageMod.prototype._runRegisteredCallbacks = function(key, wrappedWindow) {

In the other APIs, we've been using lexical closures and other techniques to secure private properties (until something better comes along, like a modified form of COWs).  We should do that here as well, which means that instead of setting _includeRules on the PageMod instance, pageModManager.register() should set it on a new private object that encompasses both the PageMod instance and the parsed include rules.  And instead of _runRegisteredCallbacks being a property of the PageMod prototype, it should be a function in the private global scope, i.e.:

PageMod constructor:

  this.__defineGetter__("include", function () opts.include);

runRegisteredCallbacks:

  function runRegisteredCallbacks(pageMod, key, wrappedWindow) {
    pageMod[key].forEach(function(fn) {
      ...
    }
  }

pageModManager.register:

  var index = this.registeredPageMods.indexOf(
    this.registeredPageMods.filter(function (v) v.pageMod === pageMod)[0]
  );
  ...
  this.registeredPageMods.push({
    pageMod: pageMod,
    includeRules: (typeof pageMod.include == "string") ?
                  [parseURLRule(pageMod.include)] :
                  [parseURLRule(rule) for each (rule in pageMod.include)]
  });

pageModManager.onContentGlobalCreated:

  self.registeredPageMods.forEach(function({ pageMod, includeRules }) {
    if (self._rulesMatchURL(includeRules, wrappedWindow.location)) {
      if ("onWindowCreate" in pageMod)
        runRegisteredCallbacks(pageMod, "onWindowCreate", wrappedWindow);
      ...
    }
  }

Note: this last bit uses destructuring assignment to destructure the cached object into two variables whose names are the same as the properties of the object: pageMod and includeRules.

The only issue with this approach is that parseURLRule both parses and validates the rules, and it would make more sense to do validation in the constructor than the "add" function.  To resolve this issue, we could either split parseURLRule into two functions, one that validates a rule and another that parses it.  Or we could just call it twice, once in the constructor and once in the "add" function, throwing away the return value in the constructor, which should work well enough.


>   var self = this;
>   function addScripts(opts, key) {
>     if (typeof opts[key] == "function") {
>       self[key] = [opts[key]];

If we keep PageMod.onWindowCreate and PageMod.onDOMReady (or any variation thereof), we should use collection.addCollectionProperty to make them collection properties.


>           console.warn("PageMod: an item in the options['" + key + "'] array is not " +
>                        "a function: " + item);

Rather than merely warning here, it seems like it's worth throwing an exception, since we can't determine what the consumer expected to happen.


> PageMod.prototype._runRegisteredCallbacks = function(key, wrappedWindow) {
>   this[key].forEach(function(fn) {
>     try {
>       fn(wrappedWindow);
>     } catch(e) {
>       Components.utils.reportError(e);
>     }
>   });
> }

Use errors.catchAndLog to handle exceptions while calling these callbacks, i.e.:

  this[key].forEach(function(fn) {
    require("errors").catchAndLog(fn)(wrappedWindow);
  });


>       throw new Error("Trying to add a page mod that has already been added.");

When encountering similar code while reviewing Page Worker, I suggested that we should just return early without throwing an error in this case, since despite the duplicate call, we can determine what the consumer expects to happen, and the end result is as they expected: the page mod has been added.


>     if (!(pageMod instanceof PageMod)) {
>       throw new Error("Trying to add an object that's not a PageMod instance.")

This, on the other hand, makes perfect sense.


>     if (index == -1) {
>       throw new Error("Trying to remove a page mod, that has not been added.");

Here I'd again say that we should return early without throwing an exception (although perhaps it's worth a warning).


>   // XXX does the user have to call remove() to clean up on shutdown?
>   pageModManager.unregister(pageMod);

No, consumers shouldn't have to call remove on shutdown.  Instead, the unload.when callback should unregister all page mods.


> // export internals for unit-testing:
> exports._internals = {
>   parseURLRule: parseURLRule,
>   pageModManager: pageModManager
> };

Instead of doing this, use test.makeSandboxedLoader to load the module, then access its internals through the sandbox created for the module, i.e. (inside the test):

  let loader = test.makeSandboxedLoader();
  let pageMod = loader.require("page-mod");
  let global = loader.findSandboxForModule("page-mod").globalScope;
  // global is the global object inside the loaded module,
  // through which you can access its private definitions.


test-page-mod.js:

> // XXX what's the best way to disable this test on Firefox versions not
> // supporting content-document-global-created? Unless there's a standard way,
> // I'm inclined to work with nsIXULAppInfo.

That seems reasonable.  I don't think there's a standard way.  Note that you don't need to access nsIXULAppInfo directly.  Just use require("xul-app").platformVersion.

Comment 3

[Nickolay_Ponomarev]

> page-mod.js:
> > function DEBUG_LOG(str) {
> I think it's best to just call console.debug directly
The reason I wanted to provide a separate switch was that page mod logging is very verbose,
since it logs for every <rule> x <loaded page> combination. I think that it's useful
to have that as an option for debugging pagemods specifically, but not generally.

But OK, I switched to just using console.debug(): http://bitbucket.org/nickolay/jetpack-packages/changeset/25438cb3db0a

> >   if (typeof opts.include == "string") {
> >     this._includeRules = [parseURLRule(opts.include)];
> >   } else {
> >     this._includeRules = [parseURLRule(rule) for each (rule in opts.include)];
> >   }
> ...
> > PageMod.prototype._runRegisteredCallbacks = function(key, wrappedWindow) {
> 
> In the other APIs, we've been using lexical closures and other techniques to
> secure private properties (until something better comes along, like a modified
> form of COWs).  We should do that here as well

This complicates the code and slows it down. What's the benefit? The modules are jetpack-private
and even if they weren't, the PageMod instances clearly belong to the jetpack that created them.

The only thing a jetpack can do by modifying these "private" properties that it can't do by
creating a new PageMod instance is causing errors when the pagemods are run.

> >   var self = this;
> >   function addScripts(opts, key) {
> >     if (typeof opts[key] == "function") {
> >       self[key] = [opts[key]];
> 
> If we keep PageMod.onWindowCreate and PageMod.onDOMReady (or any variation
> thereof), we should use collection.addCollectionProperty to make them
> collection properties.
OK, added as a note to the code for now.

> >           console.warn("PageMod: an item in the options['" + key + "'] array is not " +
> >                        "a function: " + item);
> Rather than merely warning here, it seems like it's worth throwing an
> exception, since we can't determine what the consumer expected to happen.

OK, http://bitbucket.org/nickolay/jetpack-packages/changeset/9af248bf40d0

> > PageMod.prototype._runRegisteredCallbacks = function(key, wrappedWindow) {
> >   this[key].forEach(function(fn) {
> >     try {
> >       fn(wrappedWindow);
> >     } catch(e) {
> >       Components.utils.reportError(e);
> >     }
> Use errors.catchAndLog to handle exceptions while calling these callbacks,
> i.e.:
>   this[key].forEach(function(fn) {
>     require("errors").catchAndLog(fn)(wrappedWindow);
>   });

I think this catchAndLog()() construct is less readable and would prefer to just switch
Cu.reportErrors to console.exception(), but OK: http://bitbucket.org/nickolay/jetpack-packages/changeset/b9097962d7cd

> >       throw new Error("Trying to add a page mod that has already been added.");
> When encountering similar code while reviewing Page Worker, I suggested that we
> should just return early without throwing an error in this case, since despite
> the duplicate call, we can determine what the consumer expects to happen, and
> the end result is as they expected: the page mod has been added.

I disagree for this reason: https://bugzilla.mozilla.org/show_bug.cgi?id=343416#c14

> >   // XXX does the user have to call remove() to clean up on shutdown?
> >   pageModManager.unregister(pageMod);
> 
> No, consumers shouldn't have to call remove on shutdown.  Instead, the
> unload.when callback should unregister all page mods.

But should it? Strictly speaking, unregistering page mods (i.e. removing them from an array, held by a pageModManager,
held by the page-mod module's global object) should not be necessary, since unloading a jetpack should cut off all
references from "outside" (the platform) to jetpack "island" objects, so PageMod objects would get GC'd along with
page-mod module's and the jetpack module's that uses page-mod global scopes.

Do we clear such internal references "just in case" in other modules?

> > // export internals for unit-testing:
> Instead of doing this, use test.makeSandboxedLoader to load the module, then
Thanks for the suggestion, fixed: http://bitbucket.org/nickolay/jetpack-packages/changeset/b38b69caf764

> test-page-mod.js:
> 
> > // XXX what's the best way to disable this test on Firefox versions not
> > // supporting content-document-global-created? Unless there's a standard way,
> > // I'm inclined to work with nsIXULAppInfo.
> 
> That seems reasonable.  I don't think there's a standard way.  Note that you
> don't need to access nsIXULAppInfo directly.  Just use
> require("xul-app").platformVersion.
> 
Filed bug 571674, to be able to write this:

-//if (false) {
+// XXX depends on bug 571674
+var xulApp = require("xul-app");
+var pageModSupported = xulApp.versionInRange(xulApp.platformVersion, "1.9.3a3", "*")
+
+if (pageModSupported) {

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #3)
> The reason I wanted to provide a separate switch was that page mod logging
> is very verbose, since it logs for every <rule> x <loaded page> combination.
> I think that it's useful to have that as an option for debugging pagemods
> specifically, but not generally.

Yup, I agree that would be useful.  And displaying a bunch of debug messages to developers who try out the module is a great way to highlight the problem and get folks working on a solution! ;-)


> > In the other APIs, we've been using lexical closures and other techniques
> > to secure private properties (until something better comes along, like a
> > modified form of COWs).  We should do that here as well
> 
> This complicates the code and slows it down. What's the benefit?

It does indeed complicate the code, although I haven't heard of any slowdowns because of it.

The reason we're doing it is that one of the goals of the project is to improve the security of addons, and reducing the surface area for attacks via the encapsulation of functionality is a key strategy for achieving that goal.

I don't know of a specific vulnerability in these particular private properties, but the less code modules reveal to their consumers, and the more encapsulated they are, the fewer opportunities there are for rogue code (whether in addons themselves or in web content with which addons interact) to compromise user security.


> I think this catchAndLog()() construct is less readable and would prefer to
> just switch Cu.reportErrors to console.exception(), but OK:

I agree, it is less readable.  However, it enables us to be consistent about how we handle these exceptions, and it makes it easier to update the code later (f.e. to start displaying these messages in a different console).

I'm sure folks would be open to suggestions for improvement to the interface, if you have ideas for making it more readable!


> > >       throw new Error("Trying to add a page mod that has already been
> > >       added.");
> > When encountering similar code while reviewing Page Worker, I suggested
> > that we should just return early without throwing an error in this case,
> > since despite the duplicate call, we can determine what the consumer
> > expects to happen, and the end result is as they expected: the page mod has
> > been added.
> 
> I disagree for this reason:
> https://bugzilla.mozilla.org/show_bug.cgi?id=343416#c14

Hmm, that seems like a different case, but ok, I'm fine with leaving this the way it is, and it's certainly easier to loosen the restriction than to tighten it up later if we change our minds.


> > >   // XXX does the user have to call remove() to clean up on shutdown?
> > >   pageModManager.unregister(pageMod);
> > 
> > No, consumers shouldn't have to call remove on shutdown.  Instead, the
> > unload.when callback should unregister all page mods.
> 
> But should it? Strictly speaking, unregistering page mods (i.e. removing them
> from an array, held by a pageModManager, held by the page-mod module's global
> object) should not be necessary, since unloading a jetpack should cut off all
> references from "outside" (the platform) to jetpack "island" objects, so
> PageMod objects would get GC'd along with page-mod module's and the jetpack
> module's that uses page-mod global scopes.
> 
> Do we clear such internal references "just in case" in other modules?

I believe we do, but Atul can answer this question better than me.  cc:ing him for his thoughts.

Comment 5

[Atul Varma [:atul]]

(In reply to comment #4)
> I agree, it is less readable.  However, it enables us to be consistent about
> how we handle these exceptions, and it makes it easier to update the code later
> (f.e. to start displaying these messages in a different console).

I'm not a big fan of its readability either, but FWIW, my motivation in adding that function, aside from what Myk mentioned above, was also to make unit testing code a lot easier: adding a try ... except would require one to write a test for the exception case, but just calling a function effectively removes one "branch" from the code, meaning less tests to write.

My thinking on this could be totally flawed though--I have a suspicious feeling that it actually is--so any feedback here is welcome.

Will respond to the concern about GC'ing/unloading in another comment.

Comment 6

[Atul Varma [:atul]]

(In reply to comment #4)
> > > No, consumers shouldn't have to call remove on shutdown.  Instead, the
> > > unload.when callback should unregister all page mods.
> > 
> > But should it? Strictly speaking, unregistering page mods (i.e. removing them
> > from an array, held by a pageModManager, held by the page-mod module's global
> > object) should not be necessary, since unloading a jetpack should cut off all
> > references from "outside" (the platform) to jetpack "island" objects, so
> > PageMod objects would get GC'd along with page-mod module's and the jetpack
> > module's that uses page-mod global scopes.
> > 
> > Do we clear such internal references "just in case" in other modules?
> 
> I believe we do, but Atul can answer this question better than me.  cc:ing him
> for his thoughts.

If there's a way for a developer to manually deactivate a pagemod, e.g. to conserve resources b/c it's only needed for some part of an addon's total lifetime, then you'll definitely want to unload any managed dependencies that it's using, rather than relying on the dependencies' parent modules to unload them when the addon is shutdown.

I may be misinterpreting your question, though.

Comment 7

[Nickolay_Ponomarev]

> I may be misinterpreting your question, though.
Yeah, the question was different. When unloading, do other modules unregister from the platform-global services/objects or also try to break some jetpack-internal references (i.e. in this case page-mod module's global -> PageMod objects that are shared with other modules), just in case?

Sorry I disappeared for the weekend, was out of town. Seems I missed 0.5.

The content-script part of the API turned out not very simple to implement, will talk to you later this week.

Comment 8

[Nickolay_Ponomarev]

OK, so since June I
1) made the change suggested by Myk regarding private properties a while ago - http://bitbucket.org/nickolay/jetpack-packages/changeset/4ff0476679ad
2) updated to work with Jetpack trunk and tested with Firefox 3.6.8 <http://bitbucket.org/nickolay/jetpack-packages/changeset/25a7b8f3fbed>
3) filed bug 582176 against the platform, which is causes unexpected pagemods onStart notifications with incorrect contentWindow.location when navigating to a page in bfcache.

The content scripts part turned out to be non-trivial, I meant to consult with the platform folks, but slacked off instead.

There are also leaks reported when running tests.
a) Do I really have to use makeSandboxedLoader? It appears to me that it should be done at the test harness level...
b) When I tried to use makeSandboxedLoader the leaks became intermittent and/or appearing only when multiple consecutive tests were run. Since when I looked into it in June or July running "cfx testall" also produced such leaks, I concluded they weren't real and no-one cares about them anyway. Is it correct?

Myk, if the changes since the last time you looked at this are fine  <http://bitbucket.org/nickolay/jetpack-packages>, I can prepare a patch that adds the new files in jetpack/packages/jetpack-core to update the API to be e10s compatible later in the tree, as you and Dietrich suggested.

Comment 9

[Nickolay_Ponomarev]

The WIP patch on top of my bitbucket repository, in case someone wants to check my usage of makeSandboxedLoader I mentioned in comment 8 or see the work in progress: http://bitbucket.org/nickolay/jetpack-packages-patches/src/8d92c273f375/sandbox-current.diff (it's a bitbucket patch-queue repository, which you can qclone instead of the main repo.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #8)
> a) Do I really have to use makeSandboxedLoader? It appears to me that it should
> be done at the test harness level...

It should now be possible to implement support for content scripts by using the content-symbiont module that I checked into the tree this morning.  See the panel module, which I checked in at the same time, for an example of its usage.


> b) When I tried to use makeSandboxedLoader the leaks became intermittent and/or
> appearing only when multiple consecutive tests were run. Since when I looked
> into it in June or July running "cfx testall" also produced such leaks, I
> concluded they weren't real and no-one cares about them anyway. Is it correct?

Right now, there seems to be one or more problems with our leak reporting that is causing inconsistent results.  Alternately, it could be problems with our tests or actual leaks that we haven't found yet.  In any case, we aren't currently blocking checkins on leakage until we can figure out the problem(s).


> Myk, if the changes since the last time you looked at this are fine 
> <http://bitbucket.org/nickolay/jetpack-packages>, I can prepare a patch that
> adds the new files in jetpack/packages/jetpack-core to update the API to be
> e10s compatible later in the tree, as you and Dietrich suggested.

Thanks Nickolay, that'd be great!  Afterwards, I'll find someone to take the patch the rest of the way through to final review and checkin.

(Alternately, as I know you're strapped for time these days, we can prepare the patch from your repository ourselves.  Just let me know which you would prefer.)

Comment 11

[Nickolay_Ponomarev]

>>a) Do I really have to use makeSandboxedLoader? It appears to me that it
>> should be done at the test harness level...
> 
> It should now be possible to implement support for content scripts by using 

Good to know, but it's not related to my question, right?

> > Myk, if the changes since the last time you looked at this are fine 
> > <http://bitbucket.org/nickolay/jetpack-packages>, I can prepare a patch that
> > adds the new files in jetpack/packages/jetpack-core to update the API to be
> > e10s compatible later in the tree, as you and Dietrich suggested.
> 
> Thanks Nickolay, that'd be great!  Afterwards, I'll find someone to take the
> patch the rest of the way through to final review and checkin.
> 
>(Alternately, as I know you're strapped for time these days, we can prepare the
> patch from your repository ourselves.  Just let me know which you would
> prefer.)

I'd rather not hold you up any longer if you have resources. Whoever gets to it first, OK? :)

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: Nickolay's work w/a few additions

This is Nickolay's Page Mods implementation along with the following changes:

1. added a license header to the page-mod.js module file;
2. merged test-internals.js tests into test-page-mod.js test file;
3. expanded documentation.

Nickolay: The license header I added is for the MPL/GPL/LGPL tri-license, which is the standard license for code that goes into the SDK (and most other Mozilla projects).  I just want to confirm, since it's your code: is that ok with you?

Comment 13

[Nickolay_Ponomarev]

(In reply to comment #12)
> Nickolay: The license header I added is for the MPL/GPL/LGPL tri-license, which
> is the standard license for code that goes into the SDK (and most other Mozilla
> projects).  I just want to confirm, since it's your code: is that ok with you?

Sure.

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 471391 [details] [diff] [review]
patch v1: Nickolay's work w/a few additions

>diff --git a/packages/jetpack-core/docs/page-mod.md b/packages/jetpack-core/docs/page-mod.md
>new file mode 100644
>--- /dev/null
>+++ b/packages/jetpack-core/docs/page-mod.md
>@@ -0,0 +1,116 @@
>+<!-- contributed by Nickolay Ponomarev [[e-mail]] -->
>+<!-- contributed by Myk Melez [[e-mail]] -->
>+
>+The `page-mod` module provides an easy way to run scripts on a given set
>+of pages.

nit: s/on/in the context of/ ?

>+`PageMod` constructs a new page mod.  `add` registers a page mod, activating it
>+for the pages to which it applies.  `remove` unregisters a page mode,
>+deactivating it.

nit: it'd be clearer to use "page modification" instead of "page mod", when not referring to api syntax.

>+
>+Examples
>+--------
>+
>+Add a header to a variety of pages:

s/a header/content/

>+<api name="include">
>+@property {array}
>+The pages to which the page mod should apply.  An array of rules matching
>+the URLs of pages.  There are four kinds of rules:

does this completely replace the include rules from when the pagemod was first created? should note it in the docs either way.

>+<api name="onStart">
>+@property {array}
>+Functions to call when a matching page starts loading.
>+</api>
>+
>+<api name="onReady">
>+@property {array}
>+Functions to call when a matching page's DOM is ready.
>+</api>

are these actually arrays, or collections?

do these follow the jetpack convention of both assignment and collection support? if so, should note it, and if not, should file a followup bug.

>+/**
>+ * https://wiki.mozilla.org/User:Asqueella/JEP_107 implementation.
>+ * (XXX update link)

could probably just remove this, relying instead on sdk and builder documentation.

>+
>+  collection.addCollectionProperty(this, "include");
>+  collection.addCollectionProperty(this, "onStart");
>+  collection.addCollectionProperty(this, "onReady");

file followup for setters for these?

>+
>+  // throw away parseURLRule's results -- we're only validating in the
>+  // constructor, because we can't have private properties per the
>+  // design guidelines.
>+  if (typeof opts.include == "string") {
>+    parseURLRule(opts.include);
>+  } else {
>+    for each (rule in opts.include)
>+      parseURLRule(rule);
>+  }
>+  this.include = opts.include;

i don't understand this comment.

nit: why not integrate that code into an ok() method of the include bit in validateOptions() above?

>+  var self = this;
>+  function addScripts(opts, key) {
>+    if (typeof opts[key] == "function") {
>+      self[key] = [opts[key]];
>+    } else if (opts[key]) {

should check if the value is actually an array and throw otherwise

>+var pageModManager = {
>+  registeredPageMods: [],
>+
>+  _initialized: false,
>+  init: function() {
>+    this._initialized = true;

could make a memoizing getter in the module scope that inits and returns, to avoid having the apis have to check this for each and every call.

>+  onContentGlobalCreated: function(wrappedWindow, topic, data) {
>+    var self = this;
>+    console.debug("--> PageMod running on page: <" +
>+                  wrappedWindow.location.toString() + ">");

remove before check-in

>+    console.debug("<-- All PageMods finished for page: <" +
>+                  wrappedWindow.location.toString() + ">");
>+  }

ditto

>+/**
>+ * Parses a string, possibly containing the wildcard character ('*') to
>+ * create URL-matching rule. Supported input strings with the rules they
>+ * create are listed below:
>+ *  1) * (a single asterisk) - any URL with the http(s) or ftp scheme
>+ *  2) *.domain.name - pages from the specified domain and all its subdomains,
>+ *                     regardless of their scheme.
>+ *  3) http://example.com/* - any URLs with the specified prefix.
>+ *  4) http://example.com/test - the single specified URL
>+ */
>+function parseURLRule(url) {

should document the return value.

also, there are a whole lot of conditions in there. might be more regression-proof to set the retval and return at the end, instead of all those early returns.

>+exports.add = function(pageMod) {
>+  pageModManager.register(pageMod);
>+}
>+exports.remove = function(pageMod) {
>+  pageModManager.unregister(pageMod);
>+}

why not just do, eg:

exports.add = pageModManager.register;

r=me with these addressed

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: addresses review comments

(In reply to comment #14)
> >+The `page-mod` module provides an easy way to run scripts on a given set
> >+of pages.
> 
> nit: s/on/in the context of/ ?

Fixed.


> >+`PageMod` constructs a new page mod.  `add` registers a page mod, activating it
> >+for the pages to which it applies.  `remove` unregisters a page mode,
> >+deactivating it.
> 
> nit: it'd be clearer to use "page modification" instead of "page mod", when not
> referring to api syntax.

That's four extra syllables per occurrence!  How about this?

    `PageMod` constructs a new page modification (mod).  `add` registers
    a page mod, activating it for the pages to which it applies.  `remove`
    unregisters a page mod, deactivating it.


> >+Examples
> >+--------
> >+
> >+Add a header to a variety of pages:
> 
> s/a header/content/

Fixed.


> >+<api name="include">
> >+@property {array}
> >+The pages to which the page mod should apply.  An array of rules matching
> >+the URLs of pages.  There are four kinds of rules:
> 
> does this completely replace the include rules from when the pagemod was first
> created? should note it in the docs either way.

Actually, `include` is a collection, not an array.  Docs updated to reflect that.


> >+<api name="onStart">
> >+@property {array}
> >+Functions to call when a matching page starts loading.
> >+</api>
> >+
> >+<api name="onReady">
> >+@property {array}
> >+Functions to call when a matching page's DOM is ready.
> >+</api>
> 
> are these actually arrays, or collections?

Collections!  Fixed.


> do these follow the jetpack convention of both assignment and collection
> support? if so, should note it, and if not, should file a followup bug.

Collection support noted.  Assignment not noted, as we have been planning to remove that per bug 588250.  Bug 566773 is about switching to the EventEmitter model across the entire codebase, which'll obsolete the Collection stuff too, but that should get fixed in that bug, not this one.


> >+/**
> >+ * https://wiki.mozilla.org/User:Asqueella/JEP_107 implementation.
> >+ * (XXX update link)
> 
> could probably just remove this, relying instead on sdk and builder
> documentation.

Yup, removed along with the rest of that comment, which didn't seem necessary.


> >+  collection.addCollectionProperty(this, "include");
> >+  collection.addCollectionProperty(this, "onStart");
> >+  collection.addCollectionProperty(this, "onReady");
> 
> file followup for setters for these?

require("collection").addCollectionProperty actually takes care of that for us (modulo bug 588250, modulo bug 566773).


> >+  // throw away parseURLRule's results -- we're only validating in the
> >+  // constructor, because we can't have private properties per the
> >+  // design guidelines.
> >+  if (typeof opts.include == "string") {
> >+    parseURLRule(opts.include);
> >+  } else {
> >+    for each (rule in opts.include)
> >+      parseURLRule(rule);
> >+  }
> >+  this.include = opts.include;
> 
> i don't understand this comment.

The comment points out that although we parse the rules in the constructor in order to validate them, we don't store their parsed values, since there isn't way to make them available to the registration code while hiding them from API consumers, and thus we have to parse them again when the page mod gets registered.

I have updated the comment to clarify matters and point towards a possible future solution.


> nit: why not integrate that code into an ok() method of the include bit in
> validateOptions() above?

I believe that was an artifact of an earlier version of this code that stored the parsed rules in a public property.  I have moved the code into an ok() function.


> >+  var self = this;
> >+  function addScripts(opts, key) {
> >+    if (typeof opts[key] == "function") {
> >+      self[key] = [opts[key]];
> >+    } else if (opts[key]) {
> 
> should check if the value is actually an array and throw otherwise

Both on* properties are optional, so !opts[key] is actually valid.


> >+var pageModManager = {
> >+  registeredPageMods: [],
> >+
> >+  _initialized: false,
> >+  init: function() {
> >+    this._initialized = true;
> 
> could make a memoizing getter in the module scope that inits and returns, to
> avoid having the apis have to check this for each and every call.

I think the idea here is to initialize lazily rather than when the module is required.  I don't think it's possible to be more efficient here via memoization, as register() will always have to try to trigger initialization.

At best, we could make the trigger code a bit simpler by having it invoke a getter and throw away its value, but the cost would be confusion, since it wouldn't be clear from reading register()'s code that initialization is taking place.

So I've left this as is.  But perhaps I'm missing something.  If so, let me know!


> >+  onContentGlobalCreated: function(wrappedWindow, topic, data) {
> >+    var self = this;
> >+    console.debug("--> PageMod running on page: <" +
> >+                  wrappedWindow.location.toString() + ">");
> 
> remove before check-in
> 
> >+    console.debug("<-- All PageMods finished for page: <" +
> >+                  wrappedWindow.location.toString() + ">");
> >+  }
> 
> ditto

I've commented these out rather than removing them, as it seems like they'll be useful to have in the future when debugging this module, and in the long run I'd like such statements to be active, with a log level setting determining whether or not to display them, so we can tell developers who report problems to lower the log level and send us their console output.


> >+/**
> >+ * Parses a string, possibly containing the wildcard character ('*') to
> >+ * create URL-matching rule. Supported input strings with the rules they
> >+ * create are listed below:
> >+ *  1) * (a single asterisk) - any URL with the http(s) or ftp scheme
> >+ *  2) *.domain.name - pages from the specified domain and all its subdomains,
> >+ *                     regardless of their scheme.
> >+ *  3) http://example.com/* - any URLs with the specified prefix.
> >+ *  4) http://example.com/test - the single specified URL
> >+ */
> >+function parseURLRule(url) {
> 
> should document the return value.

I have done so (and the parameter as well).


> also, there are a whole lot of conditions in there. might be more
> regression-proof to set the retval and return at the end, instead of all those
> early returns.

Sure, I have done so (along with some misc formatting changes to improve readability).


> >+exports.add = function(pageMod) {
> >+  pageModManager.register(pageMod);
> >+}
> >+exports.remove = function(pageMod) {
> >+  pageModManager.unregister(pageMod);
> >+}
> 
> why not just do, eg:
> 
> exports.add = pageModManager.register;

register() and unregister() assume |this| is pageModManager, so they have to be called on pageModManager, not |exports|, and these functions make sure that happens.  I have converted them to lambdas, however, to make them a bit shorter.

Comment 16

[Dietrich Ayala (:dietrich)]

Comment on attachment 472513 [details] [diff] [review]
patch v2: addresses review comments


>+Examples
>+--------
>+
>+Add content to a variety of pages:
>+
>+    var pageMod = require("page-mod");
>+    pageMod.add(new pageMod.PageMod({
>+      include: ["*.example.com",
>+                "http://example.org/a/specific/url",
>+                "http://example.info/*"],
>+      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>";
>+      }
>+    }));

ack, sorry i must have missed this in the last review.

the fact that our one and only example of pageMods has to expose the existence of wrappers really bums me out. we're doing something wrong if that's required.

what's the use-case for onStart? setting window scope globals? can we just auto-unwrap window for onStart, and document that the page isn't usable?

if there's absolutely no way around exposing the existence of wrappers in the example, you should document what wrappers are, why the developer must be subjected to them, and point to documentation about them for more information.

r=me with the above addressed somehow. if we have to get this in the tree such that devs have to be exposed to wrappers, so be it, but let's fix that before 0.8 is shipped.

Comment 17

[Nickolay_Ponomarev]

> has to expose the existence of wrappers really bums me out
There are two ways of working with a page: without affecting the page's JS and interacting with it. Unless you take away one of the ways you have to differentiate between them, and wrappers is our way to do that, however confusing...

For the reference, Chrome doesn't let the content script to access the page's JS objects.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: addresses review comments

(In reply to comment #16)
> the fact that our one and only example of pageMods has to expose the existence
> of wrappers really bums me out. we're doing something wrong if that's required.

This will be fixed by the E10S compatibility changes, as ContentSymbiont gives content scripts access to the unwrapped window and document objects.  Do you need me to fix it in this patch as well, or can it wait until we land the E10S compatibility changes?


> what's the use-case for onStart? setting window scope globals? can we just
> auto-unwrap window for onStart, and document that the page isn't usable?

Yup, onStart is about setting window scope globals before any script on the page has chance to run, so addons can experiment with new web platform features (like geolocation and camera access).

Once the E10S compatibility changes are done, addons will be able to load content scripts on start that have access to the unwrapped window object.


> if there's absolutely no way around exposing the existence of wrappers in the
> example, you should document what wrappers are, why the developer must be
> subjected to them, and point to documentation about them for more information.
>
> r=me with the above addressed somehow. if we have to get this in the tree such
> that devs have to be exposed to wrappers, so be it, but let's fix that before
> 0.8 is shipped.

I have added the following comment to the docs:

  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.

Is that sufficient to address the problem until we land the E10S compatibility changes?

Comment 19

[Dietrich Ayala (:dietrich)]

Comment on attachment 472741 [details] [diff] [review]
patch v3: addresses review comments

awesome, thanks for clarifying. that wfm, so r+.

one concern post-landing is that if the e10s changes are not ready in time for 0.8, i think we should consider turning off pagemods in that release. it's the api people seem most excited about. shipping it, and then changing the api in the release immediately following could rankle.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #19)
> one concern post-landing is that if the e10s changes are not ready in time for
> 0.8, i think we should consider turning off pagemods in that release. it's the
> api people seem most excited about. shipping it, and then changing the api in
> the release immediately following could rankle.

Yup, it's important to get the E10S-compatible API in place.  I'm not sure I would go so far as to disable the module, but if it isn't E10S-compatible, we can't call 0.8 "beta", which I really want to do.  So E10S-compatibility for the module is a top priority in the 0.8 dev plan.

Fixed by changeset https://hg.mozilla.org/labs/jetpack-sdk/rev/f64727add69b.

Thanks Nickolay for the great work here!  I'm looking forward to seeing what people do with it.

Comment 21

[Matěj Cepl]

(In reply to comment #19)
> Comment on attachment 472741 [details] [diff] [review]
> patch v3: addresses review comments
> 
> awesome, thanks for clarifying. that wfm, so r+.
> 
> one concern post-landing is that if the e10s changes are not ready in time for
> 0.8, i think we should consider turning off pagemods in that release. it's the
> api people seem most excited about. shipping it, and then changing the api in
> the release immediately following could rankle.

Also, how is this backwards compatible with FF 3.6.* ? Or are you throwing all your current users over the board?

Comment 22

[thanhbv]

Hi all,

I read about jetpack just 2 day ago.
I download & read docs of jetpack 0.7 => did not know about PageMode module.
So in the main.js file of my test extension, I use the following trick to attach scripts into page context:

===============
const tabs = require("tabs");
const contentSymbionts = require("content-symbiont");
const data = require("self").data;

exports.main = function(options, callbacks) {
  tabs.onOpen.add(attachContentScripts);
  attachContentScripts(tabs.activeTab);
}
function attachContentScripts(tab){
    contentSymbionts.ContentSymbiont({

//Note: I change tabs.js, add the following line:
//this.__defineGetter__("ownerBrowser", function() browser);
//in tabConstructor, bellow the line:
//let browser = win.gBrowser.getBrowserForTab(element);

//Note2: In jetpack 0.8pre, tabConstructor has been moved to tab-browser.js
        frame: tab.ownerBrowser,
        contentScriptURL: ["jquery-1.4.2.min.js", "test.js"].map(data.url),
        contentScriptWhen: "ready",
        onMessage: function onMessage(msg, callback) {
            switch (msg.method) {
                case "log":
                    console.log("onMessage", msg.arg);
                    break;
            }
        },
        globalName: "tab"
    });
}
===============

file test.js:
tab.sendMessage({method: "log", arg: "From test.js"});

===============

Then, I found PageMod module in jetpack trunk (revision 51becd21b53a)
But It's seem that PageMod is not fully implemented JEP 107.
I just think that my method above can be used in PageMod but not sure (& can't dig more into the bulk of jetpack's code :D).
So can you explain to me?

Comment 23

[thanhbv]

Can PageMod work with iframes? (modify and manipulate content document within iframes)

Comment 24

[Myk Melez [:myk] [@mykmelez]]

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".