Allow mochitest iframe to go fullscreen

RESOLVED WONTFIX

Status

defect
RESOLVED WONTFIX
8 years ago
2 years ago

People

(Reporter: diogogmt, Assigned: diogogmt)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

The majority of mochitests for Bug 633602 need to go fullscreen.
However, the iframe that runs the tests is not allowed to go fullscreen.
For now all the tests are being run inside a harness that opens another window.
What are the drawbacks of allowing the mochitest iframe to go fullscreen?
With this change the mochitest iframe runs all the tests that require fullscreen mode.
Not sure if there is a specific reason not to allow the iframe to go fullscreen.
Attachment #598888 - Flags: feedback?(ctalbert)
The Mochitest harness predates the fullscreen APIs by several years, so I'd imagine it's just that nobody had yet figured this was important.
Comment on attachment 598888 [details] [diff] [review]
Add mozallowfullscreen to mochitest iframe

I think this is fine. That said, I would *definitely* run this through try server because the entire mochitest family is very fragile when it comes to focus and if your test leaves anything in full-screen or doesn't reset the focus properly when you switch back, then you'll probably break all the tests that follow you.
Attachment #598888 - Flags: feedback?(ctalbert) → feedback+
Try run for 4039e09e0e15 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4039e09e0e15
Results (out of 120 total builds):
    exception: 1
    success: 109
    warnings: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ctalbert@mozilla.com-4039e09e0e15
Diogo, the try run looks good. I suppose adding the attribute really won't break anything. The interesting bit will be when you add your tests to make use of the addon. :)

Happy coding.
(In reply to Clint Talbert ( :ctalbert ) from comment #6)
> Diogo, the try run looks good. I suppose adding the attribute really won't
> break anything. The interesting bit will be when you add your tests to make
> use of the addon. :)
> 
> Happy coding.

Thanks a lot Clint :)
We're making sure to cancel fullscreen before finishing all the tests, but to prevent crashes in the future we could add a check on SimpleTest.finish to cancel fullscreen if for some reason the test fails to cancel it.

I remember seeing that if SimpleTest.finish isn't called and the test hangs for more than 5 mins, SimpleTest.finish will run by default(not sure) so the execution can keep going.

So if one of the tests sets the iframe to fullscreen and doesn't cancel it or fails to finish, when SimpleTest.finish gets called it would reset the fullscreen mode and keep running the other tests.
Assignee: nobody → diogo.gmt
Blocks: 633602
(In reply to Diogo Golovanevsky Monteiro from comment #7)
> (In reply to Clint Talbert ( :ctalbert ) from comment #6)
> > Diogo, the try run looks good. I suppose adding the attribute really won't
> > break anything. The interesting bit will be when you add your tests to make
> > use of the addon. :)
> > 
> > Happy coding.
> 
> Thanks a lot Clint :)
> We're making sure to cancel fullscreen before finishing all the tests, but
> to prevent crashes in the future we could add a check on SimpleTest.finish
> to cancel fullscreen if for some reason the test fails to cancel it.
> 
That makes a lot of sense to me. Do you want to do that as part of this patch when you add your full screen test to it?
Posted patch v2 (obsolete) — Splinter Review
When SimpleTest.finish() is called and an element is still in fullscreen mode, fullscreen gets cancelled so the next test can have a fresh start.
Attachment #598888 - Attachment is obsolete: true
Attachment #601652 - Flags: review?(ctalbert)
Comment on attachment 601652 [details] [diff] [review]
v2

This looks great, but it needs a test. Can you add a simple sanity test for this behavior?  You can see one of the other SimpleTest sanity tests here: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/test_sanity.html?force=1

I would just write a new html file in that directory that ensures that the full screen is canceled if a test calls finish.

Thanks, Clint
Attachment #601652 - Flags: review?(ctalbert) → review-
Posted patch fix with tests (obsolete) — Splinter Review
Mochitest iframe is able to go fullscreen and SimpleTest.finish make sure fullscreen mode is cancelled after every test is finished

Sanity test checks if SimpleTest.finish is cancelling fullscreen.
The test only works when running the sanity harness, as a stand alone test it won't work since the assertion is being made after SimpleTest.finish is called.

Thanks for the help Clint :)
Attachment #601652 - Attachment is obsolete: true
Attachment #604600 - Flags: review?(ctalbert)
Clint, we're getting pretty close on bug 633602, which needs this.  Any chance of another review soon?
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
No longer blocks: 633602
Comment on attachment 604600 [details] [diff] [review]
fix with tests

Hey Diogo, 
Sorry it took so long to get to this review. Been consumed with end-of-quarter/beginning-of-new-quarter stuff. Thanks for the patch and the test.
Attachment #604600 - Flags: review?(ctalbert) → review+
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Did this never land?
It did not. Diogo - please don't resolve bugs unless their patch has actually landed. Otherwise, you're begging for it to slip through the cracks like this one did.

https://hg.mozilla.org/integration/mozilla-inbound/rev/09da30d622c0
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: FIXED → ---
Target Milestone: --- → mozilla15
Unfortunately, I had to disable the new test due to it making the tree orange for not actually checking anything. Please fix up the test and post a follow-up patch.
https://hg.mozilla.org/integration/mozilla-inbound/rev/85eb1308f976

35 INFO TEST-START | /tests/Harness_sanity/test_cancelFullScreen.html
36 ERROR TEST-UNEXPECTED-FAIL | /tests/Harness_sanity/test_cancelFullScreen.html | [SimpleTest.finish()] No checks actually run. (You need to call ok(), is(), or similar functions at least once.  Make sure you use SimpleTest.waitForExplicitFinish() if you need it.)
37 INFO TEST-PASS | /tests/Harness_sanity/test_cancelFullScreen.html | SimpleTest.finish should cancel fullscreen mode - false should equal false
38 INFO TEST-END | /tests/Harness_sanity/test_cancelFullScreen.html | finished in 114ms
Ryan, sorry I've backed this out for now, since I'm uncomfortable with just disabling the test and leaving the rest in, especially without review and/or commenting the test out (rather than removing it's entry entirely) so it doesn't get forgotten.

Diogo, once the test is fixed up, we'll get this landed for you as soon as possible. Just ask if you have any questions about how to do so, either here or on IRC in #ateam :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f6c4236bbb
Target Milestone: mozilla15 → ---
(In reply to Ed Morley [:edmorley] from comment #17)
> Ryan, sorry I've backed this out for now, since I'm uncomfortable with just
> disabling the test and leaving the rest in, especially without review and/or
> commenting the test out (rather than removing it's entry entirely) so it
> doesn't get forgotten.
> 
> Diogo, once the test is fixed up, we'll get this landed for you as soon as
> possible. Just ask if you have any questions about how to do so, either here
> or on IRC in #ateam :-)
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f6c4236bbb

Sorry for the trouble of setting the flags on the bug wrongly.

The mochitest should check if the fullscreen is being canceled after SimpleTest.finish is called, that's why the only check in the test happens after SimpleTest.finish is called:

37 INFO TEST-PASS | /tests/Harness_sanity/test_cancelFullScreen.html | SimpleTest.finish should cancel fullscreen mode - false should equal false

I'll add a dummy check before calling SimpleTest.finish to fix the first fail.

Again, sorry for the trouble.
(In reply to Diogo Golovanevsky Monteiro [:diogogmt] from comment #18)
> Again, sorry for the trouble.

It's no trouble at all - thank you for the patch! :-)
Posted patch v4Splinter Review
I've added a dummy check before calling SimpleTest.finish in the cancelFullScreen sanity test.

That solved the problem of the test failing due to no check being performed.
Attachment #604600 - Attachment is obsolete: true
Attachment #625825 - Flags: review?(ctalbert)
Comment on attachment 625825 [details] [diff] [review]
v4

looks good.
Attachment #625825 - Flags: review?(ctalbert) → review+
Ok, this time went ahead and landed on inbound so we don't lose the patch again.

http://hg.mozilla.org/integration/mozilla-inbound/rev/cfac7b6a1f9c
Target Milestone: --- → mozilla15
Really sorry, backed out of inbound for pretty frequent Linux failures in test_fullscreen-api.html:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=885dd2aa83ad&jobname=mochitests-1 (press the down arrow once to see the pushes between the landing and the backout)

eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=11982530&tree=Mozilla-Inbound

{
86846 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_fullscreen-api.html | Shouldn't be able to set window fullscreen from content - got true, expected false
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/885dd2aa83ad
Target Milestone: mozilla15 → ---
From the logs it looked like the test switched into full screen mode and then never switched back out causing other tests to fail.
(In reply to Clint Talbert ( :ctalbert ) from comment #25)
> From the logs it looked like the test switched into full screen mode and
> then never switched back out causing other tests to fail.

I'll see if I can recreate the failures on my machine.
Maybe SimpleTest.finish is not properly clearing the fullscreen state.


But are this failures related to fullscreen?
It looks like this tests are failing due to some discrepancy in the width/height of the elements.
86846 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_fullscreen-api.html | Shouldn't be able to set window fullscreen from content - got true, expected false
87734 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | Checking doc width - got 1600, expected 400
87735 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | Checking doc height - got 1168, expected 300
87736 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | image width - got 800, expected 400
87737 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | image height - got 600, expected 300
87746 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | image width - got 800, expected 400
87747 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | image height - got 600, expected 300
87753 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | Checking scrollLeft - got 0, expected 400
87754 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | Checking scrollTop - got 0, expected 300
87756 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | image width - got 800, expected 400
87757 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | image height - got 600, expected 300
Mass closing mochitest bugs that haven't had activity in the past 5 years. Please re-open or file a new bug with modern context if this is still relevant.
Status: REOPENED → RESOLVED
Closed: 7 years ago2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.