update Widget API for forwards-compatibility with electrolysis

RESOLVED FIXED

Status

Add-on SDK
General
P1
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: myk, Assigned: dietrich)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

8 years ago
The Widget API should be updated to be forwards-compatible with electrolysis.
(Assignee)

Comment 1

8 years ago
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.
Assignee: dietrich → warner-bugzilla
Status: NEW → ASSIGNED
Priority: -- → P2
Version: unspecified → Trunk
(Reporter)

Comment 2

7 years ago
Taking in order to implement the solution we've been hashing out over in bug 494238.
Assignee: warner-bugzilla → myk
Target Milestone: 0.5 → 0.7
(Reporter)

Comment 3

7 years ago
I'm not sure I'll be the one fixing this; unassigning myself from it for now.
Assignee: myk → nobody
Status: ASSIGNED → NEW
(Reporter)

Updated

7 years ago
Priority: P2 → P1
Target Milestone: 0.7 → 0.8
(Reporter)

Comment 4

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

Updated

7 years ago
Assignee: nobody → dietrich
Target Milestone: 0.8 → --
(Assignee)

Comment 5

7 years ago
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.
(Assignee)

Comment 6

7 years ago
Outlining some basic E10s guideline for module developers: http://etherpad.mozilla.com:9000/JetpackE10s.
(Assignee)

Comment 7

7 years ago
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.
(Assignee)

Comment 8

7 years ago
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.
(Assignee)

Comment 9

7 years ago
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
(Reporter)

Comment 10

7 years ago
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.
(Assignee)

Comment 11

7 years ago
Ah, yeah makes sense to keep the ones that are not tied to rich content in the widget in the Jetpack-side api, thanks.
(Reporter)

Comment 12

7 years ago
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.
(Assignee)

Comment 13

7 years ago
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.
(Assignee)

Comment 14

7 years ago
Created attachment 488472 [details] [diff] [review]
step 1. move to content symbiont
(Assignee)

Comment 15

7 years ago
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.
(Reporter)

Comment 16

7 years ago
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).
(Assignee)

Comment 17

7 years ago
(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?
(Reporter)

Comment 18

7 years ago
(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.
(Assignee)

Comment 19

7 years ago
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.
(Assignee)

Comment 20

7 years ago
Created attachment 488729 [details] [diff] [review]
step 2. move Widget ctor to Loader API
Attachment #488472 - Attachment is obsolete: true
(Assignee)

Comment 21

7 years ago
Next step is to remove onLoad/onReady, using contentScripts instead.
(Assignee)

Comment 22

7 years ago
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.

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.
Attachment #488729 - Attachment is obsolete: true
(Assignee)

Comment 23

7 years ago
Created attachment 488810 [details] [diff] [review]
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.
Attachment #488757 - Attachment is obsolete: true
(Assignee)

Comment 24

7 years ago
(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.
(Assignee)

Comment 25

7 years ago
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?
Attachment #488810 - Attachment is obsolete: true
Attachment #488811 - Flags: feedback?
(Assignee)

Updated

7 years ago
Attachment #488811 - Flags: feedback? → feedback?(rFobic)
(Assignee)

Comment 26

7 years ago
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.
(Assignee)

Comment 27

7 years ago
Created attachment 488813 [details] [diff] [review]
v1.1

Remove the last bits of the removal exception hack.
Attachment #488811 - Attachment is obsolete: true
Attachment #488813 - Flags: feedback?(rFobic)
Attachment #488811 - Flags: feedback?(rFobic)
(Assignee)

Comment 28

7 years ago
Pull request: https://github.com/mozilla/addon-sdk/pull/27
(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.
(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.
(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.
(Assignee)

Comment 32

7 years ago
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
(Assignee)

Comment 33

7 years ago
Ready for review from Myk: https://github.com/mozilla/addon-sdk/pull/32
(Assignee)

Comment 34

7 years ago
Created attachment 489239 [details] [diff] [review]
v2
Attachment #488813 - Attachment is obsolete: true
Attachment #489239 - Flags: review?(myk)
Attachment #488813 - Flags: feedback?(rFobic)
(Reporter)

Comment 35

7 years ago
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".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
(Reporter)

Comment 36

7 years ago
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.
Attachment #489239 - Flags: review?(myk) → review+
(Assignee)

Comment 37

7 years ago
Note to self: update the /examples.
(Reporter)

Comment 38

7 years ago
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.
Attachment #489239 - Flags: review+ → review-
(Assignee)

Comment 39

7 years ago
(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!
(Assignee)

Comment 40

7 years ago
Created attachment 492027 [details] [diff] [review]
v3

fixed comments, updated to latest changes, also fixed a bug in the panel test (calling test.done() twice).
Attachment #489239 - Attachment is obsolete: true
Attachment #492027 - Flags: review?(myk)
(Reporter)

Comment 41

7 years ago
Comment on attachment 492027 [details] [diff] [review]
v3

Looks good, r=myk.
Attachment #492027 - Flags: review?(myk) → review+
(Assignee)

Comment 42

7 years ago
https://github.com/mozilla/addon-sdk/commit/bba8ab322a159bd955b9dcd947085137ebcf5862
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.