Closed Bug 1191597 Opened 4 years ago Closed 4 years ago

Enable fullscreen-api tests on e10s

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
e10s + ---
firefox42 --- affected
firefox46 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Depends on 2 open bugs, Blocks 3 open bugs)

Details

Attachments

(7 files)

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.
Depends on: 1196159
Depends on: 1196163
Depends on: 1205938
No longer depends on: 1196159
No longer depends on: 1205938
Summary: Enable Fullscreen API tests on e10s → Enable fullscreen-api tests on e10s
Depends on: 1213710
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).
Blocks: 1215369
No longer blocks: fullscreen-api
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/
Attachment #8704966 - Flags: review?(bugs)
Errrr... It works on Windows, but it seems there are still some issues on Linux...
I'll continue investigating the remaining issues, but the patches already submitted are needed anyway.
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/
Attachment #8705132 - Flags: review?(bugs)
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/
Attachment #8704969 - Attachment description: MozReview Request: Bug 1191597 part 4 - Enable fullscreen-api test on e10s. → MozReview Request: Bug 1191597 part 6 - Enable fullscreen-api test on e10s.
I need to go to sleep now, so haven't seen the test result... but I guess it should be fine now.
Still pending on try. it says ~55mins.
I think it passed, but there was some unrelated error with leakcheck. I think.
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.
Attachment #8704966 - Flags: review?(bugs) → review+
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 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.
Attachment #8704967 - Flags: review?(bugs) → review+
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
https://bugzilla.mozilla.org/show_bug.cgi?id=1114997 if you're interested.
Really a blocker for anyone doing reviews.
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.)
Attachment #8704968 - Flags: review?(bugs) → review-
Attachment #8705132 - Flags: review?(bugs) → review+
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 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
Attachment #8705133 - Flags: review?(bugs) → review+
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+)
Attachment #8704969 - Flags: review?(bugs) → review+
(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.
(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.
(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.
Depends on: 1237853
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 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 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/
Attachment #8704968 - Flags: review- → review?(bugs)
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 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 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/
Attachment #8704969 - Attachment description: MozReview Request: Bug 1191597 part 6 - Enable fullscreen-api test on e10s. → MozReview Request: Bug 1191597 part 7 - Enable fullscreen-api test on e10s.
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 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 :)
Attachment #8704968 - Flags: review?(bugs) → review+
Attachment #8705579 - Flags: review?(bugs) → review+
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
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
Assignee: nobody → quanxunzhen
Depends on: 1239258
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.