Closed Bug 1448531 Opened 3 years ago Closed 3 years ago

Use BrowserTestUtils.waitForNewTab in test for drag-and-drop URLs

Categories

(Core :: DOM: Drag & Drop, enhancement)

enhancement
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(6 files, 3 obsolete files)

18.04 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
15.30 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
4.23 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
9.41 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
6.34 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
1.71 KB, patch
dao
: review+
Details | Diff | Splinter Review
Currently the following tests are counting the number of opened tab/window, but we should check the URLs of the actually opened tabs/windows.

https://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_newTabDrop.js
https://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_newWindowDrop.js
https://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_tabDrop.js

We can use BrowserTestUtils.waitForNewTab for tabs.
tests for tab can be rewritten easily.

the test for window is troublesome, because the current BrowserTestUtils doesn't support waiting for error page in new window, and also waiting for *any* new window with given URL.
supporting them will introduce 1 or more parameters into some helper functions, and I wonder if I should change the functions signature first (use single object instead of multiple parameters)
Changed manual TabOpen event handling to BrowserTestUtils.waitForNewTab, with expected URLs.

ANY_URL is used for search case, to accept any URL.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #8962147 - Flags: review?(enndeakin)
So, I needed the similar testing function for window case,
and added BrowserTestUtils.waitForAnyNewBrowserWindow
(added separate function because this behaves very differently than BrowserTestUtils.waitForNewWindow)

Then, some URLs results in error page, and "load" event is not dispatched, and browserLoaded didn't work.
So added maybeErrorPage parameter which uses DOMContentLoaded event that is dispatched for error page case,
and also in that case it uses document.location.href, because event.target.documentURI is "about:neterror..."
Attachment #8962148 - Flags: review?(enndeakin)
Comment on attachment 8962148 [details] [diff] [review]
Part 2: Add BrowserTestUtils.waitForAnyNewBrowserWindow and use it in tests for drag-and-drop URLs for window.

I'd rather not have two wait-for-window-open functions, especially since it isn't clear when you would use one or other. I don't see anything that stands out as being too different about the two functions.
Okay, I'll merge them into one.

then, I think it's better making the parameter an object, to clarify which "true" is what
(I think it's better fixing other APIs as well)

this patch just changes the signature of BrowserTestUtils.waitForNewWindow,
and fixes tests that passes the parameter.
Attachment #8962148 - Attachment is obsolete: true
Attachment #8962148 - Flags: review?(enndeakin)
Attachment #8963856 - Flags: review?(enndeakin)
and this patch adds anyWindow parameter.
Attachment #8963857 - Flags: review?(enndeakin)
and maybeErrorPage parameter to BrowserTestUtils.waitForNewWindow and BrowserTestUtils.browserLoaded.
(I'll file a bug to fix BrowserTestUtils.browserLoaded parameter if this way is fine)
Attachment #8963858 - Flags: review?(enndeakin)
and finally fixes browser_newWindowDrop.js by using BrowserTestUtils.waitForNewWindow
Attachment #8963859 - Flags: review?(enndeakin)
Attachment #8962147 - Flags: review?(enndeakin) → review+
Attachment #8963856 - Flags: review?(enndeakin) → review+
Comment on attachment 8963857 [details] [diff] [review]
Part 3: Add anyWindow parameter to BrowserTestUtils.waitForNewWindow.

>+
>+        let win = subject.QueryInterface(Ci.nsIDOMWindow);

QueryInterface isn't needed here. You can just call the argument 'win'.
Attachment #8963857 - Flags: review?(enndeakin) → review+
Comment on attachment 8963858 [details] [diff] [review]
Part 4: Add maybeErrorPage parameter to BrowserTestUtils.waitForNewWindow and BrowserTestUtils.browserLoaded.

>       let mm = browser.ownerGlobal.messageManager;
>-      mm.addMessageListener("browser-test-utils:loadEvent", function onLoad(msg) {
>+      let eventName = maybeErrorPage
>+          ? "browser-test-utils:DOMContentLoadedEvent"
>+          : "browser-test-utils:loadEvent";
>+      mm.addMessageListener(eventName, function onLoad(msg) {
>         if (msg.target == browser && (!msg.data.subframe || includeSubFrames) &&
>-            isWanted(msg.data.url)) {
>-          mm.removeMessageListener("browser-test-utils:loadEvent", onLoad);
>+            isWanted(maybeErrorPage ? msg.data.href : msg.data.url)) {
>+          mm.removeMessageListener(eventName, onLoad);

It's a bit confusing having 'href' and 'url' here being different in a way that isn't clear what the distinction is. I'm not really sure myself.
Attachment #8963859 - Flags: review?(enndeakin) → review+
Okay, I renamed them to internalURL/visibleURL, and also added some comment.
Attachment #8963858 - Attachment is obsolete: true
Attachment #8963858 - Flags: review?(enndeakin)
Attachment #8964784 - Flags: review?(enndeakin)
Attachment #8964784 - Flags: review?(enndeakin) → review+
Thank you for reviewing :)

unfortunately these patches fail on debug build, because of a leak:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fe272799329307fcdfc672d6b9aad79dc72aefd&group_state=expanded&selectedJob=172230608

looks like the anyWindow option leaks the dialog, because it's waiting forever on confirm dialog that is not what the test expects.
I'll try fixing shortly.
So, it turns out that observer for browser-delayed-startup-finished is the reason of the leak.
because the observer is not removed if non-browser window is opened.

I've made TestUtils.topicObserved to return a function that removes the observer, as an optional out-parameter,
and made BrowserTestUtils.waitForNewWindow to use it.
Attachment #8965585 - Flags: review?(dao+bmo)
Comment on attachment 8965585 [details] [diff] [review]
Part 6: Remove unnecessary observer in BrowserTestUtils.waitForNewWindow.

>+        let startupObservationCanceller = { value: null };
>         let promises = [
>           TestUtils.topicObserved("browser-delayed-startup-finished",
>-                                  subject => subject == win),
>+                                  subject => subject == win,
>+                                  startupObservationCanceller),
>         ];
> 
>         if (url) {
>           await this.waitForEvent(win, "DOMContentLoaded");
> 
>           if (win.document.documentURI != "chrome://browser/content/browser.xul") {
>+            // For non-browser window, browser-delayed-startup-finished
>+            // won't be notified.  We should remove the observer.
>+            startupObservationCanceller.value();

Can you just check win.location before adding the observer (and before waiting for DOMContentLoaded, I guess)?
Attachment #8965585 - Flags: review?(dao+bmo) → review-
Thank you for reviewing!

(In reply to Dão Gottwald [::dao] from comment #15)
> Can you just check win.location before adding the observer (and before
> waiting for DOMContentLoaded, I guess)?

unfortunately, win.location.href is "about:blank" at that point.
if there's any other way to get the URL specified by openWindow/openDialog, we could use it tho...
(In reply to Tooru Fujisawa [:arai] from comment #16)
> Thank you for reviewing!
> 
> (In reply to Dão Gottwald [::dao] from comment #15)
> > Can you just check win.location before adding the observer (and before
> > waiting for DOMContentLoaded, I guess)?
> 
> unfortunately, win.location.href is "about:blank" at that point.

Hmm. Did bug 1336227 "break" this?

Alternatively, you can add the observer after DOMContentLoaded.
(In reply to Dão Gottwald [::dao] from comment #17)
> (In reply to Tooru Fujisawa [:arai] from comment #16)
> > Thank you for reviewing!
> > 
> > (In reply to Dão Gottwald [::dao] from comment #15)
> > > Can you just check win.location before adding the observer (and before
> > > waiting for DOMContentLoaded, I guess)?
> > 
> > unfortunately, win.location.href is "about:blank" at that point.
> 
> Hmm. Did bug 1336227 "break" this?

possibly.

> Alternatively, you can add the observer after DOMContentLoaded.

yeah, it seems to work locally.
I'll ask review after try run, to make sure the reordering doesn't break other tests.
Okay, waiting for DOMContentLoaded before adding browser-delayed-startup-finished observer works
Attachment #8965585 - Attachment is obsolete: true
Attachment #8966092 - Flags: review?(dao+bmo)
Attachment #8966092 - Flags: review?(dao+bmo) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f344c2aa32c929855c05250e1a58bde0ece1d0b0
Bug 1448531 - Part 1: Use BrowserTestUtils.waitForNewTab in tests for drag-and-drop URLs for tab. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a6b2d9e232e47cbb718190056576f0bca79c2e5
Bug 1448531 - Part 2: Make BrowserTestUtils.waitForNewWindow receive parameters object. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab9912ab15f7daf6d0fea0fabdaed1d7b07fce88
Bug 1448531 - Part 3: Add anyWindow parameter to BrowserTestUtils.waitForNewWindow. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bbfaef732e42910ca7b1a2c110f4f15b0f443e
Bug 1448531 - Part 4: Add maybeErrorPage parameter to BrowserTestUtils.waitForNewWindow and BrowserTestUtils.browserLoaded. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/de4ceede58ecf75ce76e5442b241926c06d35843
Bug 1448531 - Part 5: Add BrowserTestUtils.waitForNewWindow and use it in tests for drag-and-drop URLs for window. r=enndeakin

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b269a30cba2645488b6f67e10a681ba750120c
Bug 1448531 - Part 6: Wait for DOMContentLoaded before adding observer. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/73a3eabd2bd29d2ef8945648615f49ed29b326c9
Bug 1448531 - Part 7: Request longer timeout for browser/base/content/test/general/browser_newTabDrop.js. r=bustage
You need to log in before you can comment on or make changes to this bug.