Alexandre Poirot [:ochameau] Assignee
	Description • 8 years ago

In order to help 1266134, there is various cleanups we can do around toolbox.js and the Toolbox object:

 - Stop using various toolbox internals like toolbox._host, toolbox._target
 - Use helpers instead of computing them again and again:
     for ex, use toobox.win instead of toolbox._host.frame.contentWindow
 - We can get rid of toolbox.frame as all the usages of it can be replaced with equivalent using toolbox.win (this will help loading toolbox in a tab as it won't have access to the frame element when running in content)
 - Cleanup the toolbox on unload instead of relying on window-closed event of the host, which is just the unload event of the parent document. (this will reduce the amount of code to refactor in bug 1266134)

	Alexandre Poirot [:ochameau] Assignee
	Comment 4 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65ad501fe2b3

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 5 • 8 years ago
mozreview-review

Comment on attachment 8785026 [details]
Bug 1298082 - Various cleanups to use helpers.

https://reviewboard.mozilla.org/r/74322/#review72850

Thanks, looks good!

::: devtools/shared/fronts/css-properties.js:179
(Diff revision 1)
>  /**
>   * Get the current browser version.
>   * @returns {string} The browser version.
>   */
>  function getClientBrowserVersion(toolbox) {
> -  if (!toolbox._host) {
> +  const regexResult = toolbox.win.navigator

I guess there is always a host?  Not sure what prompted this check to be here originally...

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 6 • 8 years ago
mozreview-review

Comment on attachment 8785027 [details]
Bug 1298082 - Replace toolbox.frame usages with equivalents around toolbox.win.

https://reviewboard.mozilla.org/r/74324/#review72854

Thanks, seems reasonable!

::: devtools/client/framework/devtools-browser.js:620
(Diff revision 1)
>  
>      BrowserMenus.removeMenus(win.document);
>  
>      // Destroy toolboxes for closed window
>      for (let [target, toolbox] of gDevTools._toolboxes) {
> -      if (toolbox.frame && toolbox.frame.ownerDocument.defaultView == win) {
> +      if (toolbox.win == win) {

What if the frame is null here?  Maybe you should change the `win` getter to abort early if there is no frame?  Or does it never happen?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 7 • 8 years ago
mozreview-review

Comment on attachment 8785027 [details]
Bug 1298082 - Replace toolbox.frame usages with equivalents around toolbox.win.

https://reviewboard.mozilla.org/r/74324/#review72880

::: devtools/client/framework/toolbox.js
(Diff revision 1)
>    },
>  
>    /**
> -   * Get the iframe containing the toolbox UI.
> -   */
> -  get frame() {

Hmm, what about the other getters that use this, like `win` below?  Did you run this?? :P

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 8 • 8 years ago
mozreview-review

Comment on attachment 8785028 [details]
Bug 1298082 - Cleanup toolbox on its document unload.

https://reviewboard.mozilla.org/r/74326/#review72862

Seems reasonable, but I can't test this yet because of issues with the previous patches.

::: devtools/client/framework/toolbox.js:623
(Diff revision 1)
> +      this.doc.removeEventListener("keypress", this._splitConsoleOnKeypress, false);
> +      this.doc.removeEventListener("focus", this._onFocus, true);
> +      this.win.removeEventListener("unload", this.destroy);
> +    }
> +  },
>    },

Seems like an extra closing brace?

	Alexandre Poirot [:ochameau] Assignee
	Comment 9 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=424d55121db3

	Alexandre Poirot [:ochameau] Assignee
	Comment 10 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81fc058034fb

	Alexandre Poirot [:ochameau] Assignee
	Comment 14 • 8 years ago
mozreview-review-reply

Comment on attachment 8785027 [details]
Bug 1298082 - Replace toolbox.frame usages with equivalents around toolbox.win.

https://reviewboard.mozilla.org/r/74324/#review72854

> What if the frame is null here?  Maybe you should change the `win` getter to abort early if there is no frame?  Or does it never happen?

It happens in between Toolbox() constructor call and toolbox.open() call, and more precisely host.create().
I haven't tested this patch set alone, but only with bug 1266134 patches, which exposes window differently.
I pushed a new patch which set a `_win` attribute during toolbox.open.
It aligns well with 1266134 patch which sets _win immediately during Toolbox() constructor.

	Alexandre Poirot [:ochameau] Assignee
	Comment 15 • 8 years ago
mozreview-review-reply

Comment on attachment 8785026 [details]
Bug 1298082 - Various cleanups to use helpers.

https://reviewboard.mozilla.org/r/74322/#review72850

> I guess there is always a host?  Not sure what prompted this check to be here originally...

It was just to accommodate devtools/server/tests/mochitest/test_css-properties_01.html which doesn't set the _host in its Mock object for the toolbox. I tweaked the test to expose the `win` getter in the mock.

	Alexandre Poirot [:ochameau] Assignee
	Comment 16 • 8 years ago
mozreview-review-reply

Comment on attachment 8785028 [details]
Bug 1298082 - Cleanup toolbox on its document unload.

https://reviewboard.mozilla.org/r/74326/#review72862

Should be fixed.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 17 • 8 years ago

It looks like you rebased here between MozReview submissions.  If you do that, it ruins the diff display by stirring in various hunks from rebased work that is not related to what you did.  I usually try to not rebase until I've gotten r+, but this is easier for me with separate feature branches...

In any case, if you need to rebase, it's best to first "close" the existing MozReview so that it starts a fresh one when you submit the rebased work.  This avoid the confused diffs from rebasing.

From the review page:

1. Look for a "Close" tab in the top header
  * If it's not there, click the "Review Summary" link first and it should appear
2. Choose "Discard" from the "Close" tab
3. Now it's fine to submit the rebased patches

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 18 • 8 years ago
mozreview-review

Comment on attachment 8785027 [details]
Bug 1298082 - Replace toolbox.frame usages with equivalents around toolbox.win.

https://reviewboard.mozilla.org/r/74324/#review74078

::: devtools/client/framework/toolbox.js:110
(Diff revision 2)
>   * @param {object} hostOptions
>   *        Options for host specifically
>   */
>  function Toolbox(target, selectedTool, hostType, hostOptions) {
>    this._target = target;
> +  this._win = null;

The `destroy` path should null this out once it's safe to do so.

::: devtools/client/framework/toolbox.js:1815
(Diff revision 2)
>  
>      let newHost = this._createHost(hostType);
>      return newHost.create().then(iframe => {
>        // change toolbox document's parent to the new host
>        iframe.QueryInterface(Ci.nsIFrameLoaderOwner);
> -      iframe.swapFrameLoaders(this.frame);
> +      iframe.swapFrameLoaders(this._host.frame);

Is it okay to keep this reference to the frame?  It seemed like your goal was to remove them all?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 19 • 8 years ago
mozreview-review

Comment on attachment 8785028 [details]
Bug 1298082 - Cleanup toolbox on its document unload.

https://reviewboard.mozilla.org/r/74326/#review74094

Great, this seems to work well in my testing!

	Alexandre Poirot [:ochameau] Assignee
	Comment 23 • 8 years ago
mozreview-review-reply

Comment on attachment 8785027 [details]
Bug 1298082 - Replace toolbox.frame usages with equivalents around toolbox.win.

https://reviewboard.mozilla.org/r/74324/#review74078

> The `destroy` path should null this out once it's safe to do so.

Just added _win = null in destroy(), just after destroyHost() which should be the place where this._host.frame.contentWindow starts to be null.

> Is it okay to keep this reference to the frame?  It seemed like your goal was to remove them all?

Yes, that's fine.
It will be removed in bug 1266134. I was waiting for this bug to land before r? it.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 24 • 8 years ago
mozreview-review

Comment on attachment 8785027 [details]
Bug 1298082 - Replace toolbox.frame usages with equivalents around toolbox.win.

https://reviewboard.mozilla.org/r/74324/#review74568

	Alexandre Poirot [:ochameau] Assignee
	Comment 25 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab95ab61334a

	Pulsebot
	Comment 26 • 8 years ago

Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f6209eb17797
Various cleanups to use helpers. r=jryans
https://hg.mozilla.org/integration/autoland/rev/592cca4e57ad
Replace toolbox.frame usages with equivalents around toolbox.win. r=jryans
https://hg.mozilla.org/integration/autoland/rev/dc09df75de0b
Cleanup toolbox on its document unload. r=jryans

	Phil Ringnalda (:philor)
	Comment 27 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f6209eb17797
https://hg.mozilla.org/mozilla-central/rev/592cca4e57ad
https://hg.mozilla.org/mozilla-central/rev/dc09df75de0b