621903 - Add a way to match related tab for each worker in page-mod API.

Status: RESOLVED FIXED

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

Description

[Alexandre Poirot [:ochameau]]

Attachment: Implementation proposal

I think current discussions in the jetpack groups on this feature are really a common missing feature:
http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/bfff7456a53bae1?hl=en
and
http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/fa913e9ea2be9813?hl=en

So I went ahead and implement one way to match tab for each page-mod worker.

We can't add this feature in tabs API 
(for example Tab.getTabFromWorker(worker)),
as Worker._window is private and has to be kept private!

So I added a relatedTab getter on worker,
which is going to return a Tab object only on Worker attached to a tab.
(ie will return null on page-worker, and workers binded to hidden iframes, extensions iframes, ...)

Because page-mod seems to handle ALL documents loaded in Firefox:
content, sidebar, widgets/panels/... documents and documents loaded in hidden window!

Improments/alternatives:
1/ I may move this relatedTab attribute from Worker.js to Page-mod.js.
By building an inline composition of Worker and this new getter, and so have a `TabWorker` which is the only one with this new attribute.
So we have former Worker as before, and new specific TabWorker for page-mod, with this new getter. (And in the same time, we may limit page-mod to only web content document)

2/ Another way to give this information is via onAttach:
let the first argument being the worker, 
and add a second one, being the tab.

3/ The key function is in api-utils/tabs/tab.js
let tab = require("tabs/tab").getTabForWindow(window);
I'm not sure it's the best place to be, I had to add chrome dependencie for this module.

4/ Note that I added in the same time: Worker.documentURL

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Add a test that demonstrate page-mod working on hidden window documents!

Comment 2

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

Comment on attachment 500196 [details] [diff] [review]
Implementation proposal

I like it!  I would just call it simply "tab" rather than "relatedTab".  And while it isn't strictly necessary, it'd be great if the final patch to implement the feature also came with an example addon that demonstrated a use case for having the feature!

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Modifications after feedback + fixes

I've renamed worker.relatedTab to worker.tab and fix some bugs discovered with real world usecases!

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Implementation for worker.documentURL

As this bug is not related to this modification, I've done a seperate patch for adding worker.documentURL attribute.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: An example of use of worker.tab attribute

Here is a really simple use of this new attribute.
It's an extension that pin automatically of tabs opened on Mozilla's websites.
We could easily powerup this first example to be closer to real world extension.

Comment 6

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

(In reply to comment #4)
> Created attachment 504783 [details] [diff] [review]
> Implementation for worker.documentURL
> 
> As this bug is not related to this modification, I've done a seperate patch for
> adding worker.documentURL attribute.

Can you file a new bug for this change?  Since this is a separate enhancement, it would be better to track its status and resolution via its own bug report.

Comment 7

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

Comment on attachment 504779 [details] [diff] [review]
Modifications after feedback + fixes

diff --git a/packages/api-utils/lib/tabs/tab.js b/packages/api-utils/lib/tabs/tab.js

+  if (topContentWindow.wrappedJSObject)
+    topContentWindow.wrappedJSObject.__jetpackTabId=uid;
+  else
+    topContentWindow.__jetpackTabId=uid;

Hmm, this is going to expose the __jetpackTabId property to content, which is a problem, as it lets content know that it was modded.

What kind of wrappers were preventing you from comparing windows directly, and is there perhaps some way to unwrap the window objects and then compare them?

Comment 8

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

Comment on attachment 504783 [details] [diff] [review]
Implementation for worker.documentURL

This seems fine, except that we've been calling such properties simply "url" in other APIs, and we should make this one consistent.

r=myk with that change!

Comment 9

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

Comment on attachment 504784 [details] [diff] [review]
An example of use of worker.tab attribute

Hmm, wouldn't it be simpler to register a tabs.onReady listener that does this without using a page mod?  Of course the purpose here is to demonstrate this feature of page mods, I'm just a bit worried that we'll be inadvertently suggesting to addon developers that they should use a page mod in a situation where it is not warranted.

Ideally, we'd have an example in which the use of the page mod is the best option.  Or maybe I misunderstand the example, and that's the case here?

Comment 10

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

Comment on attachment 504784 [details] [diff] [review]
An example of use of worker.tab attribute

>diff --git a/examples/pagemod-example/README.md b/examples/pagemod-example/README.md

>+This is the pagemod-example add-on.  
>\ No newline at end of file

Nit: add a newline to the end of this file.


>diff --git a/examples/pagemod-example/docs/main.md b/examples/pagemod-example/docs/main.md

>+The main module is a program that watch for new tabs opened on mozilla websites
>+and automatically pin these tabs. They are unpinned when you visit another website.

Nit: watch -> watches, mozilla -> Mozilla, pin -> pins


>diff --git a/examples/pagemod-example/lib/main.js b/examples/pagemod-example/lib/main.js

>+      // Then we catch for futher location change to unpin it 
>+      // if we go off from mozilla

Nit: catch -> watch, futher -> further, go off from mozilla -> leave the Mozilla website.


>diff --git a/examples/pagemod-example/package.json b/examples/pagemod-example/package.json

>+    "id": "jid0-odZnbNd6i36qKsqvB3AgsC7np8k", 

This needs to have an anonymous ID like the other examples (i.e. anonid0-pagemod-example) in order for SDK users to be able to try it out easily (otherwise they have to edit this file to try it).


>diff --git a/examples/pagemod-example/tests/test-main.js b/examples/pagemod-example/tests/test-main.js

>+exports.test_open_mozilla_tab = function(test) {

Nit: in other test files, we have either used the testStudlyCaps or ["test brackets with spaces"] naming conventions when defining test functions on the exports object, and we should stick to those conventions in example apps too, i.e. either do:

  exports.testOpenMozillaTab = function(test) {

Or:

  exports["test open Mozilla tab"] = function(test) {


>+  test.waitUntilDone(10000);

It's probably better not to specify the timeout here, as the default timeout should be sufficient, and I expect we'll want users who are using this code as the basis for their own addons to use the default timeout most of the time.

Otherwise, looks great!  (I'm still a bit concerned that this might not be exactly the right example, but perhaps I just misunderstand.)

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to comment #8)
> Comment on attachment 504783 [details] [diff] [review]
> Implementation for worker.documentURL
> 
> This seems fine, except that we've been calling such properties simply "url" in
> other APIs, and we should make this one consistent.
> 
> r=myk with that change!

I renamed it and attached a patch in another bug https://bugzilla.mozilla.org/show_bug.cgi?id=630143

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to comment #7)
> Comment on attachment 504779 [details] [diff] [review]
> Modifications after feedback + fixes
> 
> diff --git a/packages/api-utils/lib/tabs/tab.js
> b/packages/api-utils/lib/tabs/tab.js
> 
> +  if (topContentWindow.wrappedJSObject)
> +    topContentWindow.wrappedJSObject.__jetpackTabId=uid;
> +  else
> +    topContentWindow.__jetpackTabId=uid;
> 
> Hmm, this is going to expose the __jetpackTabId property to content, which is a
> problem, as it lets content know that it was modded.
> 
> What kind of wrappers were preventing you from comparing windows directly, and
> is there perhaps some way to unwrap the window objects and then compare them?

I try hard to unwrap these XRayWrappers without success! This may to be related to Compartments ...
But I found another solution used by greasemonkey "nsIDOMWindowUtils.currentInnerWindowID", that seems to work pretty well!

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Use nsIDOMWindowUtils.currentInnerWindowID instead of adding attributes to content's window objects

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: page-mod example with worker.tab

(In reply to comment #10)
> Comment on attachment 504784 [details] [diff] [review]
> An example of use of worker.tab attribute
> 
> >diff --git a/examples/pagemod-example/README.md b/examples/pagemod-example/README.md
> 
> >+This is the pagemod-example add-on.  
> >\ No newline at end of file
> 
> Nit: add a newline to the end of this file.
> 
> 
> >diff --git a/examples/pagemod-example/docs/main.md b/examples/pagemod-example/docs/main.md
> 
> >+The main module is a program that watch for new tabs opened on mozilla websites
> >+and automatically pin these tabs. They are unpinned when you visit another website.
> 
> Nit: watch -> watches, mozilla -> Mozilla, pin -> pins
> 
> 
> >diff --git a/examples/pagemod-example/lib/main.js b/examples/pagemod-example/lib/main.js
> 
> >+      // Then we catch for futher location change to unpin it 
> >+      // if we go off from mozilla
> 
> Nit: catch -> watch, futher -> further, go off from mozilla -> leave the
> Mozilla website.
> 
> 
> >diff --git a/examples/pagemod-example/package.json b/examples/pagemod-example/package.json
> 
> >+    "id": "jid0-odZnbNd6i36qKsqvB3AgsC7np8k", 
> 
> This needs to have an anonymous ID like the other examples (i.e.
> anonid0-pagemod-example) in order for SDK users to be able to try it out easily
> (otherwise they have to edit this file to try it).
> 
> 
> >diff --git a/examples/pagemod-example/tests/test-main.js b/examples/pagemod-example/tests/test-main.js
> 
> >+exports.test_open_mozilla_tab = function(test) {
> 
> Nit: in other test files, we have either used the testStudlyCaps or ["test
> brackets with spaces"] naming conventions when defining test functions on the
> exports object, and we should stick to those conventions in example apps too,
> i.e. either do:
> 
>   exports.testOpenMozillaTab = function(test) {
> 
> Or:
> 
>   exports["test open Mozilla tab"] = function(test) {
> 
> 
> >+  test.waitUntilDone(10000);
> 
> It's probably better not to specify the timeout here, as the default timeout
> should be sufficient, and I expect we'll want users who are using this code as
> the basis for their own addons to use the default timeout most of the time.
> 
> Otherwise, looks great!  (I'm still a bit concerned that this might not be
> exactly the right example, but perhaps I just misunderstand.)

I've updated the example according to your comments.
I had the same thought about this example. We would definitively prefer tabs API over page-mod for this particular feature.

The problem is that we can't do that much with a tab object, for now. We can pin/unpin it, get its url/title/favicon and move it. 
We have to wait for an idea that mix:
1/ one action in content: match a particular content or event in a webpage,
2/ one action in the related tab: pin it or implements style attribute in order to highlight it

I had the idea to change color of a tab on some content event like new mail in gmail, on some DOMSubtreeModified events, ...

Comment 15

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

Comment on attachment 508361 [details] [diff] [review]
Use nsIDOMWindowUtils.currentInnerWindowID instead of adding attributes to content's window objects

Almost there!  A few more issues...


diff --git a/packages/addon-kit/tests/test-tabs.js b/packages/addon-kit/tests/test-tabs.js

  -
  +/*
   // test tab.activeTab getter
   exports.testActiveTab_getter = function(test) {
     test.waitUntilDone();
  @@ -490,6 +490,50 @@ exports.testPerTabEvents = function(test) {
       });
     });
   };
  +*/

These tests were presumably commented out to ease development of the patch but will need to be uncommented in the version that lands.


  +        function waitForFrames() {
  +          let iframe = window.content.document.getElementById("iframe");
  +          if (!iframe) return timer.setTimeout(waitForFrames, 100);
  +          let iframeWin = iframe.contentWindow;
  +          let subIframe = iframeWin.document.getElementById("sub-iframe");
  +          if (!subIframe) return timer.setTimeout(waitForFrames, 100);
  +          let subIframeWin = subIframe.contentWindow;
  +          let subSubIframe = subIframeWin.document.getElementById("sub-sub-iframe");
  +          if (!subSubIframe) return timer.setTimeout(waitForFrames, 100);
  +          let subSubIframeWin = subSubIframe.contentWindow;

waitForFrames triggers a classic JavaScript strict warning that the function does not always return a value, since it returns values early under certain conditions but doesn't return a value at the end of the function.  The best fix would be to avoid returning values when returning early, i.e.:

  function waitForFrames() {
    let iframe = window.content.document.getElementById("iframe");
    if (!iframe) {
      timer.setTimeout(waitForFrames, 100);
      return;
    }
  
    let iframeWin = iframe.contentWindow;
    let subIframe = iframeWin.document.getElementById("sub-iframe");
    if (!subIframe) {
      timer.setTimeout(waitForFrames, 100);
      return;
    }
  
    let subIframeWin = subIframe.contentWindow;
    let subSubIframe = subIframeWin.document.getElementById("sub-sub-iframe");
    if (!subSubIframe) {
      timer.setTimeout(waitForFrames, 100);
      return;
    }

    ...

Doing this makes the code longer, to be sure, but it also has the side-effect of making it clearer, which is itself beneficial.


  +          let matchedTab = tabAPI.getTabForWindow(iframeWin);
  +          test.assertEqual(matchedTab, tab, "We are able to find the tab with an iframe window object");
  +          
  +          let matchedTab = tabAPI.getTabForWindow(subIframeWin);
  +          test.assertEqual(matchedTab, tab, "We are able to find the tab with a sub-iframe window object");
  +          
  +          let matchedTab = tabAPI.getTabForWindow(subSubIframeWin);
  +          test.assertEqual(matchedTab, tab, "We are able to find the tab with a sub-sub-iframe window object");

matchedTab should only be declared with a let definition the first time it is assigned; subsequent assignments should leave out the "let" keyword, i.e.:

  let matchedTab = tabAPI.getTabForWindow(iframeWin);
  test.assertEqual(matchedTab, tab, "We are able to find the tab with an iframe window object");
  
  matchedTab = tabAPI.getTabForWindow(subIframeWin);
  test.assertEqual(matchedTab, tab, "We are able to find the tab with a sub-iframe window object");
  
  matchedTab = tabAPI.getTabForWindow(subSubIframeWin);
  test.assertEqual(matchedTab, tab, "We are able to find the tab with a sub-sub-iframe window object");


Also, a couple more things I missed in the initial review:

1. This API enhancement should be documented somewhere.  Unfortunately, it isn't clear where, since there is no documentation for Worker objects specifically.  Perhaps in packages/addon-kit/docs/page-mod.md or static-files/md/dev-guide/addon-development/web-content.md?  Requesting feedback from Will on this.

2. It would be nice if there were an explicit api-utils test for getTabForWindow rather than just the implicit test provided by the addon-kit test.  It looks like there aren't any api-utils tabs/tab.js tests, though, so this is ok as it is for now.

Comment 16

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

Comment on attachment 508365 [details] [diff] [review]
page-mod example with worker.tab

Looks good!  r=myk

Comment 17

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

(In reply to comment #15)
> 1. This API enhancement should be documented somewhere.  Unfortunately, it
> isn't clear where, since there is no documentation for Worker objects
> specifically.

Erm, I'm wrong, there actually is documentation of Worker objects in packages/api-utils/docs/content/worker.md, so documentation for the tab getter could go in there (although ideally we'd also incorporate it into the high-level API docs at some point).

Comment 18

[Will Bamberg [:wbamberg]]

> 1. This API enhancement should be documented somewhere.  Unfortunately, it
> isn't clear where, since there is no documentation for Worker objects
> specifically.  Perhaps in packages/addon-kit/docs/page-mod.md or
> static-files/md/dev-guide/addon-development/web-content.md?  Requesting
> feedback from Will on this.

There is, as you say, documentation on worker already, and the existence of this API should be documented there too. But as I understand it this change is primarily designed to work with page-mod, so something more high level would be really (more?) useful.

The page-mod doc already has stuff that talks about how you get multiple workers when multiple pages are matched. So it seems very natural to extend that by talking about how you could map, in the add-on code, from the page to the particular worker.

I'll raise a bug and take this on if someone will review it for me.

As for examples, the use case ehsnaul describes in http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/bfff7456a53bae1?hl=en&pli=1 seems to me as if it would be a good one.

"I just want to be able to run some JS code in the context of the web
page the user is focused on, on demand. So I have a widget set up,
which is supposed to make this happen when clicked."

I came across what (I think) is the same problem with the annotator. It has a widget that you can use to switch it on. Switching it on means that a content script highlights annotatable items in the page. If I have multiple tabs open it might be nice if switching it on only operated on the active tab. But it's not obvious how I can do that with the current APIs (so currently the annotator on/off operates globally).

So an example showing something like that would be really useful: a pattern showing how you could identify the current 'active worker'. Then the page-mod doc could talk through the example.

(Unless I'm confused, and you can do all this already without the new API).

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: Worker.tab + unittests + doc

I moved previous test from addon-kit/test-tabs to api-utils/test-tab
and added a more high-level one in test-page-mod.
In the same time I converted test-tabs to strict mode.
I've added a really simple note in api-utils/docs/content/worker
as Will seems to write a nicer one in page-mod.

Comment 20

[Alexandre Poirot [:ochameau]]

Attachment: Worker.tab + unittests + doc

Oops forgot to add new test file!

Comment 21

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

Comment on attachment 512136 [details] [diff] [review]
Worker.tab + unittests + doc

This looks good, except that I don't think api-utils tests should depend on addon-kit modules.  Can test-tab.js be made to use api-utils' tab-browser API to open a tab instead of addon-kit's tabs API?

Comment 22

[Alexandre Poirot [:ochameau]]

tabs and windows separation between addon-kit/api-utils is very messy.
I think there is a cleaning work in progress that is still not finished.
It's almost impossible to instanciate anything from api-utils side without just doing what is done in addon-kit. (addon-kit/lib/tabs.js is a good example)
We could open a tab manually and create a Tab object manually too,
but Tab is waiting for a "addon-kit" window object in his contructor.

So we have to choose if the priority is to move this test to api-utils because it tests something written in api-utils or move it to addon-kit because it's using addon-kit in order to check values.

Having said that, I'm convinced that we could have a really more simple windows/tabs code that would benefit to e10s, api-utils/addon-kit split and some stuff like anchor. My first idea is to have almost the same capabilities/features/API in addon-kit and api-utils except that api-utils would expose nodes and chrome objects. So api-utils would be in chrome process and addon-kit version in addon process.

Comment 23

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

(In reply to comment #22)
> tabs and windows separation between addon-kit/api-utils is very messy.
> I think there is a cleaning work in progress that is still not finished.
> It's almost impossible to instanciate anything from api-utils side without just
> doing what is done in addon-kit.

Yeah, I can imagine that there are many messy interdependencies it the existing code.  Nevertheless, it seems like it should be possible to use tab-browser in this case, no?  I.e. you could do:

  require("tab-browser").addTab(url, {
    onLoad: function() {
      ...
    }
  });

But perhaps I misunderstand.  Is that problematic in some way?

Comment 24

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

(In reply to comment #23)
> But perhaps I misunderstand.

I did misunderstand.  Alex pointed out to me that the api-utils tabs/tab API actually returns an addon-kit tab, which is what the test needs to compare to the tab being returned by the worker.  So the test does need to use the addon-kit tab module here.  So the current patch is fine, although we should circle around to this confusing mixture of the two packages at a later date.

Comment 25

[Alexandre Poirot [:ochameau]]

Myk: do you expect something on this bug before pushing the last 512136 patch ?
I supposed you didn't commit it because of closed tree but just to be sure we land this patch :)

Comment 26

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

Argh, I meant to check this in for 1.0b3 but forgot to do so. :-(

1.0b4 it is:

https://github.com/mozilla/addon-sdk/commit/9a94255c328fa32611b5108edce1744f611a7a16