1348318 - `toolbox.win.top` doesn't work well for undocked windows

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

In bug 1346647, we fixed one usage of `toolbox.win.top` that was breaking RDM button for undocked toolboxes.

In general, it may not make sense to check windows like this given that undocked toolboxes exist.

The only other case currently is:

http://searchfox.org/mozilla-central/rev/006005beff40d377cfd2f69d3400633c5ff09127/devtools/client/framework/devtools-browser.js#754

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

When closing an undocked toolbox currently, that if check on the window does miss the toolbox, but `toolbox.destroy` ends up being called through another path, probably because of unloading the toolbox's window.

So, it doesn't seem too critical, just a nice to have fix so that this works in all toolbox types.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1348318 - Avoid toolbox.win.top with undocked toolboxes.

`toolbox.win.top` is a bit suspect when we support undocked toolboxes, as this
doesn't allow you to reach a browser window for the toolbox.  Here we take a
different approach that should work for all docking types.

Review commit: https://reviewboard.mozilla.org/r/121454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121454/

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8848550 [details]
Bug 1348318 - Avoid toolbox.win.top with undocked toolboxes.

https://reviewboard.mozilla.org/r/121454/#review123496

Thanks for this cleanup!

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

You may want to check that target.tab is defined as it may not necessarely be set. Only TabTarget have it defined.
It will be undefined for toolboxes from bug  	1348012... I don't know if there is any other such scenario that already exists.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8848550 [details]
Bug 1348318 - Avoid toolbox.win.top with undocked toolboxes.

https://reviewboard.mozilla.org/r/121454/#review123496

> You may want to check that target.tab is defined as it may not necessarely be set. Only TabTarget have it defined.
> It will be undefined for toolboxes from bug  	1348012... I don't know if there is any other such scenario that already exists.

Yep, seems safest to check, I'll add it!

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8848550 [details]
Bug 1348318 - Avoid toolbox.win.top with undocked toolboxes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121454/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d06ce06386ad
Avoid toolbox.win.top with undocked toolboxes. r=ochameau

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d06ce06386ad