980410 - Toolbox panel API

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: WIP implementation

Comment 2

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

Attachment: Fixes from rpl

Comment 3

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

Attachment: Feature proposal

Comment 4

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

Attachment: First cut

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

This is pretty huge, even not counting volcan. In the future can we please please please get smaller, incremental changesets for reviewing and testing so we don't run into giant reviews and surprises? Having seen the signal patch, tab/windows patch, and this one, all introduce many new utilities and digesting them all at once for a large surface area is pretty rough for the reviewer, and for the subsequent maintainers.


What are the examples testing? Just that they run correctly? I haven't even looked at them yet.
I don't see any tests for all of the utility methods and files added other than for the panel.
I don't like `lib/dev` for the directory housing all of these methods. `dev` is meaningless in this context, as everything could pretty much be `dev`. How about `devtools`?

I added some comments, but for a more thorough review, it'll be a few

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8437255 [details] [review]
First cut

Lots of comments made in GitHub and above ^, a lot needs clarification

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

ONE comments related to the Pane object and setup.define method.

It would be great if frame reference is also passed into the
panel.setup method:

panel.setup({ debuggee: debuggee, frame: frame });

This allows derived objects to manipulate the final location of the frame object within the DOM. It's useful in cases when an extension wants to change layout of the panel content.

To make sure the extension can move the frame element in the DOM, the onFrameRemove should be added *after* panel.setup executes (otherwise remove event it's being fired). Here is suggested order of actions:

setup.define(Panel, (panel, {window, toolbox, url}) => {
  // Hack: Given that iframe created by devtools API is no good for us,
  // we obtain original iframe and replace it with the one that has
  // desired configuration.
  const original = getFrameElement(window);
  const frame = original.cloneNode(true);

  setAttributes(frame, {
    "src": url,
    "sandbox": "allow-scripts",
//    "remote": true,
    "type": "content",
    "transparent": true,
    "seamless": "seamless"
  });

  original.parentNode.replaceChild(frame, original);
  frame.style.visibility = "hidden";

  const debuggee = fromTarget(toolbox.target);
  // associate debuggee with a panel.
  debuggees.set(panel, debuggee);

  // xxxHonza: pass also frame into the setup method.
  // xxxHonza: call setup before listeners registration,
  // so the setup can move the frame element to another
  // DOM location when changing panel's layout.
  panel.setup({ debuggee: debuggee, frame: frame });

  // associate panel model with a frame view.
  panels.set(frame, panel);

  // Setup listeners for the frame message manager.
  const { messageManager } = frame.frameLoader;
  messageManager.addMessageListener("sdk/event/ready", onStateChange);
  messageManager.addMessageListener("sdk/event/load", onStateChange);
  messageManager.addMessageListener("sdk/event/unload", onStateChange);
  messageManager.addMessageListener("sdk/port/message", onPortMessage);
  messageManager.loadFrameScript(FRAME_SCRIPT, false);

  managers.set(panel, messageManager);

  // show frame when it is initialized.
  inited(frame).then(onFrameInited);

  // set listeners if there are ones defined on the prototype.
  setListeners(panel, Object.getPrototypeOf(panel));

  removed(frame).then(onFrameRemove);
});


What do you think?
Honza

Comment 8

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

Attachment: v2.0

Comment 9

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

Note I have not updated dev to devtools, mainly because I would not like to conflict with devtools namespace. That being said I'm not married to dev/ either, but let's decide on the directory independently from this change please.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

What about my comment #7, any chance to get the frame reference?

Honza

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8455898 [details] [review]
v2.0

Looks good, let's get it landed! I'd like more tests for the underlying components, but we can add that as we go

Comment 12

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/f9c07a2ecc62bcef35111d21fbb7dd88feecf54a
Merge pull request #1509 from Gozala/devtools

Bug 980410 - Implementation of toolbox panel r=@jsantell

Comment 13

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

Attachment: Proposed API changes to enable low level hooks

This is an attempt to address Honza's request of adding more hooks to take part in iframe creation process. While this is not exactly the way changes were requested (for reasons disclosed in pull) I do believe it address all the points made in the request. Asking for feedback to make sure that is the case.

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #13)

1) The `createView` hook has two arguments `panel` and parent `document`. There should also be the panel container (the default parentNode), so extensions know where to put additional XUL stuff. This argument could replace the `document` since container.ownerDocument === document.

Something like:
const frame = createView(panel, container);

2) Could we pass `toolbox` into the setup method? The toolbox is pretty useful object that can be used to get the debugging target, reference to the other panels, etc. This would make possible to get access to it. Or is there any other preferred way how to get the reference to the current toolbox? (from within a panel)

Honza

Comment 15

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

(In reply to Jan Honza Odvarko [:Honza] from comment #14)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #13)
> 
> 1) The `createView` hook has two arguments `panel` and parent `document`.
> There should also be the panel container (the default parentNode), so
> extensions know where to put additional XUL stuff. This argument could
> replace the `document` since container.ownerDocument === document.
> 
> Something like:
> const frame = createView(panel, container);


I'm not totally opposed to this idea, although still wonder if this is really necessary. If you decide to override where the iframe target then you should probably know how to reach that target without default container. If you just wanna style container why not style the iframe itself before returning it. Passing a container would imply that there is a container and there always be same kind of container, so I'd rather don't commit to either of these things. But if this is a must have, I'm not totally opposed. 

 
> 2) Could we pass `toolbox` into the setup method? The toolbox is pretty
> useful object that can be used to get the debugging target, reference to the
> other panels, etc. This would make possible to get access to it. Or is there
> any other preferred way how to get the reference to the current toolbox?
> (from within a panel)
> 

Well exposing low level mechanics to the public APIs like setup is something we would really avoid, as it would mean that we commit to all of the toolbox APIs which would be too much burden for SDK team. That is why separate low level hook was provided to avoid exposing stuff like iframes. That being said `viewFor(panel) -> HTMLIFrameElement` has being added which can be used to get iframe, with which toolbox element can be obtained as it's in the parent element chain.

As of getting debugging target, we already expose debugging target in form of `debuggee` object implementing MessagePort API. For other useful things that toolbox has we would rather design APIs tailored for them.

In my view if you have `iframe` element you can reach `toolbox` on your own & also think in the process whether or not it is a good idea as you have to touch things that have not being passed to you. If we hand `toolbox` to you we would leave wrong impression that it's supported by us. I hope this explanation makes sense.

I would not be opposed to adding some kind of helper function to our low level module: https://github.com/mozilla/addon-sdk/blob/master/lib/dev/utils.js to make it easy to get hold of toolbox given the iframe.
 

> 
> Honza

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

> If you decide to override where the iframe target then you should probably
> know how to reach that target without default container.
Ok, extensions can use Panel.id to query for the parent element.

> `viewFor(panel) -> HTMLIFrameElement` has being added which can be used to get iframe
Nice!

> In my view if you have `iframe` element you can reach `toolbox`
Ok, extensions can do it this way.

> I would not be opposed to adding some kind of helper function to our low level module: 
Yep, sounds good to me. Firebug has:
https://github.com/firebug/firebug.next/blob/master/lib/firebug.js#L242
... but it could be yet somehow generalized I think.

---

One more note. The Panel setup method implementation calls setListeners at the end:

// set listeners if there are ones defined on the prototype.
setListeners(panel, Object.getPrototypeOf(panel));

This iterates methods of the actual panel instance (its prototype), but not derived methods.
If there is a hierarchy of panels, it doesn't work as expected.

Let's imagine the following hierarchy:
Panel
  BasePanel
    MyPanel 

If e.g. onReady is implemented by BasePanel and the developer creates an instance of MyPanel (derived
from BasePanel) the event is not handled since the method is not available directly on the MyPanel prototype. This is quite confusing, can we change that?

Or what would be the workaround to make sure that onReady on BasePanel is always executed (once) no matter if the derived panels (e.g. MyPanel) implements it or not.


Honza

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Irakli: please see my previous comment, starts with "One more note..." 

Honza

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Irakli: could we lend the attached patch?

The pull request is here:
https://github.com/mozilla/addon-sdk/pull/1571

See also my comment #16

Thanks!
Honza

Comment 19

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8468118 [details] [review]
Proposed API changes to enable low level hooks

Looks good!

Comment 20

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Isn't this landed?