Provide a WebContents API to discover the playback of a fullscreen video.

Provide a WebContents API to discover the playback of a fullscreen video.

That means a video is playing fullscreen, either directly (native controls) or
indirectly (custom controls). This is only exposed to the Java WebContents API
at the moment.

BUG=690960
Review-Url: https://codereview.chromium.org/2693203002
Cr-Commit-Position: refs/heads/master@{#452487}
Committed: https://chromium.googlesource.com/chromium/src/+/e52c1b5442d81049d4b47b716d994ba746dde8a3

[mlamouri (slow - plz ping), 2017-02-14 18:46:27]

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

[commit-bot: I haz the power, 2017-02-14 18:47:23]

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

[mlamouri (slow - plz ping), 2017-02-14 18:47:50]

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

[mlamouri (slow - plz ping), 2017-02-14 18:47:53]

liberato@, this is still missing tests but I would be interested to get an early
feedback.

[commit-bot: I haz the power, 2017-02-14 19:19:54]

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

[commit-bot: I haz the power, 2017-02-14 19:19:55]

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

[liberato (no reviews please), 2017-02-14 23:00:01]

i like this idea.

at a high level, what bothers me is that it's very hard to tell what
MediaWebContentsObserver::HasActive...() is going to do.  if there aren't remote
routes, then it's going to reduce to "IsFullScreen" since the intersection timer
will be turned off.  at least, if i remember correctly.

that could be really confusing.

thanks
-fl

https://codereview.chromium.org/2693203002/diff/1/content/browser/media/media...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/2693203002/diff/1/content/browser/media/media...
content/browser/media/media_web_contents_observer.h:48: bool
HasActiveFullscreenDominantVideo() const;
naming is a little confusing.

personally, i'd go more for something like this:

- remove 'dominant' from the name - maybe HasActiveEffectivelyFullscreenVideo()
or similar.
- returns 'no' if webcontents isn't full screen, rather than dcheck.

otherwise, 'dominant' and/or 'fullscreen' seem overloaded.

as in, the caller wants to know 'is the user looking at a really big video in
this webcontents right now?'

https://codereview.chromium.org/2693203002/diff/1/content/browser/media/media...
content/browser/media/media_web_contents_observer.h:137:
std::pair<MediaPlayerId, FullscreenDominantInfo> fullscreen_dominant_video_ =
nit: perhaps move MediaPlayerId into FullscreenDominantInfo.  all of the
'.first.first' are a bit confusing.

https://codereview.chromium.org/2693203002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2693203002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.h:791: // |IsFullscreen| must
return |true| when this method is called.
why?

[mlamouri (slow - plz ping), 2017-02-17 16:32:08]

Description was changed from

==========
Provide a WebContents API to discover the existance of a fullscreen dominant
video.

That means a video that is either directly fullscreen or dominant in a
fullscreen WebContents. This is only exposed to the Java WebContents API
at the moment.

BUG=690960
==========

to

==========
Provide a WebContents API to discover the playback of a fullscreen video.

That means a video is playing fullscreen, either directly (native controls) or
indirectly (custom controls). This is only exposed to the Java WebContents API
at the moment.

BUG=690960
==========

[mlamouri (slow - plz ping), 2017-02-17 16:36:25]

PTAL

Note that this is not implementing the detection for fullscreen custom controls.
zqzhang@ is working on it.

https://codereview.chromium.org/2693203002/diff/1/content/browser/media/media...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/2693203002/diff/1/content/browser/media/media...
content/browser/media/media_web_contents_observer.h:48: bool
HasActiveFullscreenDominantVideo() const;
On 2017/02/14 at 23:00:01, liberato wrote:
> naming is a little confusing.
> 
> personally, i'd go more for something like this:
> 
> - remove 'dominant' from the name - maybe
HasActiveEffectivelyFullscreenVideo() or similar.
> - returns 'no' if webcontents isn't full screen, rather than dcheck.
> 
> otherwise, 'dominant' and/or 'fullscreen' seem overloaded.
> 
> as in, the caller wants to know 'is the user looking at a really big video in
this webcontents right now?'

Renamed to `HasActiveEffectivelyFullscreenVideo`. It's quite better :)

https://codereview.chromium.org/2693203002/diff/1/content/browser/media/media...
content/browser/media/media_web_contents_observer.h:137:
std::pair<MediaPlayerId, FullscreenDominantInfo> fullscreen_dominant_video_ =
On 2017/02/14 at 23:00:01, liberato wrote:
> nit: perhaps move MediaPlayerId into FullscreenDominantInfo.  all of the
'.first.first' are a bit confusing.

I switched to base::Optional<MediaPlayerId>. Should be simpler :)

https://codereview.chromium.org/2693203002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2693203002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.h:791: // |IsFullscreen| must
return |true| when this method is called.
On 2017/02/14 at 23:00:01, liberato wrote:
> why?

The DCHECK() is just because it would basically mean checking the things
multiple time. Instead of having the callers make the call and the method check
too, better to have a "contract" which requires that callers know what they are
doing. The current expectations is that this would only be called while
fullscreen. It could change though.

[mlamouri (slow - plz ping), 2017-02-18 13:30:51]

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

[commit-bot: I haz the power, 2017-02-18 13:30:59]

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, 2017-02-18 13:44:24]

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

[commit-bot: I haz the power, 2017-02-18 13:44:25]

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

[mlamouri (slow - plz ping), 2017-02-21 22:13:37]

Friend ping :)

[mlamouri (slow - plz ping), 2017-02-21 22:13:38]

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

[commit-bot: I haz the power, 2017-02-21 22:14:46]

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, 2017-02-21 22:46:46]

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

[commit-bot: I haz the power, 2017-02-21 22:46:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[liberato (no reviews please), 2017-02-22 18:28:20]

sorry for the delay. lgtm % nits.

thanks
-fl

https://codereview.chromium.org/2693203002/diff/40001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/2693203002/diff/40001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:189: if
(*fullscreen_player_ == id)
has_value?

https://codereview.chromium.org/2693203002/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_delegate.h (right):

https://codereview.chromium.org/2693203002/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_delegate.h:120: // differentiate native controls
fullscreen and custom controls fullscreen.
i like these new semantics better.  might want to include 'Effectively' here
too, just so that it matches the observer.

[mlamouri (slow - plz ping), 2017-02-22 19:23:25]

mlamouri@chromium.org changed reviewers:
+ creis@chromium.org, dcheng@chromium.org

[mlamouri (slow - plz ping), 2017-02-22 19:23:26]

+dcheng@ for content/common/media/media_player_delegate_messages.h

+creis@ for content/ changes :)

https://codereview.chromium.org/2693203002/diff/40001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/2693203002/diff/40001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:189: if
(*fullscreen_player_ == id)
On 2017/02/22 at 18:28:20, liberato wrote:
> has_value?

Good catch! :)

https://codereview.chromium.org/2693203002/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_delegate.h (right):

https://codereview.chromium.org/2693203002/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_delegate.h:120: // differentiate native controls
fullscreen and custom controls fullscreen.
On 2017/02/22 at 18:28:20, liberato wrote:
> i like these new semantics better.  might want to include 'Effectively' here
too, just so that it matches the observer.

I agree. Updated :)

[mlamouri (slow - plz ping), 2017-02-22 19:23:33]

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

[commit-bot: I haz the power, 2017-02-22 19:24:28]

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, 2017-02-22 21:01:05]

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

[commit-bot: I haz the power, 2017-02-22 21:01:06]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-02-23 00:18:07]

creis@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[Charlie Reis, 2017-02-23 00:18:08]

[+dalecurtis]

content/ LGTM with nits and a few unrelated questions.

https://codereview.chromium.org/2693203002/diff/60001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/2693203002/diff/60001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:86: // TODO(dalecurtis):
These should no longer be FrameHostMsg.
Unrelated: Is this TODO stale?  I don't see any FrameHostMsgs below.

https://codereview.chromium.org/2693203002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2693203002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4260: return
media_web_contents_observer_->HasActiveEffectivelyFullscreenVideo();
I found this pretty weird at first glance-- that WebContents would implement a
method by asking one of its observers.  (Seems oddly circular.)

I think I understand now that MediaWebContentsObserver is really more of its own
class that manages some state (and happens to be a WebContentsObserver to get
its job done).  In that sense, maybe that class would make more sense if it
didn't have WebContentsObserver in its name?

Anyway, it's not a problem for this CL, and not something I feel too strongly
about.  Just a way to possibly reduce confusion in the future.

https://codereview.chromium.org/2693203002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2693203002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:799: // native or custom
controls.
I think the description of "effectively fullscreen" from
media_web_contents_observer.h was clearer, and this is the comment more people
are likely to see.  Can you copy some of that here?

[DaleCurtis, 2017-02-23 01:11:57]

https://codereview.chromium.org/2693203002/diff/60001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/2693203002/diff/60001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:86: // TODO(dalecurtis):
These should no longer be FrameHostMsg.
On 2017/02/23 at 00:18:08, Charlie Reis wrote:
> Unrelated: Is this TODO stale?  I don't see any FrameHostMsgs below.

Yup, looks like it!

[dcheng, 2017-02-23 03:18:52]

ipc lgtm with nits

https://codereview.chromium.org/2693203002/diff/60001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/2693203002/diff/60001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:69: if
(!fullscreen_player_.has_value())
Nit: omit .has_value()

https://codereview.chromium.org/2693203002/diff/60001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:190: if
(fullscreen_player_.has_value() && *fullscreen_player_ == id)
Nit: combine the if statements and remove has_value()

[mlamouri (slow - plz ping), 2017-02-23 13:31:53]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-02-23 13:31:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org, creis@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2693203002/#ps80001
(title: "apply review comments")

[commit-bot: I haz the power, 2017-02-23 13:32:01]

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

[mlamouri (slow - plz ping), 2017-02-23 13:34:19]

Thanks, comments applied.

https://codereview.chromium.org/2693203002/diff/60001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/2693203002/diff/60001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:69: if
(!fullscreen_player_.has_value())
On 2017/02/23 at 03:18:52, dcheng wrote:
> Nit: omit .has_value()

I actually like explicit has_value() but I guess implicit boolean conversion
should be fine :)

https://codereview.chromium.org/2693203002/diff/60001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:86: // TODO(dalecurtis):
These should no longer be FrameHostMsg.
On 2017/02/23 at 01:11:57, DaleCurtis wrote:
> On 2017/02/23 at 00:18:08, Charlie Reis wrote:
> > Unrelated: Is this TODO stale?  I don't see any FrameHostMsgs below.
> 
> Yup, looks like it!

Removed it.

https://codereview.chromium.org/2693203002/diff/60001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:190: if
(fullscreen_player_.has_value() && *fullscreen_player_ == id)
On 2017/02/23 at 03:18:52, dcheng wrote:
> Nit: combine the if statements and remove has_value()

Okay for has_value() but I can't combine the if statements as it would have a
side effect (fullscreen_player_ = id)

https://codereview.chromium.org/2693203002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2693203002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4260: return
media_web_contents_observer_->HasActiveEffectivelyFullscreenVideo();
On 2017/02/23 at 00:18:08, Charlie Reis wrote:
> I found this pretty weird at first glance-- that WebContents would implement a
method by asking one of its observers.  (Seems oddly circular.)
> 
> I think I understand now that MediaWebContentsObserver is really more of its
own class that manages some state (and happens to be a WebContentsObserver to
get its job done).  In that sense, maybe that class would make more sense if it
didn't have WebContentsObserver in its name?
> 
> Anyway, it's not a problem for this CL, and not something I feel too strongly
about.  Just a way to possibly reduce confusion in the future.

Ack

https://codereview.chromium.org/2693203002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2693203002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:799: // native or custom
controls.
On 2017/02/23 at 00:18:08, Charlie Reis wrote:
> I think the description of "effectively fullscreen" from
media_web_contents_observer.h was clearer, and this is the comment more people
are likely to see.  Can you copy some of that here?

Done.

[commit-bot: I haz the power, 2017-02-23 14:57:54]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487856713749970,
"parent_rev": "ea269029bfe2f93ee58b6bb86208b36811b1c19b", "commit_rev":
"e52c1b5442d81049d4b47b716d994ba746dde8a3"}

[commit-bot: I haz the power, 2017-02-23 14:58:32]

Description was changed from

==========
Provide a WebContents API to discover the playback of a fullscreen video.

That means a video is playing fullscreen, either directly (native controls) or
indirectly (custom controls). This is only exposed to the Java WebContents API
at the moment.

BUG=690960
==========

to

==========
Provide a WebContents API to discover the playback of a fullscreen video.

That means a video is playing fullscreen, either directly (native controls) or
indirectly (custom controls). This is only exposed to the Java WebContents API
at the moment.

BUG=690960

Review-Url: https://codereview.chromium.org/2693203002
Cr-Commit-Position: refs/heads/master@{#452487}
Committed:
https://chromium.googlesource.com/chromium/src/+/e52c1b5442d81049d4b47b716d99...
==========

[commit-bot: I haz the power, 2017-02-23 14:58:33]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e52c1b5442d81049d4b47b716d99...