Closed Bug 896711 Opened 6 years ago Closed 6 years ago

BrowserChromeTests.runWhenReady is being abused

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: Gavin, Assigned: Gavin)

Details

Attachments

(1 file, 3 obsolete files)

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?
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.
(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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #782204 - Flags: review?(dao)
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 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.
Attached patch complete patch with test fixes (obsolete) — Splinter Review
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)
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+
This was backed out because it broke metro's chrome harness (related to bug 886109).
Depends on: 886109
Attached patch unbitrotted patch (obsolete) — Splinter Review
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 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+
No longer depends on: 886109
(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.
(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.
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.
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)
IIRC this is waiting for documentation about why executeSoon is needed (at all).
Flags: needinfo?(vporof)
https://hg.mozilla.org/mozilla-central/rev/193c049cea42
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.