1198563 - Continuously calling requestFullscreen() multiple times causes unexpected result

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+11]

If the content calls requestFullscreen() for multiple times in the same event handler, and the fullscreen transition is not disabled, the later fullscreen request would be applied first before we actually toggle the window state.

This is a regression since we have the fullscreen transition.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1198563 part 1 - Encapsulate iterating fullscreen request list code.

Bug 1198563 part 1 - Encapsulate iterating fullscreen request list code.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1198563 part 2 - Do no directly apply fullscreen state when window object reports in fullscreen when there are pending request before.

Bug 1198563 part 2 - Do no directly apply fullscreen state when window object reports in fullscreen when there are pending request before.

Comment 3

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

Comment on attachment 8652717 [details]
MozReview Request: Bug 1198563 part 2 - Do no directly apply fullscreen state when window object reports in fullscreen when there are pending request before.

>+  if ((static_cast<nsGlobalWindow*>(rootWin.get())->FullScreen() &&
>+       PendingFullscreenRequestList::Iterator(this).AtEnd()) ||
>+      nsContentUtils::GetRootDocument(this)->IsFullScreenDoc()) {
AtEnd() is a bit odd. Could you add something like IsEmpty() to the iterator?

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Olli Pettay [:smaug] (high review load) from comment #3)
> Comment on attachment 8652717 [details]
> MozReview Request: Bug 1198563 part 2 - Do no directly apply fullscreen
> state when window object reports in fullscreen when there are pending
> request before.
> 
> >+  if ((static_cast<nsGlobalWindow*>(rootWin.get())->FullScreen() &&
> >+       PendingFullscreenRequestList::Iterator(this).AtEnd()) ||
> >+      nsContentUtils::GetRootDocument(this)->IsFullScreenDoc()) {
> AtEnd() is a bit odd. Could you add something like IsEmpty() to the iterator?

I don't think IsEmpty() makes any sense for an iterator... I agree AtEnd() is a bit odd here, but it's probably better to describe it with a comment instead of adding a weird method to the iterator.

Comment 5

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

Comment on attachment 8652716 [details]
MozReview Request: Bug 1198563 part 1 - Encapsulate iterating fullscreen request list code.

>+ auto lastRequest = PendingFullscreenRequestList::GetLast();
Trying to sell me 'auto' ;)

No no, that doesn't improve readability at all, since the actual type of lastRequest is totally hidden in that context.
So, just keep FullscreenRequest*

Comment 6

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

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #4)
> I don't think IsEmpty() makes any sense for an iterator... I agree AtEnd()
> is a bit odd here, but it's probably better to describe it with a comment
> instead of adding a weird method to the iterator.
Fine, add a comment.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/42175a953992b5cc83a3cdbd6c1b45b52edffe30
changeset:  42175a953992b5cc83a3cdbd6c1b45b52edffe30
user:       Xidorn Quan <[e-mail]>
date:       Thu Aug 27 11:21:29 2015 +1000
description:
Bug 1198563 part 1 - Encapsulate iterating fullscreen request list code. r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/690c350cf96ca008fada30cab9752c1a5d72e7c3
changeset:  690c350cf96ca008fada30cab9752c1a5d72e7c3
user:       Xidorn Quan <[e-mail]>
date:       Thu Aug 27 11:21:29 2015 +1000
description:
Bug 1198563 part 2 - Do no directly apply fullscreen state when window object reports in fullscreen when there are pending request before. r=smaug

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42175a953992
https://hg.mozilla.org/mozilla-central/rev/690c350cf96c