Closed
Bug 1266134
Opened 8 years ago
Closed 8 years ago
Pull out Toolbox Host management out of toolbox.js
Categories
(DevTools :: Framework, enhancement, P1)
DevTools
Framework
Tracking
(firefox52 verified)
Tracking | Status | |
---|---|---|
firefox52 | --- | verified |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Depends on 1 open bug)
Details
(Whiteboard: [devtools-html])
Attachments
(5 files, 20 obsolete files)
(deleted),
text/x-review-board-request
|
jryans
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jryans
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jryans
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jryans
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jryans
:
review+
|
Details |
In a future where toolbox.xul is replaced with toolbox.html, one particular piece of code that depends on XUL in the Toolbox Hosts. Today, it is fully entangled into Toolbox initialization, within toolbox.js. But toolbox.js should end up only using non-privileged code. This code, toolbox hosts, should be pulled out of the toolbox on stay in the set of privileged code that are here to stay, like gDevToolsBrowser or similar.
Assignee | ||
Comment 1•8 years ago
|
||
Toolbox host are defined over here: http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox-hosts.js And here are the various functions related to them within toolbox.js: http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#1744
Assignee | ||
Updated•8 years ago
|
Blocks: devtools-html-3
Updated•8 years ago
|
Severity: normal → enhancement
Whiteboard: [devtools-html]
Updated•8 years ago
|
Updated•8 years ago
|
Flags: qe-verify+
Priority: -- → P2
QA Contact: alexandra.lucinet
Updated•8 years ago
|
Priority: P2 → P3
Whiteboard: [devtools-html] → [reserve-html]
Updated•8 years ago
|
No longer blocks: devtools-html-2
Whiteboard: [reserve-html] → [devtools-html] [triage]
Updated•8 years ago
|
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 2•8 years ago
|
||
Taking as I already started looking into this.
Assignee: nobody → poirot.alex
Updated•8 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
Updated•8 years ago
|
QA Contact: adalucin → petruta.rasa
Updated•8 years ago
|
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29
Assignee | ||
Comment 3•8 years ago
|
||
I'm making serious progress on this. It is going to be quite a quest as it involves some critical path that are very messy: toolbox init and destroy. I already discovered that we very easily leak toolboxes if we slightly delay host destruction! Switching host.destroy() from settleAll(outstanding) resolution to "destroyed" event ends up introducing toolbox object leaks which are hopefully catched on try with various test timeout. Tests timeout because the leak detector takes a big additional time parsing the various objects held by the toolbox, but it doesn't report any leak... https://treeherder.mozilla.org/#/jobs?repo=try&revision=5545ae082094 My plan here is to stop calling toolbox.js code directly from the chrome code. The code managing the host and firefox UI integration is chrome and going to stay chrome. My current patches use DOM "message" event to communicate between the chrome and the toolbox. I'm not bound to this, the hard work isn't here. It is about finding the critical paths and all the races that you run into when slightly modifying the call sites.
Assignee | ||
Comment 4•8 years ago
|
||
Without the leak and with even less call between chrome and toolbox.js https://treeherder.mozilla.org/#/jobs?repo=try&revision=0da1a017b50b
Assignee | ||
Comment 5•8 years ago
|
||
Here is a first patch queue. I tried to start splitting in meaningful pieces. This patch is just a naive cleanup in order to use existing helpers instead of manually querying what these helpers try to simplify.
Assignee | ||
Comment 6•8 years ago
|
||
This cleanup simplify the shutdown codepath a bit. Today, there is a special case just for WINDOW host. We listen for the unload event on the chrome window opened for the toolbox, and manually call toolbox.destroy via the window-closed event sent from toolbox-hosts.js to toolbox.js. But we could just listen for unload event on toolbox.xul, that's what we are already doing for about:devtools-toolbox, when it is loaded in a tab.
Assignee | ||
Comment 7•8 years ago
|
||
Main patch where we do move host management out of toolbox.js to a dedicated module toolbox-wrapper.js
Assignee | ||
Comment 8•8 years ago
|
||
And all the necessary tweaks to be done on tests in order to have a green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f136e77cdac5&selectedJob=26215968 Most modifications are about waiting differently for host title change. We now have to wait for a DOM message event to be fired on the host window. toolbox.js send a message to its parent document "set-host-title" whenever it wants to update the toolbox title.
Assignee | ||
Comment 9•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2915a9c09a4
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c000ac7bc29
Assignee | ||
Comment 11•8 years ago
|
||
I think these patches are ready for review. I was finally able to fully setup git cinnabar! I will shortly try to submit patches via mozreview. One followup after this would be to stop passing the target object from chrome to the toolbox, that, to let the toolbox compute it from the url query parameter, like about:devtools-toolbox loaded in a tab. With this current patch queue, we still create the Toolbox object from the chrome.
Assignee | ||
Comment 12•8 years ago
|
||
I'm moving all the cleanup patches to bug 1298082 in order to get them landed sooner than later, they should all be straightforward. Whereas the host refactoring may be subject to debate.
Updated•8 years ago
|
Iteration: 51.2 - Aug 29 → 51.3 - Sep 12
Assignee | ||
Updated•8 years ago
|
Attachment #8784133 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8784134 -
Attachment is obsolete: true
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c47b8061fb7b
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8784135 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8784137 -
Attachment is obsolete: true
Assignee | ||
Comment 17•8 years ago
|
||
I'm r? early as the first patch is touching critical codepaths around toolbox start/destruction. But try reports leaks on debug builds that I still need to address.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•8 years ago
|
||
The second patch in the queue should fix the leak, or at least one! I could have merged it into the first patch, but kept it seperated to help understanding this. We have to listen for "unload" event on capture... The last patch allow to fix this gcli test which always fails locally for me when running it alone on a debug build. It looks like this is a general issue we could have in many test involve iframes! I tried to push a general fix for all tests, but I will submit a patch in a dedicated bug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee8f04f1939f https://hg.mozilla.org/try/rev/91c9889eaf47482c49e37ef86652e7a432222f64
Assignee | ||
Comment 23•8 years ago
|
||
I opened bug 1300822 for the issue with load events.
I'll try to look at this tomorrow, but I am a bit busy that day as well. Sorry for the delay.
Assignee | ||
Comment 25•8 years ago
|
||
There is still other leaks to be chased, so still not rush in reviewing.
Comment 26•8 years ago
|
||
mozreview-review |
Comment on attachment 8788384 [details] Bug 1266134 - Pull host management out of toolbox.xul. https://reviewboard.mozilla.org/r/76878/#review76038 Overall, I think the idea makes sense, but still some things to work through. ::: devtools/client/framework/devtools.js:421 (Diff revision 1) > > return hostPromise.then(function () { > toolbox.raise(); > return toolbox; > }); > } Nit: change to } else { ::: devtools/client/framework/devtools.js:426 (Diff revision 1) > - toolbox = new Toolbox(target, toolId, hostType, hostOptions); > - > - this.emit("toolbox-created", toolbox); > > + wrapper.create(toolId) > + .then(toolbox => { Let's convert this function to Task.async instead of dropping more .then()s around. ::: devtools/client/framework/toolbox-wrapper.js:12 (Diff revision 1) > +loader.lazyRequireGetter(this, "Hosts", "devtools/client/framework/toolbox-hosts", true); > + > +/** > + * Implement a wrapper on the chrome side to setup a Toolbox within Firefox UI. > + * > + * This components handles iframe creation within Firefox, in which we are loading Nit: component ::: devtools/client/framework/toolbox-wrapper.js:17 (Diff revision 1) > + * This components handles iframe creation within Firefox, in which we are loading > + * the toolbox document. Then both the chrome and the toolbox document communicate > + * via "message" events. > + * > + * Messages sent by the toolbox to the chrome: > + * - switch-host: order to display the toolbox in another host (side, bottom or window) There are a lot more, update the comments. ::: devtools/client/framework/toolbox-wrapper.js:26 (Diff revision 1) > + */ > + > +const LAST_HOST = "devtools.toolbox.host"; > +let ID_COUNTER = 1; > + > +function ToolboxWrapper(target, hostType, hostOptions) { This needs a less ambiguous name... Perhaps `ToolboxChromeManager`? `ToolboxBrowserIntegration`? ::: devtools/client/framework/toolbox-wrapper.js:41 (Diff revision 1) > + this.host = this.createHost(hostType, hostOptions); > +} > + > +ToolboxWrapper.prototype = { > + create(toolId) { > + return this.host.create() Use Task.async ::: devtools/client/framework/toolbox-wrapper.js:49 (Diff revision 1) > + this.host.frame.ownerDocument.defaultView.addEventListener("message", this); > + this.host.frame.addEventListener("unload", this); > + > + let toolbox = new Toolbox(this.target, toolId, this.host.type, this.host.frame.contentWindow, this.frameId); > + > + // Prevent reloading the toolbox when loading the tools in a tab (e.g. from about:debugging) I don't follow how the comment and this block are related. ::: devtools/client/framework/toolbox-wrapper.js:73 (Diff revision 1) > + break; > + } > + }, > + > + onMessage(event) { > + if (!event.data) return; Nit: use brackets, new line ::: devtools/client/framework/toolbox-wrapper.js:115 (Diff revision 1) > + }, > + > + /** > + * Create a host object based on the given host type. > + * > + * Warning: some hosts require that the toolbox target provides a reference to Seems best to provide more details about which hosts require a tab. ::: devtools/client/framework/toolbox-wrapper.js:148 (Diff revision 1) > + this.postMessage({ > + name: "host-maximized" > + }); > + }, > + > + switchHost(hostType) { Use Task.async ::: devtools/client/framework/toolbox-wrapper.js:153 (Diff revision 1) > + switchHost(hostType) { > + let iframe = this.host.frame; > + let newHost = this.createHost(hostType); > + return newHost.create().then(newIframe => { > + // change toolbox document's parent to the new host > + newIframe.QueryInterface(Ci.nsIFrameLoaderOwner); I know this `QueryInterface` is in the old code, but it shouldn't be needed, so let's drop it here. ::: devtools/client/framework/toolbox.js (Diff revision 1) > * The object the toolbox is debugging. > * @param {string} selectedTool > * Tool to select initially > * @param {Toolbox.HostType} hostType > * Type of host that will host the toolbox (e.g. sidebar, window) > - * @param {object} hostOptions Document the meaning of the new options. ::: devtools/client/framework/toolbox.js:190 (Diff revision 1) > > _prefs: { > - LAST_HOST: "devtools.toolbox.host", > LAST_TOOL: "devtools.toolbox.selectedTool", > SIDE_ENABLED: "devtools.toolbox.sideEnabled", > PREVIOUS_HOST: "devtools.toolbox.previousHost" Is `PREVIOUS_HOST` still used? If so, should it leave? ::: devtools/client/framework/toolbox.js:267 (Diff revision 1) > /** > * Get/alter the host of a Toolbox, i.e. is it in browser or in a separate > * tab. See HostType for more details. > */ > get hostType() { > - return this._host.type; > + return this._hostType; It seems a bit strange for the toolbox to know the host type now that someone else creates the host. ::: devtools/client/framework/toolbox.js:267 (Diff revision 1) > /** > * Get/alter the host of a Toolbox, i.e. is it in browser or in a separate > * tab. See HostType for more details. > */ > get hostType() { > - return this._host.type; > + return this._hostType; It seems a bit strange for the toolbox to know the host type now that someone else creates the host. ::: devtools/client/framework/toolbox.js:625 (Diff revision 1) > + this.win.removeEventListener("message", this._onHostMessage); > + } > + }, > + > + // Called whenever the host, on the chrome side, send a message > + _onHostMessage: function (event) { The messages are from the "wrapper" (that needs a better name) not "host", right? `_onHostMessage` seems like a strange name for it. ::: devtools/client/framework/toolbox.js:1807 (Diff revision 1) > - > - /** > * Switch to the last used host for the toolbox UI. > * This is determined by the devtools.toolbox.previousHost pref. > */ > switchToPreviousHost: function () { It's not clear to me who is meant to own this functionality now...
Attachment #8788384 -
Flags: review?(jryans) → review-
Comment 27•8 years ago
|
||
mozreview-review |
Comment on attachment 8788499 [details] Bug 1266134 - Fix toolbox destroy when destroy-host isn't able to reach chrome code. https://reviewboard.mozilla.org/r/76978/#review76314 Seems reasonable, thanks!
Attachment #8788499 -
Flags: review?(jryans) → review+
Comment 28•8 years ago
|
||
mozreview-review |
Comment on attachment 8788385 [details] Bug 1266134 - Fix test after host refactoring https://reviewboard.mozilla.org/r/76880/#review76318 I'd like to know more about the message event guards. ::: devtools/client/debugger/test/mochitest/head.js:750 (Diff revision 2) > > return groups; > }), > > _onMessage: function (event) { > + if (typeof(event.data) !== "string") { Hmm, what's this guard for? ::: devtools/client/framework/test/browser_toolbox_custom_host.js:30 (Diff revision 2) > }, true); > > content.location = "data:text/html,test custom host"; > > function onMessage(event) { > + if (typeof(event.data) !== "string") { Hmm, what's this guard for? ::: devtools/client/framework/test/browser_toolbox_window_title_frame_select.js:97 (Diff revision 2) > function getTitle() { > return Services.wm.getMostRecentWindow("devtools:toolbox").document.title; > } > + > +// Wait for a given toolbox to get its title updated > +function waitForTitleChange(toolbox) { Maybe move this to framework/shared-head.js so it can used in most tests.
Attachment #8788385 -
Flags: review?(jryans)
Comment 29•8 years ago
|
||
mozreview-review |
Comment on attachment 8788386 [details] Bug 1266134 - Convert browser_dbg_on-pause-raise.js to Task and wait for TabSelect before asserting the newly selected tab. https://reviewboard.mozilla.org/r/76882/#review76326 Looks like a good cleanup, thanks!
Attachment #8788386 -
Flags: review?(jryans) → review+
Comment 30•8 years ago
|
||
mozreview-review |
Comment on attachment 8788500 [details] Bug 1266134 - Fix tab load event waiting in browser_cmd_csscoverage_startstop.js. https://reviewboard.mozilla.org/r/76980/#review76330 Let's try to use BrowserTestUtils if possible. ::: devtools/client/commandline/test/head.js:31 (Diff revision 1) > executeSoon(aCallback); > } > }, "browser-delayed-startup-finished", false); > } > > +var waitForTabLoad = function (browser) { Seems like this should be `BrowserTestUtils.browserLoaded` instead?
Attachment #8788500 -
Flags: review?(jryans) → review-
Assignee | ||
Comment 31•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b713f52ffed
Assignee | ||
Comment 32•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ae4237e506c
Assignee | ||
Comment 33•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58fafccece52
Assignee | ||
Comment 34•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d2fdf05248e
Assignee | ||
Comment 35•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=692e4acbfab3
Assignee | ||
Comment 36•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d740425341dc
Assignee | ||
Comment 37•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f82ea562647
Updated•8 years ago
|
Iteration: 51.3 - Sep 19 → 52.1 - Oct 3
Assignee | ||
Comment 38•8 years ago
|
||
Comment on attachment 8788500 [details] Bug 1266134 - Fix tab load event waiting in browser_cmd_csscoverage_startstop.js. This test has been fixed in bug 1253655.
Attachment #8788500 -
Attachment is obsolete: true
Assignee | ||
Comment 39•8 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) (on PTO Sept. 10 - 25) from comment #28) > Comment on attachment 8788385 [details] > Bug 1266134 - Fix test after host refactoring > > https://reviewboard.mozilla.org/r/76880/#review76318 > > I'd like to know more about the message event guards. > This is to prevent mixup between messages sent by the ToobloxBrowserIntegration (ToolboxWrapper) and the one sent by the custom hosts: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox-hosts.js#371-383 We could easily drop custom hosts by tweaking WebIDE, but I don't know if that's worth? At least, I would like to followup to get rid of these duplicated messages with slightly different format.
Assignee | ||
Comment 40•8 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #26) > Comment on attachment 8788384 [details] > Bug 1266134 - Pull host management out of toolbox.xul. > > ::: devtools/client/framework/toolbox-wrapper.js:26 > (Diff revision 1) > > + */ > > + > > +const LAST_HOST = "devtools.toolbox.host"; > > +let ID_COUNTER = 1; > > + > > +function ToolboxWrapper(target, hostType, hostOptions) { > > This needs a less ambiguous name... Perhaps `ToolboxChromeManager`? > `ToolboxBrowserIntegration`? Renamed to ToolboxBrowserIntegration, but if you come up with something even better I can still rename it. (I also renamed the file) > ::: devtools/client/framework/toolbox-wrapper.js:49 > (Diff revision 1) > > + this.host.frame.ownerDocument.defaultView.addEventListener("message", this); > > + this.host.frame.addEventListener("unload", this); > > + > > + let toolbox = new Toolbox(this.target, toolId, this.host.type, this.host.frame.contentWindow, this.frameId); > > + > > + // Prevent reloading the toolbox when loading the tools in a tab (e.g. from about:debugging) > > I don't follow how the comment and this block are related. It happens that when we load a toolbox in a tab from about:debugging, when debugging a tab, we go through this codepath. Here host.frame is going to be the xul:browser element. But the browser element is alreadu going to be loaded on the right about:devtools-toolbox?type=tab&id=xxxx url. If we don't prevent settting src attribute here, it will be reloaded. In all other cases, we do have to set the iframe src attribute here as it is no longer done in toolbox.open method. I haven't updated the comment, but tell me if I can make it clearer. > ::: devtools/client/framework/toolbox.js:190 > (Diff revision 1) > > > > _prefs: { > > - LAST_HOST: "devtools.toolbox.host", > > LAST_TOOL: "devtools.toolbox.selectedTool", > > SIDE_ENABLED: "devtools.toolbox.sideEnabled", > > PREVIOUS_HOST: "devtools.toolbox.previousHost" > > Is `PREVIOUS_HOST` still used? If so, should it leave? > Yes, by this: > ::: devtools/client/framework/toolbox.js:1807 > (Diff revision 1) > > - > > - /** > > * Switch to the last used host for the toolbox UI. > > * This is determined by the devtools.toolbox.previousHost pref. > > */ > > switchToPreviousHost: function () { > > It's not clear to me who is meant to own this functionality now... Same feeling. I'm trying to keep most of the things in the toolbox in order to keep the chrome/browser part the smallest. I expect the chrome/browser part to be more stable and harder to modify. That's the only reason why I kept it here, but if you feel It is better with the chrome part of the host management I can move it there. (It would require a switch-to-previous-host message) > ::: devtools/client/framework/toolbox.js:267 > (Diff revision 1) > > /** > > * Get/alter the host of a Toolbox, i.e. is it in browser or in a separate > > * tab. See HostType for more details. > > */ > > get hostType() { > > - return this._host.type; > > + return this._hostType; > > It seems a bit strange for the toolbox to know the host type now that > someone else creates the host. The toolbox still makes various things differently depending on the host type. For example key shortcuts. Or display different set of things depending on the host: minimize button for bottom, close button for window. > ::: devtools/client/framework/toolbox.js:625 > (Diff revision 1) > > + this.win.removeEventListener("message", this._onHostMessage); > > + } > > + }, > > + > > + // Called whenever the host, on the chrome side, send a message > > + _onHostMessage: function (event) { > > The messages are from the "wrapper" (that needs a better name) not "host", > right? `_onHostMessage` seems like a strange name for it. Renamed to _onBrowserMessage.
Assignee | ||
Updated•8 years ago
|
Attachment #8788384 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8788499 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8788385 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8788386 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 49•8 years ago
|
||
Sorry for the mass r? I discarded the previous mozreview to prevent weird interdiff as I rebased this branch. I'll most likely merged the first five changeset into the first one before landing, but keeping them apart to help the review.
Comment 50•8 years ago
|
||
mozreview-review |
Comment on attachment 8795211 [details] Bug 1266134 - Pull host management out of toolbox.xul. https://reviewboard.mozilla.org/r/81310/#review80160 ::: devtools/client/framework/toolbox.js:1804 (Diff revision 1) > - > - /** > * Switch to the last used host for the toolbox UI. > * This is determined by the devtools.toolbox.previousHost pref. > */ > switchToPreviousHost: function () { At the moment I think this makes more sense if it's moved to the integration module. This means the integration module is the only one tracking last and previous host (which feel like they belong together in the same module due to their similar purpose), so it won't be split between toolbox and integration like it is now.
Attachment #8795211 -
Flags: review?(jryans)
Comment 51•8 years ago
|
||
mozreview-review |
Comment on attachment 8795213 [details] Bug 1266134 - address review for: Pull host management out of toolbox.xul.fix pull out host. https://reviewboard.mozilla.org/r/81314/#review80156 ::: devtools/client/framework/devtools.js:416 (Diff revision 1) > } > > - return hostPromise.then(function () { > - toolbox.raise(); > + toolbox.raise(); > - return toolbox; > - }); > + } else { > + let wrapper = new ToolboxBrowserIntegration(target, hostType, hostOptions); Local var name should be related to the object name, so "wrapper" doesn't really fit anymore. ::: devtools/client/framework/toolbox-browser-integration.js:2 (Diff revision 1) > +const Services = require("Services"); > +const { Ci } = require("chrome"); Nit: Choose one style for braces ::: devtools/client/framework/toolbox-browser-integration.js:19 (Diff revision 1) > + * This component handles iframe creation within Firefox, in which we are loading > + * the toolbox document. Then both the chrome and the toolbox document communicate > + * via "message" events. > + * > + * Messages sent by the toolbox to the chrome: > + * - switch-host: order to display the toolbox in another host (side, bottom Maybe move the explanation to the next line, so they can all start from the same character? ::: devtools/client/framework/toolbox-browser-integration.js:38 (Diff revision 1) > + */ > + > +const LAST_HOST = "devtools.toolbox.host"; > +let ID_COUNTER = 1; > + > +function ToolboxBrowserIntegration(target, hostType, hostOptions) { ToolboxHostManager? ToolboxHostFactory? AbstractAutowireCapableBeanFactory? (This one is just a joke from my Java days...) :) Still don't have a strong preference for any of the names... ::: devtools/client/framework/toolbox-browser-integration.js:57 (Diff revision 1) > + create: Task.async(function* (toolId) { > + yield this.host.create(); > + > + this.host.frame.setAttribute("aria-label", L10N.getStr("toolbox.label")); > + this.host.frame.ownerDocument.defaultView.addEventListener("message", this); > + this.host.frame.addEventListener("unload", this, true); Do you have to use a capturing listener here followed by `executeSoon`? Could you use a bubbling listener instead, so that you know the event passed through the toolbox document already? Not totally sure I know the ordering of unload events across frames... ::: devtools/client/framework/toolbox-browser-integration.js:84 (Diff revision 1) > + if (!event.target.location.href.startsWith("about:devtools-toolbox")) { > + break; > + } > + // Don't destroy the host during unload event (esp., don't remove the > + // iframe from DOM!). Otherwise the unload event for the toolbox > + // document doesn't fire within the toolbox *document*! (here this is Unclose paren in comment. Maybe just "(here this" -> "This"? ::: devtools/client/framework/toolbox.js:107 (Diff revision 1) > * @param {Toolbox.HostType} hostType > * Type of host that will host the toolbox (e.g. sidebar, window) > + * @param {DOMWindow} contentWindow > + * The window object of the toolbox document > + * @param {string} frameId > + * A unique identifier to differenciate toolbox documents from the Nit: differentiate
Attachment #8795213 -
Flags: review?(jryans)
Comment 52•8 years ago
|
||
mozreview-review |
Comment on attachment 8795212 [details] Bug 1266134 - Fix toolbox destroy when destroy-host isn't able to reach chrome code. https://reviewboard.mozilla.org/r/81312/#review80166
Attachment #8795212 -
Flags: review?(jryans) → review+
Comment 53•8 years ago
|
||
mozreview-review |
Comment on attachment 8795214 [details] Bug 1266134 - Fix responsive design possible leak on shutdown. https://reviewboard.mozilla.org/r/81316/#review80168
Attachment #8795214 -
Flags: review?(jryans) → review+
Comment 54•8 years ago
|
||
mozreview-review |
Comment on attachment 8795215 [details] Bug 1266134 - Fix test after host refactoring https://reviewboard.mozilla.org/r/81318/#review80170
Attachment #8795215 -
Flags: review?(jryans) → review+
Comment 55•8 years ago
|
||
mozreview-review |
Comment on attachment 8795216 [details] Bug 1266134 - Convert browser_dbg_on-pause-raise.js to Task and wait for TabSelect before asserting the newly selected tab. https://reviewboard.mozilla.org/r/81320/#review80172
Attachment #8795216 -
Flags: review?(jryans) → review+
Comment 56•8 years ago
|
||
mozreview-review |
Comment on attachment 8795217 [details] Bug 1266134 - Wait for window close before ending test in browser_styleeditor_private_perwindowpb.js. https://reviewboard.mozilla.org/r/81322/#review80174
Attachment #8795217 -
Flags: review?(jryans) → review+
Comment 57•8 years ago
|
||
mozreview-review |
Comment on attachment 8795218 [details] Bug 1266134 - Prevent browser_computed_keybindings_01.js from opening options panel. https://reviewboard.mozilla.org/r/81324/#review80178
Attachment #8795218 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 58•8 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #51) > Comment on attachment 8795213 [details] > ::: devtools/client/framework/toolbox-browser-integration.js:38 > (Diff revision 1) > > + */ > > + > > +const LAST_HOST = "devtools.toolbox.host"; > > +let ID_COUNTER = 1; > > + > > +function ToolboxBrowserIntegration(target, hostType, hostOptions) { > > ToolboxHostManager? ToolboxHostFactory? > AbstractAutowireCapableBeanFactory? (This one is just a joke from my Java > days...) :) > > Still don't have a strong preference for any of the names... I've kept ToolboxBrowserIntegration, and "wrapper" in devtools.js. I would like to change that before landing but need some time to think. > > ::: devtools/client/framework/toolbox-browser-integration.js:57 > (Diff revision 1) > > + create: Task.async(function* (toolId) { > > + yield this.host.create(); > > + > > + this.host.frame.setAttribute("aria-label", L10N.getStr("toolbox.label")); > > + this.host.frame.ownerDocument.defaultView.addEventListener("message", this); > > + this.host.frame.addEventListener("unload", this, true); > > Do you have to use a capturing listener here followed by `executeSoon`? > Could you use a bubbling listener instead, so that you know the event passed > through the toolbox document already? Not totally sure I know the ordering > of unload events across frames... Unfortunately, the unload event doesn't fire on bubble. That's the main reason I'm using capture here :/
Assignee | ||
Comment 59•8 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #50) > Comment on attachment 8795211 [details] > Bug 1266134 - Pull host management out of toolbox.xul. > > https://reviewboard.mozilla.org/r/81310/#review80160 > > ::: devtools/client/framework/toolbox.js:1804 > (Diff revision 1) > > - > > - /** > > * Switch to the last used host for the toolbox UI. > > * This is determined by the devtools.toolbox.previousHost pref. > > */ > > switchToPreviousHost: function () { > > At the moment I think this makes more sense if it's moved to the integration > module. This means the integration module is the only one tracking last and > previous host (which feel like they belong together in the same module due > to their similar purpose), so it won't be split between toolbox and > integration like it is now. Done.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 66•8 years ago
|
||
mozreview-review |
Comment on attachment 8795213 [details] Bug 1266134 - address review for: Pull host management out of toolbox.xul.fix pull out host. https://reviewboard.mozilla.org/r/81314/#review80280
Assignee | ||
Comment 67•8 years ago
|
||
I pushed this interdiff addressing all review comments but the toolbox-browser-integration name.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 74•8 years ago
|
||
Just pushed a quick fix in toolbox.js:switchToPreviousHost(): + return this.once("host-changed"); to fix tests.
Comment 75•8 years ago
|
||
mozreview-review |
Comment on attachment 8795710 [details] Bug 1266134 - address review 2 for: Pull host management out of toolbox.xul.fix pull out host. https://reviewboard.mozilla.org/r/81676/#review80466 Thanks, I think it does seem better this way.
Attachment #8795710 -
Flags: review?(jryans) → review+
Comment 76•8 years ago
|
||
mozreview-review |
Comment on attachment 8795211 [details] Bug 1266134 - Pull host management out of toolbox.xul. https://reviewboard.mozilla.org/r/81310/#review80470
Attachment #8795211 -
Flags: review+
Comment 77•8 years ago
|
||
mozreview-review |
Comment on attachment 8795213 [details] Bug 1266134 - address review for: Pull host management out of toolbox.xul.fix pull out host. https://reviewboard.mozilla.org/r/81314/#review80472 I believe the name is the only unresolved piece, but we could proceed with current name even though it's not great, so seems fine to mark r+. Maybe you'll think up a great one in the mean time. :)
Attachment #8795213 -
Flags: review+
Updated•8 years ago
|
Iteration: 52.1 - Oct 3 → 52.2 - Oct 17
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8795212 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8795213 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8795710 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8795215 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 93•8 years ago
|
||
So I went for ToolboxHostManager as discussed in our 1:1.
Comment 94•8 years ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/03250652b3a0 Pull host management out of toolbox.xul. r=jryans https://hg.mozilla.org/integration/autoland/rev/47a702919083 Fix responsive design possible leak on shutdown. r=jryans https://hg.mozilla.org/integration/autoland/rev/87e1e46c6708 Convert browser_dbg_on-pause-raise.js to Task and wait for TabSelect before asserting the newly selected tab. r=jryans https://hg.mozilla.org/integration/autoland/rev/5ad2b64b4df7 Wait for window close before ending test in browser_styleeditor_private_perwindowpb.js. r=jryans https://hg.mozilla.org/integration/autoland/rev/e17d5bf3dcfa Prevent browser_computed_keybindings_01.js from opening options panel. r=jryans
Comment 95•8 years ago
|
||
Backed out for failing browser_dbg_on-pause-raise.js on OSX and Windows: https://hg.mozilla.org/integration/autoland/rev/d3effaa91ca9db4d0c59143592247a982804dce3 https://hg.mozilla.org/integration/autoland/rev/a77f768d3a7d32e28cfa03aca2cd3e1c5fdbd52e https://hg.mozilla.org/integration/autoland/rev/c3103f17698ff5b0ea869a06db8cdd4611bfe4b6 https://hg.mozilla.org/integration/autoland/rev/a686cb30fd8c892641144d98126e43fcd168770f https://hg.mozilla.org/integration/autoland/rev/7421808509ca9f14e1543d4876d15a07c183c967 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e17d5bf3dcfa36df57814cc12b3fcde399525436 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=4676306&repo=autoland 06:49:54 INFO - 254 INFO TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_on-pause-raise.js | The highlighted class is not present now after the resume - 06:49:54 INFO - 255 INFO TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_on-pause-raise.js | The tab is not selected - 06:49:54 INFO - 256 INFO Switching to a toolbox window host. 06:49:54 INFO - 257 INFO Console message: OpenGL compositor Initialized Succesfully. 06:49:54 INFO - Version: 2.1 INTEL-10.6.33 06:49:54 INFO - Vendor: Intel Inc. 06:49:54 INFO - Renderer: Intel Iris OpenGL Engine 06:49:54 INFO - FBO Texture Target: TEXTURE_2D 06:49:54 INFO - 258 INFO Focusing main window. 06:49:54 INFO - 259 INFO TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_on-pause-raise.js | Test timed out - 06:49:54 INFO - 260 INFO Main window focused. 06:49:54 INFO - 261 INFO Run tests against window host. 06:49:54 INFO - ************************* 06:49:54 INFO - A coding exception was thrown and uncaught in a Task. 06:49:54 INFO - Full message: TypeError: panelWin.gThreadClient is null
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 96•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6677d880070
Assignee | ||
Comment 97•8 years ago
|
||
It looks like it is not due to the test refactoring to task: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e860d89dc70e
Assignee | ||
Comment 98•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39c485a4228e
Assignee | ||
Comment 99•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6792de395fe6
Assignee | ||
Comment 100•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cdf16512044
Assignee | ||
Comment 101•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=626d53573a9e
Assignee | ||
Comment 102•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e60e1d389e1
Assignee | ||
Updated•8 years ago
|
Attachment #8795211 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8795214 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8795216 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8795217 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8795218 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 109•8 years ago
|
||
Comment on attachment 8800766 [details] Bug 1266134 - fix browser_dbg_on-pause-raise.js Sorry, I had to discard the previous one to be able to merge it correctly once r+... There is just this changeset which is new.
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 110•8 years ago
|
||
It looks like after all, we don't need to focus the window in middle of the test anymore.
What has changed since last time? I don't have an easy way to know since it's not just an update to the last review...
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 112•8 years ago
|
||
Comment on attachment 8800766 [details] Bug 1266134 - fix browser_dbg_on-pause-raise.js Nothing, there is just the last changeset, this one that is new. All the other changeset are the same, except may be some rebase conflict resolution... I had to discard the previous mozreview due to the backout, it reask review for everything :/ I'm not sure I can r+ these patches from bugzilla and keep you flagged on mozreview as reviewer?
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #112) > Comment on attachment 8800766 [details] > Bug 1266134 - fix browser_dbg_on-pause-raise.js > > Nothing, there is just the last changeset, this one that is new. All the > other changeset are the same, except may be some rebase conflict > resolution... > > I had to discard the previous mozreview due to the backout, it reask review > for everything :/ > I'm not sure I can r+ these patches from bugzilla and keep you flagged on > mozreview as reviewer? I think you should be able to set r+ from bugzilla for the ones without changes, but anyway, now I know what to look for at least!
Comment 114•8 years ago
|
||
mozreview-review |
Comment on attachment 8800766 [details] Bug 1266134 - fix browser_dbg_on-pause-raise.js https://reviewboard.mozilla.org/r/85626/#review85070 Seems fine, but I am curious why it's safe to remove the focusing... Maybe I've forgotten too much about the rest of this series already... :) ::: devtools/client/debugger/test/mochitest/browser_dbg_on-pause-raise.js (Diff revision 1) > + // testResume selected the console, select back the debugger. > + yield toolbox.selectTool("jsdebugger"); > + > info("Switching to a toolbox window host."); > yield toolbox.switchHost(Toolbox.HostType.WINDOW); > - yield focusMainWindow(); Why is this part no longer needed?
Attachment #8800766 -
Flags: review?(jryans) → review+
Comment 115•8 years ago
|
||
mozreview-review |
Comment on attachment 8800761 [details] Bug 1266134 - Pull host management out of toolbox.xul. https://reviewboard.mozilla.org/r/85616/#review85072
Attachment #8800761 -
Flags: review?(jryans) → review+
Comment 116•8 years ago
|
||
mozreview-review |
Comment on attachment 8800762 [details] Bug 1266134 - Fix responsive design possible leak on shutdown. https://reviewboard.mozilla.org/r/85618/#review85074
Attachment #8800762 -
Flags: review?(jryans) → review+
Comment 117•8 years ago
|
||
mozreview-review |
Comment on attachment 8800763 [details] Bug 1266134 - Convert browser_dbg_on-pause-raise.js to Task and wait for TabSelect before asserting the newly selected tab. https://reviewboard.mozilla.org/r/85620/#review85076
Attachment #8800763 -
Flags: review?(jryans) → review+
Comment 118•8 years ago
|
||
mozreview-review |
Comment on attachment 8800764 [details] Bug 1266134 - Wait for window close before ending test in browser_styleeditor_private_perwindowpb.js. https://reviewboard.mozilla.org/r/85622/#review85078
Attachment #8800764 -
Flags: review?(jryans) → review+
Comment 119•8 years ago
|
||
mozreview-review |
Comment on attachment 8800765 [details] Bug 1266134 - Prevent browser_computed_keybindings_01.js from opening options panel. https://reviewboard.mozilla.org/r/85624/#review85080
Attachment #8800765 -
Flags: review?(jryans) → review+
Updated•8 years ago
|
Iteration: 52.2 - Oct 17 → 52.3 - Nov 7
Assignee | ||
Comment 120•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5f5f4045a54
Assignee | ||
Comment 121•8 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #114) > Comment on attachment 8800766 [details] > ::: devtools/client/debugger/test/mochitest/browser_dbg_on-pause-raise.js > (Diff revision 1) > > + // testResume selected the console, select back the debugger. > > + yield toolbox.selectTool("jsdebugger"); > > + > > info("Switching to a toolbox window host."); > > yield toolbox.switchHost(Toolbox.HostType.WINDOW); > > - yield focusMainWindow(); > > Why is this part no longer needed? Hum. Looks like my comment was never sent... So I first thought this forced focus was a relic from the past. But last try run proves it was still needed before my patch. Also, looking at Windows, focus is messed up when switching hosts. It looks like we no longer need to blur the tool at the start of switchHost, but still do need to focus at the end of it. I need to confirm it will keep try green and it also works fine on linux.
Assignee | ||
Comment 122•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f01878c7b2c9
Assignee | ||
Comment 123•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=627b01cce6a4
Assignee | ||
Comment 124•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c14edca2a9a
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 132•8 years ago
|
||
Comment on attachment 8804823 [details] Bug 1266134 - Cleanup focus workarounds Ok. so here is a patch to fix and clean the workarounds about focus while switching the hosts. There is two distinct issues: - swapFrameLoader which mess up with the focus, especially when hitting the key shortcuts. - clicking on host switching button put the focus on the button, but we want to focus to stay in the webconsole input for example If there was only the first issue, we could have moved the focus workaround to ToolboxHostManager. Unfortunately there is the second, where we can't know on which element we have to restore focus as the focus is already set to the button. So I somewhat reverted to what we were doing before my patch and updated the comments. Also I modified switchToPreviousHost in order to go though the same focus workaround than switchHost.
Comment 133•8 years ago
|
||
mozreview-review |
Comment on attachment 8804823 [details] Bug 1266134 - Cleanup focus workarounds https://reviewboard.mozilla.org/r/88678/#review87788 Looks reasonable to me!
Attachment #8804823 -
Flags: review?(jryans) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8800766 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8804823 -
Attachment is obsolete: true
Assignee | ||
Comment 139•8 years ago
|
||
Just pushed merged and rebased changesets, running a last cross platform try.
Comment 140•8 years ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/24775cdd79fb Pull host management out of toolbox.xul. r=jryans https://hg.mozilla.org/integration/autoland/rev/273e04b405d5 Fix responsive design possible leak on shutdown. r=jryans https://hg.mozilla.org/integration/autoland/rev/31a80037ca8e Convert browser_dbg_on-pause-raise.js to Task and wait for TabSelect before asserting the newly selected tab. r=jryans https://hg.mozilla.org/integration/autoland/rev/0b6042ee4d03 Wait for window close before ending test in browser_styleeditor_private_perwindowpb.js. r=jryans https://hg.mozilla.org/integration/autoland/rev/2268d466af6c Prevent browser_computed_keybindings_01.js from opening options panel. r=jryans
Comment 141•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/24775cdd79fb https://hg.mozilla.org/mozilla-central/rev/273e04b405d5 https://hg.mozilla.org/mozilla-central/rev/31a80037ca8e https://hg.mozilla.org/mozilla-central/rev/0b6042ee4d03 https://hg.mozilla.org/mozilla-central/rev/2268d466af6c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment 142•8 years ago
|
||
Performed several check-up tests using Nightly 52.0a1 2016-11-10 under Win 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-dthtml]
Flags: qe-verify+
Assignee | ||
Comment 143•8 years ago
|
||
Thanks for the verification Petruta!
Depends on: 1319928
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•