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

RESOLVED INCOMPLETE

Status

defect
RESOLVED INCOMPLETE
7 years ago
11 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)

No description provided.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Posted patch patch (obsolete) — Splinter Review
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: nobody → dcrewi
Yes, I can write a test. I probably ought to know by now that I should have written one from the beginning.
Posted patch patch v2, with test (obsolete) — Splinter Review
Attachment #733511 - Attachment is obsolete: true
Attachment #734710 - Flags: review?(jwalker)
Attachment #734710 - Flags: review?(jwalker)
Posted patch patch v3, with test (obsolete) — Splinter Review
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+
(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.
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]
Posted patch patch v5Splinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/229cd2ebe225
Status: NEW → RESOLVED
Closed: 6 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 → ---
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 → --
Product: Firefox → DevTools
Per bug 1491875, this component has been closed, and the affected code is being removed from Firefox. Closing this bug as incomplete.
Status: REOPENED → RESOLVED
Closed: 6 years ago11 months ago
Resolution: --- → INCOMPLETE
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.