Closed Bug 1271120 Opened 8 years ago Closed 8 years ago

Finish porting editor chrome mochitests to plain mochitests so they work on e10s

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
e10s + ---
firefox52 --- fixed

People

(Reporter: ayg, Assigned: ayg)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

0006-Bug-1271120-WIP.patch
14.69 KB, patch
Details | Diff | Splinter Review
patch
1.36 KB, patch
Details | Diff | Splinter Review
Bug 1271120 - Port test_composition_event_created_in_chrome.html from chrome to plain;
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1271120 - Port test_bug569988.html from chrome to plain;
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1271120 - Port test_bug636465.xul from chrome to plain;
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1271120 - Port test_bug1209414.html from chrome to plain;
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1271120 - Port test_bug1219928.html from chrome to plain;
58 bytes, text/x-review-board-request
masayuki
: review+
Details 
Bug 1269209 did the easy ones.  The following ones presented difficulties.

editor/libeditor/tests/:

* test_bug569988.html: Need to call synthesizeKey() in a chrome worker.  Blake suggests in bug 1269209 comment 11 to try importing BrowserTestUtils.jsm in the chrome script.
* test_bug635636.html: mainWindow.gBrowser is undefined, only with e10s.  In bug 1269209 comment 8, Blake describes what it's trying to do, but said he didn't know off the top of his head how to fix it.
* test_bug830600.html: "Error: Unable to restore focus, expect failures and timeouts" followed by "WARNING: TabChild::SetFocus not supported in TabChild" and hang, only with e10s.  Blake had no idea without debugging.
* test_bug1102906.html: Depends on ChromeUtils.js, so porting has to wait for bug 1271115.  Porting after that requires no changes to the file.

editor/composer/test/:

* test_bug1219928.html: NS_ERROR_ILLEGAL_VALUE returned by nsIEditorSpellCheck.GetNextMisspelledWord, only with e10s.  Blake says further debugging needed.
* test_bug1209414.html: Steps to proceed in bug 1269209 comment 7.

For the ones that failed only on e10s, my patch in bug 1269209 ports them to plain but disables on e10s.  For the others, I'll attach a patch with my work to date.
* editor/libeditor/tests/test_dragdrop.html and test_contenteditable_text_input_handling.html: Also depend on ChromeUtils.js (don't know why I didn't notice that the first time), so also have to wait for bug 1271115.  Also require no special changes to port, beyond getting rid of "chrome://" and Components -> SpecialPowers.C*, although adding a doctype to test_dragdrop.html might be a nice idea.
On reflection, I think the ChromeUtils.js ones would fit best in another bug, so I filed bug 1271125.
No longer depends on: 1271115
This is my work to date on two of the test files, on the off chance anyone else wants to take it up before I get back.
* editor/libeditor/tests/test_composition_event_created_in_chrome.html: NS_ERROR_XPC_BAD_CONVERT_JS at line 146 of specialpowersAPI.js, which traces back to calling SpecialPowers.wrap() on aInputElement, which is just a plain old <input> element.  No idea what's happening here.  The wrap() is needed in order to call QueryInterface.  Blake, any idea? <https://treeherder.mozilla.org/logviewer.html#?job_id=20523514&repo=try#L5414>
Flags: needinfo?(mrbkap)
tracking-e10s: --- → ?
Attached patch patch — — Splinter Review 
This took longer than it should have for me to spot. It's a pretty simple fix.
Flags: needinfo?(mrbkap)
Thanks, works perfectly!  For test_bug569988.html, if I import BrowserTestUtils.jsm as you suggested into the chrome script that I'm running with SpecialPowers.loadChromeScript, how do I get a browser object to pass to synthesizeKey()?  I tried to cargo-cult it for some time but failed.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Flags: needinfo?(mrbkap)
(In reply to :Aryeh Gregor (working until September 2) from comment #6)
> For test_bug569988.html, if I import
> BrowserTestUtils.jsm as you suggested into the chrome script that I'm
> running with SpecialPowers.loadChromeScript, how do I get a browser object
> to pass to synthesizeKey()?  I tried to cargo-cult it for some time but
> failed.

This is hard :-(

Can you maybe try to cargo-cult [1] passing the dialog's contentWindow in as the last argument? I can look more tomorrow if you need.

[1] http://searchfox.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/AsyncUtilsContent.js#64
Flags: needinfo?(mrbkap) → needinfo?(ayg)
I tried, but

1) How do I import EventUtils?  "Services" is not defined, and if I use Components.utils.import, it says window is not defined, even if I do this:

  var window = {};
  Components.utils.import("chrome://mochikit/content/tests/SimpleTest/EventUtils.js");

2) What is the exact way to get the dialog's contentWindow in this file?
Flags: needinfo?(ayg) → needinfo?(mrbkap)
(In reply to Aryeh Gregor (:ayg) (working until September 2) from comment #8)
> 1) How do I import EventUtils?  "Services" is not defined, and if I use
> Components.utils.import, it says window is not defined, even if I do this:

Cargo-cult harder! See the outlined sections at [1]. In particular, we need to pass in a scope object that sets up the environment in a way that EventUtils.js expects. Then you'll have the global objects on the object you passed in and ready to use.

[1] http://searchfox.org/mozilla-central/rev/0dd403224a5acb0702bdbf7ff405067f5d29c239/testing/mochitest/tests/SimpleTest/AsyncUtilsContent.js#9-16,18

> 2) What is the exact way to get the dialog's contentWindow in this file?

I think it might be as simple as gPromptInput.ownerDocument.defaultView.
Flags: needinfo?(mrbkap)
Success!  \o/
I'm going to refocus this bug on just test_composition_event_created_in_chrome.html, test_bug569988.html, and test_bug1209414.html (which I have patches for) and open separate bugs for the others.
Summary: Finish porting editor chrome mochitests to plain mochitests so they work on e10s → Port some more editor chrome mochitests to plain mochitests so they work on e10s
I take it back.  There were only like two more tests that weren't ported, and I just ported them, so I'll leave this as the one to finish it.  Some of the ported tests don't work on e10s, though.
Summary: Port some more editor chrome mochitests to plain mochitests so they work on e10s → Finish porting editor chrome mochitests to plain mochitests so they work on e10s
Comment on attachment 8804708 [details]
Bug 1271120 - Port test_composition_event_created_in_chrome.html from chrome to plain;

https://reviewboard.mozilla.org/r/88642/#review87862
Attachment #8804708 - Flags: review?(masayuki) → review+
Comment on attachment 8804710 [details]
Bug 1271120 - Port test_bug636465.xul from chrome to plain;

https://reviewboard.mozilla.org/r/88646/#review87864

Didn't you remove test_bug636465.xul once and recreate it?

Therefore, I don't see the diff in the file. And if we'd land this changeset with such history, I guess that the annotation for the file would be broken.
Attachment #8804710 - Flags: review?(masayuki) → review-
Comment on attachment 8804712 [details]
Bug 1271120 - Port test_bug1219928.html from chrome to plain;

https://reviewboard.mozilla.org/r/88650/#review87866

::: editor/composer/test/mochitest.ini:23
(Diff revision 1)
> +[test_bug1219928.html]
> +skip-if = e10s

I hope that if you know the reason why we cannot test this with e10s, I'd be happy if you add short comment for explaining the reason.
Attachment #8804712 - Flags: review?(masayuki) → review+
Comment on attachment 8804711 [details]
Bug 1271120 - Port test_bug1209414.html from chrome to plain;

https://reviewboard.mozilla.org/r/88648/#review87870

::: editor/composer/test/test_bug1209414.html:75
(Diff revision 1)
> +    addMessageListener("contextMenu-not-null", () => contextMenu != null);
> +    addMessageListener("de_DE-exists", () => de_DE.exists());

I hope that these lines are moved to near |is(script.sendSyncMessage("contextMenu-not-null")[0][0], true,| and |is(script.sendSyncMessage("de_DE-exists")[0][0], true,| because I took some time to look for where are the message sender.
Attachment #8804711 - Flags: review?(masayuki) → review+
Comment on attachment 8804709 [details]
Bug 1271120 - Port test_bug569988.html from chrome to plain;

https://reviewboard.mozilla.org/r/88644/#review87874

::: editor/libeditor/tests/test_bug569988.html:67
(Diff revision 1)
> +      var EventUtils = {};
> +      EventUtils.window = {};
> +      EventUtils._EU_Ci = Components.interfaces;
> +      EventUtils._EU_Cc = Components.classes;
> +      Components.classes['@mozilla.org/moz/jssubscript-loader;1']
> +                .getService(Components.interfaces.mozIJSSubScriptLoader)
> +                .loadSubScript("chrome://mochikit/content/tests/SimpleTest/EventUtils.js",
> +                               EventUtils);
> +      EventUtils.synthesizeKey("VK_ESCAPE", {},
> +                               gPromptInput.ownerDocument.defaultView);

Although, this is not a scope of this bug, I want a utility like SpecialPowers for loaded chrome script...

::: editor/libeditor/tests/test_bug569988.html:75
(Diff revision 1)
> +      EventUtils.synthesizeKey("VK_ESCAPE", {},
> +                               gPromptInput.ownerDocument.defaultView);
> +      sendAsyncMessage("finished");

I don't understand why we need to send async message of "finished" from onPromptFocus(). If you check the value after |prompt()|, I can understand the reason that it checks if the handlers in the prompt actually ran.

If I'm wrong, please request review again.
Attachment #8804709 - Flags: review?(masayuki) → review-
Comment on attachment 8804712 [details]
Bug 1271120 - Port test_bug1219928.html from chrome to plain;

https://reviewboard.mozilla.org/r/88650/#review87866

> I hope that if you know the reason why we cannot test this with e10s, I'd be happy if you add short comment for explaining the reason.

I don't know.  Bug 1269209 comment 3 says further debugging is needed.  I added that info to the commit message.
Comment on attachment 8804710 [details]
Bug 1271120 - Port test_bug636465.xul from chrome to plain;

https://reviewboard.mozilla.org/r/88646/#review87864

This is a problem with using git to push to hg.  :(  I'll manually record the rename on the hg side and re-request review.  (I think this is one feature of git that's really lame, but I don't think there's anything I can do about it.)
Comment on attachment 8804709 [details]
Bug 1271120 - Port test_bug569988.html from chrome to plain;

https://reviewboard.mozilla.org/r/88644/#review88290
Attachment #8804709 - Flags: review?(masayuki) → review+
Comment on attachment 8804710 [details]
Bug 1271120 - Port test_bug636465.xul from chrome to plain;

https://reviewboard.mozilla.org/r/88646/#review88292
Attachment #8804710 - Flags: review?(masayuki) → review+
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1334f6bb2ea0
Port test_composition_event_created_in_chrome.html from chrome to plain; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe1a0812871c
Port test_bug569988.html from chrome to plain; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ff4e1962fb
Port test_bug636465.xul from chrome to plain; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/bed54475d25b
Port test_bug1209414.html from chrome to plain; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/6357eaf453b0
Port test_bug1219928.html from chrome to plain; r=masayuki
Greenish try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe56506f5253  The failing tests on Android were disabled for the inbound push.  The failure in test_bug636465.html is intermittent and I have no special reason to think it's related to this bug -- it's presumably some flakiness in the screenshots.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: