Refactor overlay fullscreen video handling into a single callback

Refactor overlay fullscreen video handling into a single callback

The motivation is twofold.

First, the timing of modifying the fullscreen element stack (later top
layer) will change, so that it doesn't happen synchronously with the
enter/exit callbacks, but in an animation frame task following it. That
means that it has to be the code that changes the fullscreen element
that notifies FullscreenController.

Second, FullscreenController itself doesn't strictly need
m_provisionalFullscreenElement or m_fullscreenFrame, but in the current
structure they're needed for the overlay fullscreen video handling.
Changing this is a prerequisite for further refactoring:
https://codereview.chromium.org/2495423004/

TEST=https://codereview.chromium.org/2527093003/

BUG=402376
Committed: https://crrev.com/527a3270a1a0da47cc48dbd3c9e76c614435b7c6
Cr-Commit-Position: refs/heads/master@{#435223}

[foolip, 2016-11-24 14:53:02]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 14:53:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 15:57:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 15:57:26]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[foolip, 2016-11-24 20:28:41]

Description was changed from

==========
Consolidate overlay fullscreen video handling

BUG=
==========

to

==========
Refactor overlay fullscreen video handling into a single callback

The motivation is twofold.

First, the timing of modifying the fullscreen element stack (later top
layer) will change, so that it doesn't happen synchronously with the
enter/exit callbacks, but in an animation frame task following it. That
means that it has to be the code that changes the fullscreen element
that notifies FullscreenController.

Second, FullscreenController itself doesn't strictly need
m_provisionalFullscreenElement or m_fullscreenFrame, but in the current
structure they're needed for the overlay fullscreen video handling.
Changing this is a prerequisite for further refactoring:
https://codereview.chromium.org/2495423004/

TEST=https://codereview.chromium.org/2527093003/

BUG=402376
==========

[foolip, 2016-11-24 21:03:38]

foolip@chromium.org changed reviewers:
+ eae@chromium.org, esprehn@chromium.org, watk@chromium.org

[foolip, 2016-11-24 21:03:39]

eae@, esprehn@, PTAL. If one (or none) of you would like to handle all
Fullscreen reviews, just say the word.

watk@, can you check if you see something wrong in HTMLMediaElement.cpp where I
commented?

Depends on https://codereview.chromium.org/2527093003 but I've run this through
a dry run together with additional changes in
https://codereview.chromium.org/2495423004/#ps100001 so hopefully no surprises.

https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1198:
m_webMediaPlayer->enteredFullscreen();
This change isn't strictly necessary in this CL, but it fits here and would
otherwise have to be changed in a coming CL. Destroying the WebMediaPlayer
doesn't exit fullscreen or, AFAICT, mess up any other state, so merely telling
the new WebMediaPlayer about the situation should suffice. Relevant previous
CLs:
https://codereview.chromium.org/1254613003
https://codereview.chromium.org/1689043002

[eae, 2016-11-24 21:47:22]

LGTM

[foolip, 2016-11-28 16:20:10]

https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FullscreenController.cpp:170:
DCHECK_NE(fromElement, toElement);
Turns out this assert can actually fail for the "request then exit+request" case
in https://github.com/whatwg/fullscreen/issues/63 so I'll make sure there's an
equivalent test and fix this before landing.

[foolip, 2016-11-29 13:35:07]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 13:35:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 15:07:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 15:08:00]

Dry run: This issue passed the CQ dry run.

[foolip, 2016-11-29 15:21:30]

https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FullscreenController.cpp:170:
DCHECK_NE(fromElement, toElement);
On 2016/11/28 16:20:10, foolip wrote:
> Turns out this assert can actually fail for the "request then exit+request"
case
> in https://github.com/whatwg/fullscreen/issues/63 so I'll make sure there's an
> equivalent test and fix this before landing.

I've fixed that now, this is now ready to land after
https://codereview.chromium.org/2527093003

[watk, 2016-11-29 21:11:01]

https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1198:
m_webMediaPlayer->enteredFullscreen();
On 2016/11/24 21:03:39, foolip wrote:
> This change isn't strictly necessary in this CL, but it fits here and would
> otherwise have to be changed in a coming CL. Destroying the WebMediaPlayer
> doesn't exit fullscreen or, AFAICT, mess up any other state, so merely telling
> the new WebMediaPlayer about the situation should suffice. Relevant previous
> CLs:
> https://codereview.chromium.org/1254613003
> https://codereview.chromium.org/1689043002

I don't remember the details but I think this is fine. This was needed
previously because we might stay in fullscreen but change which WebMediaPlayer
is fullscreened, which means usesOverlayFullscreenVideo() could change while in
fullscreen. But I assume with this change we will call
fullscreenElementChanged() in this case?

[foolip, 2016-11-29 21:52:28]

https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1198:
m_webMediaPlayer->enteredFullscreen();
On 2016/11/29 21:11:00, watk wrote:
> On 2016/11/24 21:03:39, foolip wrote:
> > This change isn't strictly necessary in this CL, but it fits here and would
> > otherwise have to be changed in a coming CL. Destroying the WebMediaPlayer
> > doesn't exit fullscreen or, AFAICT, mess up any other state, so merely
telling
> > the new WebMediaPlayer about the situation should suffice. Relevant previous
> > CLs:
> > https://codereview.chromium.org/1254613003
> > https://codereview.chromium.org/1689043002
> 
> I don't remember the details but I think this is fine. This was needed
> previously because we might stay in fullscreen but change which WebMediaPlayer
> is fullscreened, which means usesOverlayFullscreenVideo() could change while
in
> fullscreen. But I assume with this change we will call
> fullscreenElementChanged() in this case?

Just the m_inOverlayFullscreenVideo should be enough to make sure that the
transparency is restored. Maybe what happened here is that first
frame->chromeClient().enterFullscreenForElement(this) was added and found to fix
part of the problem, then m_inOverlayFullscreenVideo was added, but
enterFullscreenForElement was left because it works. I'm pretty sure that
m_webMediaPlayer->enteredFullscreen() will now suffice, fingers crossed.

[watk, 2016-11-29 22:05:55]

On 2016/11/29 21:52:28, foolip wrote:
>
https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1198:
> m_webMediaPlayer->enteredFullscreen();
> On 2016/11/29 21:11:00, watk wrote:
> > On 2016/11/24 21:03:39, foolip wrote:
> > > This change isn't strictly necessary in this CL, but it fits here and
would
> > > otherwise have to be changed in a coming CL. Destroying the WebMediaPlayer
> > > doesn't exit fullscreen or, AFAICT, mess up any other state, so merely
> telling
> > > the new WebMediaPlayer about the situation should suffice. Relevant
previous
> > > CLs:
> > > https://codereview.chromium.org/1254613003
> > > https://codereview.chromium.org/1689043002
> > 
> > I don't remember the details but I think this is fine. This was needed
> > previously because we might stay in fullscreen but change which
WebMediaPlayer
> > is fullscreened, which means usesOverlayFullscreenVideo() could change while
> in
> > fullscreen. But I assume with this change we will call
> > fullscreenElementChanged() in this case?
> 
> Just the m_inOverlayFullscreenVideo should be enough to make sure that the
> transparency is restored. Maybe what happened here is that first
> frame->chromeClient().enterFullscreenForElement(this) was added and found to
fix
> part of the problem, then m_inOverlayFullscreenVideo was added, but
> enterFullscreenForElement was left because it works. I'm pretty sure that
> m_webMediaPlayer->enteredFullscreen() will now suffice, fingers crossed.

I just remembered something: after all the changes have settled we're back to a
world where usesOverlayFullscreenVideo() == OS_ANDROID. So we can go back to
making this a REF if we want. But for this CL it means that
usesOverlayFullscreenVideo() never changes regardless of whether the WMP
changes.

[foolip, 2016-11-29 23:04:32]

On 2016/11/29 22:05:55, watk wrote:
> On 2016/11/29 21:52:28, foolip wrote:
> >
>
https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2530883002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1198:
> > m_webMediaPlayer->enteredFullscreen();
> > On 2016/11/29 21:11:00, watk wrote:
> > > On 2016/11/24 21:03:39, foolip wrote:
> > > > This change isn't strictly necessary in this CL, but it fits here and
> would
> > > > otherwise have to be changed in a coming CL. Destroying the
WebMediaPlayer
> > > > doesn't exit fullscreen or, AFAICT, mess up any other state, so merely
> > telling
> > > > the new WebMediaPlayer about the situation should suffice. Relevant
> previous
> > > > CLs:
> > > > https://codereview.chromium.org/1254613003
> > > > https://codereview.chromium.org/1689043002
> > > 
> > > I don't remember the details but I think this is fine. This was needed
> > > previously because we might stay in fullscreen but change which
> WebMediaPlayer
> > > is fullscreened, which means usesOverlayFullscreenVideo() could change
while
> > in
> > > fullscreen. But I assume with this change we will call
> > > fullscreenElementChanged() in this case?
> > 
> > Just the m_inOverlayFullscreenVideo should be enough to make sure that the
> > transparency is restored. Maybe what happened here is that first
> > frame->chromeClient().enterFullscreenForElement(this) was added and found to
> fix
> > part of the problem, then m_inOverlayFullscreenVideo was added, but
> > enterFullscreenForElement was left because it works. I'm pretty sure that
> > m_webMediaPlayer->enteredFullscreen() will now suffice, fingers crossed.
> 
> I just remembered something: after all the changes have settled we're back to
a
> world where usesOverlayFullscreenVideo() == OS_ANDROID. So we can go back to
> making this a REF if we want. But for this CL it means that
> usesOverlayFullscreenVideo() never changes regardless of whether the WMP
> changes.

Ah, good point. I'll see about simplifying that in a separate CL.

[foolip, 2016-11-30 09:27:09]

foolip@chromium.org changed reviewers:
+ mkwst@chromium.org

[foolip, 2016-11-30 09:27:10]

mkwst@ has reviewed the added test, so I'm going to land this now.

[Mike West, 2016-11-30 09:37:51]

Yup. LGTM.

[foolip, 2016-11-30 10:14:31]

The CQ bit was checked by foolip@chromium.org

[foolip, 2016-11-30 10:14:31]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2530883002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-30 10:14:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 11:51:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480500871206970,
"parent_rev": "da9a550783677b5b7a9d960a48763a0399382ca8", "commit_rev":
"175476ee5718ccbc682c825773f1f606f969744d"}

[commit-bot: I haz the power, 2016-11-30 11:51:33]

Description was changed from

==========
Refactor overlay fullscreen video handling into a single callback

The motivation is twofold.

First, the timing of modifying the fullscreen element stack (later top
layer) will change, so that it doesn't happen synchronously with the
enter/exit callbacks, but in an animation frame task following it. That
means that it has to be the code that changes the fullscreen element
that notifies FullscreenController.

Second, FullscreenController itself doesn't strictly need
m_provisionalFullscreenElement or m_fullscreenFrame, but in the current
structure they're needed for the overlay fullscreen video handling.
Changing this is a prerequisite for further refactoring:
https://codereview.chromium.org/2495423004/

TEST=https://codereview.chromium.org/2527093003/

BUG=402376
==========

to

==========
Refactor overlay fullscreen video handling into a single callback

The motivation is twofold.

First, the timing of modifying the fullscreen element stack (later top
layer) will change, so that it doesn't happen synchronously with the
enter/exit callbacks, but in an animation frame task following it. That
means that it has to be the code that changes the fullscreen element
that notifies FullscreenController.

Second, FullscreenController itself doesn't strictly need
m_provisionalFullscreenElement or m_fullscreenFrame, but in the current
structure they're needed for the overlay fullscreen video handling.
Changing this is a prerequisite for further refactoring:
https://codereview.chromium.org/2495423004/

TEST=https://codereview.chromium.org/2527093003/

BUG=402376
==========

[commit-bot: I haz the power, 2016-11-30 11:51:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-30 11:55:36]

Description was changed from

==========
Refactor overlay fullscreen video handling into a single callback

The motivation is twofold.

First, the timing of modifying the fullscreen element stack (later top
layer) will change, so that it doesn't happen synchronously with the
enter/exit callbacks, but in an animation frame task following it. That
means that it has to be the code that changes the fullscreen element
that notifies FullscreenController.

Second, FullscreenController itself doesn't strictly need
m_provisionalFullscreenElement or m_fullscreenFrame, but in the current
structure they're needed for the overlay fullscreen video handling.
Changing this is a prerequisite for further refactoring:
https://codereview.chromium.org/2495423004/

TEST=https://codereview.chromium.org/2527093003/

BUG=402376
==========

to

==========
Refactor overlay fullscreen video handling into a single callback

The motivation is twofold.

First, the timing of modifying the fullscreen element stack (later top
layer) will change, so that it doesn't happen synchronously with the
enter/exit callbacks, but in an animation frame task following it. That
means that it has to be the code that changes the fullscreen element
that notifies FullscreenController.

Second, FullscreenController itself doesn't strictly need
m_provisionalFullscreenElement or m_fullscreenFrame, but in the current
structure they're needed for the overlay fullscreen video handling.
Changing this is a prerequisite for further refactoring:
https://codereview.chromium.org/2495423004/

TEST=https://codereview.chromium.org/2527093003/

BUG=402376

Committed: https://crrev.com/527a3270a1a0da47cc48dbd3c9e76c614435b7c6
Cr-Commit-Position: refs/heads/master@{#435223}
==========

[commit-bot: I haz the power, 2016-11-30 11:55:37]

Patchset 5 (id:??) landed as
https://crrev.com/527a3270a1a0da47cc48dbd3c9e76c614435b7c6
Cr-Commit-Position: refs/heads/master@{#435223}