Closed Bug 1198563 Opened 4 years ago Closed 4 years ago

Continuously calling requestFullscreen() multiple times causes unexpected result

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox40 --- unaffected
firefox41 --- unaffected
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

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.
Assignee: nobody → quanxunzhen
Bug 1198563 part 1 - Encapsulate iterating fullscreen request list code.
Attachment #8652716 - Flags: review?(bugs)
Bug 1198563 part 2 - Do no directly apply fullscreen state when window object reports in fullscreen when there are pending request before.
Attachment #8652717 - Flags: review?(bugs)
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?
Attachment #8652717 - Flags: review?(bugs) → review+
(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 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*
Attachment #8652716 - Flags: review?(bugs) → review+
(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.
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/42175a953992b5cc83a3cdbd6c1b45b52edffe30
changeset:  42175a953992b5cc83a3cdbd6c1b45b52edffe30
user:       Xidorn Quan <quanxunzhen@gmail.com>
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 <quanxunzhen@gmail.com>
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
https://hg.mozilla.org/mozilla-central/rev/42175a953992
https://hg.mozilla.org/mozilla-central/rev/690c350cf96c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Blocks: 1189200
No longer blocks: 1189200
Depends on: 1189200
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.