Notify WebMediaPlayer when HTML video enters full screen.

Notify WebMediaPlayer when HTML video enters full screen.

In immersive video playback scenario, the tab mirroring efficiency and
quality can be improved by bypassing the decoding, local rendering,
and real-time encoding process. WebMediaPlayer will stop the normal
renderer in this case, and switch to a remoting renderer which will
send the demuxed stream directly to browser, and then transmit to the
Cast receiver over network.

In order to switch between the normal and remoting renderer,
WebMediaPlayer needs to know when to enter/exit immersive video
playback scenario. A good indicator for this is that the video
enters/exits full screen. Therefore, WebMediaPlayer needs to be
notified when video enters/exits full screen, no matter what the full
screen element is.

Original WIP CL: https://codereview.chromium.org/2204673004/

[xjz, 2016-08-09 22:45:43]

Description was changed from

==========
Notify WebMediaPlayer when HTML video enters full screen.

In immersive video playback scenario, the tab mirroring efficiency and
quality can be improved by bypassing the decoding, local rendering,
and real-time encoding process. WebMediaPlayer will stop the normal
renderer in this case, and switch to a remoting renderer which will
send the demuxed stream directly to browser, and then transmit to the
Cast receiver over network.

In order to switch between the normal and remoting renderer,
WebMediaPlayer needs to know when to enter/exit immersive video
playback scenario. A good indicator for this is that the video
enters/exits full screen. Therefore, WebMediaPlayer needs to be
notified when video enters/exits full screen, no matter what the full
screen element is.

Original WIP CL: https://codereview.chromium.org/2204673004/
==========

to

==========
Notify WebMediaPlayer when HTML video enters full screen.

In immersive video playback scenario, the tab mirroring efficiency and
quality can be improved by bypassing the decoding, local rendering,
and real-time encoding process. WebMediaPlayer will stop the normal
renderer in this case, and switch to a remoting renderer which will
send the demuxed stream directly to browser, and then transmit to the
Cast receiver over network.

In order to switch between the normal and remoting renderer,
WebMediaPlayer needs to know when to enter/exit immersive video
playback scenario. A good indicator for this is that the video
enters/exits full screen. Therefore, WebMediaPlayer needs to be
notified when video enters/exits full screen, no matter what the full
screen element is.

Original WIP CL: https://codereview.chromium.org/2204673004/
==========

[xjz, 2016-08-09 22:45:43]

xjz@chromium.org changed reviewers:
+ liberato@chromium.org, miu@chromium.org, mlamouri@chromium.org

[xjz, 2016-08-09 22:46:23]

xjz@chromium.org changed reviewers:
+ aelias@chromium.org

[xjz, 2016-08-09 22:52:52]

mlamouri@: PTAL changes on third_party/WebKit/Source/core/html/*

aelias@: PTAL changes on  third_party/WebKit/Source/web/* and
third_party/WebKit/public/platform/WebMediaPlayer.h

Verified with YouTube, Vimeo, and Amazon videos.

[aelias_OOO_until_Jul13, 2016-08-10 01:14:59]

https://codereview.chromium.org/2235473002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2235473002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMediaPlayer.h:207: virtual void
updateMediaInFullscreen(bool isFullscreen) {}
Please file a BUG= tracking the underlying feature, and show me a link to the
pending patchset intended to use this new API.

I surmise this is Chromecast code already existing in your private branch that
you're now upstreaming?  Could you link to a design doc for your upstreaming
plan or otherwise give me some context (I haven't heard what's happening with
Chromecast lately)?

One basic question I have is whether this is part of a project to imminently
upstream Chromecast to 100%, or whether you plan to leave the other half inside
the private branch indefinitely.  In my experience, landing "dead" code with an
pseudo-API surface exposed only to a private branch is not maintainable.  The
Blink web API is more of an implementation detail of content/ rather than a true
API with external commitments -- in general any 
third_party/WebKit/public/ API with no user in content/ is something we consider
a loose end that should be deleted, and likewise we should not be adding any new
dead code like this either.

[fs, 2016-08-10 17:30:13]

https://codereview.chromium.org/2235473002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2235473002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FullscreenController.cpp:73: TagCollection*
collection = element->getElementsByTagName("video");
Drive-by: You should be able to do:

for (HTMLVideoElement& videoElement :
Traversal<HTMLVideoElement>::descendantsOf(element))
    videoElement.update...;

instead of this. Or maybe you want this to be about HTMLMediaElement - it's hard
to tell since there's a bit of a mix here. (I don't think you want to use
getElementsByTagName, since it will add caches et.c.)

[xjz, 2016-08-10 21:33:30]

https://codereview.chromium.org/2235473002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2235473002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FullscreenController.cpp:73: TagCollection*
collection = element->getElementsByTagName("video");
On 2016/08/10 17:30:13, fs (OoO until Aug 15) wrote:
> Drive-by: You should be able to do:
> 
> for (HTMLVideoElement& videoElement :
> Traversal<HTMLVideoElement>::descendantsOf(element))
>     videoElement.update...;
> 
> instead of this. Or maybe you want this to be about HTMLMediaElement - it's
hard
> to tell since there's a bit of a mix here. (I don't think you want to use
> getElementsByTagName, since it will add caches et.c.)

Thanks for the review! I'll address it in the next patch set. :)

https://codereview.chromium.org/2235473002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2235473002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMediaPlayer.h:207: virtual void
updateMediaInFullscreen(bool isFullscreen) {}
On 2016/08/10 01:14:59, aelias wrote:
> Please file a BUG= tracking the underlying feature, and show me a link to the
> pending patchset intended to use this new API.
> 
> I surmise this is Chromecast code already existing in your private branch that
> you're now upstreaming?  Could you link to a design doc for your upstreaming
> plan or otherwise give me some context (I haven't heard what's happening with
> Chromecast lately)?
> 
> One basic question I have is whether this is part of a project to imminently
> upstream Chromecast to 100%, or whether you plan to leave the other half
inside
> the private branch indefinitely.  In my experience, landing "dead" code with
an
> pseudo-API surface exposed only to a private branch is not maintainable.  The
> Blink web API is more of an implementation detail of content/ rather than a
true
> API with external commitments -- in general any 
> third_party/WebKit/public/ API with no user in content/ is something we
consider
> a loose end that should be deleted, and likewise we should not be adding any
new
> dead code like this either.

There is no private branch. The other part should be in WebMediaPlayer, which is
not finalized yet. The design doc is still under working. Maybe please hold off
the review of this CL first. I'll update when the design doc is available.
Thanks!

[aelias_OOO_until_Jul13, 2016-08-10 21:55:12]

https://codereview.chromium.org/2235473002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2235473002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMediaPlayer.h:207: virtual void
updateMediaInFullscreen(bool isFullscreen) {}
On 2016/08/10 at 21:33:29, xjz wrote:
> There is no private branch. The other part should be in WebMediaPlayer, which
is not finalized yet. The design doc is still under working. Maybe please hold
off the review of this CL first. I'll update when the design doc is available.
Thanks!

Good to hear, thanks!  Please ping me again when the patch that makes use of it
is uploaded.  (I like to review code end-to-end to get a better perspective and
possibly suggest cross-cutting changes.)