Use internal hooks for notifying fullscreen changes to media controls

Use internal hooks for notifying fullscreen changes to media controls

The timing of the fullscreenchange and webkitfullscreenchange events
changed in issue 402376, which caused a regression in issue 679281.

In order to reland without this regression, use an internal hook that
will not change its timing. When the all the fullscreen changes are
done, a new mechanism that depends only web-exposed events may be
possible, although likely that would require spec changes to either
the Fullscreen API or the Screen Orientation API.

Note that MediaCustomControlsFullscreenDetectorTest.* would continue to
pass without any changes to the test. This is because
MediaCustomControlsFullscreenDetector also adds event listeners in the
same way. Nevertheless, the test is updated.

BUG=402376, 679281

[foolip, 2017-04-04 06:46:45]

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

[commit-bot: I haz the power, 2017-04-04 06:46:59]

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

[foolip, 2017-04-04 07:16:35]

Description was changed from

==========
Use internal hooks for notifying fullscreen changes to media controls

The timing of the fullscreenchange and webkitfullscreenchange events
changed in issue 402376, which caused a regression in issue 679281.

In order to reland without this regression, use an internal hook that
will not change its timing. When the all the fullscreen changes are
done, a new mechanism that depends only web-exposed events may be
possible, although likely that would require spec changes to either
the Fullscreen API or the Screen Orientation API.

BUG=402376, 679281
==========

to

==========
Use internal hooks for notifying fullscreen changes to media controls

The timing of the fullscreenchange and webkitfullscreenchange events
changed in issue 402376, which caused a regression in issue 679281.

In order to reland without this regression, use an internal hook that
will not change its timing. When the all the fullscreen changes are
done, a new mechanism that depends only web-exposed events may be
possible, although likely that would require spec changes to either
the Fullscreen API or the Screen Orientation API.

Note that MediaCustomControlsFullscreenDetectorTest.* would continue to
pass without any changes to the test. This is because
MediaCustomControlsFullscreenDetector also adds event listeners in the
same way. Nevertheless, the test is updated.

BUG=402376, 679281
==========

[foolip, 2017-04-04 09:04:28]

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

[commit-bot: I haz the power, 2017-04-04 09:04:44]

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

[foolip, 2017-04-04 09:05:28]

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

[foolip, 2017-04-04 09:05:29]

PTAL. I threw this together today thinking it might conflict with
https://codereview.chromium.org/2795783004/ and/or
https://codereview.chromium.org/2780403004/ which I will now review.

[mlamouri (slow - plz ping), 2017-04-04 09:23:16]

I'm concerned by this change in general as we actually moved away from this
internals hooks for MediaControls recently. The goal being that MediaControls
should be implemented using web primitives and if we need these hooks, that
means that a website can't implement similar controls. Is this correct? Or is
this done merely to pass internal tests that rely on specific timing?

[foolip, 2017-04-04 09:40:13]

On 2017/04/04 09:23:16, mlamouri wrote:
> I'm concerned by this change in general as we actually moved away from this
> internals hooks for MediaControls recently. The goal being that MediaControls
> should be implemented using web primitives and if we need these hooks, that
> means that a website can't implement similar controls. Is this correct? Or is
> this done merely to pass internal tests that rely on specific timing?

That's correct, per
https://bugs.chromium.org/p/chromium/issues/detail?id=679281#c8 I think it
probably isn't possible to implement a feature like this using the per-spec
fullscreenchange event timing, because it is fired during the animation frame
following the fullscreen element change. The order of events will thus be:

1. enter fullscreen
2. lock orientation
3. exit fullscreen
4. unlock orientation

This means that in FullscreenController::didExitFullscreen() the orientation
lock is still held, and it's not possible to restore to the state that was
before entering fullscreen.

I think the reason it currently works is that the fullscreenchange event is
enqueued in Fullscreen::exitFullscreen() and will be fired before the
FullscreenController::didExitFullscreen() call. And that is a part that needs to
change in order to make progress towards unprefixing fullscreen.

My preferred plan is to go back to internal hooks, unprefix fullscreen, then try
to figure out what's missing in order to make it possible to build this using
web-exposed behavior. Other options?

[commit-bot: I haz the power, 2017-04-04 10:59:34]

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

[commit-bot: I haz the power, 2017-04-04 10:59:35]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2017-04-04 13:00:47]

On 2017/04/04 at 09:40:13, foolip wrote:
> On 2017/04/04 09:23:16, mlamouri wrote:
> > I'm concerned by this change in general as we actually moved away from this
> > internals hooks for MediaControls recently. The goal being that
MediaControls
> > should be implemented using web primitives and if we need these hooks, that
> > means that a website can't implement similar controls. Is this correct? Or
is
> > this done merely to pass internal tests that rely on specific timing?
> 
> That's correct, per
https://bugs.chromium.org/p/chromium/issues/detail?id=679281#c8 I think it
probably isn't possible to implement a feature like this using the per-spec
fullscreenchange event timing, because it is fired during the animation frame
following the fullscreen element change. The order of events will thus be:
> 
> 1. enter fullscreen
> 2. lock orientation
> 3. exit fullscreen
> 4. unlock orientation
> 
> This means that in FullscreenController::didExitFullscreen() the orientation
lock is still held, and it's not possible to restore to the state that was
before entering fullscreen.
> 
> I think the reason it currently works is that the fullscreenchange event is
enqueued in Fullscreen::exitFullscreen() and will be fired before the
FullscreenController::didExitFullscreen() call. And that is a part that needs to
change in order to make progress towards unprefixing fullscreen.
> 
> My preferred plan is to go back to internal hooks, unprefix fullscreen, then
try to figure out what's missing in order to make it possible to build this
using web-exposed behavior. Other options?

I'm a bit surprised that unprefixing fullscreen for an API that we know will be
missing real use cases is more important than fixing the API to handle these use
cases. Wouldn't it make more sense to change the API to address these issues
before unprefixing? I'm probably missing some context here so I apologise if the
reasons are obvious. Also, I don't want the internal media controls
implementation to block the progress of your project but I see this change as a
regression (you are pretty much reverting a CL I wrote a few months ago) and it
should be a signal that we are going the wrong path.

[foolip, 2017-04-05 10:15:58]

On 2017/04/04 13:00:47, mlamouri wrote:
> On 2017/04/04 at 09:40:13, foolip wrote:
> > On 2017/04/04 09:23:16, mlamouri wrote:
> > > I'm concerned by this change in general as we actually moved away from
this
> > > internals hooks for MediaControls recently. The goal being that
> MediaControls
> > > should be implemented using web primitives and if we need these hooks,
that
> > > means that a website can't implement similar controls. Is this correct? Or
> is
> > > this done merely to pass internal tests that rely on specific timing?
> > 
> > That's correct, per
> https://bugs.chromium.org/p/chromium/issues/detail?id=679281#c8 I think it
> probably isn't possible to implement a feature like this using the per-spec
> fullscreenchange event timing, because it is fired during the animation frame
> following the fullscreen element change. The order of events will thus be:
> > 
> > 1. enter fullscreen
> > 2. lock orientation
> > 3. exit fullscreen
> > 4. unlock orientation
> > 
> > This means that in FullscreenController::didExitFullscreen() the orientation
> lock is still held, and it's not possible to restore to the state that was
> before entering fullscreen.
> > 
> > I think the reason it currently works is that the fullscreenchange event is
> enqueued in Fullscreen::exitFullscreen() and will be fired before the
> FullscreenController::didExitFullscreen() call. And that is a part that needs
to
> change in order to make progress towards unprefixing fullscreen.
> > 
> > My preferred plan is to go back to internal hooks, unprefix fullscreen, then
> try to figure out what's missing in order to make it possible to build this
> using web-exposed behavior. Other options?
> 
> I'm a bit surprised that unprefixing fullscreen for an API that we know will
be
> missing real use cases is more important than fixing the API to handle these
use
> cases. Wouldn't it make more sense to change the API to address these issues
> before unprefixing? I'm probably missing some context here so I apologise if
the
> reasons are obvious. Also, I don't want the internal media controls
> implementation to block the progress of your project but I see this change as
a
> regression (you are pretty much reverting a CL I wrote a few months ago) and
it
> should be a signal that we are going the wrong path.

I agree this is a change for the worse for media controls internals and that it
would be great if it were possible to build something like this using
web-exposed and standardized APIs. I believe it currently works for the wrong
reasons and that the current timing of webkitfullscreenchange on exit would be a
hard sell to stanardize on, but before I push ahead I will try to reland
https://codereview.chromium.org/2654083006 with some of the other changes and
try to understand the regression in more detail. Hopefully I am mistaken.

In the meantime, what behavior do you think would be ideal? I assume that a site
would call screen.lock() in the fullscreenchange event when entering fullscreen,
and screen.unlock() in the fullscreenchange event when exiting fullscreen.
Unless we block paint somehow, will be at least one bad frame in both
transitions, but what should the stable layout after that be? If the answer is
that after screen.unlock() the layout is the same as before entering fullscreen,
then we will need to teach the screen orientation code about restoring
scale+offset saved when entering fullscreen. But, what should happen if
orientation is unlocked long after exiting fullscreen? Maybe the answer is to
privilege only screen.unlock() calls from within a fullscreenchange event with
the scale+offset-restoring behavior?

[mlamouri (slow - plz ping), 2017-04-05 16:15:51]

On 2017/04/05 at 10:15:58, foolip wrote:
> On 2017/04/04 13:00:47, mlamouri wrote:
> > On 2017/04/04 at 09:40:13, foolip wrote:
> > > On 2017/04/04 09:23:16, mlamouri wrote:
> > > > I'm concerned by this change in general as we actually moved away from
this
> > > > internals hooks for MediaControls recently. The goal being that
> > MediaControls
> > > > should be implemented using web primitives and if we need these hooks,
that
> > > > means that a website can't implement similar controls. Is this correct?
Or
> > is
> > > > this done merely to pass internal tests that rely on specific timing?
> > > 
> > > That's correct, per
> > https://bugs.chromium.org/p/chromium/issues/detail?id=679281#c8 I think it
> > probably isn't possible to implement a feature like this using the per-spec
> > fullscreenchange event timing, because it is fired during the animation
frame
> > following the fullscreen element change. The order of events will thus be:
> > > 
> > > 1. enter fullscreen
> > > 2. lock orientation
> > > 3. exit fullscreen
> > > 4. unlock orientation
> > > 
> > > This means that in FullscreenController::didExitFullscreen() the
orientation
> > lock is still held, and it's not possible to restore to the state that was
> > before entering fullscreen.
> > > 
> > > I think the reason it currently works is that the fullscreenchange event
is
> > enqueued in Fullscreen::exitFullscreen() and will be fired before the
> > FullscreenController::didExitFullscreen() call. And that is a part that
needs to
> > change in order to make progress towards unprefixing fullscreen.
> > > 
> > > My preferred plan is to go back to internal hooks, unprefix fullscreen,
then
> > try to figure out what's missing in order to make it possible to build this
> > using web-exposed behavior. Other options?
> > 
> > I'm a bit surprised that unprefixing fullscreen for an API that we know will
be
> > missing real use cases is more important than fixing the API to handle these
use
> > cases. Wouldn't it make more sense to change the API to address these issues
> > before unprefixing? I'm probably missing some context here so I apologise if
the
> > reasons are obvious. Also, I don't want the internal media controls
> > implementation to block the progress of your project but I see this change
as a
> > regression (you are pretty much reverting a CL I wrote a few months ago) and
it
> > should be a signal that we are going the wrong path.
> 
> I agree this is a change for the worse for media controls internals and that
it would be great if it were possible to build something like this using
web-exposed and standardized APIs. I believe it currently works for the wrong
reasons and that the current timing of webkitfullscreenchange on exit would be a
hard sell to stanardize on, but before I push ahead I will try to reland
https://codereview.chromium.org/2654083006 with some of the other changes and
try to understand the regression in more detail. Hopefully I am mistaken.
> 
> In the meantime, what behavior do you think would be ideal? I assume that a
site would call screen.lock() in the fullscreenchange event when entering
fullscreen, and screen.unlock() in the fullscreenchange event when exiting
fullscreen. Unless we block paint somehow, will be at least one bad frame in
both transitions, but what should the stable layout after that be? If the answer
is that after screen.unlock() the layout is the same as before entering
fullscreen, then we will need to teach the screen orientation code about
restoring scale+offset saved when entering fullscreen. But, what should happen
if orientation is unlocked long after exiting fullscreen? Maybe the answer is to
privilege only screen.unlock() calls from within a fullscreenchange event with
the scale+offset-restoring behavior?

Maybe we should a VC to discuss this because I might be missing a ton of
information. The behaviour that we should have is the one that we have today
which, it seems, wouldn't require any special code, right? If I understand, you
expect to break the ability to unlock during a when exiting fullscreen. Wouldn't
this break websites? We can change our own code but if a website is implementing
exactly what you describe above, it would be broken, right?

[foolip, 2017-04-07 14:52:36]

On 2017/04/05 16:15:51, mlamouri wrote:
> On 2017/04/05 at 10:15:58, foolip wrote:
> > In the meantime, what behavior do you think would be ideal? I assume that a
> site would call screen.lock() in the fullscreenchange event when entering
> fullscreen, and screen.unlock() in the fullscreenchange event when exiting
> fullscreen. Unless we block paint somehow, will be at least one bad frame in
> both transitions, but what should the stable layout after that be? If the
answer
> is that after screen.unlock() the layout is the same as before entering
> fullscreen, then we will need to teach the screen orientation code about
> restoring scale+offset saved when entering fullscreen. But, what should happen
> if orientation is unlocked long after exiting fullscreen? Maybe the answer is
to
> privilege only screen.unlock() calls from within a fullscreenchange event with
> the scale+offset-restoring behavior?
> 
> Maybe we should a VC to discuss this because I might be missing a ton of
> information. The behaviour that we should have is the one that we have today
> which, it seems, wouldn't require any special code, right? If I understand,
you
> expect to break the ability to unlock during a when exiting fullscreen.
Wouldn't
> this break websites? We can change our own code but if a website is
implementing
> exactly what you describe above, it would be broken, right?

The history of this problem is fortunately not very long, so here goes. Issue
402376 changed the timing of the webkitfullscreenchange event to always be at
animation frame timing after the actual change, which is already the timing of
the resize events. This caused the regression in issue 679281, and since there
had already been several other regressions I decided to revert, speculating on
the cause in https://bugs.chromium.org/p/chromium/issues/detail?id=679281#c8

What I did not spell out there explicitly is what logic I think is going wrong
here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/Fullscreen...

In FullscreenController::didUpdateLayout() there is code that should run on the
first layout after exiting fullscreen, to restore the scale and offset of the
viewport from before entering fullscreen. That is needed because when in
fullscreen, we force the scale to 1. No spec actually covers this behavior
though.

So, the trouble is with when we restore initial scale and offset. There must be
some screen orientation code that also affects the scale, because when rotating
it could happen that a scale change is needed so that the page fills the
viewport. I don't know where this code is though.

[mlamouri (slow - plz ping), 2017-04-10 12:59:31]

On 2017/04/07 at 14:52:36, foolip wrote:
> On 2017/04/05 16:15:51, mlamouri wrote:
> > On 2017/04/05 at 10:15:58, foolip wrote:
> > > In the meantime, what behavior do you think would be ideal? I assume that
a
> > site would call screen.lock() in the fullscreenchange event when entering
> > fullscreen, and screen.unlock() in the fullscreenchange event when exiting
> > fullscreen. Unless we block paint somehow, will be at least one bad frame in
> > both transitions, but what should the stable layout after that be? If the
answer
> > is that after screen.unlock() the layout is the same as before entering
> > fullscreen, then we will need to teach the screen orientation code about
> > restoring scale+offset saved when entering fullscreen. But, what should
happen
> > if orientation is unlocked long after exiting fullscreen? Maybe the answer
is to
> > privilege only screen.unlock() calls from within a fullscreenchange event
with
> > the scale+offset-restoring behavior?
> > 
> > Maybe we should a VC to discuss this because I might be missing a ton of
> > information. The behaviour that we should have is the one that we have today
> > which, it seems, wouldn't require any special code, right? If I understand,
you
> > expect to break the ability to unlock during a when exiting fullscreen.
Wouldn't
> > this break websites? We can change our own code but if a website is
implementing
> > exactly what you describe above, it would be broken, right?
> 
> The history of this problem is fortunately not very long, so here goes. Issue
402376 changed the timing of the webkitfullscreenchange event to always be at
animation frame timing after the actual change, which is already the timing of
the resize events. This caused the regression in issue 679281, and since there
had already been several other regressions I decided to revert, speculating on
the cause in https://bugs.chromium.org/p/chromium/issues/detail?id=679281#c8
> 
> What I did not spell out there explicitly is what logic I think is going wrong
here:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/Fullscreen...
> 
> In FullscreenController::didUpdateLayout() there is code that should run on
the first layout after exiting fullscreen, to restore the scale and offset of
the viewport from before entering fullscreen. That is needed because when in
fullscreen, we force the scale to 1. No spec actually covers this behavior
though.
> 
> So, the trouble is with when we restore initial scale and offset. There must
be some screen orientation code that also affects the scale, because when
rotating it could happen that a scale change is needed so that the page fills
the viewport. I don't know where this code is though.

Could the logic related to orientation be:
https://cs.chromium.org/webrtc/src/third_party/WebKit/Source/web/WebViewImpl....
(or maybe
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra...)

[foolip, 2017-07-06 21:41:51]

Description was changed from

==========
Use internal hooks for notifying fullscreen changes to media controls

The timing of the fullscreenchange and webkitfullscreenchange events
changed in issue 402376, which caused a regression in issue 679281.

In order to reland without this regression, use an internal hook that
will not change its timing. When the all the fullscreen changes are
done, a new mechanism that depends only web-exposed events may be
possible, although likely that would require spec changes to either
the Fullscreen API or the Screen Orientation API.

Note that MediaCustomControlsFullscreenDetectorTest.* would continue to
pass without any changes to the test. This is because
MediaCustomControlsFullscreenDetector also adds event listeners in the
same way. Nevertheless, the test is updated.

BUG=402376, 679281
==========

to

==========
Use internal hooks for notifying fullscreen changes to media controls

The timing of the fullscreenchange and webkitfullscreenchange events
changed in issue 402376, which caused a regression in issue 679281.

In order to reland without this regression, use an internal hook that
will not change its timing. When the all the fullscreen changes are
done, a new mechanism that depends only web-exposed events may be
possible, although likely that would require spec changes to either
the Fullscreen API or the Screen Orientation API.

Note that MediaCustomControlsFullscreenDetectorTest.* would continue to
pass without any changes to the test. This is because
MediaCustomControlsFullscreenDetector also adds event listeners in the
same way. Nevertheless, the test is updated.

BUG=402376, 679281
==========