Implement the unprefixed the Fullscreen API

Implement the unprefixed the Fullscreen API

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/GLl6aWs9-EM/uTqxEmM4iGcJ

The API is only enabled for testing, to allow the recent spec changes
to be implemented with tests for the new API: http://crbug.com/402376

This does not add support for the :fullscreen pseudo-class, which is
tracked by a dedicate issue: http://crbug.com/402378

BUG=383813
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180052

[falken, 2014-07-17 03:45:02]

https://codereview.chromium.org/395633011/diff/1/Source/core/dom/FullscreenEl...
File Source/core/dom/FullscreenElementStack.cpp (right):

https://codereview.chromium.org/395633011/diff/1/Source/core/dom/FullscreenEl...
Source/core/dom/FullscreenElementStack.cpp:61: // The prefixed events bubble,
the unprefixed ones do not.
I'm probably missing something... why the behavior difference? I was assuming
unprefixing was essentially a name change.

https://codereview.chromium.org/395633011/diff/1/Source/core/dom/FullscreenEl...
Source/core/dom/FullscreenElementStack.cpp:495: event =
createEvent(EventTypeNames::fullscreenchange, document);
Why is the target different between unprefixed and prefixed?

[philipj_slow, 2014-07-17 09:33:02]

When I started working on this, I thought that it would be rather trivial, but
the events made things complicated.

In short, the first fullscreen API was the video-specific
webkitEnterFullscreen(), and it fired events on the element. The Mozilla-style
API was implemented in WebKit before Gecko. Gecko initially targeted the element
as well, but quickly changed to the document. When the W3C-style API was added
the spec did say to fire on the document, but this was ignored or overlooked.

Today, the spec, Gecko and IE11 all target the document, so going with that
seems like the fastest path to interop. Libraries that I've looked at seem to
listen on the document, which works because the prefixed event bubbles:
https://github.com/sindresorhus/screenfull.js/blob/gh-pages/src/screenfull.js

That being said, this stuff is not without risk. I've found two problems related
to this in the wild:
https://github.com/jwplayer/jwplayer/pull/103 (ought to currently break Gecko
and IE)
https://github.com/johndyer/mediaelement/pull/1240 (ought to currently break IE)


It's possible things like that will break Blink once the prefixed API is
enabled, in which case we'll have to disable it and try to figure out a path
forward.

In other news, this is all a big mess!

[falken, 2014-07-18 02:15:43]

On 2014/07/17 09:33:02, philipj wrote:
> When I started working on this, I thought that it would be rather trivial, but
> the events made things complicated.
> 
> In short, the first fullscreen API was the video-specific
> webkitEnterFullscreen(), and it fired events on the element. The Mozilla-style
> API was implemented in WebKit before Gecko. Gecko initially targeted the
element
> as well, but quickly changed to the document. When the W3C-style API was added
> the spec did say to fire on the document, but this was ignored or overlooked.
> 
> Today, the spec, Gecko and IE11 all target the document, so going with that
> seems like the fastest path to interop. Libraries that I've looked at seem to
> listen on the document, which works because the prefixed event bubbles:
> https://github.com/sindresorhus/screenfull.js/blob/gh-pages/src/screenfull.js
> 
> That being said, this stuff is not without risk. I've found two problems
related
> to this in the wild:
> https://github.com/jwplayer/jwplayer/pull/103 (ought to currently break Gecko
> and IE)
> https://github.com/johndyer/mediaelement/pull/1240 (ought to currently break
IE)
> 
> 
> It's possible things like that will break Blink once the prefixed API is
> enabled, in which case we'll have to disable it and try to figure out a path
> forward.
> 
> In other news, this is all a big mess!

Thanks, that makes sense to me. I'm not sure that this kind of behavior change
was clear in the intent to ship. Would you be willing to update the thread about
this new direction, just to be safe?

[philipj_slow, 2014-07-18 07:32:21]

On 2014/07/18 02:15:43, falken wrote:
> On 2014/07/17 09:33:02, philipj wrote:
> > When I started working on this, I thought that it would be rather trivial,
but
> > the events made things complicated.
> > 
> > In short, the first fullscreen API was the video-specific
> > webkitEnterFullscreen(), and it fired events on the element. The
Mozilla-style
> > API was implemented in WebKit before Gecko. Gecko initially targeted the
> element
> > as well, but quickly changed to the document. When the W3C-style API was
added
> > the spec did say to fire on the document, but this was ignored or
overlooked.
> > 
> > Today, the spec, Gecko and IE11 all target the document, so going with that
> > seems like the fastest path to interop. Libraries that I've looked at seem
to
> > listen on the document, which works because the prefixed event bubbles:
> >
https://github.com/sindresorhus/screenfull.js/blob/gh-pages/src/screenfull.js
> > 
> > That being said, this stuff is not without risk. I've found two problems
> related
> > to this in the wild:
> > https://github.com/jwplayer/jwplayer/pull/103 (ought to currently break
Gecko
> > and IE)
> > https://github.com/johndyer/mediaelement/pull/1240 (ought to currently break
> IE)
> > 
> > 
> > It's possible things like that will break Blink once the prefixed API is
> > enabled, in which case we'll have to disable it and try to figure out a path
> > forward.
> > 
> > In other news, this is all a big mess!
> 
> Thanks, that makes sense to me. I'm not sure that this kind of behavior change
> was clear in the intent to ship. Would you be willing to update the thread
about
> this new direction, just to be safe?

I've been meaning to do just that, no problem!

[philipj_slow, 2014-08-08 13:11:33]

rebase

[falken, 2014-08-11 02:50:51]

On 2014/08/08 13:11:33, philipj wrote:
> rebase

Do you want me to review this now?

Thanks for the update to blink-dev though it'd be better if an API owner
responded. It might be good to send a really short note like "Just to be clear,
the unprefixed API will have slightly different event behavior than the prefixed
one. This wasn't covered in the original intent to ship."

I can also send such a note if you like.

[philipj_slow, 2014-08-11 11:38:30]

The spec bugs I filed resulted in a bunch of changes I'd like to implement
before shipping:
https://code.google.com/p/chromium/issues/detail?id=402376

There's also a bit of work to figure out how close :fullscreen is to
:-webkit-full-screen is and whether or not they can be simple aliases:
https://code.google.com/p/chromium/issues/detail?id=402378

Since I'm going to miss the M38 branch point, my current thinking is to add some
tests for mixed API usage and land this with status=test. Then do all of the
spec changes, able to write tests against the unprefixed API and keeping the
existing tests as they are. Finally, if things are looking good, shipping is
just flipping status=test to status=stable. I'm thinking that I can send an
update to the intent thread (or a new intent to ship) at that time.

Does that sound like a plan?

(I'm still undecided about whether or not to do the top-layer stuff.)

[philipj_slow, 2014-08-11 15:23:08]

tests

[philipj_slow, 2014-08-11 15:28:54]

OK, I'm ready for review now. I've updated the description to clarify the scope
of this CL. Since there's a non-trivial regression risk in implementing the spec
changes, taking small steps seems wise here.

The tests in api/ are written to be suitable for web-platform-tests, where it's
customary to create directories for sections of the spec. In other words, tests
for the :fullscreen pseudo-selector and any reftests will go in a rendering/
directory. I hope this is agreeable. One things are more stable, maybe it would
be a good idea to convert some existing tests to the unprefixed API and put what
remains in a prefixed/ directory.

[philipj_slow, 2014-08-11 15:39:02]

rebase

[falken, 2014-08-12 03:26:08]

lgtm with some comments/nits

On 2014/08/11 11:38:30, philipj wrote:
> Since I'm going to miss the M38 branch point, my current thinking is to add
some
> tests for mixed API usage and land this with status=test. Then do all of the
> spec changes, able to write tests against the unprefixed API and keeping the
> existing tests as they are. Finally, if things are looking good, shipping is
> just flipping status=test to status=stable. I'm thinking that I can send an
> update to the intent thread (or a new intent to ship) at that time.
> 
> Does that sound like a plan?

Yep, SGTM!

On 2014/08/11 15:28:54, philipj wrote:
> The tests in api/ are written to be suitable for web-platform-tests, where
it's
> customary to create directories for sections of the spec. In other words,
tests
> for the :fullscreen pseudo-selector and any reftests will go in a rendering/
> directory. I hope this is agreeable.

Sounds great.

https://codereview.chromium.org/395633011/diff/60001/LayoutTests/fullscreen/a...
File LayoutTests/fullscreen/api/document-exit-fullscreen.html (right):

https://codereview.chromium.org/395633011/diff/60001/LayoutTests/fullscreen/a...
LayoutTests/fullscreen/api/document-exit-fullscreen.html:6: <div id="log"></div>
The test harness creates this automatically, is it necessary to add it
ourselves?

https://codereview.chromium.org/395633011/diff/60001/LayoutTests/fullscreen/t...
File LayoutTests/fullscreen/trusted-event.js (right):

https://codereview.chromium.org/395633011/diff/60001/LayoutTests/fullscreen/t...
LayoutTests/fullscreen/trusted-event.js:1: // Invoke callback from a trusted
event.
nit: Typically function documentation is written in non-imperative form, that
is, "Invokes" not "Invoke".

https://codereview.chromium.org/395633011/diff/60001/LayoutTests/fullscreen/t...
LayoutTests/fullscreen/trusted-event.js:32: // Invoke
element.requestFullscreen() from a trusted event.
"Invokes"

https://codereview.chromium.org/395633011/diff/60001/Source/core/dom/Fullscre...
File Source/core/dom/FullscreenElementStack.cpp (right):

https://codereview.chromium.org/395633011/diff/60001/Source/core/dom/Fullscre...
Source/core/dom/FullscreenElementStack.cpp:78: initializer.bubbles = type ==
EventTypeNames::webkitfullscreenchange || type ==
EventTypeNames::webkitfullscreenerror;
This boolean expression appears again later. wdyt of making a function like
isPrefixedEvent()? Then the comment can also be removed as it's
self-explanatory.

[philipj_slow, 2014-08-12 08:09:53]

https://codereview.chromium.org/395633011/diff/60001/LayoutTests/fullscreen/a...
File LayoutTests/fullscreen/api/document-exit-fullscreen.html (right):

https://codereview.chromium.org/395633011/diff/60001/LayoutTests/fullscreen/a...
LayoutTests/fullscreen/api/document-exit-fullscreen.html:6: <div id="log"></div>
On 2014/08/12 03:26:08, falken wrote:
> The test harness creates this automatically, is it necessary to add it
> ourselves?

It's not really necessary, no. I'm in a habit of including it because if the
test uses iframes or videos, appending the results to the end of document.body
will push down the results by 150px or so.

https://codereview.chromium.org/395633011/diff/60001/LayoutTests/fullscreen/t...
File LayoutTests/fullscreen/trusted-event.js (right):

https://codereview.chromium.org/395633011/diff/60001/LayoutTests/fullscreen/t...
LayoutTests/fullscreen/trusted-event.js:1: // Invoke callback from a trusted
event.
On 2014/08/12 03:26:08, falken wrote:
> nit: Typically function documentation is written in non-imperative form, that
> is, "Invokes" not "Invoke".

Done. I much appreciate the attention to detail, and
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
explains why you're right.

https://codereview.chromium.org/395633011/diff/60001/Source/core/dom/Fullscre...
File Source/core/dom/FullscreenElementStack.cpp (right):

https://codereview.chromium.org/395633011/diff/60001/Source/core/dom/Fullscre...
Source/core/dom/FullscreenElementStack.cpp:78: initializer.bubbles = type ==
EventTypeNames::webkitfullscreenchange || type ==
EventTypeNames::webkitfullscreenerror;
On 2014/08/12 03:26:08, falken wrote:
> This boolean expression appears again later. wdyt of making a function like
> isPrefixedEvent()? Then the comment can also be removed as it's
> self-explanatory.

Good idea, done!

[philipj_slow, 2014-08-12 08:10:35]

nits

[philipj_slow, 2014-08-12 08:14:05]

tkent, can you please review Source/platform?

[tkent, 2014-08-12 08:32:11]

lgtm

[philipj_slow, 2014-08-12 08:40:09]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-08-12 08:41:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/philipj@opera.com/395633011/80001

[commit-bot: I haz the power, 2014-08-12 09:32:12]

Change committed as 180052