855108 - Commands toggled by the developer toolbar should not persist after the toolbar closes

Status: REOPENED

Description

[Siddhartha Dugar [:sdrocking]]

Comment 1

[David Creswick]

Created attachment 733511 [details] [diff] [review]
patch

Could some nice person with editbugs permissions please change the title of this bug to something like "Commands toggled by the developer toolbar should not persist after the toolbar closes", because the attached patch affects more than just the 3D view.

Comment 2

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Comment on attachment 733511 [details] [diff] [review]
patch

Review of attachment 733511 [details] [diff] [review]:
-----------------------------------------------------------------

I like it, thanks.
I think we need a unit test. Have you got time to add one?

Comment 3

[David Creswick]

Yes, I can write a test. I probably ought to know by now that I should have written one from the beginning.

Comment 4

[David Creswick]

Created attachment 734710 [details] [diff] [review]
patch v2, with test

Comment 5

[David Creswick]

Created attachment 735254 [details] [diff] [review]
patch v3, with test

Comment 6

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Comment on attachment 735254 [details] [diff] [review]
patch v3, with test

Review of attachment 735254 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent thanks.

::: browser/devtools/shared/test/Makefile.in
@@ +14,5 @@
>  XPCSHELL_TESTS = unit
>  
>  MOCHITEST_BROWSER_FILES = \
>    browser_browser_basic.js \
> +  browser_bug855108.js \

Please could you call this file browser_toolbar_nopersist.js or similar. Bug numbers are useful to start with, but quickly get to be a mess when you've got several, including fixes to fixes and so on.

::: browser/devtools/shared/test/browser_bug855108.js
@@ +122,5 @@
> +      .then(function () target.destroy())
> +      .then(function () testNextCommand(tab));
> +  }
> +
> +  addTab(URL, function (browser, tab) {

I think this could be a confusing name because there is an existing addTab which comes from head.js, so people might assume the wrong one. But then I'm not sure it's actually used is it?

Comment 7

[David Creswick]

(In reply to Joe Walker [:jwalker] from comment #6)
> Comment on attachment 735254 [details] [diff] [review]
> patch v3, with test
> 

> > +  addTab(URL, function (browser, tab) {
> 
> I think this could be a confusing name because there is an existing addTab
> which comes from head.js, so people might assume the wrong one. But then I'm
> not sure it's actually used is it?

This does use the existing addTab. It's not creating a new function.

Comment 8

[David Creswick]

Created attachment 738070 [details] [diff] [review]
patch v4, with review feedback

Comment 9

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

(In reply to David Creswick from comment #7)
> > > +  addTab(URL, function (browser, tab) {
> > 
> > I think this could be a confusing name because there is an existing addTab
> > which comes from head.js, so people might assume the wrong one. But then I'm
> > not sure it's actually used is it?
> 
> This does use the existing addTab. It's not creating a new function.

*blink*. I'm not sure what to say. Yes, obviously that was a stupid comment.
'(URL' != '='

Comment 10

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

I'll get this landed.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Running mochitests locally I'm getting leaks in a test introduced by this patch:

13:44.54 TEST-UNEXPECTED-FAIL| leakcheck | 3764571 bytes leaked (AsyncStatement, AtomImpl, BackstagePass, BodyRule, CalculateFrecencyFunction, ...)
13:44.54 
INFO | runtests.py | Running tests: end.
13:44.56 TEST-UNEXPECTED-FAIL| chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_buttons_nopersist.js | resize toggle is untoggled after detached toobox closed - Got true, expected false
13:44.56 TEST-UNEXPECTED-FAIL| chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_buttons_nopersist.js | tilt toggle is untoggled after detached toobox closed - Got true, expected false
13:44.56 TEST-UNEXPECTED-FAIL| chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_buttons_nopersist.js | paintflashing toggle is untoggled after detached toobox closed - Got true, expected false
13:44.56 TEST-UNEXPECTED-FAIL| chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_buttons_nopersist.js | leaked until shutdown [nsGlobalWindow #3385 about:blank]
13:44.56 TEST-UNEXPECTED-FAIL| chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_buttons_nopersist.js | leaked until shutdown [nsGlobalWindow #3263 about:blank]
13:44.56 TEST-UNEXPECTED-FAIL| chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_buttons_nopersist.js | leaked until shutdown [nsGlobalWindow #3324 about:blank]
13:44.56 TEST-UNEXPECTED-FAIL| chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_buttons_nopersist.js | leaked until shutdown [nsGlobalWindow #3386 about:blank]
13:44.56 TEST-UNEXPECTED-FAIL| chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_buttons_nopersist.js | leaked until shutdown [nsGlobalWindow #3264 about:blank]
13:44.56 TEST-UNEXPECTED-FAIL| chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_buttons_nopersist.js | leaked until shutdown [nsGlobalWindow #3325 about:blank]
13:44.56 TEST-UNEXPECTED-FAIL| leakcheck | 3764571 bytes leaked (AsyncStatement, AtomImpl, BackstagePass, BodyRule, CalculateFrecencyFunction, ...)

Comment 12

[David Creswick]

Created attachment 740131 [details] [diff] [review]
patch v5

This fixes the test failures for me locally. Sorry about that. For me, the test would pass when run in isolation, but failed in the way you describe when run along with the other tests. It was pretty annoying to debug, but lesson learned.

Comment 13

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=d2ae034f8b2e
Assuming it's green, I'll land this tomorrow.

Comment 14

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=8911b764dc1e
https://hg.mozilla.org/integration/fx-team/rev/229cd2ebe225

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/229cd2ebe225

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Backed out ...

https://hg.mozilla.org/integration/fx-team/rev/12e6b44a4a8b

... for causing mochitest-bc failures:

https://tbpl.mozilla.org/php/getParsedLog.php?id=22327658&tree=Mozilla-Central
https://tbpl.mozilla.org/php/getParsedLog.php?id=22327676&tree=Fx-Team

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_buttons_nopersist.js | Test timed out

Comment 17

[David Creswick]

I thought it already passed a try run? Given that I don't have a platform on which the test fails, I'm at a loss for how to progress.

Comment 18

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

I'll take a look David - thanks for getting this far

Comment 19

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Resetting priorities because these P* levels are very out of date.
Sorry for the bug spam. Filter on Lobster Thermidor