Detect change on video/viewport intersection when rendered remotely

Detect change on video/viewport intersection when rendered remotely.

Peoriodically (every 1s) check whether video/viewport intersection is
changed when video is rendered remotely.

BUG=643964
Committed: https://crrev.com/af29d4183dd123c234c142edbb818180ce0529ab
Cr-Commit-Position: refs/heads/master@{#438980}

[xjz, 2016-12-08 00:07:53]

xjz@chromium.org changed reviewers:
+ miu@chromium.org, ojan@chromium.org, szager@chromium.org

[xjz, 2016-12-08 00:07:53]

This is a follow-up CL to detect video/viewport intersection change when video
is paused. Adds a new timer to check the intersection with a lower frequency (2s
for now) when video is paused. PTAL. Thanks!

[szager1, 2016-12-08 00:18:59]

lgtm

grumble grumble... tests... grumble grumble

[Z_DONOTUSE, 2016-12-08 00:43:27]

ojan@google.com changed reviewers:
+ ojan@google.com

[Z_DONOTUSE, 2016-12-08 00:43:27]

Is the goal here to stop casting when the video goes offscreen, start casting
when it goes on screen, or both?

I'm worried about the battery impact of polling indefinitely, so I'd like to
find a solution that stops the polling at some reasonable point.

[xjz, 2016-12-08 01:21:00]

On 2016/12/08 00:43:27, Z_DONOTUSE wrote:
> Is the goal here to stop casting when the video goes offscreen, start casting
> when it goes on screen, or both?
> 
> I'm worried about the battery impact of polling indefinitely, so I'd like to
> find a solution that stops the polling at some reasonable point.

The goal here is to stop remoting (and back to tab mirroring or local playback)
when video does not stably fill most of the viewport, and start remoting when
video stably fills most of the viewport.
If the video is not visible, for sure we'll not start remoting.
So we only need this detection when video is visible and paused. Does this sgty?
I'll see if I can use the ElementVisibilityObserver/IntersectionObserver for
this purpose. Thanks!

[Z_DONOTUSE, 2016-12-08 03:12:59]

On 2016/12/08 at 01:21:00, xjz wrote:
> On 2016/12/08 00:43:27, Z_DONOTUSE wrote:
> > Is the goal here to stop casting when the video goes offscreen, start
casting
> > when it goes on screen, or both?
> > 
> > I'm worried about the battery impact of polling indefinitely, so I'd like to
> > find a solution that stops the polling at some reasonable point.
> 
> The goal here is to stop remoting (and back to tab mirroring or local
playback) when video does not stably fill most of the viewport, and start
remoting when video stably fills most of the viewport.
> If the video is not visible, for sure we'll not start remoting.
> So we only need this detection when video is visible and paused. Does this
sgty? I'll see if I can use the ElementVisibilityObserver/IntersectionObserver
for this purpose. Thanks!

Something like this sounds good. But, I don't think we want to poll at all for
videos that aren't actually casting. What would you think of plumbing through to
HTMLMediaElement whether the video is casting. That way we only need to do it
when the video is casting and can stop polling when we go to local playback or
tab mirroring.

[xjz, 2016-12-08 17:58:05]

On 2016/12/08 03:12:59, Z_DONOTUSE wrote:
> On 2016/12/08 at 01:21:00, xjz wrote:
> > On 2016/12/08 00:43:27, Z_DONOTUSE wrote:
> > > Is the goal here to stop casting when the video goes offscreen, start
> casting
> > > when it goes on screen, or both?
> > > 
> > > I'm worried about the battery impact of polling indefinitely, so I'd like
to
> > > find a solution that stops the polling at some reasonable point.
> > 
> > The goal here is to stop remoting (and back to tab mirroring or local
> playback) when video does not stably fill most of the viewport, and start
> remoting when video stably fills most of the viewport.
> > If the video is not visible, for sure we'll not start remoting.
> > So we only need this detection when video is visible and paused. Does this
> sgty? I'll see if I can use the ElementVisibilityObserver/IntersectionObserver
> for this purpose. Thanks!
> 
> Something like this sounds good. But, I don't think we want to poll at all for
> videos that aren't actually casting. What would you think of plumbing through
to
> HTMLMediaElement whether the video is casting. That way we only need to do it
> when the video is casting and can stop polling when we go to local playback or
> tab mirroring.

Yes, that will be perfect. 
But currently I don't see a way to let HTMLMediaElement know whether we are in
casting(tab mirroring).
Will discuss this with miu@. Thanks!

+miu

[miu, 2016-12-08 20:57:55]

CIL...(3 possible solutions mentioned)

On 2016/12/08 17:58:05, xjz wrote:
> On 2016/12/08 03:12:59, Z_DONOTUSE wrote:
> > > > I'm worried about the battery impact of polling indefinitely, so I'd
like
> to
> > > > find a solution that stops the polling at some reasonable point.

This is one reason I envisioned we should stop the polling after N seconds. If
N=600 (10 minutes), would that be acceptable?

Otherwise, what if we went back to our prior solution, where we were evaluating
the viewport intersection for changes at each compositor draw?

> > What would you think of plumbing through
> to
> > HTMLMediaElement whether the video is casting. That way we only need to do
it
> > when the video is casting and can stop polling when we go to local playback
or
> > tab mirroring.

There is strong precedent for keeping this information from the Web layer, which
I'm happy to discuss further offline.

Though, I suppose we could add something like an
enableViewportDominanceMonitoring(bool) (naming?) API.

[xjz, 2016-12-09 22:33:55]

Description was changed from

==========
Detect change on video/viewport intersection when video is paused.

Peoriodically (every 2s) check whether video/viewport intersection is
changed when video is paused.

BUG=643964
==========

to

==========
Detect change on video/viewport intersection when video is paused.

Peoriodically (every 2s) check whether video/viewport intersection is
changed when video is paused.

BUG=643964
==========

[xjz, 2016-12-09 22:33:55]

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

[xjz, 2016-12-09 22:42:01]

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

[xjz, 2016-12-09 22:42:02]

PTAL. PS#3 only tracks the intersection with viewport when we are in
mirroring/remoting. Tests are not updated yet. Thanks!

[xjz, 2016-12-10 00:04:25]

PTAL PS#4. Thanks! 
Added an API to WebMediaPlayerClient to notify HTMLMediaElement when
starts/stops mirroring (remoting).

[miu, 2016-12-10 02:18:37]

I like this approach better: It will also allow us to more easily plumb-in the
events to the Remote Playback API, as discussed in our Intent to Implement on
blink-dev@.

Comments on PS4:

https://codereview.chromium.org/2556333002/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2556333002/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:224: void OnRemoteDisplayChanged(bool
is_displayed_remotely);
naming nit: s/rendering/display/ in method name, argument name, and comment.

https://codereview.chromium.org/2556333002/diff/60001/media/remoting/remoting...
File media/remoting/remoting_renderer_controller.cc (right):

https://codereview.chromium.org/2556333002/diff/60001/media/remoting/remoting...
media/remoting/remoting_renderer_controller.cc:120: DCHECK(!cb.is_null());
nit: It's probably okay for this to be a null callback, if client code ever
wants to "unset" the callback.

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

https://codereview.chromium.org/2556333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:131: constexpr double
kCheckIntersectWhenPausedIntervalInSeconds = 2;
If the "stable" check is 5 seconds, the timer interval should be something that
is zero modulo 5 seconds: Probably 1 second would be fine.

https://codereview.chromium.org/2556333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3284:
m_checkIntersectionWhenPausedTimer.stop();
If you take my advice below, this extra stop() call here becomes unnecessary.

https://codereview.chromium.org/2556333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4044:
m_enableMonitorViewportIntersection = enable;
IMHO, this would all be simpler if we didn't have two different timers that
might call checkViewportIntersectionChanged(). Let's just have one and then:

1. Rename m_checkIntersectionWhenPausedTimer to just
m_checkViewportIntersectionTimer.

2. Get rid of the m_enableMonitorViewportIntersection boolean.

3. Only call start() or stop() on the timer from
enableMonitorViewportIntersection():

  void HTMLMediaElement::enableMonitorViewportIntersection(bool enable) {
    if (enable && !m_checkViewportIntersectionTimer.isActive())
      m_checkViewportIntersectionTimer.startRepeating(...);
    else if (!enable)
      m_checkViewportIntersectionTimer.stop();
  }

[xjz, 2016-12-12 18:54:27]

Patchset #5 (id:70001) has been deleted

[xjz, 2016-12-12 18:58:45]

Addressed miu's comments. PTAL

https://codereview.chromium.org/2556333002/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2556333002/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:224: void OnRemoteDisplayChanged(bool
is_displayed_remotely);
On 2016/12/10 02:18:37, miu wrote:
> naming nit: s/rendering/display/ in method name, argument name, and comment.

Done.

https://codereview.chromium.org/2556333002/diff/60001/media/remoting/remoting...
File media/remoting/remoting_renderer_controller.cc (right):

https://codereview.chromium.org/2556333002/diff/60001/media/remoting/remoting...
media/remoting/remoting_renderer_controller.cc:120: DCHECK(!cb.is_null());
On 2016/12/10 02:18:37, miu wrote:
> nit: It's probably okay for this to be a null callback, if client code ever
> wants to "unset" the callback.

Done.

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

https://codereview.chromium.org/2556333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:131: constexpr double
kCheckIntersectWhenPausedIntervalInSeconds = 2;
On 2016/12/10 02:18:37, miu wrote:
> If the "stable" check is 5 seconds, the timer interval should be something
that
> is zero modulo 5 seconds: Probably 1 second would be fine.

Done.

https://codereview.chromium.org/2556333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3284:
m_checkIntersectionWhenPausedTimer.stop();
On 2016/12/10 02:18:37, miu wrote:
> If you take my advice below, this extra stop() call here becomes unnecessary.

Done.

https://codereview.chromium.org/2556333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4044:
m_enableMonitorViewportIntersection = enable;
On 2016/12/10 02:18:37, miu wrote:
> IMHO, this would all be simpler if we didn't have two different timers that
> might call checkViewportIntersectionChanged(). Let's just have one and then:
> 
> 1. Rename m_checkIntersectionWhenPausedTimer to just
> m_checkViewportIntersectionTimer.
> 
> 2. Get rid of the m_enableMonitorViewportIntersection boolean.
> 
> 3. Only call start() or stop() on the timer from
> enableMonitorViewportIntersection():
> 
>   void HTMLMediaElement::enableMonitorViewportIntersection(bool enable) {
>     if (enable && !m_checkViewportIntersectionTimer.isActive())
>       m_checkViewportIntersectionTimer.startRepeating(...);
>     else if (!enable)
>       m_checkViewportIntersectionTimer.stop();
>   }

Done.

[xjz, 2016-12-12 20:33:25]

ping ojan and esprehn: Please see whether this approach (PS# 5) lgty. Thanks!

[miu, 2016-12-13 21:31:09]

PS5 lgtm.

[miu, 2016-12-13 21:32:40]

On 2016/12/13 21:31:09, miu wrote:
> PS5 lgtm.

Please update change description. The nice thing about this change is that it
doesn't just solve the problem for "when video is paused," but every conceivable
situation where before/during/after remoting could be engaged.

[xjz, 2016-12-13 21:40:04]

Description was changed from

==========
Detect change on video/viewport intersection when video is paused.

Peoriodically (every 2s) check whether video/viewport intersection is
changed when video is paused.

BUG=643964
==========

to

==========
Detect change on video/viewport intersection when rendered remotely.

Peoriodically (every 1s) check whether video/viewport intersection is
changed when video is rendered remotely.

BUG=643964
==========

[xjz, 2016-12-13 21:40:20]

On 2016/12/13 21:32:40, miu wrote:
> On 2016/12/13 21:31:09, miu wrote:
> > PS5 lgtm.
> 
> Please update change description. The nice thing about this change is that it
> doesn't just solve the problem for "when video is paused," but every
conceivable
> situation where before/during/after remoting could be engaged.

Thanks!
Done. :)

[ojan, 2016-12-14 00:39:08]

lgtm

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

https://codereview.chromium.org/2556333002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:131: constexpr double
kCheckViewportIntersectionIntervalInSeconds = 1;
Naming nit: Other variables leave out the "In", e.g. see the one in the line
above.

https://codereview.chromium.org/2556333002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/2556333002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:400: void
enableMonitorViewportIntersection(bool) final;
I would have gone with a more generic name like "setIsRemoting" or something.
Mostly, I'm trying to find ways to have a more minimal API surface between
platform and media directories so we don't need to do plumbing for every change
we want to make.

But this is fine for now. We can rename it in the future if we find other cases
were we want to act differently based off whether the video is casting.

[xjz, 2016-12-14 00:55:05]

Thanks Ojan! Addressed your comment.

esprehn: PTAL changes on
third_party/WebKit/public/platform/WebMediaPlayerClient.h

xhwang: PTAL on changes on media/blink/*.

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

https://codereview.chromium.org/2556333002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:131: constexpr double
kCheckViewportIntersectionIntervalInSeconds = 1;
On 2016/12/14 00:39:08, ojan wrote:
> Naming nit: Other variables leave out the "In", e.g. see the one in the line
> above.

Done.

https://codereview.chromium.org/2556333002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/2556333002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:400: void
enableMonitorViewportIntersection(bool) final;
On 2016/12/14 00:39:08, ojan wrote:
> I would have gone with a more generic name like "setIsRemoting" or something.
> Mostly, I'm trying to find ways to have a more minimal API surface between
> platform and media directories so we don't need to do plumbing for every
change
> we want to make.
> 
> But this is fine for now. We can rename it in the future if we find other
cases
> were we want to act differently based off whether the video is casting.

Yes, currently this is only used to enable monitoring changes on viewport
intersection. I agree to rename it if we have more application in future.

[xhwang, 2016-12-14 18:11:41]

lg with naming comments

https://codereview.chromium.org/2556333002/diff/110001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2556333002/diff/110001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:224: void OnRemoteRenderingChanged(bool
is_displayed_remotely);
Do rendering and displayed mean the same thing?

Does it make sense to add an enum to make it more readable?

enum class RenderingState {
  LOCAL,
  REMOTE
};

onRenderingStateChange(RenderingState rendering_state);

https://codereview.chromium.org/2556333002/diff/110001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/2556333002/diff/110001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaPlayerClient.h:97: virtual void
enableMonitorViewportIntersection(bool) {}
I know blink/webkit doesn't like comments, but IMHO this new API is really hard
to understand. Will we always "enable" but use the "bool" as auxiliary info? Or
will we actually "disable" when the "bool" is false?

Since we already have interfaces methods with nice comments (see below), can we
follow that style?

https://codereview.chromium.org/2556333002/diff/110001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaPlayerClient.h:97: virtual void
enableMonitorViewportIntersection(bool) {}
BTW, I am not a fan of using enableFoo(false) to disable Foo... How about:

viewportIntersectionMonitorStateChanged(bool enable)

[miu, 2016-12-14 19:15:50]

https://codereview.chromium.org/2556333002/diff/110001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/2556333002/diff/110001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaPlayerClient.h:97: virtual void
enableMonitorViewportIntersection(bool) {}
On 2016/12/14 18:11:41, xhwang wrote:
> BTW, I am not a fan of using enableFoo(false) to disable Foo... How about:
> 
> viewportIntersectionMonitorStateChanged(bool enable)

No, that wouldn't be right: This is a control method, not an observer callback.

Though, you do make a good point about the namong being a little confusing. How
about activateViewportIntersectionMonitoring()?

[ojan, 2016-12-14 21:00:56]

https://codereview.chromium.org/2556333002/diff/110001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/2556333002/diff/110001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaPlayerClient.h:97: virtual void
enableMonitorViewportIntersection(bool) {}
On 2016/12/14 at 18:11:41, xhwang wrote:
> I know blink/webkit doesn't like comments, but IMHO this new API is really
hard to understand. Will we always "enable" but use the "bool" as auxiliary
info? Or will we actually "disable" when the "bool" is false?

Just want to clarify: Blink has no policy against comments. Comments that
explain "why" code is doing something non-obvious are always welcome. For
comments that explain "what" the code is doing is something more like: "Use
descriptive names for methods and avoid comments that just state relatively
clear implications of what the method is doing based off the name." This is
consistent with the spirit of the Google C++ style guide
(https://google.github.io/styleguide/cppguide.html#Function_Comments), but has a
different opinion on what counts as "simple and obvious."

For example, the following would be a comment to leave out because it provides
no value over the obvious meaning of the name.

// Activates viewport intersection monitoring. True value means activated.
virtual void activateViewportIntersectionMonitoring(bool) {}

[xjz, 2016-12-15 00:01:41]

Addressed xhwang's comments. PTAL.

https://codereview.chromium.org/2556333002/diff/110001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2556333002/diff/110001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:224: void OnRemoteRenderingChanged(bool
is_displayed_remotely);
On 2016/12/14 18:11:41, xhwang wrote:
> Do rendering and displayed mean the same thing?
> 
> Does it make sense to add an enum to make it more readable?
> 
> enum class RenderingState {
>   LOCAL,
>   REMOTE
> };
> 
> onRenderingStateChange(RenderingState rendering_state);

This name seems confusing. This is not actually telling whether we are using
local or remoting renderer. This is actually used to activate whether to monitor
the changes on viewport intersection. Renamed.

https://codereview.chromium.org/2556333002/diff/110001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/2556333002/diff/110001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaPlayerClient.h:97: virtual void
enableMonitorViewportIntersection(bool) {}
On 2016/12/14 21:00:56, ojan wrote:
> On 2016/12/14 at 18:11:41, xhwang wrote:
> > I know blink/webkit doesn't like comments, but IMHO this new API is really
> hard to understand. Will we always "enable" but use the "bool" as auxiliary
> info? Or will we actually "disable" when the "bool" is false?
> 
> Just want to clarify: Blink has no policy against comments. Comments that
> explain "why" code is doing something non-obvious are always welcome. For
> comments that explain "what" the code is doing is something more like: "Use
> descriptive names for methods and avoid comments that just state relatively
> clear implications of what the method is doing based off the name." This is
> consistent with the spirit of the Google C++ style guide
> (https://google.github.io/styleguide/cppguide.html#Function_Comments), but has
a
> different opinion on what counts as "simple and obvious."
> 
> For example, the following would be a comment to leave out because it provides
> no value over the obvious meaning of the name.
> 
> // Activates viewport intersection monitoring. True value means activated.
> virtual void activateViewportIntersectionMonitoring(bool) {}

Thanks miu and ojan. Took the suggestion, and renamed this as
"activateViewportIntersectionMonitoring".

[xjz, 2016-12-15 00:29:13]

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

[commit-bot: I haz the power, 2016-12-15 00:29:53]

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-12-15 00:38:54]

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

[commit-bot: I haz the power, 2016-12-15 00:38:55]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[xhwang, 2016-12-15 04:10:23]

https://codereview.chromium.org/2556333002/diff/150001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/2556333002/diff/150001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaPlayerClient.h:97: virtual void
activateViewportIntersectionMonitoring(bool) {}
Sorry for the delay. To me, "activate" is pretty much the same as "enable"...
updateViewportIntersectionMonitoring(ENABLE) would be more clear but isn't
perfect either. So it's up to you.

I agree comments like "activate viewport intersection monitoring" would be
redundant. But for this particular one, if I am going to implement
WebMediaPlayer interface, I really have now idea when I should "activate or
deactivate viewport intersection". If I activate it, what will happen? Will I
get any notification if viewport intersection actually changed?

Also, this is the only method on this interface that's not pure virtual. Any
reason for that? Should we be consistent with other existing methods?

[xjz, 2016-12-15 20:00:33]

Addressed xhwang's comments. PTAL

https://codereview.chromium.org/2556333002/diff/150001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/2556333002/diff/150001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaPlayerClient.h:97: virtual void
activateViewportIntersectionMonitoring(bool) {}
On 2016/12/15 04:10:23, xhwang wrote:
> Sorry for the delay. To me, "activate" is pretty much the same as "enable"...
> updateViewportIntersectionMonitoring(ENABLE) would be more clear but isn't
> perfect either. So it's up to you.
> 
> I agree comments like "activate viewport intersection monitoring" would be
> redundant. But for this particular one, if I am going to implement
> WebMediaPlayer interface, I really have now idea when I should "activate or
> deactivate viewport intersection". If I activate it, what will happen? Will I
> get any notification if viewport intersection actually changed?
> 
> Also, this is the only method on this interface that's not pure virtual. Any
> reason for that? Should we be consistent with other existing methods?

Thanks. Added comments to explain what this is currently used for. PTAL

As for naming, I prefer the current one as it sounds more like a control method.

[xjz, 2016-12-15 20:02:04]

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

[commit-bot: I haz the power, 2016-12-15 20:02:50]

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

[esprehn, 2016-12-15 20:04:13]

lgtm

[xhwang, 2016-12-15 20:31:11]

lgtm % nits

https://codereview.chromium.org/2556333002/diff/170001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2556333002/diff/170001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaPlayer.h:267: //
HTMLMediaElement::activateViewportIntersectionMonitoring().
s/HTMLMediaElement/WMPClient..

https://codereview.chromium.org/2556333002/diff/170001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/2556333002/diff/170001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaPlayerClient.h:98: // After the
monitoring is activated, the client can inform WebMediaPlayer
s/can/will

[xjz, 2016-12-15 20:42:37]

Thanks. Addressed comments.

https://codereview.chromium.org/2556333002/diff/170001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2556333002/diff/170001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaPlayer.h:267: //
HTMLMediaElement::activateViewportIntersectionMonitoring().
On 2016/12/15 20:31:11, xhwang wrote:
> s/HTMLMediaElement/WMPClient..

Done.

https://codereview.chromium.org/2556333002/diff/170001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/2556333002/diff/170001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaPlayerClient.h:98: // After the
monitoring is activated, the client can inform WebMediaPlayer
On 2016/12/15 20:31:11, xhwang wrote:
> s/can/will

Done.

[xjz, 2016-12-15 20:42:49]

The CQ bit was checked by xjz@chromium.org

[xjz, 2016-12-15 20:42:50]

The patchset sent to the CQ was uploaded after l-g-t-m from szager@chromium.org,
miu@chromium.org, ojan@chromium.org, esprehn@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2556333002/#ps190001
(title: "Fix nits.")

[commit-bot: I haz the power, 2016-12-15 20:43:41]

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-12-15 23:18:12]

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

[commit-bot: I haz the power, 2016-12-15 23:18:13]

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

[xjz, 2016-12-15 23:28:23]

The CQ bit was checked by xjz@chromium.org

[commit-bot: I haz the power, 2016-12-15 23:29:30]

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-12-16 01:52:36]

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1481844503201780,
"parent_rev": "06c0ebbcc51c1160fcda0a6d6c47ac8079050530", "commit_rev":
"03120544d45456eaf17de288261d2bfdec289f9f"}

[commit-bot: I haz the power, 2016-12-16 01:53:12]

Description was changed from

==========
Detect change on video/viewport intersection when rendered remotely.

Peoriodically (every 1s) check whether video/viewport intersection is
changed when video is rendered remotely.

BUG=643964
==========

to

==========
Detect change on video/viewport intersection when rendered remotely.

Peoriodically (every 1s) check whether video/viewport intersection is
changed when video is rendered remotely.

BUG=643964

Review-Url: https://codereview.chromium.org/2556333002
==========

[commit-bot: I haz the power, 2016-12-16 01:53:15]

Committed patchset #10 (id:190001)

[commit-bot: I haz the power, 2016-12-16 01:57:18]

Description was changed from

==========
Detect change on video/viewport intersection when rendered remotely.

Peoriodically (every 1s) check whether video/viewport intersection is
changed when video is rendered remotely.

BUG=643964

Review-Url: https://codereview.chromium.org/2556333002
==========

to

==========
Detect change on video/viewport intersection when rendered remotely.

Peoriodically (every 1s) check whether video/viewport intersection is
changed when video is rendered remotely.

BUG=643964

Committed: https://crrev.com/af29d4183dd123c234c142edbb818180ce0529ab
Cr-Commit-Position: refs/heads/master@{#438980}
==========

[commit-bot: I haz the power, 2016-12-16 01:57:20]

Patchset 10 (id:??) landed as
https://crrev.com/af29d4183dd123c234c142edbb818180ce0529ab
Cr-Commit-Position: refs/heads/master@{#438980}