Closed
Bug 660860
Opened 13 years ago
Closed 11 years ago
no detach events triggered on widgets when they are removed from toolbar
Categories
(Add-on SDK Graveyard :: General, defect, P2)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: irakli, Unassigned)
References
Details
Attachments
(1 file)
10.85 KB,
patch
|
Details | Diff | Splinter Review |
Sometimes users may remove add-on widgets using customize feature in which case add-on's are supposed to send messages to them as content document is destroyed. Unfortunately 'detach' events are not triggered on workers so there is no way for add-on to know that they should stop! Also here is example of error which is logged into console every time message is send to a worker who's widget was removed: Traceback (most recent call last): File "resource://about-downloads-api-utils-lib/timer.js", line 77, in notifyOnInterval this._callback.apply(null, this._params); File "resource://about-downloads-about-downloads-lib/about-downloads.js", line 15, in worker.port.emit('change', Math.round(Math.random() * 100), 2) File "resource://about-downloads-addon-kit-lib/widget.js", line 395, in self._chrome.update(self._baseWidget, "emit", arguments); File "resource://about-downloads-addon-kit-lib/widget.js", line 678, in WC_update let port = this._symbiont.port; TypeError: this._symbiont is null error: An exception occurred. Traceback (most recent call last): File "resource://about-downloads-api-utils-lib/timer.js", line 77, in notifyOnInterval this._callback.apply(null, this._params); File "resource://about-downloads-about-downloads-lib/about-downloads.js", line 15, in worker.port.emit('change', Math.round(Math.random() * 100), 2) File "resource://about-downloads-addon-kit-lib/widget.js", line 395, in self._chrome.update(self._baseWidget, "emit", arguments); File "resource://about-downloads-addon-kit-lib/widget.js", line 678, in WC_update let port = this._symbiont.port; TypeError: this._symbiont is null
Updated•13 years ago
|
Priority: -- → P2
Target Milestone: --- → 1.0
Comment 1•13 years ago
|
||
(automatic reprioritization of 1.0 bugs)
Priority: P2 → P1
Target Milestone: 1.0 → 1.1
Updated•13 years ago
|
Assignee: nobody → poirot.alex
Comment 2•13 years ago
|
||
Unfortunately, we are not really handling this customize dialog. I'll use this bug as the main discussion holder to fix this. (bug 660857 is related) So toolbars customization is quite complex. And the way it behaves doesn't work well with our iframe approach. When you click on the "customize" context menu, all toolbaritems are wrapped into a toolbarpaletteitem. So all iframe documents are reloaded. (This is already fixed) But when you move an item to the dialog in order to remove it, a copy of our DOM nodes is made with document.importNode. So the iframe is unloaded and another one is loaded into this dialog. The main issue is that we do not control this importNode call so that we can hardly track this new iframe and make it work. So my approach here is to replace DOM nodes before/after customization. I'm replacing iframe toolbaritem by a simple toobarbuttonitem with an icon/label before switching to customization mode so that we copy an item that will be able to display correctly by itself in the dialog. And I'm doing the opposite in case we drop an item from this dialog. This patch is quite complex and we may found alternative approach so I'm letting it bake for some time.
Comment 3•13 years ago
|
||
(In reply to comment #2) > The main issue is that we do not control this importNode call so that we can > hardly track this new iframe and make it work. The Firefox and platform teams are quite open to taking changes to make our APIs work better, so I would take an expansive view of how we can fix this bug and the related ones. That doesn't necessarily mean those teams will take on the task of fixing every issue themselves. In some cases, it'll make the most sense for Jetpack engineers to put together the patches for the Firefox/platform code changes. But if the best fix for this bug involves a change to Firefox's code, and it's a reasonable fix (in terms of complexity, robustness, time to ship, etc.), then let's aim to do that. Let's not get ahead of ourselves, however. The first step is to figure out the right behavior. Only once we've done that can we figure out the best way to implement that behavior (i.e. through changes in the SDK, Firefox, or in both products). And only after that can we identify who will do the work (a Jetpack engineer, a Firefox engineer, or a combination of the two) and make it happen. So, regarding step one... at first glance, it looks like the overall best behavior is for widget content to stay loaded in most cases, including across transitions into and out of toolbar customization mode and when a widget is dragged between locations on the toolbars. But if a widget is dragged onto the palette, then I would expect its content to be unloaded and for its appearance in the palette to be static (i.e. to be represented by a graphical icon and/or text label), so that users don't incur the cost of dynamic content for an item that isn't visible. And if a widget is dragged from the palette onto a toolbar, then I would expect its content to be loaded again. So the only time widget content is unloaded or reloaded during customization is when when an item is dragged onto or off of the palette. Irakli, Alex: does that sound like the ideal behavior to you as well? Or is there something I am missing here?
Comment 4•13 years ago
|
||
That sounds good. I made the assumption that they had a good reason to implement such complex thing and that it would be beyond our scope to refactor such piece of Firefox. So I'll try to reach Firefox folks about this strange toolbarpaletteitem. By any chance, do you know who I can ping about that? (David Hyatt, Blake Ross and Joe Hewitt are mentioned in the source) Then, we will need to translate this behavior description in API description with "class" names and events. For example, do we destroy WidgetViews? Do we sent `detach` event?
Updated•13 years ago
|
Priority: P1 → P2
Updated•13 years ago
|
Target Milestone: 1.1 → 1.2
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: 1.2 → ---
Updated•11 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Summary: no detach events triggered on widgets when they are removed from panel → no detach events triggered on widgets when they are removed from toolbar
Updated•11 years ago
|
Blocks: sdk/widget
Updated•11 years ago
|
Assignee: poirot.alex → nobody
Comment 6•11 years ago
|
||
When Australis lands widget will be deprecated so we won't be working on this.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•