1168028 - Revert fullscreen state after window finish changing if it causes that

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+11]

This is the second half of delaying fullscreen state change when the window needs to resize. The first half is bug 1161802 which is for delaying the state change for entering.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 2

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

Comment on attachment 8612683 [details] [diff] [review]
patch

>+  if (exitingFullscreen) {
>+    // If we are fully exiting fullscreen, don't touch anything here,
>+    // just wait for the window to get out from fullscreen first.
>+    if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+      (new AsyncEventDispatcher(
>+        this, NS_LITERAL_STRING("MozDOMFullscreen:Exit"),
>+        /* Bubbles */ true, /* ChromeOnly */ true))->PostDOMEvent();
>+    } else {
>+      SetWindowFullScreen(this, false);
>+    }
(I still wish we'd find some way to handle this without explicit process type checks)

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

A potential solution would be to copy the code in nsGlobalWindow::SetFullScreenInternal() which checks whether the top level window is a chrome window.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d73a15153b3b

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for browser_domFullscreen_fullscreenMode.js failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=10624164&repo=mozilla-inbound


https://hg.mozilla.org/integration/mozilla-inbound/rev/bc2394237fe5

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 - ensure exit fullscreen state

I thought this change should be trivial and shouldn't break anything, but it proves I was wrong. I should have pushed a try build before pushing to m-i.

Anyway, this is the prerequisite patch which ensures even if we are exiting to fullscreen mode, we still exit DOM fullscreen properly.

Sorry for new review request, smaug.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - exit fullscreen state after window changed, r=dao

The only change to this patch is that three ok() in file_fullscreen-api.html are removed, since they are no longer true.

This change is for fixing M2 failure in this try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2f5e2633e47

Comment 8

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

Comment on attachment 8620809 [details] [diff] [review]
patch 2 - exit fullscreen state after window changed, r=dao

Hmm, so mozFullScreen reflects the actual value. Fine, but this may break pages, though apparently mozFullScreen is a gecko-ism which we should get rid of at some point.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Olli Pettay [:smaug] (for generic DOM reviews, you may want to look for other reviewers too ;)) from comment #8)
> Comment on attachment 8620809 [details] [diff] [review]
> patch 2 - exit fullscreen state after window changed, r=dao
> 
> Hmm, so mozFullScreen reflects the actual value. Fine, but this may break
> pages, though apparently mozFullScreen is a gecko-ism which we should get
> rid of at some point.

It may, but I think that meets what the spec says, and pages should generally listen to fullscreenchange event instead of check the property synchronously.

Anyway, I don't have any data, but if bug 1161802 doesn't break pages, I believe this bug shouldn't either.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac89f253db18
https://hg.mozilla.org/integration/mozilla-inbound/rev/861ad06a6d3a

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

May I close this bug with fixed?

I really have no idea about that intermittent, but that shouldn't affect the fact that this bug has been fixed, should it?

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Yes. Not entirely sure why this didn't automatically get marked fixed when those commits got merged to m-c...