1261251 - [e10s] Make toolkit/components/passwordmgr/test/test_bug_627616.html work under e10s

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Drew Willcoxon :adw]

Make toolkit/components/passwordmgr/test/test_bug_627616.html work under e10s.

Comment 1

[Drew Willcoxon :adw]

Attachment: MozReview Request: Bug 1261251 - [e10s] Make toolkit/components/passwordmgr/test/test_bug_627616.html work under e10s. r?MattN

Here's a pretty straightforward conversion to a chrome script.  I'm not sure whether this fits in with what you were planning for these tests, and I haven't looked at other tests in this directory, but it shouldn't be hard to make any changes you'd like.

I also don't know if there's a better way to make only this test in the mochitest.ini enabled on e10s other than adding skip-ifs for everything else...

Review commit: https://reviewboard.mozilla.org/r/43649/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43649/

Comment 2

[Drew Willcoxon :adw]

getDialogDoc is copied from prompt_common.js BTW.

Comment 3

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63e24f440fa8

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8737016 [details]
MozReview Request: Bug 1261251 - [e10s] Make toolkit/components/passwordmgr/test/test_bug_627616.html work under e10s. r?MattN

https://reviewboard.mozilla.org/r/43649/#review40487

::: toolkit/components/passwordmgr/test/mochitest.ini:2
(Diff revision 1)
>  [DEFAULT]
> -skip-if = buildapp == 'mulet' || buildapp == 'b2g' || e10s
> +skip-if = buildapp == 'mulet' || buildapp == 'b2g'

> I also don't know if there's a better way to make only this test in the mochitest.ini enabled on e10s other than adding skip-ifs for everything else...

Yeah, just `hg move` the file to the "mochitest" subdirectory where we're putting the e10s compatible tests.

::: toolkit/components/passwordmgr/test/prompt_chrome.js:21
(Diff revision 1)
> +let pps = Cc["@mozilla.org/network/protocol-proxy-service;1"].
> +          getService(Ci.nsIProtocolProxyService);
> +pps.asyncResolve(channel, 0, {
> +  onProxyAvailable(req, uri, pi, status) {
> +    let mozproxy = "moz-proxy://" + pi.host + ":" + pi.port;
> +    gLogin = Cc["@mozilla.org/login-manager/loginInfo;1"].
> +             createInstance(Ci.nsILoginInfo);
> +    gLogin.init(mozproxy, null, "proxy_realm", "proxy_user", "proxy_pass",
> +                "", "");
> +    Services.logins.addLogin(gLogin);
> +
> +    gLogin2 = Cc["@mozilla.org/login-manager/loginInfo;1"].
> +              createInstance(Ci.nsILoginInfo);
> +    gLogin2.init("http://mochi.test:8888", null, "mochirealm", "user1name",
> +                 "user1pass", "", "");
> +    Services.logins.addLogin(gLogin2);

It seems like this isn't something to be shared with other prompt tests so it's probably better running as a function sent to the parent with runInParent(…) from test_bug_627616.html.

::: toolkit/components/passwordmgr/test/prompt_chrome.js:78
(Diff revision 1)
> +  var wm = Cc["@mozilla.org/appshell/window-mediator;1"].
> +           getService(Ci.nsIWindowMediator);
> +  //var enumerator = wm.getEnumerator("navigator:browser");
> +  var enumerator = wm.getXULWindowEnumerator(null);

Nit: May as well switch to `let` and `Services.wm` while blame is changing…

::: toolkit/components/passwordmgr/test/prompt_chrome.js:100
(Diff revision 1)
> +          continue;
> +        var childDoc = childDocShell.QueryInterface(Ci.nsIDocShell)
> +                                    .contentViewer
> +                                    .DOMDocument;
> +
> +        //ok(true, "Got window: " + childDoc.location.href);

I believe you can do `assert.ok` if you want this check here, otherwise remove it

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 8737016 [details]
MozReview Request: Bug 1261251 - [e10s] Make toolkit/components/passwordmgr/test/test_bug_627616.html work under e10s. r?MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43649/diff/1-2/

Comment 6

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8633c85dbaa5

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8737016 [details]
MozReview Request: Bug 1261251 - [e10s] Make toolkit/components/passwordmgr/test/test_bug_627616.html work under e10s. r?MattN

https://reviewboard.mozilla.org/r/43649/#review40787

::: toolkit/components/passwordmgr/test/mochitest/test_bug_627616.html:133
(Diff revision 2)
> +        Services.logins.removeLogin(gLogin);
> +        Services.logins.removeLogin(gLogin2);

Nit: You don't need to do this anymore as I added a shared cleanup function that runs by default in pwmgr_common (which this test now uses) and calls removeAllLogins(). You can then make the login variables local instead of "global".

::: toolkit/components/passwordmgr/test/mochitest/test_bug_627616.html:140
(Diff revision 2)
> +      function startWatchingForPrompts() {
> +        gInterval = setInterval(() => {
> +          let doc = getDialogDoc();

I'm pretty sure we don't need to poll as there should be an observer notification you can use instead. It may be worth looking at the prompt component tests to copy their approach for e10s.

I'll leave it up to you to fix this or not as this should work as-is even if it's somewhat less than ideal.

Comment 8

[Drew Willcoxon :adw]

Thanks, I used a common-dialog-loaded observer and it made the test much nicer.

https://hg.mozilla.org/integration/fx-team/rev/7eebd9f0738a

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7eebd9f0738a