Skip to content
This repository has been archived by the owner on Feb 26, 2022. It is now read-only.

Bug 708190: Apply page-mod on existing documents. #404

6 changes: 6 additions & 0 deletions packages/addon-kit/docs/page-mod.md
Expand Up @@ -367,6 +367,12 @@ Creates a PageMod.
option are loaded *after* those specified by the `contentStyleFile` option.
Optional.

@prop [target] {array}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in bugzilla I don't think target is a good property name here, it's used for many different things in different contexts, most commonly as a an event target. Also commonly to refer to a target element. In this case it has totally different meaning that may be confusing. I can also imagine that in a future we may use target property limit mods for tabs of the specific window for example.

I have suggested to use name handle or cover instead. If you dislike those names maybe we could find something better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ochameau mentioned on IRC that he does not likes names I have proposed. @Mossop @wbamberg what do you think ? Do they read awkward ? Maybe you have a better suggestions ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"handle" and "cover" both sound very generic to me. As I said yesterday on IRC, the best I can think of is "attachTo". I like the way it reads like a complete command: 'attachTo:["existing"]'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also make sure that it will work for other possible values too: attachTo: [ "existing", "top", "frames" ], which reads bit awkward to me, but I'm non native so I could be wrong. Also attachTo conflicts something else I had in mind for bug 755963 maybe we should decide on naming there as well to make sure we won't run into naming issues again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

target already has a meaning in the web which seems close enough to what we're talking about here that we should avoid it to save confusion. attachTo reads well for all of the cases to me, but unless there is a better option for Bug 755963 we should avoid it. I guess applyTo is too similar as well. I'm traditionally poor a picking names out of my head, but maybe windowTypes or something along those lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mossop I have the same limitation when it comes to names!
documentType may be an option. window sounds weird to me as there is no windows. I don't know for you but the word window is only associated to top level window for me. And singular as it looks like we are always using singular? (contentScript, contentScriptFile, ...)
@Gozala @wbamberg your thoughts?

Option to specify on which documents PageMod should be applied.
For now, it only accepts one value: "existing". If `target` contains
"existing", the PageMod will be automatically applied on already opened
tabs.

@prop [onAttach] {function}
A function to call when the PageMod attaches content scripts to
a matching page. The function will be called with one argument, a `worker`
Expand Down
59 changes: 58 additions & 1 deletion packages/addon-kit/lib/page-mod.js
Expand Up @@ -16,6 +16,7 @@ const { validateOptions : validate } = require('api-utils/api-utils');
const { validationAttributes } = require('api-utils/content/loader');
const { Cc, Ci } = require('chrome');
const { merge } = require('api-utils/utils/object');
const { windowIterator } = require("window-utils");

// Whether or not the host application dispatches a document-element-inserted
// notification when the document element is inserted into the DOM of a page.
Expand Down Expand Up @@ -98,6 +99,7 @@ function readURI(uri) {
const PageMod = Loader.compose(EventEmitter, {
on: EventEmitter.required,
_listeners: EventEmitter.required,
target: [],
contentScript: Loader.required,
contentScriptFile: Loader.required,
contentScriptWhen: Loader.required,
Expand All @@ -121,6 +123,8 @@ const PageMod = Loader.compose(EventEmitter, {
this.on('attach', options.onAttach);
if ('onError' in options)
this.on('error', options.onError);
if ('target' in options)
this.target = options.target;

let include = options.include;
let rules = this.include = Rules();
Expand Down Expand Up @@ -154,6 +158,11 @@ const PageMod = Loader.compose(EventEmitter, {
pageModManager.add(this._public);

this._loadingWindows = [];

// `_applyOnExistingDocuments` has to be called after `pageModManager.add()`
// otherwise its calls to `_onContent` method won't do anything.
if ('target' in options && options.target.indexOf('existing') !== -1)
this._applyOnExistingDocuments();
},

destroy: function destroy() {
Expand All @@ -172,6 +181,23 @@ const PageMod = Loader.compose(EventEmitter, {

_loadingWindows: [],

_applyOnExistingDocuments: function _applyOnExistingDocuments() {
// Returns true if the URL match one rule
function isMatchingURL(uri, rules) {
for each(let rule in rules) {
if (RULES[rule].test(uri))
return true;
}
return false;
};
for each (let tab in getAllTabs()) {
if (isMatchingURL(tab.uri, this.include)) {
// Fake a newly created document
this._onContent(tab.content);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again simple .filter(isMatchingURL).forEach(..) is easier to read and maintain + is not moz specific.

},

_onContent: function _onContent(window) {
// not registered yet
if (!pageModManager.has(this))
Expand All @@ -183,7 +209,12 @@ const PageMod = Loader.compose(EventEmitter, {
this._loadingWindows.push(window);
}

if ('start' == this.contentScriptWhen) {
// Immediatly evaluate content script if the document state is already
// matching contentScriptWhen expectations
let state = window.document.readyState;
if ('start' == this.contentScriptWhen ||
'complete' == state ||
('ready' == this.contentScriptWhen && state == 'interactive') ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're stick to === convention unless there is a reason to prefer ==.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No particular reason, just used to type == ;)

this._createWorker(window);
return;
}
Expand Down Expand Up @@ -321,3 +352,29 @@ const PageModManager = Registry.resolve({
}
});
const pageModManager = PageModManager();

// Iterate over all tabs on all currently opened windows
function getAllTabs() {
let tabList = [];
// Iterate over all chrome windows
for (let window in windowIterator()) {
// Get a reference to the main <xul:tabbrowser> node
let tabbrowser = window.document.getElementById("content");
// It may not be a browser window but a jsconsole ...
if (!tabbrowser)
continue;
let tabs = tabbrowser.tabContainer;
if (!tabs)
continue;
// Iterate over its tabs
for (let i = 0; i < tabs.children.length; i++) {
let tab = tabs.children[i];
let browser = tab.linkedBrowser;
tabList.push({
uri: browser.currentURI.spec,
content: browser.contentWindow
});
}
}
return tabList;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would very much prefer to have more utility functions than this complex iterator. For example we already have utility function for getting window tabs:
https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/tabs/utils.js#L23

I think what we need is:

  1. window/utils.getWindows to get all windows
  2. getBrowser(tab) -> tab.linkedBrowser
  3. getWindow(tab) -> getBrowser(tab).contentWindow
  4. getURI(tab) -> getBrowser(tab).currentURI.spec

Than in _applyOnExistingDocuments you will just do:

getWindows().
  reduce(function(tabs, window) { return tabs.concat(getTabs(window)); }, []).
  filter(function(tab) { return isMatching(getURI(tab), this.include)); }, this).
  forEach(function(tab) { this._onContent(getWindow(tab); }, this)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still kept this internal method. I can move it to tabs/utils but I can't use existing methods as-is.
getTabs for example is inefficient. The overuse of reduce and filter is very inefficient too.
So I can see multiple options here:

  • keep it as-is: an internal method in page-mod
  • move it as-is to tabs/utils
  • move it to tabs/utils and try to use some methods from this module. I would have to modify getTabs and avoid using reduce.

I'm extra carefull about performances here as this code is going to be executed on any new content document (once per installed addon using page-mod!), so that it is going to slow down web pages loading. We have to lower this effect as much as possible.

FYI, performances of reduce http://jsperf.com/eval-join-vs-reduce/3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is sign of premature optimization at cost of code maintainability. According to that benchmark reduce took 0.00323918114 ms. In addition these benchmarks does not really apply to this case as since they compare simple array
iterations which is different from our case where we wrap xpcom simple enumerator with JS iterator wrapped in yet another JS iterator + access to different getters. Cost of reduce here is lost in cost of other more expensive operations and is completely irrelevant. In addition it's known that built-in map / filter / reduce is much slower than hand written one bug 743634 also visible here http://jsperf.com/eval-join-vs-reduce/4 and finally JS team doing some awsome work on function inlineing which will be able to compile such filter / map / reduce to a same thing as plain for loop so we better let them do optimizations. We can start doing targeted optimizations once that will be a bottleneck.

That all being said I'm ok with keeping it as an internal utility function for the time being.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew that operating on iterators was slower than regular array operations and your comment above irritated my curiosity so I tried following https://gist.github.com/2714933 as it turns out .filter(...).forEach(...) is indeed faster. That not to say I changed my mind, you still can go ahead with internal utility function & utility functions I suggested earlier can be added in a future.

23 changes: 23 additions & 0 deletions packages/addon-kit/tests/test-page-mod.js
Expand Up @@ -347,6 +347,29 @@ exports.testRelatedTab = function(test) {

};

exports.testWorksWithExistingTabs = function(test) {
test.waitUntilDone();

let url = "data:text/html," + encodeURI("Test unique document");
let { PageMod } = require("page-mod");
tabs.open({
url: url,
onReady: function onReady(tab) {
let pageMod = new PageMod({
include: url,
target: ["existing"],
onAttach: function(worker) {
test.assertEqual(tab, worker.tab, "A worker has been created on this existing tab");
pageMod.destroy();
tab.close();
test.done();
}
});
}
});

};

exports['test tab worker on message'] = function(test) {
test.waitUntilDone();

Expand Down