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)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(firefox52 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Iteration:
52.3 - Nov 14
Tracking Flags:
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)

Bug 1266134 - Pull host management out of toolbox.xul.
58 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1266134 - Fix responsive design possible leak on shutdown.
58 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1266134 - Convert browser_dbg_on-pause-raise.js to Task and wait for TabSelect before asserting the newly selected tab.
58 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1266134 - Wait for window close before ending test in browser_styleeditor_private_perwindowpb.js.
58 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1266134 - Prevent browser_computed_keybindings_01.js from opening options panel.
58 bytes, 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.
Blocks: devtools-html-3
Severity: normal → enhancement
Whiteboard: [devtools-html]
Blocks: devtools-html-2
No longer blocks: devtools-html-3
Flags: qe-verify+
Priority: -- → P2
QA Contact: alexandra.lucinet
Priority: P2 → P3
Whiteboard: [devtools-html] → [reserve-html]
No longer blocks: devtools-html-2
Whiteboard: [reserve-html] → [devtools-html] [triage]
Blocks: devtools-html-phase2
Priority: P3 → --
Whiteboard: [devtools-html] [triage] → [devtools-html]
Priority: -- → P2
Taking as I already started looking into this.
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
QA Contact: adalucin → petruta.rasa
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29
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.
Without the leak and with even less call between chrome and toolbox.js
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0da1a017b50b
Attached patch various cleanups to use helpers (obsolete) — — Splinter Review 
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.
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.
Main patch where we do move host management out of toolbox.js to a dedicated module toolbox-wrapper.js
Attached patch fix test after host refactoring (obsolete) — — Splinter Review 
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.
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.
Depends on: 1298082
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.
Iteration: 51.2 - Aug 29 → 51.3 - Sep 12
Depends on: 1299501
Attachment #8784133 - Attachment is obsolete: true
Attachment #8784134 - Attachment is obsolete: true
Attachment #8784135 - Attachment is obsolete: true
Attachment #8784137 - Attachment is obsolete: true
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.
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
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.
There is still other leaks to be chased, so still not rush in reviewing.
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 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 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 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 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-
Depends on: 1302148
Depends on: 1253655
Iteration: 51.3 - Sep 19 → 52.1 - Oct 3
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
(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.
(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.
Attachment #8788384 - Attachment is obsolete: true
Attachment #8788499 - Attachment is obsolete: true
Attachment #8788385 - Attachment is obsolete: true
Attachment #8788386 - Attachment is obsolete: true
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 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 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 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 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 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 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 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+
(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 :/
(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 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
I pushed this interdiff addressing all review comments but the toolbox-browser-integration name.
Just pushed a quick fix in toolbox.js:switchToPreviousHost():
+    return this.once("host-changed");
to fix tests.
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 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+
Iteration: 52.1 - Oct 3 → 52.2 - Oct 17
Attachment #8795212 - Attachment is obsolete: true
Attachment #8795213 - Attachment is obsolete: true
Attachment #8795710 - Attachment is obsolete: true
Attachment #8795215 - Attachment is obsolete: true
So I went for ToolboxHostManager as discussed in our 1:1.
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
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)
It looks like it is not due to the test refactoring to task:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e860d89dc70e
Attachment #8795211 - Attachment is obsolete: true
Attachment #8795214 - Attachment is obsolete: true
Attachment #8795216 - Attachment is obsolete: true
Attachment #8795217 - Attachment is obsolete: true
Attachment #8795218 - Attachment is obsolete: true
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)
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)
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 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 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 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 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 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 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+
Iteration: 52.2 - Oct 17 → 52.3 - Nov 7
(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.
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 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+
Attachment #8800766 - Attachment is obsolete: true
Attachment #8804823 - Attachment is obsolete: true
Just pushed merged and rebased changesets, running a last cross platform try.
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
Depends on: 1313536
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]
status-firefox52: fixed → verified
Flags: qe-verify+
Thanks for the verification Petruta!
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: