622077 - Create a worker from a Tab

Status: RESOLVED FIXED

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

Description

[Alexandre Poirot [:ochameau]]

Attachment: Implementation proposal

Currently the only way to create a Worker for content documents is to use Page-mod and match these documents on their URL.
But a common use of worker or content modification is based on activeTab or on more complex browser UI events.
So it could be very handy to have a way to create a Worker from a Tab object.

This feature request follow up this group discussion:
http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/bfff7456a53bae1?hl=en

Here is an implementation proposal, that add Tab.createWorker(workerOptions).
I added this method in Tab in order to have access to Tab contentWindow, for worker creation.

Comment 1

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

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

I like it!  A couple questions:

1. When does the content script get executed if the page in the tab has already finished loading?  Both contentScriptWhen "start" and "ready" have already happened at that point.  It seems like the content script should get executed immediately in that case, disregarding the value of contentScriptWhen.  Is that what happens?  There should be a test to verify the expected behavior in that case.

2. Does the worker remain valid after the current page is unloaded and get re-executed when the next page is loaded in the tab, or does it become invalid at that point?  It's not yet clear to me whether the use cases for this feature are tab-centric (i.e. they are about modifying all pages loaded in a particular tab), in which case it would make more sense for the worker to remain valid; or page-centric (i.e. they are about modifying the page that is currently loaded in a particular tab), in which case it would make more sense for the worker to become invalid.


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

>+            contentScript : 'postMessage(document.location.href)',
>+            contentScriptWhen: 'ready',
>+            onMessage : function (msg) {

Nit: consistently omit a space before the colon in property definitions.


Also, I would call this "modify" rather than "createWorker", to emphasize that this method allows you to modify a page in the tab in the same way that Page Mods allows you to modify all pages matching a given URL (and to de-emphasize the word "worker", which is quite unrelated to the task that the consumer is performing).


Finally, this needs some documentation in packages/addon-kit/docs/tabs.md.

Comment 2

[Alexandre Poirot [:ochameau]]

(In reply to comment #1)
> Comment on attachment 500324 [details] [diff] [review]
> Implementation proposal
> 
> I like it!  A couple questions:
> 
> 1. When does the content script get executed if the page in the tab has already
> finished loading?  Both contentScriptWhen "start" and "ready" have already
> happened at that point.  It seems like the content script should get executed
> immediately in that case, disregarding the value of contentScriptWhen.  Is that
> what happens?  There should be a test to verify the expected behavior in that
> case.
In fact there is not contentScriptWhen, I just overused copy'n paste!
The worker is executed immediatly in the current tab document.

> 
> 2. Does the worker remain valid after the current page is unloaded and get
> re-executed when the next page is loaded in the tab, or does it become invalid
> at that point?  It's not yet clear to me whether the use cases for this feature
> are tab-centric (i.e. they are about modifying all pages loaded in a particular
> tab), in which case it would make more sense for the worker to remain valid; or
> page-centric (i.e. they are about modifying the page that is currently loaded
> in a particular tab), in which case it would make more sense for the worker to
> become invalid.
> 
So this worker is executed only into the current document; it will not be used for the next one.
If you would like to execute a worker for each document loaded in a tab, you have to create a new worker on each ready event. Like this:
  tab.on('ready',function(tab){
    tab.modify({ ... });
  });

> 
> >diff --git a/packages/addon-kit/tests/test-tabs.js b/packages/addon-kit/tests/test-tabs.js
> 
> >+            contentScript : 'postMessage(document.location.href)',
> >+            contentScriptWhen: 'ready',
> >+            onMessage : function (msg) {
> 
> Nit: consistently omit a space before the colon in property definitions.
> 
> 
> Also, I would call this "modify" rather than "createWorker", to emphasize that
> this method allows you to modify a page in the tab in the same way that Page
> Mods allows you to modify all pages matching a given URL (and to de-emphasize
> the word "worker", which is quite unrelated to the task that the consumer is
> performing).
> 
> 
> Finally, this needs some documentation in packages/addon-kit/docs/tabs.md.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: New proposal

Add documentation with example of typical usecases, 
add new tests to highlight that these workers are called only on the current tab document, 
fix indentation, 
rename createWorker to modify.

Comment 4

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

Comment on attachment 505417 [details] [diff] [review]
New proposal

diff --git a/packages/addon-kit/docs/tabs.md b/packages/addon-kit/docs/tabs.md

+**Example**
+
+    var tabs = require("tabs");
+    
+    tabs.activeTab.modify({
+      contentScript : 'document.body.style.border="5px solid black";' +
+                      'postMessage(document.getElementById("#my-watched-element").innerText);',
+      onMessage : function (data) {
+        // data is equal to the text of my DOM element with ID "#my-watched-element"

Nit: omit space before the colon in property definitions.


+@param options {object}
+  An object containing 

Nit: this would be more consistent with other descriptions of similar parameters if it were phrased as "Options for the page mod, with the following keys:".


+@prop [onMessage] {function}
+    A function called when we receives a message from content scripts. 
+    Listeners are passed a single argument, the message posted from the 
+    content script.

Nit: we -> the page mod


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

   unpin: function unpin() {
     this._window.gBrowser.unpinTab(this._tab);
   },
+  
+  /**
+   * Create a worker for this tab, first argument is options given to Worker.
+   * @type {Worker}
+   */
+  modify: function modify(options) {
+    let { Worker } = require("content/worker");
+    options.window = this._contentWindow;
+    return Worker(options);
+  },
   /**
    * Make this tab active.
    * Please note: That this function is called synchronous since in E10S that

Nit: add a blank line after the property definition.

Otherwise, this looks great!  r=myk.  Let's get this in once the tree thaws for 1.0b3 development.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Yet another proposal

I mess up with the meaning of "omit", I thought I forgot to add space, but it was the opposite ;)

Comment 6

[Michal Tuszynski]

I tested this patch recently and came across a bug in tab.modify()function in /packages/api-utils/lib/tabs/tab.js. This function passes to the Worker constructor the following option:

options.window = this._contentWindow

However, the _contentWindow object is not a standard JS window object but a XrayWrapper'ed pseudo-object, so the code in the tab doesn't have access to standard JS window properties. The object passed to the Worker constructor should be unwrapped before passing, so this line should be:

options.window = this._contentWindow.wrappedJSObject;

This change is also consistent with what other pieces of code that call Worker constructor do (i.e. page-mods and page-worker).

By the way, I think that the name of the function is a bit misleading. If I understand correctly, each call to tab.modify() will create a new worker and will not modify any workers already created. A better name may be tab.attach(), which would be consistent with page-mods. It'd also be nice to be able to delete a worker created by tab.modify().

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Implements tabworker=tab.modify() and tabworker.destroy()

Thanks Michal for this report, nice catch!

Here is an updated patch that fix the wrapper problem and add a destroy method on the worker. Unfortunately I was not able to add this destroy method directly in Worker.js:Worker and instead added it dynamically in tabs.js:modify function.
Because destroy function is already used in objects that compose with Worker (Page,Panel... via Symbiont).

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: The same than 511044 but with destroy on Worker

Here is an alternative with destroy added directly into Worker Trait.
I managed to do that by forcing a conflict in Symbiont (renaming _destructor to destroy) in order to avoid deep conflict between Page.destroy and Worker.destroy and so let Symbiont call worker.destroy at the right time (instead of calling Worker.destroy directly from Page!)
The side effect is that Symbiont has a public destroy method and all objects composed of Worker may get a new public destroy method too.

And about the "modify" function name, I didn't change anything because as I'm far for being fluent in english, I let you guys decide what you think is the best.

Comment 9

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

Comment on attachment 511052 [details] [diff] [review]
The same than 511044 but with destroy on Worker

Looks good, r=myk!

I think Michal is right about the name, though.  `attach` is better.  So the name should be `attach`, and the description should be:

  Create a page mod and attach it to the document in the tab.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Tab.attach + Worker.destroy + doc

Here is the same than 511052, with attach instead of modify.

Comment 11

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

https://github.com/mozilla/addon-sdk/commit/ab41edc378595fc30623b9129d2176c48667a3ac