Closed Bug 1252723 Opened 8 years ago Closed 8 years ago

[e10s] Make toolkit/components/prompts/test work under e10s

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

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

People

(Reporter: adw, Assigned: adw)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

[e10s] Make toolkit/components/prompts/test work under e10s -- at least the following tests:

test_bug619644.html
test_bug620145.html
test_bug625187.html
test_bug861605.html

Dolske's signed up for the other two tests in this directory.
Blocks: e10s-tests
Comment on attachment 8725507 [details]
MozReview Request: Bug 1252723 - [e10s] Make toolkit/components/prompts/test work under e10s. r?dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37541/diff/1-2/
The first patch fixed e10s but broke non-e10s, whoops.  Second patch fixes that.
We should be able to remove prompt_common.js after all tests are converted, too.
Oh, the summary of changes is pretty simple: These tests all use synthesizeKey() to close tab-modal prompts from mochitests running in content.  The patch uses a chrome script to do that now.  I couldn't figure out how to properly dispatch a key event to prompts, so I just call onButtonClick(1) on them.
Comment on attachment 8725507 [details]
MozReview Request: Bug 1252723 - [e10s] Make toolkit/components/prompts/test work under e10s. r?dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37541/diff/2-3/
Forgot to remove the e10s skip-if from the mochitest.ini.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=061bb47d671e
Attachment #8725507 - Flags: review?(dolske)
Comment on attachment 8725507 [details]
MozReview Request: Bug 1252723 - [e10s] Make toolkit/components/prompts/test work under e10s. r?dolske

https://reviewboard.mozilla.org/r/37541/#review37815

Sorry for the absurd delay. :( Would be r+, except for the first comment about the ESC keypress being needed for bug 619644's test.

::: toolkit/components/prompts/test/chromeScript.js:31
(Diff revision 3)
> +      infoTitle: {
> +        hidden: prompts[0].ui.infoTitle.getAttribute("hidden") == "true",
> +      },
> +    },
> +  });
> +  prompts[0].onButtonClick(1);

Did you perhaps try loading EventUtils into the chrome environment? I see a couple of tests (code_frame-script.js, browser_newtab_sponsored_icon_click.js) that do something similar... Would be nice if that worked.

The bug 619644 test really needs to be an actual ESC keypress, since that bugfix is checking to see that the event.stopPropigation() call actually worked. (Might be easier to convert this test to be a browser-chrome test?)

Otherwise this is fine for the other tests, which just want to close the prompt. I was a bit concerned about the way ConfirmEx prompts can have wonky button order/defaults... Was looking at if onKeyAction("cancel") would be better, although it needs an event, but I see it ultimately just calls onButtonClick(1) anyway. Which is what you're already doing.

::: toolkit/components/prompts/test/test_bug620145.html:95
(Diff revision 3)
>    var utils = SpecialPowers.getDOMWindowUtils(win);
>    utils.dispatchDOMEventViaPresShell(target, e, true);
>    ok(true, type + " sent to " + target.id);
>  }
>  
> -function handleDialog(ui, testNum)
> +function handleDialog()

Since "handleDialog" is normally a function called by the prompt_common.js / startCallbackTimer() stuff -- which you've removed -- let's rename this to avoid any confusion... checkSelection() or your favorite bikeshed color. :)

::: toolkit/components/prompts/test/test_bug620145.html
(Diff revision 3)
>      synthesizeMouse($("text"), 25, 55, { type: "mousemove" });
>      is(window.getSelection().toString(), "", "selection not made");
>    }
> -
> -  ok(true, "handleDialog sending mouseclick to dialog...");
> -  synthesizeMouse(ui.button0, 5, 5, { }, SpecialPowers.unwrap(ui.button0.ownerDocument.defaultView));

Ha, I guess it's a happy accident that your code "clicking" button1 still closes an alert(), which only has a button0. Could change the alert() to a confirm(), but it doesn't really matter.

::: toolkit/components/prompts/test/test_bug625187.html:84
(Diff revision 3)
>  
> +  chrome.destroy();
>    SimpleTest.finish();
>  }
>  
>  function handleDialog(ui)

Ditto on the function name here.
Comment on attachment 8725507 [details]
MozReview Request: Bug 1252723 - [e10s] Make toolkit/components/prompts/test work under e10s. r?dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37541/diff/3-4/
Attachment #8725507 - Flags: review?(dolske)
Ah, that works, thanks.  I actually did something similar in a previous test that I should have remembered here.
Comment on attachment 8725507 [details]
MozReview Request: Bug 1252723 - [e10s] Make toolkit/components/prompts/test work under e10s. r?dolske

https://reviewboard.mozilla.org/r/37541/#review39683
Attachment #8725507 - Flags: review?(dolske) → review+
https://hg.mozilla.org/mozilla-central/rev/7e4c0bee56f3
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.