569479 - update Widget API for forwards-compatibility with electrolysis

Status: RESOLVED FIXED

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

Description

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

The Widget API should be updated to be forwards-compatible with electrolysis.

Comment 1

[Dietrich Ayala (:dietrich)]

Assigning to Brian, as the first step is his analysis of what we need to do here. Brian, assign back over when you post your comments.

Comment 2

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

Taking in order to implement the solution we've been hashing out over in bug 494238.

Comment 3

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

I'm not sure I'll be the one fixing this; unassigning myself from it for now.

Comment 4

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

Note bug 593913, which will eventually affect Widget's interface (although probably not in 0.8).

Comment 5

[Dietrich Ayala (:dietrich)]

Looks like I'll need to switch to use contentSymbiont for the widget content. Widgets are mirrored across windows, and persistently visible (when the addon bar is visible), so probably don't need the frameloader swapping stuff.

I'm not sure how best to handle the events. Maybe a content script that runs in the context of the widget content. Maybe proxied events back to add-on module space (without access to the event details). Some mix maybe, depending on the use-case of the different events.

Comment 6

[Dietrich Ayala (:dietrich)]

Outlining some basic E10s guideline for module developers: http://etherpad.mozilla.com:9000/JetpackE10s.

Comment 7

[Dietrich Ayala (:dietrich)]

wrt events, should probably just move to content scripts, and messaging, for more full-featured approach, and so that add-on devs get proper event object access like they do now.

should ensure that the api is consistent with Panel's implementation of this approach.

Comment 8

[Dietrich Ayala (:dietrich)]

Myk, i see what you mean now. the only reasonable approach is to convert Widget to behave almost exactly like Panel does now. they're both cases of content being wrapped and exposed.

Comment 9

[Dietrich Ayala (:dietrich)]

Proposed API for e10s compatible Widget. Maybe we want to keep image for convenience/clarity?

Properties staying the same:

label
panel
width
tooltip

New properties:

contentURL
allow
contentScriptURL
contentScript
contentScriptWhen
onMessage

Removed properties:

content (replaced with contentURL)
image (ditto)
onClick (replaceable by content script, ditto for all event handlers)
onLoad
onMouseover
onMouseout
onReady

Comment 10

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

Overall, this makes sense.  However, I would keep onClick (and perhaps onMouseover and onMouseout) but make it fire asynchronously (so not block the chrome process in which it originates), since it is much easier to use for cases that do not require access to the DOM content of the widget, f.e. an image widget that opens a tab when you click it.

Comment 11

[Dietrich Ayala (:dietrich)]

Ah, yeah makes sense to keep the ones that are not tied to rich content in the widget in the Jetpack-side api, thanks.

Comment 12

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

It might even be worth distinguishing between the "simple widget" and the "web content widget" cases in the documentation, with an example that shows how to create a simple widget using an image and an onClick handler (or a panel) and another example that shows how to create a web content widget using a web page and content scripts.

Comment 13

[Dietrich Ayala (:dietrich)]

Actually, content can be a URL or raw content. What do you think of leaving it as-is? Moving to contentURL doesn't give that same flexibility.

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: step 1. move to content symbiont

Comment 15

[Dietrich Ayala (:dietrich)]

Hm, so the Widget ctor needs to expose Symbiont API, but we need the content itself to exist in each window.

Maybe I could compose Widget from Symbiont, but not actually have that it load anything. Then create new Symbiont for each widget for each window (like the current patch does), and actually load the content in those.

Comment 16

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

The Page Mods API might be instructive, since it does something similar (exposes a Content Scripts API but loads content scripts in each individual page).

Comment 17

[Dietrich Ayala (:dietrich)]

(In reply to comment #10)
> However, I would keep onClick (and perhaps
> onMouseover and onMouseout) but make it fire asynchronously (so not block the
> chrome process in which it originates)

Is there a right way to do this with EventEmitter? Does it have something built-in for ensuring async?

Comment 18

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

(In reply to comment #17)
> (In reply to comment #10)
> > However, I would keep onClick (and perhaps
> > onMouseover and onMouseout) but make it fire asynchronously (so not block the
> > chrome process in which it originates)
> 
> Is there a right way to do this with EventEmitter? Does it have something
> built-in for ensuring async?

Hmm, not at the moment, although it should be relatively easy to add an "async" boolean param to EventEmitter._emit that calls listeners in a timeout.  And once we start updating the modules to work across processes, we'll get this by default.

Comment 19

[Dietrich Ayala (:dietrich)]

Note: going to keep .content, it's purpose being for raw content, and in the back-end we'll create the data uri for it. Talked to Myk on IRC, we might want to do this across all content/ APIs. Filed bug 609938 for this.

Comment 20

[Dietrich Ayala (:dietrich)]

Attachment: step 2. move Widget ctor to Loader API

Comment 21

[Dietrich Ayala (:dietrich)]

Next step is to remove onLoad/onReady, using contentScripts instead.

Comment 22

[Dietrich Ayala (:dietrich)]

Attachment: step 3. move from onLoad/onReady to content scripts

mostly there. need to convert the internal listener to a content script, and there are a few tests that need fixing.

the only hack so far is to remove the exception thrown when removing a widget that's not in the registry. take this scenario: a widget, two windows (so two live instances in the ui). on click, the widget posts a message that removes itself. the first time the callback is called back the widget is removed. the second time, there's an exception because the widget is already gone from the registry.

haven't tried to figure out yet why this isn't happening in the current implementation. the hackaround might be ok to live with for now, handle in a followup, since the scenario is not likely to happen in normal usage - i just hit it as an artifact of the unit test.

Comment 23

[Dietrich Ayala (:dietrich)]

Attachment: step 3.1 - all tests passing

So, the proper solution to the previous conundrum is to code your onMessage handler to expect multiple calls back, since there might be multiple windows open. Eg: If your content script does a postMessage on "ready", and there are multiple windows open, your widget.onMessage will get called once per window.

Instead of rewriting all the tests to handle that (there's already a multiple windows test), i changed the runner to run the tests in the current content window instead of a new one.

The multiple-windows-equals-multiple-callbacks scenario could lead to some confusion for developers though. I was certainly confused when my tests were running in overlap (I was shift()ing the next test on onMessage, so if multiple messages were received, multiple tests would start running before the current one completed).

I'll update the documentation to reflect this.

Comment 24

[Dietrich Ayala (:dietrich)]

(In reply to comment #22)
> need to convert the internal listener to a content script, and

I forgot, these are listeners on the xul iframe, so don't actually need to do this... I think. I'm still not really clear on what code runs where.

Comment 25

[Dietrich Ayala (:dietrich)]

Attachment: v1

Removed the removal hack, and cleaned out the debugging bits, ready for first feedback review.

The only slightly hacky thing left is the __contentURL bit, since if I use _contentURL I get the "Error: Remaining conflicting property: _contentURL" error. Any ideas Irakli?

Comment 26

[Dietrich Ayala (:dietrich)]

More notes:

* Look into better image detection. Had to move to checking the extension, which is probably fine a majority of the time. Probably ok to file followup on this.

* "TODO: check that content/worker.js handles this properly" - wrt to handling transient about:blank loads

* "// Content-specific document modifications" - that bit of code is commented out for now, since we're not handling load events internally. Need to figure out if that's actually a problem or not, or something we should just pass off to content scripts to handle. If the style mods for proper display in the widget sizes is consistently required, then we should handle it internally as much as possible.

Comment 27

[Dietrich Ayala (:dietrich)]

Attachment: v1.1

Remove the last bits of the removal exception hack.

Comment 28

[Dietrich Ayala (:dietrich)]

Pull request: https://github.com/mozilla/addon-sdk/pull/27

Comment 29

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

(In reply to comment #25)
> Created attachment 488811 [details] [diff] [review]
> v1
> 
> Removed the removal hack, and cleaned out the debugging bits, ready for first
> feedback review.
> 
> The only slightly hacky thing left is the __contentURL bit, since if I use
> _contentURL I get the "Error: Remaining conflicting property: _contentURL"
> error. Any ideas Irakli?

Comments have been added to the pull request.

Comment 30

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

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #10)
> > > However, I would keep onClick (and perhaps
> > > onMouseover and onMouseout) but make it fire asynchronously (so not block the
> > > chrome process in which it originates)
> > 
> > Is there a right way to do this with EventEmitter? Does it have something
> > built-in for ensuring async?
> 
> Hmm, not at the moment, although it should be relatively easy to add an "async"
> boolean param to EventEmitter._emit that calls listeners in a timeout. 

_emit take's unlimited amount of arguments that is passed through to the listeners so adding boolean param is not a function. I think factoring out AsyncEventEmitter out of worker to the event module is a better option.

https://github.com/mozilla/addon-sdk/blob/master/packages/jetpack-core/lib/content/worker.js#L55 

> And
> once we start updating the modules to work across processes, we'll get this by
> default.

Comment 31

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

(In reply to comment #22)
> Created attachment 488757 [details] [diff] [review]
> step 3. move from onLoad/onReady to content scripts
> 
> mostly there. need to convert the internal listener to a content script, and
> there are a few tests that need fixing.
> 
> the only hack so far is to remove the exception thrown when removing a widget
> that's not in the registry. take this scenario: a widget, two windows (so two
> live instances in the ui). on click, the widget posts a message that removes
> itself. the first time the callback is called back the widget is removed. the
> second time, there's an exception because the widget is already gone from the
> registry.

I guess remove should be async (in the next turn of event loop), then there will be no exceptions and all the listeners will be called as expected.

> 
> haven't tried to figure out yet why this isn't happening in the current
> implementation. the hackaround might be ok to live with for now, handle in a
> followup, since the scenario is not likely to happen in normal usage - i just
> hit it as an artifact of the unit test.

Comment 32

[Dietrich Ayala (:dietrich)]

Pull request fixing Irakli's review comments: https://github.com/mozilla/addon-sdk/pull/30

Next up:

* make sure things that should be async are.
* investigate issues in comment #26
* documentation update

Comment 33

[Dietrich Ayala (:dietrich)]

Ready for review from Myk: https://github.com/mozilla/addon-sdk/pull/32

Comment 34

[Dietrich Ayala (:dietrich)]

Attachment: v2

Comment 35

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

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 36

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

Comment on attachment 489239 [details] [diff] [review]
v2

+widget when it first loads, or when it's DOM is ready. See the example code

Nit: it's -> its


-    var widgets = require("widget");
-

Why remove this?  The examples don't work as-is without it.


Also, the current pull request has a merge conflict in widget.md, now that I've checked in Will's docs updates.


Otherwise, though, this looks good.

Comment 37

[Dietrich Ayala (:dietrich)]

Note to self: update the /examples.

Comment 38

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

Comment on attachment 489239 [details] [diff] [review]
v2

Erm, one more problem!  Event handlers cannot be passed Event objects, as such objects cannot cross process boundaries, so this code'll need to stop passing them.

Comment 39

[Dietrich Ayala (:dietrich)]

(In reply to comment #36)
> -    var widgets = require("widget");
> -
> 
> Why remove this?  The examples don't work as-is without it.

accidental.

(In reply to comment #38)
> Erm, one more problem!  Event handlers cannot be passed Event objects, as such
> objects cannot cross process boundaries, so this code'll need to stop passing
> them.

Oops. I'd converted the test code to not use the events, but forgot to remove it from the emit call!

Comment 40

[Dietrich Ayala (:dietrich)]

Attachment: v3

fixed comments, updated to latest changes, also fixed a bug in the panel test (calling test.done() twice).

Comment 41

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

Comment on attachment 492027 [details] [diff] [review]
v3

Looks good, r=myk.

Comment 42

[Dietrich Ayala (:dietrich)]

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