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

REOPENED
Assigned to

Status

REOPENED
6 years ago
2 months ago

People

(Reporter: sdrocking, Assigned: dcrewi)

Tracking

unspecified
Firefox 23
x86_64
Windows 7

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
(Assignee)

Comment 1

5 years ago
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.
Attachment #733511 - Flags: review?(jwalker)
Summary: Closing Developer Tools should also close the 3D View → Commands toggled by the developer toolbar should not persist after the toolbar closes
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?
Attachment #733511 - Flags: review?(jwalker)
(Assignee)

Updated

5 years ago
Assignee: nobody → dcrewi
(Assignee)

Comment 3

5 years ago
Yes, I can write a test. I probably ought to know by now that I should have written one from the beginning.
(Assignee)

Comment 4

5 years ago
Created attachment 734710 [details] [diff] [review]
patch v2, with test
Attachment #733511 - Attachment is obsolete: true
Attachment #734710 - Flags: review?(jwalker)
(Assignee)

Updated

5 years ago
Attachment #734710 - Flags: review?(jwalker)
(Assignee)

Comment 5

5 years ago
Created attachment 735254 [details] [diff] [review]
patch v3, with test
Attachment #734710 - Attachment is obsolete: true
Attachment #735254 - Flags: review?(jwalker)
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?
Attachment #735254 - Flags: review?(jwalker) → review+
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
Created attachment 738070 [details] [diff] [review]
patch v4, with review feedback
Attachment #735254 - Attachment is obsolete: true
Component: Developer Tools: 3D View → Developer Tools: Graphic Commandline and Toolbar
(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' != '='
I'll get this landed.
Whiteboard: [land-in-fx-team]
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, ...)
Whiteboard: [land-in-fx-team]
(Assignee)

Comment 12

5 years ago
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.
Attachment #738070 - Attachment is obsolete: true
Attachment #740131 - Flags: review?(jwalker)
Attachment #740131 - Flags: review?(jwalker) → review+
Try run: https://tbpl.mozilla.org/?tree=Try&rev=d2ae034f8b2e
Assuming it's green, I'll land this tomorrow.
https://hg.mozilla.org/mozilla-central/rev/229cd2ebe225
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 17

5 years ago
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.
I'll take a look David - thanks for getting this far
Resetting priorities because these P* levels are very out of date.
Sorry for the bug spam. Filter on Lobster Thermidor
Priority: P3 → --

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.