611763 - switch to construct/destroy model for Panel

Status: RESOLVED FIXED

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

Description

[Atul Varma [:atul]]

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.

Comment 1

[Atul Varma [:atul]]

Work-in-progress branch is here:

https://github.com/toolness/jetpack-sdk/tree/bug-611763

Comment 2

[Atul Varma [:atul]]

Attachment: Pointer to Github pull request containing patch.

Drew, can you take a look at this and let me know what you think?

Comment 3

[Drew Willcoxon :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.

Comment 4

[Drew Willcoxon :adw]

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.

Comment 5

[Drew Willcoxon :adw]

(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.

Comment 6

[Atul Varma [:atul]]

Attachment: Pointer to pull request

Comment 7

[Atul Varma [:atul]]

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.

Comment 8

[Atul Varma [:atul]]

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.

Comment 9

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

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.

Comment 10

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

I have commented how that event can be removed entirely:

https://github.com/toolness/jetpack-sdk/commit/4d38530444b5bbc36575f2d35ed93c99021119cc#commitcomment-206293

Comment 11

[Atul Varma [:atul]]

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.