Closed Bug 912891 Opened 6 years ago Closed 6 years ago

[app manager] toolbox in a new tab

Categories

(DevTools :: WebIDE, defect, P1)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(4 files, 5 obsolete files)

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.
I confirm that it works with CodeMirror.
Priority: -- → P2
Depends on: 919978
No longer depends on: 816756
Attached patch Use TAB host. Patch v1 (obsolete) — Splinter Review
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #815395 - Flags: review?(poirot.alex)
Attached patch Implement TAB host. Patch v1 (obsolete) — Splinter Review
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.
Attachment #815401 - Flags: feedback?(fayearthur)
Close button is not hidden.
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 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.
Attachment #815401 - Flags: feedback?(fayearthur) → feedback+
Attachment #815395 - Flags: review?(poirot.alex)
I'm trying a different approach: opening the toolbox in a tab in index.xul (inside about:app-manager)
Priority: P2 → P1
Depends on: 918695
Attached patch v2 (obsolete) — Splinter Review
Still need some polish and tests.
Attachment #815395 - Attachment is obsolete: true
Attachment #815401 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=7b9304a90372
Attachment #827254 - Attachment is obsolete: true
Duplicate of this bug: 916487
Try failed for an unknown reason. Re-pushing: https://tbpl.mozilla.org/?tree=Try&rev=669f264088eb
Attached patch Part 1: Custom host (obsolete) — Splinter Review
Attachment #827996 - Flags: review?
Attachment #827996 - Flags: review? → review?(fayearthur)
Attachment #827599 - Attachment is obsolete: true
Attachment #828008 - Flags: review?(poirot.alex)
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.
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 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.
Attachment #828008 - Flags: review?(poirot.alex) → review+
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.
Attachment #827996 - Flags: review?(fayearthur) → review+
(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.
Talked to Alex: we will fix this top.UI thing in bug 919502 when we will reboot the way buttons are disabled/enabled.
Addressed Heather's comment (added a comment).
Attachment #827996 - Attachment is obsolete: true
Attachment #828603 - Flags: review+
Keywords: checkin-needed
Duplicate of this bug: 935263
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
https://hg.mozilla.org/mozilla-central/rev/e5b40752f743
https://hg.mozilla.org/mozilla-central/rev/a0737b57f233
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
I think this might have caused bug 936379 :(
Depends on: 936379
Attached video 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
Flags: needinfo?(paul)
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]
Depends on: 936404
(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
Depends on: 936892
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attached patch addendum 1Splinter Review
Attachment #830877 - Flags: review?(fayearthur)
Flags: needinfo?(paul)
2 greens. Should be enough to land again.
Comment on attachment 830877 [details] [diff] [review]
addendum 1

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

Looks good to me
Attachment #830877 - Flags: review?(fayearthur) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b0848c25b932
https://hg.mozilla.org/mozilla-central/rev/12ae6af81b1c
https://hg.mozilla.org/mozilla-central/rev/aa1b7501afb3
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.