Closed Bug 611763 Opened 10 years ago Closed 10 years ago

switch to construct/destroy model for Panel

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: avarma, Assigned: avarma)

References

Details

Attachments

(1 file, 1 obsolete file)

This is a sub-component of bug 596053 that I'll be taking up, since I'm going to be trying to port Panel to e10s awesomeness shortly as well.
Assignee: nobody → avarma
Blocks: 596053
Status: NEW → ASSIGNED
Blocks: 611766
Work-in-progress branch is here:

https://github.com/toolness/jetpack-sdk/tree/bug-611763
Drew, can you take a look at this and let me know what you think?
Attachment #490608 - Flags: feedback?(adw)
Comment on attachment 490608 [details]
Pointer to Github pull request containing patch.

I added some in-line comments to the commits.  Let's talk about your destroyable mixin today at the meeting.
Attachment #490608 - Flags: feedback?(adw) → feedback+
Oh also, the widget commit conflicts with my widget patch in bug 612169.  I'll hold off on landing that patch until we talk more about how we want destroyed objects to behave.
(In reply to comment #4)
> Oh also, the widget commit conflicts with my widget patch in bug 612169.  I'll
> hold off on landing that patch until we talk more about how we want destroyed
> objects to behave.

I talked to Myk briefly yesterday and he thinks that the destroyable mixin and consistent behavior of destroyed objects across APIs aren't beta blockers, so I went ahead and landed my widget patch.  If you do end up making a destroyable mixin, I'd be happy to write follow-up patches that use it for widget and the other APIs I'm converting to construct/destroy.
Attachment #490608 - Attachment is obsolete: true
Comment on attachment 493760 [details]
Pointer to pull request

Ok, requesting review on this. I'm not particularly confident that this is correct, since I don't know the symbiont or panel code very well, and there's a lot of asynchronous eventery going on here that I don't fully understand. I've mentioned at least one doubt of mine on the GitHub pull request.
Attachment #493760 - Flags: review?(adw)
Comment on attachment 493760 [details]
Pointer to pull request

Reassigning the review to myk since this code is mondo complicated and Irakli said myk knew it well.
Attachment #493760 - Flags: review?(adw) → review?(myk)
Comment on attachment 493760 [details]
Pointer to pull request

diff --git a/packages/addon-kit/lib/panel.js b/packages/addon-kit/lib/panel.js

   /* Public API: Panel.show */
   show: function show(anchor) {
     // do nothing if already open
-    if (!PanelRegistry.has(this))
-      throw new Error(ERR_ADD);
-

The "do nothing" comment here doesn't make sense anymore.


diff --git a/packages/jetpack-core/docs/self.md b/packages/jetpack-core/docs/self.md

-    var panels = require("panel");
-    var myPanel = panels.Panel({
+    var myPanel = require("panel").Panel({

Nit: this simplification would be handy in examples/reddit-panel/lib/main.js too!


+    // perform all deferred tasks like initSymbiont, show, hide ...
+    this._emit('inited');
+    this._removeAllListeners('inited');

Hrm, this API should not expose an internal event like "inited" publicly (not to mention event type names should be present tense), but that's another bug.


It's difficult for me to follow the convoluted renamings of functions through the stack of inherited traits (which Irakli and I have previously discussed flattening significantly down to a single level of inheritance), but the reason Panel has to wait for initialization is that its content is stored in a hidden frame until the panel is shown, and we can't guarantee synchronous creation of hidden frames, because we create them in the hidden window, which is HTML on Windows/Linux, so we have to first load an XML page in a frame of the hidden window and then load the hidden frame inside that XML file once it finishes loading on those OSes.
Attachment #493760 - Flags: review?(myk) → review+
Thanks, I fixed myk's nits in comment 9 and pushed:

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

By the way, myk, the reason I didn't remove the comment 'do nothing if already open' was because the code it was directly above (which was removed in this patch) didn't actually seem relevant to it, since the removed code was about throwing an exception if the panel wasn't already added to the panel registry. So I assumed the comment was for the function in general, though in retrospect it was more like end-user documentation rather than implementation-specific documentation, so I still removed it.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.