Closed
Bug 912891
Opened 11 years ago
Closed 11 years ago
[app manager] toolbox in a new tab
Categories
(DevTools Graveyard :: WebIDE, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 28
People
(Reporter: paul, Assigned: paul)
References
Details
Attachments
(4 files, 5 obsolete files)
(deleted),
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
paul
:
review+
|
Details | Diff | Splinter Review |
(deleted),
video/webm
|
Details | |
(deleted),
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
I confirm that it works with CodeMirror.
Assignee | ||
Updated•11 years ago
|
Priority: -- → P2
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
Close button is not hidden.
Assignee | ||
Comment 5•11 years ago
|
||
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•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Attachment #815395 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 7•11 years ago
|
||
I'm trying a different approach: opening the toolbox in a tab in index.xul (inside about:app-manager)
Assignee | ||
Updated•11 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 8•11 years ago
|
||
Still need some polish and tests.
Attachment #815395 -
Attachment is obsolete: true
Attachment #815401 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #827254 -
Attachment is obsolete: true
Assignee | ||
Comment 11•11 years ago
|
||
Try failed for an unknown reason. Re-pushing: https://tbpl.mozilla.org/?tree=Try&rev=669f264088eb
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #827996 -
Flags: review?
Assignee | ||
Updated•11 years ago
|
Attachment #827996 -
Flags: review? → review?(fayearthur)
Assignee | ||
Updated•11 years ago
|
Attachment #827599 -
Attachment is obsolete: true
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #828008 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 14•11 years ago
|
||
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.
Assignee | ||
Comment 15•11 years ago
|
||
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•11 years ago
|
||
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 17•11 years ago
|
||
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+
Assignee | ||
Comment 18•11 years ago
|
||
(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.
Assignee | ||
Comment 19•11 years ago
|
||
Talked to Alex: we will fix this top.UI thing in bug 919502 when we will reboot the way buttons are disabled/enabled.
Assignee | ||
Comment 20•11 years ago
|
||
Addressed Heather's comment (added a comment).
Attachment #827996 -
Attachment is obsolete: true
Attachment #828603 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 21•11 years ago
|
||
Comment 23•11 years ago
|
||
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: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Comment 25•11 years ago
|
||
I think this might have caused bug 936379 :(
Comment 26•11 years ago
|
||
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
Updated•11 years ago
|
Flags: needinfo?(paul)
Comment 27•11 years ago
|
||
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
Assignee | ||
Comment 28•11 years ago
|
||
(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•11 years ago
|
||
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 → ---
Comment 30•11 years ago
|
||
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
Assignee | ||
Comment 31•11 years ago
|
||
Attachment #830877 -
Flags: review?(fayearthur)
Flags: needinfo?(paul)
Assignee | ||
Comment 32•11 years ago
|
||
Assignee | ||
Comment 33•11 years ago
|
||
Green. Let's try again: https://tbpl.mozilla.org/?tree=Try&rev=20ac32214603
Assignee | ||
Comment 34•11 years ago
|
||
2 greens. Should be enough to land again.
Comment 35•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 36•11 years ago
|
||
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
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 37•11 years ago
|
||
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: 11 years ago → 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•5 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•