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

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, enhancement)

Description

[Tooru Fujisawa [:arai]]

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.

Comment 1

[Tooru Fujisawa [:arai]]

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)

Comment 2

[Tooru Fujisawa [:arai]]

try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fd424083df674065966c4f637091705b98426fa

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Use BrowserTestUtils.waitForNewTab in tests for drag-and-drop URLs for tab.

Changed manual TabOpen event handling to BrowserTestUtils.waitForNewTab, with expected URLs.

ANY_URL is used for search case, to accept any URL.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add BrowserTestUtils.waitForAnyNewBrowserWindow and use it in tests for drag-and-drop URLs for window.

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..."

Comment 5

[Neil Deakin]

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.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Make BrowserTestUtils.waitForNewWindow receive parameters object.

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.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Add anyWindow parameter to BrowserTestUtils.waitForNewWindow.

and this patch adds anyWindow parameter.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Add maybeErrorPage parameter to BrowserTestUtils.waitForNewWindow and BrowserTestUtils.browserLoaded.

and maybeErrorPage parameter to BrowserTestUtils.waitForNewWindow and BrowserTestUtils.browserLoaded.
(I'll file a bug to fix BrowserTestUtils.browserLoaded parameter if this way is fine)

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Add BrowserTestUtils.waitForNewWindow and use it in tests for drag-and-drop URLs for window.

and finally fixes browser_newWindowDrop.js by using BrowserTestUtils.waitForNewWindow

Comment 10

[Neil Deakin]

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'.

Comment 11

[Neil Deakin]

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.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Add maybeErrorPage parameter to BrowserTestUtils.waitForNewWindow and BrowserTestUtils.browserLoaded.

Okay, I renamed them to internalURL/visibleURL, and also added some comment.

Comment 13

[Tooru Fujisawa [:arai]]

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.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Remove unnecessary observer in BrowserTestUtils.waitForNewWindow.

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.

Comment 15

[Dão Gottwald [:dao]]

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)?

Comment 16

[Tooru Fujisawa [:arai]]

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...

Comment 17

[Dão Gottwald [:dao]]

(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.

Comment 18

[Tooru Fujisawa [:arai]]

(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.

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Wait for DOMContentLoaded before adding observer.

Okay, waiting for DOMContentLoaded before adding browser-delayed-startup-finished observer works

Comment 20

[Tooru Fujisawa [:arai]]

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

Comment 21

[Tooru Fujisawa [:arai]]

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

Comment 22

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f344c2aa32c9
https://hg.mozilla.org/mozilla-central/rev/0a6b2d9e232e
https://hg.mozilla.org/mozilla-central/rev/ab9912ab15f7
https://hg.mozilla.org/mozilla-central/rev/b6bbfaef732e
https://hg.mozilla.org/mozilla-central/rev/de4ceede58ec
https://hg.mozilla.org/mozilla-central/rev/d2b269a30cba
https://hg.mozilla.org/mozilla-central/rev/73a3eabd2bd2