Closed
Bug 569479
Opened 14 years ago
Closed 14 years ago
update Widget API for forwards-compatibility with electrolysis
Categories
(Add-on SDK Graveyard :: General, defect, P1)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: myk, Assigned: dietrich)
Details
Attachments
(1 file, 7 obsolete files)
53.59 KB,
patch
|
myk
:
review+
|
Details | Diff | Splinter Review |
The Widget API should be updated to be forwards-compatible with electrolysis.
Assignee | ||
Comment 1•14 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•14 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•14 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•14 years ago
|
Priority: P2 → P1
Target Milestone: 0.7 → 0.8
Reporter | ||
Comment 4•14 years ago
|
||
Note bug 593913, which will eventually affect Widget's interface (although probably not in 0.8).
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → dietrich
Target Milestone: 0.8 → --
Assignee | ||
Comment 5•14 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•14 years ago
|
||
Outlining some basic E10s guideline for module developers: http://etherpad.mozilla.com:9000/JetpackE10s.
Assignee | ||
Comment 7•14 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•14 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•14 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•14 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•14 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•14 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•14 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•14 years ago
|
||
Assignee | ||
Comment 15•14 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•14 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•14 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•14 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•14 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•14 years ago
|
||
Attachment #488472 -
Attachment is obsolete: true
Assignee | ||
Comment 21•14 years ago
|
||
Next step is to remove onLoad/onReady, using contentScripts instead.
Assignee | ||
Comment 22•14 years ago
|
||
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•14 years ago
|
||
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•14 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•14 years ago
|
||
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•14 years ago
|
Attachment #488811 -
Flags: feedback? → feedback?(rFobic)
Assignee | ||
Comment 26•14 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•14 years ago
|
||
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•14 years ago
|
||
Pull request: https://github.com/mozilla/addon-sdk/pull/27
Comment 29•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
(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•14 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•14 years ago
|
||
Ready for review from Myk: https://github.com/mozilla/addon-sdk/pull/32
Assignee | ||
Comment 34•14 years ago
|
||
Attachment #488813 -
Attachment is obsolete: true
Attachment #489239 -
Flags: review?(myk)
Attachment #488813 -
Flags: feedback?(rFobic)
Reporter | ||
Comment 35•14 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•14 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•14 years ago
|
||
Note to self: update the /examples.
Reporter | ||
Comment 38•14 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•14 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•14 years ago
|
||
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•14 years ago
|
||
Comment on attachment 492027 [details] [diff] [review] v3 Looks good, r=myk.
Attachment #492027 -
Flags: review?(myk) → review+
Assignee | ||
Comment 42•14 years ago
|
||
https://github.com/mozilla/addon-sdk/commit/bba8ab322a159bd955b9dcd947085137ebcf5862
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•