630962 - It's seemingly impossible to write widgets which display different contents in different windows

Status: RESOLVED FIXED

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

Description

[Jeff Walden [:Waldo]]

You write the code for the widget only once, and as far as I can see there's no way to distinguish different instances of the widget from each other, so that widgets could display different content in different windows.  My use case is to display the number of tabs in the window in the widget, and it would be pretty un-polished to display the wrong number of tabs in non-active windows, even if the number were updated when a window was activated.

A post-creation hook on the widget (onCreated, say) would be one way to implement this -- you could send the content a message with a guid of some sort, then all communications between widget and script could pass the guid as a channel identifier.  The widget and content scripts could then each filter for messages specifically directed at them.

But a way that would be more webby, in the sense of being more like HTML's postMessage functionality, would be to pass along a "source" argument for all widget-to-content and content-to-widget messages, identifying the sender of the message.  On the web this is a window object, but presumably you wouldn't want that for cross-process future-lookingness -- just some unique object (therefore also non-forgeable, unlike if you just used a guid string for the purpose) would work just as well, without cross-process concerns.  The widget would have its own source object, and each content instantiation would have its own source object.

There's probably some idea-polishing that could be done here, but this is the gist of it.

Comment 1

[Alexandre Poirot [:ochameau]]

I took some time to thought about this important feature miss!
And I ended with two concrete solutions:

1/ Consider widget object to be equal on all windows by default and allow accessing to instance specific to one window with such API:
  
  let widget = Widget({
    contentURL: "http://.../green.png"
  });
  let specific = widget.getWidgetFrom( window );
  specific.contentURL = "http://.../red.png";
  console.log( specific.contentURL );

2/ Still consider widget to be the same on all windows and allow to modify attributes after creation using this: 

  let widget = Widget({
    contentURL: "http://.../green.png"
  });
  widget.setAttributeOn(window, "contentURL", "http://.../red.png");
  console.log( widget.getAttributeOn(window, "contentURL") );

I'm pretty sure that getWidgetFrom and setAttributeOn are not the best function names, but it highlights these differents pattern we could use.

In both case we may ensure that all callback receive window object in arguments.
We may allow empty widget creation in order to allow creation of widget only on specific window condition.

I personnaly prefer the first solution with specific widget object as it may simplify internal code and I found it easier.

Finally, about internal hack to match windows, we may use the same stuff I used in #621903: nsIDOMWindowUtils.currentInnerWindowID. So we would use a string ID that is going to be e10s compliant.

Comment 2

[Jeff Walden [:Waldo]]

Note, as I mentioned in comment 0, that a simple string is forgeable, unlike an object of some sort.  I realize jetpacks are all privileged code, but I have some difficulty believing there aren't use cases which would rely on a non-forgeable source identifier of some sort.  (You could keep a hash of window id to unique object to still use window id under the hood, of course.)

Comment 3

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

My initial thinking is to reuse the existing window objects returned by the Windows API to represent the windows associated with widgets, i.e. something like the following API:

let workers = [];

Widget({
  // The "attach" event fires when a widget is loaded in a browser window.
  onAttach: function(worker) {
    // worker.window is a BrowserWindow object, just like worker.tab
    // is a Tab object for Page Mod workers.

    // You can listen for messages from just this worker.
    worker.on("message", function() { ... });

    // And you can also send messages to just this worker.
    worker.postMessage("Hi!");

    // You can cache workers to access them later.
    workers.push(worker);

    // And compare their window objects to those provided by
    // the Window API.
    if (worker.window == require("windows").browserWindows.activeWindow)
      console.log("Widget worker attached to active window.");

    // Example: tell the worker when the number of tabs
    // in its window changes.
    function notify() worker.postMessage(worker.window.tabs.length);
    worker.window.tabs.on("open", notify);
    worker.window.tabs.on("close", notify);
  }
});

This is somewhat similar to the first solution in comment 1, except that rather than have two kinds of widgets, the initial Widget object and a "specific widget", there is only one kind of widget, and then there are workers for the widget. And you get a worker by listening for "attach" rather than calling a method.

This API has the benefit of being similar to the Page Mods API, so developers familiar with one have an easier time understanding the other.  We might also consider adding a "detach" event to notify widgets when a worker is detached (which might be useful for Page Mods as well).

Comment 4

[Alexandre Poirot [:ochameau]]

Having the same API than pagemod is a good idea, but I still think that we need more than window attribute on the worker.

For example: 
- be able to change label/tooltip on one specific window,
- be able to know on which window we clicked,
- it would be hard to get a widget worker for a specific window. (case solved in pageMod with tab.attach method) (for ex, how would you implement http/https lock icon ?)

Comment 5

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

(In reply to comment #4)
> Having the same API than pagemod is a good idea, but I still think that we need
> more than window attribute on the worker.
> 
> For example: 
> - be able to change label/tooltip on one specific window,
> - be able to know on which window we clicked,

It's a good point.  One way of handling this would be to extend Worker objects with a WidgetWorker variant that provides window-specific access to these individual properties:

Widget({
  // The "attach" event fires when a widget is loaded in a browser window.
  onAttach: function(worker) {
    // You can change the label for just this worker.
    worker.label = Math.random();

    // You can listen for click events from just this worker.
    worker.on("click", function() { ... });
});

This is similar to your first proposal in comment 1, except that "specific" is a WidgetWorker rather than a Widget, which preserves the current semantics of Widget.


> - it would be hard to get a widget worker for a specific window. (case solved
> in pageMod with tab.attach method) (for ex, how would you implement http/https
> lock icon ?)

The way to get a widget worker for a specific window would be to cache workers and then retrieve the one you want from the cache:

  let workers = [];
  Widget({
    onAttach: function(worker) {
      workers.push(worker);
    }
  });
  
  let worker = workers.filter(function(worker) worker.window == window)[0];

I see your point that this is complicated, however.  So I can imagine implementing something like your proposal, which does this work on behalf of addons.  I'd still call the returned objects workers rather than widgets, however, and thus implement something like a `widget.getWorker(window)` method that returns a WidgetWorker.

Comment 6

[Dietrich Ayala (:dietrich)]

The window-specific widget instance sounds great. Preserves the simplicity of the common case, and preserves the Widget API in the instances.

I think worker as a name is counter-intuitive. It doesn't say "i am a window-specific instance of a widget". widget.getWindowInstance or widget.getInstanceForWindow?

Comment 7

[Alexandre Poirot [:ochameau]]

I really agree with Dietrich point of view as it goes in the same way I proposed.
I get your point about providing a pattern that will look like page-mod workers,
but at the end we have WidgetWorker that have the same API than a regular Widget.

Nevertheless I didn't proposed equivalent for onAttach function.
It is really important to know when a new widget instance is created.
onAttach, onNewInstance, onNew, ... ?

This discussion seems to be more on the word we want to choose, as it seems pretty clear that we want:
 - one specific object for each window
 - have a function to retrive one specific object about one particular window
 - dispatch an event when a new widget instance is created/destroyed

Finally, I want to highlight one other need. In events functions (click,message,mouseover) we may really appreciate having a reference to the matching widget instance/worker.
In order to do so, we can pass a new argument beeing the widget; or adding a widget attribute into the event object (for all events but message).

Comment 8

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

Yes, the question may be mostly semantic, but semantics are important in an API.

The problem with calling this new thing a "widget instance" is that we already have something called that: it is the thing returned by the Widget constructor function.

`WidgetWorker` is a fairly accurate description of this thing, which implements the Worker interface and much of the Widget interface.  Nevertheless, it's clunky, and I can see how it might be misleading, as it overemphasizes the `Worker` aspect of the interface.

Another term used to describe the kind of thing we're talking about here is `view`, i.e. one representation of some model (which in this case is the Widget instance).  Usage of that term seems reasonable here as well.  So let's do this:

1. Add an onAttach method to Widget that passes a WidgetView object that exposes Worker and Widget-like interfaces as appropriate.

2. Add a getView method to Widget that takes a Window argument and returns the WidgetView associated with the window.

3. Add a view argument to click, mouseover, and mouseout event listeners whose value is the view that emitted the event.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 152

I've submitted a pull request with choosen commit step.
I tried to write explicit commit message in order to ease the review of each big refactor step.
For now, this patch contains only main code and minimal unit-test.
I'll provide additional tests and start working on documentation tomorrow.

Comment 10

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

Comment on attachment 528498 [details]
Pull request 152

Irakli: if you have any time today (Thursday), would you be able to do a review of this code?  If you review it before the end of the European day, that'll give Alex some time Thursday to address issues raised in the review.

I'll still finish reviewing it afterward, but assuming I only find nits, because you've spotted all the significant issues already, I can then land the changes, fixing the nits myself in the process of landing it.

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 528498 [details]
Pull request 152

Thanks Alex for a lots of work. Please see some comments in pull request that needs to be addressed before this change is ready.

Also we need either add docs here or as a separate change. But I think it's important to land both code and doc changes in one cycle.

Comment 12

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

Comment on attachment 528498 [details]
Pull request 152

Irakli: thanks for the quick and great review!   Cancelling my review, as it isn't really needed after Irakli's, but I can review a followup patch.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: New commits on pull request

I've submitted a new commit to adress Irakli's comment:
https://github.com/ochameau/addon-sdk/commit/a85ff25d9f9f5ecff59a400888a1cf882087595d
I posted comments on all items that was subject to discussion, all others have been adressed.

I filled bug 653452 to provide documentation. I faced a bug that disallow me to provide my new widget example until the fix land.

Comment 14

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

Comment on attachment 528900 [details]
New commits on pull request

Some nits are noted in the pull request, but I didn't find any substantive issues, so r=myk!

Comment 15

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

Also, a=myk for landing during the freeze.  This will be our last big commit, I think.

Comment 16

[Alexandre Poirot [:ochameau]]

Thanks for pushing hard on this, I'm really impressed how fast we reviewed such big patch!
When you see all improvements done in the previous cycle and this new feature, I think we reached a really high level on this API.

Landed:
https://github.com/mozilla/addon-sdk/commit/280fbc6f54d3fad5cb79deac4269b841a282987f