1191597 - Enable fullscreen-api tests on e10s

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Most (if not all) tests for Fullscreen API are currently disabled on e10s because of various reasons. It makes us unable to catch regression on e10s as soon as possible. We should enable them.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

So currently there are three subtests of fullscreen-api test does not work on e10s:
* file_fullscreen-esc-context-menu.html
* file_fullscreen-api-keys.html
* file_fullscreen-plugins.html

I've rewrote file_fullscreen-esc-context-menu to a browser chrome test locally, so that has been solved.

file_fullscreen-api-keys does not work because we need to send keys to the chrome process to take effect in this subtest, while I'm not aware of any method to do so.

file_fullscreen-plugins does not work because enableTestPlugin.js, the script it includes, currently crashes the content process (see bug 1213710).

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66d6cf963ca4

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1191597 part 1 - Add head.js and dummy page for browser chrome test.

promiseWaitForEvent and waitForDocLoadComplete are copied from browser/base/content/test/general/head.js

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1191597 part 2 - Convert fullscreen-esc-context-menu to a browser chrome test.

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1191597 part 3 - Convert fullscreen-api-keys to a browser chrome test.

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

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1191597 part 7 - Enable fullscreen-api test on e10s.

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

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Errrr... It works on Windows, but it seems there are still some issues on Linux...

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

I'll continue investigating the remaining issues, but the patches already submitted are needed anyway.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29362d927e00

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90c71ca42b8d

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1191597 part 4 - Use inner width and height for check window state.

Inner width and height are more reliable than outer ones. Change to
outer ones may not trigger resize event on content process.

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

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1191597 part 5 - Ensure to cleanup old window size from the list when exit fullscreen.

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

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8704969 [details]
MozReview Request: Bug 1191597 part 7 - Enable fullscreen-api test on e10s.

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

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

I need to go to sleep now, so haven't seen the test result... but I guess it should be fine now.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Still pending on try. it says ~55mins.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

I think it passed, but there was some unrelated error with leakcheck. I think.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8704966 [details]
MozReview Request: Bug 1191597 part 1 - Add head.js and dummy page for browser chrome test.

https://reviewboard.mozilla.org/r/29827/#review26741

::: dom/html/test/head.js:7
(Diff revision 1)
> +function promiseWaitForEvent(object, eventName, capturing = false, chrome = false) {

whoohoo, this is using so many new JS features... I guess this all works.

::: dom/html/test/head.js:11
(Diff revision 1)
> +      object.removeEventListener(eventName, listener, capturing, chrome);

removeEventListener doesn't ever take 4 params

::: dom/html/test/head.js:16
(Diff revision 1)
> +    object.addEventListener(eventName, listener, capturing, chrome);

'chrome' is a bit wrong terminology here.
wantsUntrusted is what the 4th param is about.

::: dom/html/test/head.js:26
(Diff revision 1)
> +function waitForDocLoadComplete(aBrowser=gBrowser) {

Don't know yet why this is needed, but I assume this is just refactored out from other code.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8704967 [details]
MozReview Request: Bug 1191597 part 2 - Convert fullscreen-esc-context-menu to a browser chrome test.

(This is a type of patch where mozreview totally fails. It can't handle file removes in any sane way)

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8704967 [details]
MozReview Request: Bug 1191597 part 2 - Convert fullscreen-esc-context-menu to a browser chrome test.

+  function waitUntilActive() {
+    let doc = content.document;
+    if (doc.docShell.isActive && doc.hasFocus()) {
+      sendAsyncMessage("Test:Activated");
+    } else {
+      setTimeout(waitUntilActive, 10);
+    }
+  }
+  waitUntilActive();
So this relies on Timer.js being imported to the tabchildglobal context.
I guess http://mxr.mozilla.org/mozilla-central/source/toolkit/content/browser-child.js#13 does that.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

https://reviewboard.mozilla.org/r/29829/#review26747

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8704968 [details]
MozReview Request: Bug 1191597 part 3 - Convert fullscreen-api-keys to a browser chrome test.

ok, this is also impossible to review in mozreview

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1114997 if you're interested.
Really a blocker for anyone doing reviews.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8704968 [details]
MozReview Request: Bug 1191597 part 3 - Convert fullscreen-api-keys to a browser chrome test.

+  addMessageListener("Test:InitiateKeyEvents", code => {
I wouldn't call the message that, since the main this the listener does is dispatching those events.
Perhaps Test:DispatchUntrustedKeyEvents


+    var keyCode = content.KeyEvent["DOM_" + code];
How does this work? message listener gets an object as only param. Why code is some string here and not object?
Don't you want to rename code to message and use message.data or something like that.


(this over yield-y code makes my eyes hurt. I guess I need to somehow overcome the idea that yield is just goto+state.)

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8705132 [details]
MozReview Request: Bug 1191597 part 4 - Use inner width and height for check window state.

https://reviewboard.mozilla.org/r/29849/#review26769

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8705133 [details]
MozReview Request: Bug 1191597 part 5 - Ensure to cleanup old window size from the list when exit fullscreen.

https://reviewboard.mozilla.org/r/29867/#review26775

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8704969 [details]
MozReview Request: Bug 1191597 part 7 - Enable fullscreen-api test on e10s.

https://reviewboard.mozilla.org/r/29833/#review26777

(well, ship it once part 3 has gotten r+)

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Olli Pettay [:smaug] from comment #16)
> I think it passed, but there was some unrelated error with leakcheck. I
> think.

Although that looks unrelated, but being a new permafail would actively block enabling the test.

That issue could probably relate to the plugin subtest I suppose.

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 8704966 [details]
> MozReview Request: Bug 1191597 part 1 - Add head.js and dummy page for
> browser chrome test.
> 
> https://reviewboard.mozilla.org/r/29827/#review26741

Errrr, I should probably have mentioned that in the commit message or code comment instead of only in the comment here, to save your time from reviewing existing code.

The two long functions are all copied from another head.js inside browser/. I guess we should move those functions to some other file and make them more widely accessible to avoid the duplication, though.

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Olli Pettay [:smaug] from comment #23)
> Comment on attachment 8704968 [details]
> MozReview Request: Bug 1191597 part 3 - Convert fullscreen-api-keys to a
> browser chrome test.
> 
> (this over yield-y code makes my eyes hurt. I guess I need to somehow
> overcome the idea that yield is just goto+state.)

Well, yield is... an async way to do function call here. In the browser chrome tests, the task loop would make all the yields behave just like a chained promise.

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e28760b8c33

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e04bfe01930

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8704966 [details]
MozReview Request: Bug 1191597 part 1 - Add head.js and dummy page for browser chrome test.

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

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8704967 [details]
MozReview Request: Bug 1191597 part 2 - Convert fullscreen-esc-context-menu to a browser chrome test.

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

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8704968 [details]
MozReview Request: Bug 1191597 part 3 - Convert fullscreen-api-keys to a browser chrome test.

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

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8705132 [details]
MozReview Request: Bug 1191597 part 4 - Use inner width and height for check window state.

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

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8705133 [details]
MozReview Request: Bug 1191597 part 5 - Ensure to cleanup old window size from the list when exit fullscreen.

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

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1191597 part 6 - Remove windowed plugins before closing to work around bug 1237853.

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

Comment 38

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8704969 [details]
MozReview Request: Bug 1191597 part 7 - Enable fullscreen-api test on e10s.

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

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8704968 [details]
MozReview Request: Bug 1191597 part 3 - Convert fullscreen-api-keys to a browser chrome test.

Btw, mozreview's interdiff fails utterly totally with v1 and v2 of this patch.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8704968 [details]
MozReview Request: Bug 1191597 part 3 - Convert fullscreen-api-keys to a browser chrome test.

ok, maybe I understand that yield-y code :)

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8705579 [details]
MozReview Request: Bug 1191597 part 6 - Remove windowed plugins before closing to work around bug 1237853.

https://reviewboard.mozilla.org/r/30083/#review26855

Comment 42

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f948591dde3730c83109cbf14b9a32811f722cce
Bug 1191597 part 1 - Add head.js and dummy page for browser chrome test. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/6aacf9ff118a25d72a5ad8f95cae22db6197beb8
Bug 1191597 part 2 - Convert fullscreen-esc-context-menu to a browser chrome test. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5974ec0cf83c730dc4ba3592c2c12b865ddd67
Bug 1191597 part 3 - Convert fullscreen-api-keys to a browser chrome test. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/034a6d99a1881707846b5984cb26690d63c6ede5
Bug 1191597 part 4 - Use inner width and height for check window state. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/cec665ef2d65a4a137b1a0245912d28ee4c53113
Bug 1191597 part 5 - Ensure to cleanup old window size from the list when exit fullscreen. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/473655a444c58bdee53628374a21412154a316f7
Bug 1191597 part 6 - Remove windowed plugins before closing to work around bug 1237853. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/36ba7c6b1168ef9d82bff18459ea0735f8e92054
Bug 1191597 part 7 - Enable fullscreen-api test on e10s. r=smaug

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f948591dde37
https://hg.mozilla.org/mozilla-central/rev/6aacf9ff118a
https://hg.mozilla.org/mozilla-central/rev/1f5974ec0cf8
https://hg.mozilla.org/mozilla-central/rev/034a6d99a188
https://hg.mozilla.org/mozilla-central/rev/cec665ef2d65
https://hg.mozilla.org/mozilla-central/rev/473655a444c5
https://hg.mozilla.org/mozilla-central/rev/36ba7c6b1168