Closed
Bug 855108
Opened 8 years ago
Closed 2 years ago
Commands toggled by the developer toolbar should not persist after the toolbar closes
Categories
(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
Firefox 23
People
(Reporter: sdrocking, Assigned: dcrewi)
Details
Attachments
(1 file, 4 obsolete files)
9.29 KB,
patch
|
jwalker
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•8 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Assignee | ||
Comment 1•8 years ago
|
||
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)
Updated•8 years ago
|
Summary: Closing Developer Tools should also close the 3D View → Commands toggled by the developer toolbar should not persist after the toolbar closes
Comment 2•8 years ago
|
||
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•8 years ago
|
Assignee: nobody → dcrewi
Assignee | ||
Comment 3•8 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•8 years ago
|
||
Attachment #733511 -
Attachment is obsolete: true
Attachment #734710 -
Flags: review?(jwalker)
Assignee | ||
Updated•8 years ago
|
Attachment #734710 -
Flags: review?(jwalker)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #734710 -
Attachment is obsolete: true
Attachment #735254 -
Flags: review?(jwalker)
Comment 6•8 years ago
|
||
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•8 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•8 years ago
|
||
Attachment #735254 -
Attachment is obsolete: true
Updated•8 years ago
|
Component: Developer Tools: 3D View → Developer Tools: Graphic Commandline and Toolbar
Comment 9•8 years ago
|
||
(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' != '='
Updated•8 years ago
|
Attachment #738070 -
Flags: review+
Comment 11•8 years ago
|
||
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•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #740131 -
Flags: review?(jwalker) → review+
Comment 13•8 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=d2ae034f8b2e Assuming it's green, I'll land this tomorrow.
Comment 14•8 years ago
|
||
https://tbpl.mozilla.org/?tree=Fx-Team&rev=8911b764dc1e https://hg.mozilla.org/integration/fx-team/rev/229cd2ebe225
Whiteboard: [fixed-in-fx-team]
Comment 15•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/229cd2ebe225
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Comment 16•8 years ago
|
||
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•8 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.
Comment 18•8 years ago
|
||
I'll take a look David - thanks for getting this far
Comment 19•7 years ago
|
||
Resetting priorities because these P* levels are very out of date. Sorry for the bug spam. Filter on Lobster Thermidor
Priority: P3 → --
Updated•3 years ago
|
Product: Firefox → DevTools
Comment 20•2 years ago
|
||
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: 8 years ago → 2 years ago
Resolution: --- → INCOMPLETE
Updated•2 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•