Skip to content

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

Closed
wants to merge 11 commits into from

4 participants

@ochameau
Mozilla member

Here is random comments that can be usefull for review:

  • I've added a new apply attribute that works like allow attribute of symbiont. I've done that as we are planning to add similar flags in order to attach content script only on top level document (i.e. ignore iframes)
  • page-mod contains a small utility method that allows to list all opened tabs. I tried using tab-browser but it ended up being messy and most likely very badly efficient. As I don't know we want to document and maintain this method I've kept it as a page-mod private method. I've used yield to match window-utils behavior but I can use regular iteration too.

I'm waiting for feedback about the public API before writing docs.

@Gozala Gozala and 3 others commented on an outdated diff May 8, 2012
packages/addon-kit/docs/page-mod.md
@@ -367,6 +367,12 @@ Creates a PageMod.
option are loaded *after* those specified by the `contentStyleFile` option.
Optional.
+ @prop [target] {array}
@Gozala
Mozilla member
Gozala added a note May 8, 2012

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.

@Gozala
Mozilla member
Gozala added a note May 16, 2012

@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 ?

@wbamberg
wbamberg added a note May 17, 2012

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

@Gozala
Mozilla member
Gozala added a note May 17, 2012

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.

@Mossop
Mozilla member
Mossop added a note May 17, 2012

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?

@ochameau
Mozilla member
ochameau added a note May 28, 2012

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on an outdated diff May 8, 2012
packages/addon-kit/docs/page-mod.md
@@ -367,6 +367,12 @@ Creates a PageMod.
option are loaded *after* those specified by the `contentStyleFile` option.
Optional.
+ @prop [target] {array}
+ Option to specify on which documents should apply the PageMod.
@Gozala
Mozilla member
Gozala added a note May 8, 2012

"which documents PageMod should be applied" is a correct from I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala and 1 other commented on an outdated diff May 8, 2012
packages/addon-kit/lib/page-mod.js
@@ -172,6 +181,19 @@ const PageMod = Loader.compose(EventEmitter, {
_loadingWindows: [],
+ _applyOnExistingDocuments: function _applyOnExistingDocuments() {
+ for each(let tab in allTabsIterator()) {
+ for each(let rule in this.include) {
@Gozala
Mozilla member
Gozala added a note May 8, 2012

could you write utility function like isMatching(tab.url, this.include) instead, that way you won't need to beak and it would by way easier to follow. It took me a while to figure out what was going on here.

@Gozala
Mozilla member
Gozala added a note May 8, 2012

So it will be like:

if (isMatching(tab.url, this.include))
  this._onContent(tab.content);
@ochameau
Mozilla member
ochameau added a note May 16, 2012

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on an outdated diff May 8, 2012
packages/addon-kit/lib/page-mod.js
@@ -321,3 +348,29 @@ const PageModManager = Registry.resolve({
}
});
const pageModManager = PageModManager();
+
+// Iterate over all tabs on all currently opened windows
+function allTabsIterator() {
+ // 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++) {
@Gozala
Mozilla member
Gozala added a note May 8, 2012

Please add a space before (.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala and 1 other commented on an outdated diff May 8, 2012
packages/addon-kit/lib/page-mod.js
@@ -321,3 +348,29 @@ const PageModManager = Registry.resolve({
}
});
const pageModManager = PageModManager();
+
+// Iterate over all tabs on all currently opened windows
+function allTabsIterator() {
@Gozala
Mozilla member
Gozala added a note May 8, 2012

Do you really need an iterator here ? I think plain arrays map / reduce would work just fine here as you iterate over each tab anyway.
Also it won't require non-standard features making it easier for newcomers.

@ochameau
Mozilla member
ochameau added a note May 16, 2012

I totally agree. I used iterator to match existing window-utils iterators.
I removed it in favor of an array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala and 1 other commented on an outdated diff May 8, 2012
packages/addon-kit/lib/page-mod.js
+ 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;
+ yield {
+ tab: tab,
+ browser: browser,
@Gozala
Mozilla member
Gozala added a note May 8, 2012

You only seem to use uri and content so what's a point of adding tab and browser and use of getters are even more confusing.

@ochameau
Mozilla member
ochameau added a note May 16, 2012

I removed attributes that weren't used, and removed the use of getters.
It would make sense to use getters if we expose this. tab and browser never change, whereas uri and content can, that's why getter end up being important if usecode keep a reference to such tab object.
But as it isn't usefull for page-mod code, I removed them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala and 1 other commented on an outdated diff May 8, 2012
packages/addon-kit/lib/page-mod.js
+ 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;
+ yield {
+ tab: tab,
+ browser: browser,
+ get uri() browser.currentURI.spec,
+ get content() browser.contentWindow
+ };
+ }
+ }
+}
@Gozala
Mozilla member
Gozala added a note May 8, 2012

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)
@ochameau
Mozilla member
ochameau added a note May 16, 2012

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

@Gozala
Mozilla member
Gozala added a note May 16, 2012

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.

@Gozala
Mozilla member
Gozala added a note May 16, 2012

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on an outdated diff May 16, 2012
packages/addon-kit/lib/page-mod.js
@@ -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);
+ }
+ }
@Gozala
Mozilla member
Gozala added a note May 16, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala and 1 other commented on an outdated diff May 16, 2012
packages/addon-kit/lib/page-mod.js
@@ -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') ) {
@Gozala
Mozilla member
Gozala added a note May 16, 2012

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

@ochameau
Mozilla member
ochameau added a note May 28, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala
Mozilla member
Gozala commented May 17, 2012

To summarize I'm ok with this change once naming issue is resolved

https://github.com/mozilla/addon-sdk/pull/404/files#r835814

Feel free to decide if you want to address other comments or ask for another review if you desire. Otherwise r+ by with target property renamed.

@ochameau
Mozilla member

@gozala, I followed the trend by using more code from tab/utils, but I had to simplify it a bit.
I don't think it was a good idea to support multiple tabbrowser, it may even lead to unexpected behavior if some addon adds new ones.

Otherwise, the naming issue is quite tough, no one came up with ideal name that makes sense without conflict.
From what I can summarize from https://etherpad.mozilla.org/new-page-mod-options there is two option in debate:
target which conflict with some common html meaning, or attachTo which may conflict with https://bugzilla.mozilla.org/show_bug.cgi?id=755963

Ideally we would use attachTo here and find something distinct for bug 755963. Otherwise I think that we can't forbid us from using such common word as target because of its usage in html.

In https://github.com/mozilla/addon-sdk/wiki/JEP-Content-scripts you tend to suggest this attachTo method to be defined on ContentScript class, not PageMod one. You named it spawn. So would it be ok to use PageMod.attachTo and ContentScript.attach/spawn/attachTo/execute/runOn... ?

@Mossop
Mozilla member
Mossop commented Aug 28, 2012

attachTo wins

@Gozala Gozala was assigned Aug 29, 2012
ochameau added some commits Aug 29, 2012
@ochameau ochameau Merge branch 'master' into bug/708190-apply-page-mod-on-existing-docs
Conflicts:
	packages/addon-kit/lib/page-mod.js
	packages/api-utils/lib/tabs/utils.js
88c3207
@ochameau ochameau Last review comments. a65c4a4
@ochameau
Mozilla member
ochameau commented Sep 6, 2012

Manually landed here: 876e9d7

@ochameau ochameau closed this Sep 6, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.