912891 - [app manager] toolbox in a new tab

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect, P1)

Description

[Paul Rouget [:paul]]

The toolbox should be opened in a new tab. That means loading toolbox.xul in a tabbrowser. We'd need to get rid of the `iframe.onload` calls.

Orion won't work though.

Comment 1

[Paul Rouget [:paul]]

I confirm that it works with CodeMirror.

Comment 2

[Paul Rouget [:paul]]

Attachment: Use TAB host. Patch v1

Comment 3

[Paul Rouget [:paul]]

Attachment: Implement TAB host. Patch v1

Heather, I'm not finished yet. I need to block the reload button, make sure the toolbox get closed properly, and find a way to have a better URL.

But early feedback would be appreciated.

Comment 4

[Paul Rouget [:paul]]

Close button is not hidden.

Comment 5

[Paul Rouget [:paul]]

TypeError: this.doc is undefined: Toolbox.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:904
UI.openToolbox/</</<@chrome://browser/content/devtools/app-manager/device.js:151
EventEmitter_once/handler<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/event-emitter.js:63
EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/event-emitter.js:110
Connection.prototype._setStatus@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/client/connection-manager.js:239
Connection.prototype._onDisconnected@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/client/connection-manager.js:263
eventSource/EV_addOneTimeListener/l@resource://gre/modules/devtools/dbg-client.jsm:103
EV_notify@resource://gre/modules/devtools/dbg-client.jsm:160
DC_onClosed@resource://gre/modules/devtools/dbg-client.jsm:716
DT_onStopRequest@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/server/transport.js:133
makeInfallible/<@resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:75

TypeError: this.doc is undefined: Toolbox.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:904
UI.debug/onTargetReady/</<@chrome://browser/content/devtools/app-manager/projects.js:326
EventEmitter_once/handler<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/event-emitter.js:63
EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/event-emitter.js:110
Connection.prototype._setStatus@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/client/connection-manager.js:239
Connection.prototype._onDisconnected@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/client/connection-manager.js:263
eventSource/EV_addOneTimeListener/l@resource://gre/modules/devtools/dbg-client.jsm:103
EV_notify@resource://gre/modules/devtools/dbg-client.jsm:160
DC_onClosed@resource://gre/modules/devtools/dbg-client.jsm:716
DT_onStopRequest@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/server/transport.js:133
makeInfallible/<@resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:75

Comment 6

[Heather Arthur [:harth]]

Comment on attachment 815401 [details] [diff] [review]
Implement TAB host. Patch v1

Review of attachment 815401 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a good approach to me.

::: browser/devtools/framework/toolbox-hosts.js
@@ +296,5 @@
>  
>  /**
> + * Host object for the toolbox in its own tab
> + */
> +function TabHost(hostTab, cid) {

what is cid? comment.

@@ +331,5 @@
> +
> +  /**
> +   * Raise the host.
> +   */
> +  raise: function RH_raise() {

Might want to just call the focusTab() function here, it will also focus the right browser window.

::: browser/devtools/shared/DOMHelpers.jsm
@@ +126,5 @@
>      delete this.window;
>      delete this.treeWalker;
> +  },
> +
> +  onceWindowLoaded: function Helpers_onLocationChange(callback) {

Why do we need to do all this stuff instead of just waiting for a load event? Maybe add a comment as to why.

Comment 7

[Paul Rouget [:paul]]

I'm trying a different approach: opening the toolbox in a tab in index.xul (inside about:app-manager)

Comment 8

[Paul Rouget [:paul]]

Attachment: v2

Still need some polish and tests.

Comment 9

[Paul Rouget [:paul]]

Attachment: v3

https://tbpl.mozilla.org/?tree=Try&rev=7b9304a90372

Comment 11

[Paul Rouget [:paul]]

Try failed for an unknown reason. Re-pushing: https://tbpl.mozilla.org/?tree=Try&rev=669f264088eb

Comment 12

[Paul Rouget [:paul]]

Attachment: Part 1: Custom host

Comment 13

[Paul Rouget [:paul]]

Attachment: Part 2: toolbox in tab

Comment 14

[Paul Rouget [:paul]]

Some comments about the issues I've been running into:

We load documents in frames. tabbrowser > index.xul > toolbox.xul > panels. Being notified when these documents are available is not simple. In content docshells (tabbrowser), document events don't bubble up to the containing iframe. In windows (toolbox-window.xul and browser.xul, for non-custom hosts), they do. So we need to use the chromeEventHandler. The chromeEventHandler is reachable through the docshell, which is accessible via iframe.contentWindow. iframe.contentWindow is apparently not always available right after the creation of the iframe in the case of side or bottom hosts (in chrome docshells). That's why we need 2 paths (listen DOMContentLoaded from the DOM node or from the chromeEventHandler). Also, docshells are apparently not systematically created when iframes are not visible. That's why we show the iframe before starting the toolbox.

Comment 15

[Paul Rouget [:paul]]

In the case of chrome docshells, the iframe IS the chromeEventHandler. So maybe there's an easy way to know what is the chromeEventHandler with no access to the docshell. But it will end-up being the same kind of code path.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 828008 [details] [diff] [review]
Part 2: toolbox in tab

Review of attachment 828008 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just don't feel confortable calling JS across documents.
Otherwise I gave it a try and that works really well!

::: browser/devtools/app-manager/content/device.js
@@ +167,5 @@
>      getTargetForApp(this.connection.client,
>                      this.listTabsResponse.webappsActor,
>                      manifest).then((target) => {
> +
> +      top.UI.openAndShowToolboxForTarget(target, app.name, app.iconURL);

Shouldn't we send a DOM message instead of calling JS across documents like this?
We could just send the manifest url and let index.js get the target,icon and name... Otherwise, I think target is a JSON object, so that it can be sent in a DOM message as well.

Comment 17

[Heather Arthur [:harth]]

Comment on attachment 827996 [details] [diff] [review]
Part 1: Custom host

Review of attachment 827996 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/framework/toolbox-hosts.js
@@ +276,5 @@
>  
>  /**
> + * Host object for the toolbox in its own tab
> + */
> +function CustomHost(hostTab, options) {

My thought is that "TabHost" is more descriptive of this host, but up to you.

@@ +288,5 @@
> +
> +  _sendMessageToTopWindow: function CH__sendMessageToTopWindow(msg) {
> +    let topWindow = this.frame.ownerDocument.defaultView;
> +    let json = {name:"toolbox-" + msg, uid: this.uid}
> +    topWindow.postMessage(JSON.stringify(json), "*");

Whoa, this is some crazy stuff indeed. I trust that you need to do this. But I request that you add some comments about what's going on and why.

Comment 18

[Paul Rouget [:paul]]

(In reply to Alexandre Poirot (:ochameau) from comment #16)
> ::: browser/devtools/app-manager/content/device.js
> @@ +167,5 @@
> >      getTargetForApp(this.connection.client,
> >                      this.listTabsResponse.webappsActor,
> >                      manifest).then((target) => {
> > +
> > +      top.UI.openAndShowToolboxForTarget(target, app.name, app.iconURL);
> 
> Shouldn't we send a DOM message instead of calling JS across documents like
> this?
> We could just send the manifest url and let index.js get the target,icon and
> name... Otherwise, I think target is a JSON object, so that it can be sent
> in a DOM message as well.

A target is not a JSON object. We could just copy the value that are needed to build the toolbox, but then it's hard to track unique targets (see the Map in index.js), and I'm not even sure it would work (what about the event-emitter functions?). Building the target at index.js level would be ideal, but then index.js needs to be able to send back success / errors message to honor the promises. This needs to be done, but I'll do that later if that works for you.

Comment 19

[Paul Rouget [:paul]]

Talked to Alex: we will fix this top.UI thing in bug 919502 when we will reboot the way buttons are disabled/enabled.

Comment 20

[Paul Rouget [:paul]]

Attachment: Part 1: Custom host

Addressed Heather's comment (added a comment).

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/90624f34525a
https://hg.mozilla.org/integration/fx-team/rev/e41b160a4ada

Comment 23

[Ryan VanderMeulen [:RyanVM]]

This was backed out while investigating a test failure and re-landed after being exonerated :)

https://hg.mozilla.org/integration/fx-team/rev/e5b40752f743
https://hg.mozilla.org/integration/fx-team/rev/a0737b57f233

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e5b40752f743
https://hg.mozilla.org/mozilla-central/rev/a0737b57f233

Comment 25

[Victor Porof [:vporof][:vp]]

I think this might have caused bug 936379 :(

Comment 26

[Victor Porof [:vporof][:vp]]

Attachment: bug.webm

I managed to get the toolbox in the same state as the screenshot [0] from bug 936379, with the same error: 

INFO -  * Call to xpconnect wrapped JSObject produced this error:  *
INFO -  [Exception... "'[JavaScript Error: "closeButton is null" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js" line: 197}]' when calling method: [nsIRunnable::run]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]

STR:
1. mach build browser
2. mach run -P yourDevelopmentProfile
3. Very quickly hit Cmd+Alt+I while the window opens

Running without building inside browser first doesn't trigger the error.

There's a video attached evidentiating the bug.

[0] https://tbpl.mozilla.org/php/getParsedLog.php?id=30308828&tree=Fx-Team#error0

Comment 27

[Victor Porof [:vporof][:vp]]

The exact same error causes bug 936404:

INFO -  * Call to xpconnect wrapped JSObject produced this error:  *
INFO -  [Exception... "'[JavaScript Error: "closeButton is null" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js" line: 197}]' when calling method: [nsIRunnable::run]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]

Comment 28

[Paul Rouget [:paul]]

(keeping the needinfo as I don't want to forget about that)

We get an unexpected load event. This probably happens because when call toolbox.load [1], the iframe is not fully loaded. I'll look at this on Tuesday.


[1] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#187

Comment 29

[Carsten Book [:Tomcat]]

backed out this changes in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1df18bd40f7f for nearly perma orange causing bug  936404 and Bug 936379 and closed the trees today.

Comment 30

[Carsten Book [:Tomcat]]

note the failure was maybe 70 to 80 % on all trees so if we test that patch again, maybe some retriggers are needed to be sure its fixed

Comment 31

[Paul Rouget [:paul]]

Attachment: addendum 1

Comment 32

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=a5875209fc9c

Comment 33

[Paul Rouget [:paul]]

Green. Let's try again: https://tbpl.mozilla.org/?tree=Try&rev=20ac32214603

Comment 34

[Paul Rouget [:paul]]

2 greens. Should be enough to land again.

Comment 35

[Patrick Brosset <:pbro>]

Comment on attachment 830877 [details] [diff] [review]
addendum 1

Review of attachment 830877 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/b0848c25b932
https://hg.mozilla.org/integration/fx-team/rev/12ae6af81b1c
https://hg.mozilla.org/integration/fx-team/rev/aa1b7501afb3

Comment 37

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/b0848c25b932
https://hg.mozilla.org/mozilla-central/rev/12ae6af81b1c
https://hg.mozilla.org/mozilla-central/rev/aa1b7501afb3