Closed
Bug 896711
Opened 11 years ago
Closed 10 years ago
BrowserChromeTests.runWhenReady is being abused
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 32
People
(Reporter: Gavin, Assigned: Gavin)
Details
Attachments
(1 file, 3 obsolete files)
15.14 KB,
patch
|
Details | Diff | Splinter Review |
See bug 881049 comment 5 and the tests in http://mxr.mozilla.org/mozilla-central/search?string=runWhenReady%28 for some examples (the latter seem to all be from bug 789919). I guess we can go in two directions here - if this is a useful API, we can make it handle multiple callbacks and give it a better name. I don't think it is in general, though - we don't want to cement the existence of delayedStartup any more than it currently is, and it somewhat duplicates the existing delayed-startup-finished notification. The other direction is to get rid of it, fix the tests, and address bug 851516 some other way (i.e. attachment 726570 [details] [diff] [review]). I assume you favor option #2, Dao?
Assignee | ||
Comment 1•11 years ago
|
||
Thankfully I see no add-on users at the moment, despite having shipped this in 22, so we can probably remove it without much worry.
Comment 2•11 years ago
|
||
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #0) > I assume you favor option #2, Dao? Yes. This provides no benefit over delayed-startup-finished, so I see no point in introducing this as a blessed API.
Assignee | ||
Comment 3•11 years ago
|
||
Comment 4•11 years ago
|
||
Comment on attachment 782204 [details] [diff] [review] patch The BrowserChromeTest check in browser-test.js can go away too, right?
Attachment #782204 -
Flags: review?(dao) → review+
Comment 5•11 years ago
|
||
Comment on attachment 782204 [details] [diff] [review] patch Review of attachment 782204 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks like it's not quite up-to-date and needs to be rebased? Also, there's a couple of browser/ tests that break if we just remove it.
Assignee | ||
Comment 6•11 years ago
|
||
Tim, can you take a quick look at the test changes? They're mostly: - replace observances of of load+runWhenReady with whenDelayedStartupFinished (or equivalent) - some cleanup (adding license headers, use Services.jsm, make copies of a function consistent, add comments, adjust indentation), sorry about that - for browser_dbg_multiple-windows.js I just removed the runWhenReady because the load event always fires after delayedStartup in my testing (and the test doesn't even rely on that anyways, AFAICT)
Attachment #782204 -
Attachment is obsolete: true
Attachment #782796 -
Flags: review?(ttaubert)
Assignee | ||
Comment 7•11 years ago
|
||
Pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=0b3a1df24c4a
Comment 8•11 years ago
|
||
Comment on attachment 782796 [details] [diff] [review] complete patch with test fixes Review of attachment 782796 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/test/browser_dbg_multiple-windows.js @@ +82,5 @@ > + // Wait for the requested content document to load > + chromeWin.document.addEventListener("load", function onload(e) { > + if (e.target.documentURI != TAB2_URL) { > + return; > + } Yeah, listening for 'load' of the first (and only) tab and checking the URL should be enough here as that load is kicked off by delayedStartup().
Attachment #782796 -
Flags: review?(ttaubert) → review+
Assignee | ||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/76db98535799
Target Milestone: --- → Firefox 25
Assignee | ||
Comment 10•11 years ago
|
||
This was backed out because it broke metro's chrome harness (related to bug 886109).
Assignee | ||
Comment 11•11 years ago
|
||
Bug 886109 was apparently fixed, trying this again: https://tbpl.mozilla.org/?tree=Try&rev=7981c213ca5c
Assignee | ||
Comment 12•11 years ago
|
||
I had to make a few tweaks to the devtools tests to unbitrot this, and couldn't help doing a little extra cleanup. Victor, the only changes I want review on are to the browser/devtools test changes: - used Services.focus where applicable - renamed getDOMWindow to getChromeWindow, to better reflect what it actually does - removed a useless test in testFocusFirst ("topWindow" was unused, and "top" just referred to the current top window, i.e. window.top, which it doesn't make sense to compare to "window")
Attachment #782796 -
Attachment is obsolete: true
Attachment #8334262 -
Flags: review?(vporof)
Comment 13•11 years ago
|
||
Comment on attachment 8334262 [details] [diff] [review] unbitrotted patch Review of attachment 8334262 [details] [diff] [review]: ----------------------------------------------------------------- Devtools changes LGTM. ::: browser/devtools/debugger/test/browser_dbg_clean-exit-window.js @@ +38,5 @@ > let isActive = promise.defer(); > let isLoaded = promise.defer(); > > promise.all([isActive.promise, isLoaded.promise]).then(() => { > + executeSoon(() => { Please add a comment here specifying why runWhenReady shouldn't be used. Generally, we're trying to make a habit of documenting all executeSoons, or removing them if not required.
Attachment #8334262 -
Flags: review?(vporof) → review+
Comment 14•10 years ago
|
||
(In reply to Victor Porof [:vp] from comment #13) > Comment on attachment 8334262 [details] [diff] [review] > unbitrotted patch > > Review of attachment 8334262 [details] [diff] [review]: > ----------------------------------------------------------------- > > Devtools changes LGTM. > > ::: browser/devtools/debugger/test/browser_dbg_clean-exit-window.js > @@ +38,5 @@ > > let isActive = promise.defer(); > > let isLoaded = promise.defer(); > > > > promise.all([isActive.promise, isLoaded.promise]).then(() => { > > + executeSoon(() => { > > Please add a comment here specifying why runWhenReady shouldn't be used. This wouldn't make much sense, since this patch removes runWhenReady.
Comment 15•10 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #14) > > This wouldn't make much sense, since this patch removes runWhenReady. But it adds an executeSoon(), which also doesn't by default make sense. Documenting all executeSoons is a good practice and avoids their abuse.
Assignee | ||
Comment 16•10 years ago
|
||
IIRC (I should have taken notes in the bug...) attempting to write that comment made me reconsider whether it was actually needed. Haven't had a chance to re-visit this.
Comment 17•10 years ago
|
||
Is this still waiting for documentation on why a single executeSoon is needed or the realization that it isn't needed? Can we file a new bug on this and let devtools people figure it out?
Flags: needinfo?(vporof)
Flags: needinfo?(gavin.sharp)
Comment 18•10 years ago
|
||
IIRC this is waiting for documentation about why executeSoon is needed (at all).
Flags: needinfo?(vporof)
Assignee | ||
Comment 19•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6c638c34654b
Attachment #8334262 -
Attachment is obsolete: true
Flags: needinfo?(gavin.sharp)
Assignee | ||
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/193c049cea42
Target Milestone: Firefox 25 → Firefox 32
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/193c049cea42
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•