Closed Bug 1254085 Opened 8 years ago Closed 8 years ago

Update test_browser_drop.xul to exercise remote browsers, too

Categories

(Toolkit :: UI Widgets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox48 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

The existing test is verifying that same-process content and chrome type browsers deal well with dropped links.

We should ensure that remote content-type browsers also work. AIUI we can use browser remote="true" in mochitest-chrome tests, so we should just be able to extend the test (though I'm not sure if it won't need to be rewritten to use frame scripts in some way...)

https://dxr.mozilla.org/mozilla-central/search?q=remote+browser+path%3Atest+path%3Axul+-path%3Alist&redirect=false&case=false

I'm not sure if remote type="chrome" docshells are supported, but I guess we can try.
tracking-e10s: --- → ?
So I basically spent all of today trying to get this to work, and there are too many issues that I don't know how to best address.

1) we don't actually fire droppedLinkHandler in e10s mode ( https://dxr.mozilla.org/mozilla-central/rev/4657041c6f77b36b1fb9647c28f53f4f51757360/toolkit/content/widgets/browser.xml#1391-1394,1404-1407 ). This means that, AFAICT, there is no way to get the link's 'name' in the e10s case based on a 'real' drag and drop happening, because:
2) the e10s code has no hooks. It's stuck in https://dxr.mozilla.org/mozilla-central/rev/4657041c6f77b36b1fb9647c28f53f4f51757360/embedding/browser/nsDocShellTreeOwner.cpp#997-1022 and provides no way for a JS callback to be used even from e.g. a content task. We'd need to catch a load (or lack thereof) and stop it, or something;
3) the e10s code isn't fired by the simulated drag+drop on a remote="true" <browser>, because if it were the browser should navigate in some of the testcases in this file. I haven't figured out why. It's possible that the synthetic events fired on the <browser> are somehow not forwarded to the content process and processed on elements there. It's also possible that the docshelltreeowner makes a different decision than the browser.xml one. I'm betting on the first possibility, ie we would need to fire the drag/drop code in the content process
4) there are no helpers for doing drag/drop in the content process. Just loading chromeutils from a contenttask doesn't work, because it tries to load EventUtils, which tries to use the |window| variable on its scope, which throws exceptions immediately upon loading the file. The stuff added to BrowserTestUtils in bug 1131818 is not extensive enough to support a full drag and drop, plus I would have to copy-paste the relevant wrapper code from ChromeUtils.js to fire the individual events, which also seems like it isn't the right way to do this.
5) trying to invoke the dropped link handler directly in some kind of unit test is also far from trivial because we would need to create simulated drag-drop events. This would be quite fragile, it's not immediately obvious how to retain parity with what happens in the non-e10s case (e.g. is mozSourceNode set for these events or not?) and some things, like dataTransfer objects, have no constructor either.

All in all, I'd like to delegate this to someone who actually knows how this code is supposed to work, or at least get some pointers as to what the best direction here would be.

Neil or Olli, looks like you wrote+reviewed most of the e10s code here, can you help?
Blocks: 936092
Flags: needinfo?(enndeakin)
Flags: needinfo?(bugs)
Attached patch Work in progress patch (obsolete) — Splinter Review
This enables stuff to be async, but I still need a way to actually do/test something useful for the e10s case.
For 3/4/5, you might be able to get by with just sending drop events directly, such as what browser_newtab_bug725996.js does.
Flags: needinfo?(enndeakin)
Comment on attachment 8728552 [details] [diff] [review]
Work in progress patch

(In reply to Neil Deakin from comment #3)
> For 3/4/5, you might be able to get by with just sending drop events
> directly, such as what browser_newtab_bug725996.js does.

OK, this seems to be sufficient. I also reuse those drop events to check that the name that the droppedLinkHandler service extracts is accurate.
Attachment #8728552 - Attachment is obsolete: true
Flags: needinfo?(bugs)
Blocks: 1257481
Comment on attachment 8729577 [details]
MozReview Request: Bug 1254085 - fix test_browser_drop.xul to also tests drops on remote content browsers, r?enndeakin

https://reviewboard.mozilla.org/r/39511/#review38133

::: toolkit/content/tests/chrome/test_browser_drop.xul:26
(Diff revision 1)
> +        if (e.target.location.href == win.location.href && win.location.href != "about:blank") {
> +          win.removeEventListener("load", onLoad);
> +          SimpleTest.waitForFocus(resolve, win);
> +        }
> +      }, true);
> +    });

With that other fix, perhaps this can just use waitForFocus without waiting for the load event?

::: toolkit/content/tests/chrome/window_browser_drop.xul:24
(Diff revision 1)
>  
> -function runTest()
> -{
> -  function expectLink(element, expectedLink, expectedName, data, testid)
> -  {
> -    link = "";
> +function doDropAndStopLoad(browser, data, shouldExpectStateChange) {
> +  ContentTask.setTestScope(window); // Need this so is/isnot/ok are available inside the contenttask
> +  return ContentTask.spawn(browser, {data, shouldExpectStateChange}, function*({data, shouldExpectStateChange}) {
> +    let { interfaces: Ci, utils: Cu } = Components;
> +    Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

Use Cu?

::: toolkit/content/tests/chrome/window_browser_drop.xul:38
(Diff revision 1)
> -    var child = document.getElementById(type + "child");
> -    child.droppedLinkHandler = handleLink;
> +    }
> +
> +    let wp = docShell.QueryInterface(Ci.nsIWebProgress);
> +    let progressListener = {
> +      onStateChange: function (webProgress, req, flags, status) {
> +        dump("waitForDocLoadAndStopIt: onStateChange " + flags.toString(16) + ": " + req.name + "\n");

Extraneous dump

::: toolkit/content/tests/chrome/window_browser_drop.xul:59
(Diff revision 1)
> +    let dataTransfer = new content.DataTransfer("dragstart", false);
> +    dataTransfer.mozSetDataAt(data.type, data.data, 0);
> +    let event = content.document.createEvent("DragEvents");
> +    event.initDragEvent("drop", true, true, content, 0, 0, 0, 0, 0,
> +                        false, false, false, false, 0, null, dataTransfer);
> +    content.document.body.dispatchEvent(event);

You don't need the 'content.' part in these three places.

::: toolkit/content/tests/chrome/window_browser_drop.xul:74
(Diff revision 1)
> +    }
> +    is(shouldExpectStateChange, gotLoad, "Should have gotten a load only if we expected it.");
> +    if (!gotLoad) {
> +      wp.removeProgressListener(progressListener);
> +    }
> +    return Promise.resolve([uri, nameReturned]);

Did you mean to have the promise resolve after stopContent? Why return a promise instead of returning the values directly?

::: toolkit/content/tests/chrome/window_browser_drop.xul:90
(Diff revision 1)
> +  lastLink = lastLinkName = "";
> +  if (browser.isRemoteBrowser) {
> +    dropInfoReceived = doDropAndStopLoad(browser, data[0][0], !!expectedLink);
> +  } else {
> +    dropInfoReceived = dropOnBrowserSync();
> +  }

Why is dropinfoReceived a Promise? Both blocks just use Promise.resolve.

::: toolkit/content/tests/chrome/window_browser_drop.xul:138
(Diff revision 1)
> -             "text/x-moz-url drop on browser with cancelled drop event");
>  
> -  window.close();
> -  window.opener.wrappedJSObject.SimpleTest.finish();
> +    // canceling the event, however, should prevent the link from being handled
> +    yield expectLink(browser, "", "",
> +                     [ [ { type: "text/uri-list", data: "http://www.mozilla.org/" } ] ],
> +                     "text/x-moz-url drop on browser with stopPropagation drop event", true);

with cancelled drop event
Attachment #8729577 - Flags: review?(enndeakin)
Comment on attachment 8729577 [details]
MozReview Request: Bug 1254085 - fix test_browser_drop.xul to also tests drops on remote content browsers, r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39511/diff/1-2/
Attachment #8729577 - Flags: review?(enndeakin)
https://reviewboard.mozilla.org/r/39511/#review38133

> With that other fix, perhaps this can just use waitForFocus without waiting for the load event?

Yes, good idea. Fixed.

> You don't need the 'content.' part in these three places.

I do, I think. This is a ContentTask - all of the things that normally live on the global of the window are not in the global of a content script, and so I need to prefix with `content.`

> Did you mean to have the promise resolve after stopContent? Why return a promise instead of returning the values directly?

It's true that I can return the values directly here. Fixed.

> Why is dropinfoReceived a Promise? Both blocks just use Promise.resolve.

One of them uses `ContentTask.spawn` and returns the result. So it doesn't return directly, because it needs to load the provided function in the content process, then that function has some `yield` statements which make it more asynchronous, and then finally the returned values will be used to resolve the promise that ContentTask.spawn returned to the caller. So in that case, the result is provided asynchronously.
Comment on attachment 8729577 [details]
MozReview Request: Bug 1254085 - fix test_browser_drop.xul to also tests drops on remote content browsers, r?enndeakin

https://reviewboard.mozilla.org/r/39511/#review38145

::: toolkit/content/tests/chrome/window_browser_drop.xul:82
(Diff revision 2)
>  
> -    expectLink(child, "http://www.mozilla.org", "http://www.mozilla.org",
> -               [ [ { type: "text/plain", data: "http://www.mozilla.org" } ] ],
> +function expectLink(browser, expectedLink, expectedName, data, testid, onbody=false) {
> +  function dropOnBrowserSync() {
> +    let dropEl = onbody ? browser.contentDocument.body : browser;
> +    synthesizeDrop(dropEl, dropEl, data, "", dropEl.ownerDocument.defaultView);
> +    return Promise.resolve([lastLink, lastLinkName]);

You can just return here as well.
Attachment #8729577 - Flags: review?(enndeakin) → review+
> > +  function dropOnBrowserSync() {
> > +    let dropEl = onbody ? browser.contentDocument.body : browser;
> > +    synthesizeDrop(dropEl, dropEl, data, "", dropEl.ownerDocument.defaultView);
> > +    return Promise.resolve([lastLink, lastLinkName]);
> 
> You can just return here as well.

OK, I see. Ignore this then.
https://hg.mozilla.org/mozilla-central/rev/ebf2becf4690
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.