cast: support cursor rendering for tab capture

cast: support cursor rendering for tab capture

This refactors the current cursor rendering being done by
aura window capture machine and adds support for mouse cursor display for
both window/desktop and tab mirroring

In cast of tab capture, the mouse is rendered only when user is interacting with
the tab being mirrored and stops showing when user stops
interacting.

BUG=550555
Committed: https://crrev.com/727606cdcd89919585d5decf3579ef427adced98
Cr-Commit-Position: refs/heads/master@{#358958}

[Irfan, 2015-10-19 22:54:40]

Description was changed from

==========
cast: support cursor rendering for tab capture

This refactors the current cursor rendering being done by
aura window capture machine and adds support for mouse cursor display for
both window/desktop and tab mirroring

In cast of tab capture, the mouse is rendered only when user is interacting with
the tab being mirrored and stops showing when user stops
interacting.

BUG=
==========

to

==========
cast: support cursor rendering for tab capture

This refactors the current cursor rendering being done by
aura window capture machine and adds support for mouse cursor display for
both window/desktop and tab mirroring

In cast of tab capture, the mouse is rendered only when user is interacting with
the tab being mirrored and stops showing when user stops
interacting.

BUG=
==========

[Irfan, 2015-10-19 22:54:40]

isheriff@chromium.org changed reviewers:
+ miu@chromium.org, xjz@chromium.org

[Irfan, 2015-10-19 22:59:14]

Yuri, I have adopted your suggested approach for the lifetime of cursor
renderer. A bunch of callback changes, but this is better than what I had. No
tests yet, PTAL.

[miu, 2015-10-20 02:12:44]

Approach and structure looks good.  Comments:

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer.cc (right):

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.cc:58: 
Please include changes in: https://codereview.chromium.org/1413803002/

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.cc:97: base::TimeDelta now =
base::TimeDelta::FromInternalValue(
FromInternalValue() and ToInternalValue() are only supposed to be used for by
serializing implementations, since they defeat the compile-time checking of time
value math.  Instead, you should:

  base::TimeDelta now = base::TimeTicks::Now() - base::TimeTicks();

But, for robustness, it'd be even better to use the utility function:

  #include "ui/events/event_utils.h"
  ...
  base::TimeDelta now = ui::EventTimeForNow();

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer.h (right):

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:24: ENABLE_MOUSE_EVENTS = 1
I suggest removing this for a number of technical reasons (comments below). 
However, when it comes to the user experience, do you ever NOT want to disable
the mouse cursor when there is no movement?

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:31: explicit
CursorRenderer(aura::Window* window,
Please comment that |window| is expected to out-live CursorRenderer.

However, for the ContentCaptureSubscription (tab capture case), I'm worried the
|window| could be destroyed somehow before the CursorRenderer.  Perhaps
CursorRenderer should also be a WindowObserver so that it can call
Window::RemovePreTargetHandler() when WindowObserver::OnWindowDestroying()?

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:32: bool enable_mouse_events =
false);
Just need the |window| argument.  If it's not null, do the mouse event
listening.  If null, don't.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:50: const bool
enable_mouse_events_;
ditto here: No need for a bool since |window_| will either be null or not null.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:56: enum {
nit: These can both be in one enum block.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:59: MAX_IDLE_TIME = 2
Please add units to naming of these two enums (i.e., ..._PIXELS and
..._SECONDS).

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:63: float
last_mouse_position_x_;
The two floats and bool need initialization in the ctor.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:326:
event_type_, damage_rect, present_time, storage, &capture_frame_cb_);
Unfortunately, you can't store this in a single |capture_frame_cb_| member
because there could be multiple captures in-flight at any one time.  Instead, I
suggest you pass the callback as an argument to
FrameSubscriber::DidCaptureFrame().

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:360:
cursor_renderer_.reset(new content::CursorRenderer(
nit: These can go back into the initializer list as:

  cursor_renderer_(...args...),
  timer_subscriber_(...args..., &cursor_renderer_),

...and then you don't need to use scoped_ptr<> types.

https://codereview.chromium.org/1412173003/diff/20001/content/public/browser/...
File content/public/browser/render_widget_host_view_frame_subscriber.h (right):

https://codereview.chromium.org/1412173003/diff/20001/content/public/browser/...
content/public/browser/render_widget_host_view_frame_subscriber.h:44: gfx::Rect
/* region_in_frame */,
Need to document this in the comments.  Something like "location within the
VideoFrame where the captured content resides, with the remaining parts of the
VideoFrame blacked-out."

[xjz, 2015-10-20 17:12:08]

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer.h (right):

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:24: ENABLE_MOUSE_EVENTS = 1
On 2015/10/20 02:12:44, miu wrote:
> I suggest removing this for a number of technical reasons (comments below). 
> However, when it comes to the user experience, do you ever NOT want to disable
> the mouse cursor when there is no movement?

I think we want to disable the mouse cursor while playing video. But may want to
show cursor during presentation even when there is no mouse movement.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:63: float
last_mouse_position_x_;
Are these two the mouse location? Why they are float?

[Irfan, 2015-10-21 23:01:08]

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer.cc (right):

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.cc:58: 
On 2015/10/20 02:12:44, miu wrote:
> Please include changes in: https://codereview.chromium.org/1413803002/

Done.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.cc:97: base::TimeDelta now =
base::TimeDelta::FromInternalValue(
On 2015/10/20 02:12:44, miu wrote:
> FromInternalValue() and ToInternalValue() are only supposed to be used for by
> serializing implementations, since they defeat the compile-time checking of
time
> value math.  Instead, you should:
> 
>   base::TimeDelta now = base::TimeTicks::Now() - base::TimeTicks();
> 
> But, for robustness, it'd be even better to use the utility function:
> 
>   #include "ui/events/event_utils.h"
>   ...
>   base::TimeDelta now = ui::EventTimeForNow();

Done.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer.h (right):

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:24: ENABLE_MOUSE_EVENTS = 1
On 2015/10/20 02:12:44, miu wrote:
> I suggest removing this for a number of technical reasons (comments below). 
> However, when it comes to the user experience, do you ever NOT want to disable
> the mouse cursor when there is no movement?

I did consider keeping the mouse rendering logic for all cases, but ended up
adding this as an afterthought to retain existing behavior in other cases. I
think we can keep the behavior consistent.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:24: ENABLE_MOUSE_EVENTS = 1
On 2015/10/20 17:12:08, xjz wrote:
> On 2015/10/20 02:12:44, miu wrote:
> > I suggest removing this for a number of technical reasons (comments below). 
> > However, when it comes to the user experience, do you ever NOT want to
disable
> > the mouse cursor when there is no movement?
> 
> I think we want to disable the mouse cursor while playing video. But may want
to
> show cursor during presentation even when there is no mouse movement.

Have gone with keeping the same code for all cases right now.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:31: explicit
CursorRenderer(aura::Window* window,
On 2015/10/20 02:12:44, miu wrote:
> Please comment that |window| is expected to out-live CursorRenderer.
> 
> However, for the ContentCaptureSubscription (tab capture case), I'm worried
the
> |window| could be destroyed somehow before the CursorRenderer.  Perhaps
> CursorRenderer should also be a WindowObserver so that it can call
> Window::RemovePreTargetHandler() when WindowObserver::OnWindowDestroying()?

Done.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:32: bool enable_mouse_events =
false);
On 2015/10/20 02:12:44, miu wrote:
> Just need the |window| argument.  If it's not null, do the mouse event
> listening.  If null, don't.

Done.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:56: enum {
On 2015/10/20 02:12:44, miu wrote:
> nit: These can both be in one enum block.

I had kept them separate since the values do not mean the same thing, but adding
units to name will clarify it

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:59: MAX_IDLE_TIME = 2
On 2015/10/20 02:12:44, miu wrote:
> Please add units to naming of these two enums (i.e., ..._PIXELS and
> ..._SECONDS).

Done.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:63: float
last_mouse_position_x_;
On 2015/10/20 02:12:44, miu wrote:
> The two floats and bool need initialization in the ctor.

Done.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:63: float
last_mouse_position_x_;
On 2015/10/20 17:12:08, xjz wrote:
> Are these two the mouse location? Why they are float?

those are the units in which MouseEVent tracks x and y

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:326:
event_type_, damage_rect, present_time, storage, &capture_frame_cb_);
On 2015/10/20 02:12:44, miu wrote:
> Unfortunately, you can't store this in a single |capture_frame_cb_| member
> because there could be multiple captures in-flight at any one time.  Instead,
I
> suggest you pass the callback as an argument to
> FrameSubscriber::DidCaptureFrame().

Done.

https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:360:
cursor_renderer_.reset(new content::CursorRenderer(
On 2015/10/20 02:12:44, miu wrote:
> nit: These can go back into the initializer list as:
> 
>   cursor_renderer_(...args...),
>   timer_subscriber_(...args..., &cursor_renderer_),
> 
> ...and then you don't need to use scoped_ptr<> types.

I retained this mainly because it depends on fetching the window

https://codereview.chromium.org/1412173003/diff/20001/content/public/browser/...
File content/public/browser/render_widget_host_view_frame_subscriber.h (right):

https://codereview.chromium.org/1412173003/diff/20001/content/public/browser/...
content/public/browser/render_widget_host_view_frame_subscriber.h:44: gfx::Rect
/* region_in_frame */,
On 2015/10/20 02:12:44, miu wrote:
> Need to document this in the comments.  Something like "location within the
> VideoFrame where the captured content resides, with the remaining parts of the
> VideoFrame blacked-out."

Done.

[Irfan, 2015-10-21 23:10:12]

Still no tests. PTAL.

[xjz, 2015-10-22 17:08:42]

lgtm
Some nits.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:7: #include
<algorithm>
No need to include this.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer_aura.cc (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.cc:77: 
nit: remove one blank line

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer_dummy.h (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_dummy.h:28: private:
nit: one blank line above.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:53: #include
<algorithm>
Still think no need to include this.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:634: // to
the thread pool used; for blocking operations.
nit: remove ';'.

[miu, 2015-10-23 01:55:13]

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:48: const
CursorRenderer* const cursor_renderer,
Since the CursorRenderer is owned by AuraWindowCaptureMachine, you should be
able to access it via |machine->cursor_renderer_|.  So, you don't need this
extra argument.  You'll have to make this function a private static function of
the AuraWindowCaptureMachine class to gain access to the private member.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:54:
cursor_renderer->RenderOnVideoFrame(target);
The |cursor_renderer| pointer is invalid if the AuraWindowCaptureMachine is
destroyed.  So, this should only be done if machine.get() does not return null
(see line 59 below).

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/aura_window_capture_machine.h (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.h:114:
scoped_ptr<content::CursorRenderer> cursor_renderer_;
IMO, the type should be scoped_ptr<CursorRendererAura> since this is an all-Aura
class.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer.h (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:18: // rendering on a video
frame
style nit: Add period at end of sentence.

It's worth noting in this comment that implementations will also listen to mouse
events in a platform-specific way.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:23: virtual void Clear() = 0;
Please document all methods (what they do and/or why they are called).

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:25: virtual bool Update(const
gfx::Rect& region_in_frame) = 0;
Please document.  Something like: "Determines whether the cursor will be
rendered, which cursor image to render, and at what location in
|region_in_frame| to render it."

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:25: virtual bool Update(const
gfx::Rect& region_in_frame) = 0;
naming consideration: SnapshotCursorState(), or something that says why/when
this method should be called.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer_aura.cc (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.cc:24: int clip_byte(int x) {
Suggestion: Use 'inline' keyword for clip_byte() and alpha_blend().

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.cc:66: DVLOG(2) << "Skipping
update with window empty";
Suggestion: s/window empty/no window being tracked./

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer_aura.h (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:33: ~CursorRendererAura()
override;
For all these methods, try to use 'final' instead of 'override' to help the
compiler/linked optimize virtual method calls in release builds.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:34: void Clear() override;
style: These three methods should be headed by a comment like so:

  // CursorRenderer implementation.
  void Clear() final;
  bool Update(...) final;
  void RenderOnVideoFrame(...) final;

...and ditto for OnMouseEvent() and OnWindowDestroying() below.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:39: // Overridden from
ui::EventHandler to make decision on when
style: No need to document overridden interface methods.  Instead, just:

  // ui::EventHandler overrides.
  void OnMouseEvent(...) final;

...and:

  // aura::WindowObserver overrides.
  void OnWindowDestroying(...) final;

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:46:
base::WeakPtr<CursorRenderer> GetWeakPtr() override;
This should be next to the other three methods being overridden for the
CursorRenderer interface.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:48: ui::Cursor
last_cursor_;
Please add a comment for these four members.  Like:

  // Snapshot of cursor, source size, position, and cursor bitmap; as of the
last call to Update().

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:52: gfx::NativeView
window_;
nit: |window_| should go first since it's injected via the ctor.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:54: enum {
Consider moving these values to the CursorRenderer class, since they will also
be needed for the Mac implementation.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:55: // Minium movement
before cursor is rendered on frame
style nit: In Chromium code, all comments must be complete sentences (and end
with a period).  Please fix here, and in all other files.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:62: base::TimeDelta
last_mouse_movement_timestamp_;
Short comment needed here too about when these are updated and how they are
used.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer_dummy.cc (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_dummy.cc:9: // TODO(isheriff):
When we do have a MAC implementation, rename dummy
Oh.  If this is why you wrote this class, you should name it
CursorRendererCocoa, and use a .mm extension (instead of .cc) so that the ObjC++
compiler will be used.  Then, in each method, you can add a NOTIMPLEMENTED();
statement.

However, it would also be fine it you didn't include this class in this CL, and
just introduce it later.  Your call.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer_dummy.h (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_dummy.h:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
Suggestion: Generally, we don't write "empty" implementations for Chrome builds.
 If you think you'll need this as a no-op mock for unit testing, then consider
naming it "FakeCursorRenderer" to be consistent with other code (and only
include it in test build targets in the .gyp and .gn files).  Otherwise, IMO,
you can just delete it.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:93: #include
"content/browser/media/capture/cursor_renderer_dummy.h"
Suggest you don't provide the #else clause.  See comments below.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:148:
media::ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb,
Please pass the callback by const reference.  The use of base::Bind() will
create a copy.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:151:
gfx::Rect region_in_frame,
The gfx::Rect should be passed by const reference as well.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:158:
base::WeakPtr<content::CursorRenderer> cursor_renderer_;
No need for content:: qualifier.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:158:
base::WeakPtr<content::CursorRenderer> cursor_renderer_;
Please document why we need a weak pointer to the CursorRenderer (because
FrameSubscriber is owned externally, and may outlive it; and if FrameSubscriber
owned it, there'd be two duplicate instances).

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:357: if
(frame_subscriber_->cursor_renderer_->Update(region_in_frame))
Some considerations on the timing of the call to Update():

In AuraWindowCaptureMachine, Update() is being called to snapshot the cursor and
its position before the YUV readback.  Here, in FrameSubscriber, it's being
called after readback.  Since the operations on the GPU could take up to 100 ms
(or more), you might be snapshotting significantly old data and drawing the
cursor in an old spot.

Going even deeper, maybe AuraWindowCaptureMachine is doing this wrong too. 
Perhaps the call to Update() for both AWCM and FrameSubscriber should be at the
moment the CopyOutputRequest is being issued?

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:380:
cursor_renderer_.reset(
Please remove the #else clause here.  In Chromium code, we try hard not to waste
memory (even for a simple no-op stub).  It's fine if |cursor_renderer_| is null.

For passing the WeakPtr to the frame subscriber constructors, you can do
something like:

  base::WeakPtr<CursorRenderer> weak_cursor_renderer;
#if defined(USE_AURA)
  cursor_renderer_.reset(new CursorRendererAura(...));
  weak_cursor_renderer_ = cursor_renderer_->GetWeakPtr();
#endif

https://codereview.chromium.org/1412173003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:430: aura::Window*
DelegatedFrameHostGetNativeView() const override;
I don't think you need this anymore.

[Irfan, 2015-10-23 19:15:28]

Thank you for the detailed comments!

[Irfan, 2015-10-26 23:10:32]

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:7: #include
<algorithm>
On 2015/10/22 17:08:41, xjz wrote:
> No need to include this.

see my earlier comment

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:48: const
CursorRenderer* const cursor_renderer,
On 2015/10/23 01:55:12, miu wrote:
> Since the CursorRenderer is owned by AuraWindowCaptureMachine, you should be
> able to access it via |machine->cursor_renderer_|.  So, you don't need this
> extra argument.  You'll have to make this function a private static function
of
> the AuraWindowCaptureMachine class to gain access to the private member.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.cc:54:
cursor_renderer->RenderOnVideoFrame(target);
On 2015/10/23 01:55:12, miu wrote:
> The |cursor_renderer| pointer is invalid if the AuraWindowCaptureMachine is
> destroyed.  So, this should only be done if machine.get() does not return null
> (see line 59 below).

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/aura_window_capture_machine.h (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/aura_window_capture_machine.h:114:
scoped_ptr<content::CursorRenderer> cursor_renderer_;
On 2015/10/23 01:55:12, miu wrote:
> IMO, the type should be scoped_ptr<CursorRendererAura> since this is an
all-Aura
> class.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer.h (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:18: // rendering on a video
frame
On 2015/10/23 01:55:12, miu wrote:
> style nit: Add period at end of sentence.
> 
> It's worth noting in this comment that implementations will also listen to
mouse
> events in a platform-specific way.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:23: virtual void Clear() = 0;
On 2015/10/23 01:55:12, miu wrote:
> Please document all methods (what they do and/or why they are called).

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:25: virtual bool Update(const
gfx::Rect& region_in_frame) = 0;
On 2015/10/23 01:55:12, miu wrote:
> naming consideration: SnapshotCursorState(), or something that says why/when
> this method should be called.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer.h:25: virtual bool Update(const
gfx::Rect& region_in_frame) = 0;
On 2015/10/23 01:55:12, miu wrote:
> Please document.  Something like: "Determines whether the cursor will be
> rendered, which cursor image to render, and at what location in
> |region_in_frame| to render it."

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer_aura.cc (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.cc:24: int clip_byte(int x) {
On 2015/10/23 01:55:12, miu wrote:
> Suggestion: Use 'inline' keyword for clip_byte() and alpha_blend().

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.cc:66: DVLOG(2) << "Skipping
update with window empty";
On 2015/10/23 01:55:12, miu wrote:
> Suggestion: s/window empty/no window being tracked./

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.cc:77: 
On 2015/10/22 17:08:42, xjz wrote:
> nit: remove one blank line

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer_aura.h (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:33: ~CursorRendererAura()
override;
On 2015/10/23 01:55:12, miu wrote:
> For all these methods, try to use 'final' instead of 'override' to help the
> compiler/linked optimize virtual method calls in release builds.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:34: void Clear() override;
On 2015/10/23 01:55:12, miu wrote:
> style: These three methods should be headed by a comment like so:
> 
>   // CursorRenderer implementation.
>   void Clear() final;
>   bool Update(...) final;
>   void RenderOnVideoFrame(...) final;
> 
> ...and ditto for OnMouseEvent() and OnWindowDestroying() below.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:39: // Overridden from
ui::EventHandler to make decision on when
On 2015/10/23 01:55:12, miu wrote:
> style: No need to document overridden interface methods.  Instead, just:
> 
>   // ui::EventHandler overrides.
>   void OnMouseEvent(...) final;
> 
> ...and:
> 
>   // aura::WindowObserver overrides.
>   void OnWindowDestroying(...) final;

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:46:
base::WeakPtr<CursorRenderer> GetWeakPtr() override;
On 2015/10/23 01:55:12, miu wrote:
> This should be next to the other three methods being overridden for the
> CursorRenderer interface.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:48: ui::Cursor
last_cursor_;
On 2015/10/23 01:55:12, miu wrote:
> Please add a comment for these four members.  Like:
> 
>   // Snapshot of cursor, source size, position, and cursor bitmap; as of the
> last call to Update().

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:52: gfx::NativeView
window_;
On 2015/10/23 01:55:12, miu wrote:
> nit: |window_| should go first since it's injected via the ctor.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:54: enum {
On 2015/10/23 01:55:12, miu wrote:
> Consider moving these values to the CursorRenderer class, since they will also
> be needed for the Mac implementation.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:55: // Minium movement
before cursor is rendered on frame
On 2015/10/23 01:55:12, miu wrote:
> style nit: In Chromium code, all comments must be complete sentences (and end
> with a period).  Please fix here, and in all other files.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_aura.h:62: base::TimeDelta
last_mouse_movement_timestamp_;
On 2015/10/23 01:55:12, miu wrote:
> Short comment needed here too about when these are updated and how they are
> used.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer_dummy.cc (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_dummy.cc:9: // TODO(isheriff):
When we do have a MAC implementation, rename dummy
On 2015/10/23 01:55:12, miu wrote:
> Oh.  If this is why you wrote this class, you should name it
> CursorRendererCocoa, and use a .mm extension (instead of .cc) so that the
ObjC++
> compiler will be used.  Then, in each method, you can add a NOTIMPLEMENTED();
> statement.
> 
> However, it would also be fine it you didn't include this class in this CL,
and
> just introduce it later.  Your call.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/cursor_renderer_dummy.h (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_dummy.h:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
On 2015/10/23 01:55:13, miu wrote:
> Suggestion: Generally, we don't write "empty" implementations for Chrome
builds.
>  If you think you'll need this as a no-op mock for unit testing, then consider
> naming it "FakeCursorRenderer" to be consistent with other code (and only
> include it in test build targets in the .gyp and .gn files).  Otherwise, IMO,
> you can just delete it.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/cursor_renderer_dummy.h:28: private:
On 2015/10/22 17:08:42, xjz wrote:
> nit: one blank line above.

doesnt exist anymore

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:53: #include
<algorithm>
On 2015/10/22 17:08:42, xjz wrote:
> Still think no need to include this.

This was a lint error unrelated to my code change

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:93: #include
"content/browser/media/capture/cursor_renderer_dummy.h"
On 2015/10/23 01:55:13, miu wrote:
> Suggest you don't provide the #else clause.  See comments below.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:148:
media::ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb,
On 2015/10/23 01:55:13, miu wrote:
> Please pass the callback by const reference.  The use of base::Bind() will
> create a copy.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:151:
gfx::Rect region_in_frame,
On 2015/10/23 01:55:13, miu wrote:
> The gfx::Rect should be passed by const reference as well.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:158:
base::WeakPtr<content::CursorRenderer> cursor_renderer_;
On 2015/10/23 01:55:13, miu wrote:
> No need for content:: qualifier.

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:158:
base::WeakPtr<content::CursorRenderer> cursor_renderer_;
On 2015/10/23 01:55:13, miu wrote:
> Please document why we need a weak pointer to the CursorRenderer (because
> FrameSubscriber is owned externally, and may outlive it; and if
FrameSubscriber
> owned it, there'd be two duplicate instances).

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:357: if
(frame_subscriber_->cursor_renderer_->Update(region_in_frame))
On 2015/10/23 01:55:13, miu wrote:
> Some considerations on the timing of the call to Update():
> 
> In AuraWindowCaptureMachine, Update() is being called to snapshot the cursor
and
> its position before the YUV readback.  Here, in FrameSubscriber, it's being
> called after readback.  Since the operations on the GPU could take up to 100
ms
> (or more), you might be snapshotting significantly old data and drawing the
> cursor in an old spot.
> 
> Going even deeper, maybe AuraWindowCaptureMachine is doing this wrong too. 
> Perhaps the call to Update() for both AWCM and FrameSubscriber should be at
the
> moment the CopyOutputRequest is being issued?

I went with a comment to see if it affects any scenarios. Doing it here prior to
capture requires figuring region_in_frame calculations and am not sure about
restructuring that since we wanted cursor rendering out of delegated_frame_host
in first place.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:380:
cursor_renderer_.reset(
On 2015/10/23 01:55:13, miu wrote:
> Please remove the #else clause here.  In Chromium code, we try hard not to
waste
> memory (even for a simple no-op stub).  It's fine if |cursor_renderer_| is
null.
> 
> For passing the WeakPtr to the frame subscriber constructors, you can do
> something like:
> 
>   base::WeakPtr<CursorRenderer> weak_cursor_renderer;
> #if defined(USE_AURA)
>   cursor_renderer_.reset(new CursorRendererAura(...));
>   weak_cursor_renderer_ = cursor_renderer_->GetWeakPtr();
> #endif

Done.

https://codereview.chromium.org/1412173003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/1412173003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:430: aura::Window*
DelegatedFrameHostGetNativeView() const override;
On 2015/10/23 01:55:13, miu wrote:
> I don't think you need this anymore.

Done.

[Irfan, 2015-10-28 23:33:07]

PTAL. I have added a few unit tests.

[Irfan, 2015-10-28 23:34:14]

isheriff@chromium.org changed reviewers:
+ sadrul@chromium.org

[Irfan, 2015-10-28 23:36:33]

Sadrul, I needed to override the include rule in DEPS for unit tests on the
CursorRenderer (similar to what is done for render_view_test.cc for access to
the same API)

[Irfan, 2015-10-28 23:36:35]

Sadrul, I needed to override the include rule in DEPS for unit tests on the
CursorRenderer (similar to what is done for render_view_test.cc for access to
the same API)

[miu, 2015-10-29 01:44:49]

lgtm  Nice tests!  Just a few other minor things:

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
File content/browser/media/capture/DEPS (right):

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
content/browser/media/capture/DEPS:7: ".*\.cc": [
This should be:

  ".*test\.cc"

(To ensure only test modules can use that header.)

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
File content/browser/media/capture/cursor_renderer_aura.h (right):

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.h:33: explicit
CursorRendererAura(gfx::NativeWindow window);
nit: Since this is Aura-specific code, let's use aura::Window* instead of
gfx::NativeWindow as the type here (and for the data member on line 52).

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.h:73:
DISALLOW_COPY_AND_ASSIGN(CursorRendererAura);
nit: Add a blank line above this one.

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
File content/browser/media/capture/cursor_renderer_aura_unittest.cc (right):

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura_unittest.cc:39: "en-US",
NULL, ui::ResourceBundle::LOAD_COMMON_RESOURCES);
nit: nullptr instead of NULL

[sadrul, 2015-10-29 06:22:41]

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
File content/browser/media/capture/DEPS (right):

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
content/browser/media/capture/DEPS:7: ".*\.cc": [
On 2015/10/29 01:44:49, miu wrote:
> This should be:
> 
>   ".*test\.cc"
> 
> (To ensure only test modules can use that header.)

+1 (or even '.*unittest\.cc')

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
File content/browser/media/capture/cursor_renderer_aura_unittest.cc (right):

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura_unittest.cc:93:
aura::Env::GetInstance()->set_last_mouse_location(point);
Most tests usually use EventGenerator for sending events like this. That way,
you don't need to manually update Env mouse-location.

[Irfan, 2015-10-29 15:34:19]

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
File content/browser/media/capture/DEPS (right):

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
content/browser/media/capture/DEPS:7: ".*\.cc": [
On 2015/10/29 06:22:41, sadrul wrote:
> On 2015/10/29 01:44:49, miu wrote:
> > This should be:
> > 
> >   ".*test\.cc"
> > 
> > (To ensure only test modules can use that header.)
> 
> +1 (or even '.*unittest\.cc')

Done.

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
content/browser/media/capture/DEPS:7: ".*\.cc": [
On 2015/10/29 01:44:49, miu wrote:
> This should be:
> 
>   ".*test\.cc"
> 
> (To ensure only test modules can use that header.)

Done.

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
File content/browser/media/capture/cursor_renderer_aura.h (right):

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.h:33: explicit
CursorRendererAura(gfx::NativeWindow window);
On 2015/10/29 01:44:49, miu wrote:
> nit: Since this is Aura-specific code, let's use aura::Window* instead of
> gfx::NativeWindow as the type here (and for the data member on line 52).

Done.

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.h:73:
DISALLOW_COPY_AND_ASSIGN(CursorRendererAura);
On 2015/10/29 01:44:49, miu wrote:
> nit: Add a blank line above this one.

Done.

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
File content/browser/media/capture/cursor_renderer_aura_unittest.cc (right):

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura_unittest.cc:39: "en-US",
NULL, ui::ResourceBundle::LOAD_COMMON_RESOURCES);
On 2015/10/29 01:44:49, miu wrote:
> nit: nullptr instead of NULL

Done.

https://codereview.chromium.org/1412173003/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura_unittest.cc:93:
aura::Env::GetInstance()->set_last_mouse_location(point);
On 2015/10/29 06:22:41, sadrul wrote:
> Most tests usually use EventGenerator for sending events like this. That way,
> you don't need to manually update Env mouse-location.

This allows me to stay in control of clock. Keeping this for now.

[sadrul, 2015-10-29 16:26:36]

There seem to be some unnecessary #includes. I have commented on them. Please
remove the ones not needed.

Please update the CL description to refer to a crbug in the BUG= line.

lgtm

https://codereview.chromium.org/1412173003/diff/120001/content/browser/compos...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/1412173003/diff/120001/content/browser/compos...
content/browser/compositor/delegated_frame_host.cc:10: 
The new code in this file don't actually seem to use anything from these?

https://codereview.chromium.org/1412173003/diff/120001/content/browser/compos...
File content/browser/compositor/delegated_frame_host.h (right):

https://codereview.chromium.org/1412173003/diff/120001/content/browser/compos...
content/browser/compositor/delegated_frame_host.h:27: #include
"ui/events/event.h"
Looks like vector or this aren't necessary?

https://codereview.chromium.org/1412173003/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1412173003/diff/120001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:9: 
ditto

https://codereview.chromium.org/1412173003/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/1412173003/diff/120001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:8: #include
<vector>
This isn't necessary?

https://codereview.chromium.org/1412173003/diff/120001/content/browser/media/...
File content/browser/media/capture/aura_window_capture_machine.cc (right):

https://codereview.chromium.org/1412173003/diff/120001/content/browser/media/...
content/browser/media/capture/aura_window_capture_machine.cc:8: 
not used?

[IrfanGoogle, 2015-10-29 16:29:23]

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

[IrfanGoogle, 2015-10-29 16:29:24]

All of these includes are a result of me running "git cl lint" on my commit and
trying to fix them. It is for other places in the code.

[Irfan, 2015-11-02 20:46:28]

Patchset #9 (id:160001) has been deleted

[Irfan, 2015-11-02 20:46:43]

Patchset #9 (id:180001) has been deleted

[Irfan, 2015-11-02 20:46:52]

Patchset #12 (id:260001) has been deleted

[Irfan, 2015-11-02 20:46:58]

Patchset #11 (id:240001) has been deleted

[Irfan, 2015-11-02 20:47:03]

Patchset #10 (id:220001) has been deleted

[Irfan, 2015-11-02 20:47:10]

Patchset #9 (id:200001) has been deleted

[Irfan, 2015-11-02 20:54:21]

Description was changed from

==========
cast: support cursor rendering for tab capture

This refactors the current cursor rendering being done by
aura window capture machine and adds support for mouse cursor display for
both window/desktop and tab mirroring

In cast of tab capture, the mouse is rendered only when user is interacting with
the tab being mirrored and stops showing when user stops
interacting.

BUG=
==========

to

==========
cast: support cursor rendering for tab capture

This refactors the current cursor rendering being done by
aura window capture machine and adds support for mouse cursor display for
both window/desktop and tab mirroring

In cast of tab capture, the mouse is rendered only when user is interacting with
the tab being mirrored and stops showing when user stops
interacting.

BUG=550555
==========

[Irfan, 2015-11-03 18:18:54]

Patchset #10 (id:300001) has been deleted

[Irfan, 2015-11-03 18:19:57]

Patchset #10 (id:310001) has been deleted

[Irfan, 2015-11-03 18:20:07]

Patchset #10 (id:330001) has been deleted

[Irfan, 2015-11-03 18:20:13]

Patchset #10 (id:350001) has been deleted

[Irfan, 2015-11-03 18:21:00]

Patchset #9 (id:280001) has been deleted

[Irfan, 2015-11-03 18:25:44]

miu@, PTAL as besides the test and build fixes, I had to make a change to
RenderVideoFrame() in web_contents_video_capture_device.cc to ensure callbacks
are on browser thread.

[miu, 2015-11-06 00:22:54]

https://codereview.chromium.org/1412173003/diff/370001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1412173003/diff/370001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:366: if
(frame_subscriber_ && frame_subscriber_->cursor_renderer_ && success) {
Don't need the "&& success" at the end anymore.

https://codereview.chromium.org/1412173003/diff/370001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:449: class
ScopedClosureRunOnBrowserThread {
There's a common solution to this problem: media::BindToCurrentLoop().

What I suggest is, in WebContentsCaptureMachine::DidCopyFromBackingStore(), make
the render_thread_->PostTask() statement look like this:

#include "media/base/bind_to_current_loop.h"

render_thread_->task_runner()->PostTask(
    FROM_HERE,
    media::BindToCurrentLoop(
        base::Bind(&RenderVideoFrame, bitmap, target,
                              base::Bind(deliver_frame_cb, start_time))));

This puts an implicit "trampoline" back to the main thread when the callback is
run.  Then, you can just use the base::ScopedClosureRunner in RenderVideoFrame()
like it was before.

https://codereview.chromium.org/1412173003/diff/370001/content/content_tests....
File content/content_tests.gypi (right):

https://codereview.chromium.org/1412173003/diff/370001/content/content_tests....
content/content_tests.gypi:1214: # Avoid windows due to non-availability of
cursor resources in test.
Please put this comment in the BUILD.gn file as well.

[Irfan, 2015-11-06 17:49:26]

https://codereview.chromium.org/1412173003/diff/370001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1412173003/diff/370001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:366: if
(frame_subscriber_ && frame_subscriber_->cursor_renderer_ && success) {
On 2015/11/06 00:22:54, miu wrote:
> Don't need the "&& success" at the end anymore.

Done.

https://codereview.chromium.org/1412173003/diff/370001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:449: class
ScopedClosureRunOnBrowserThread {
On 2015/11/06 00:22:54, miu wrote:
> There's a common solution to this problem: media::BindToCurrentLoop().
> 
> What I suggest is, in WebContentsCaptureMachine::DidCopyFromBackingStore(),
make
> the render_thread_->PostTask() statement look like this:
> 
> #include "media/base/bind_to_current_loop.h"
> 
> render_thread_->task_runner()->PostTask(
>     FROM_HERE,
>     media::BindToCurrentLoop(
>         base::Bind(&RenderVideoFrame, bitmap, target,
>                               base::Bind(deliver_frame_cb, start_time))));
> 
> This puts an implicit "trampoline" back to the main thread when the callback
is
> run.  Then, you can just use the base::ScopedClosureRunner in
RenderVideoFrame()
> like it was before.

Aha, excellent

https://codereview.chromium.org/1412173003/diff/370001/content/content_tests....
File content/content_tests.gypi (right):

https://codereview.chromium.org/1412173003/diff/370001/content/content_tests....
content/content_tests.gypi:1214: # Avoid windows due to non-availability of
cursor resources in test.
On 2015/11/06 00:22:54, miu wrote:
> Please put this comment in the BUILD.gn file as well.

Done.

[Irfan, 2015-11-06 19:02:31]

Rebased. PTAL

[miu, 2015-11-06 19:38:17]

lgtm

[miu, 2015-11-06 19:39:11]

The CQ bit was checked by miu@chromium.org

[miu, 2015-11-06 19:39:12]

The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1412173003/#ps310002
(title: "Rebased")

[commit-bot: I haz the power, 2015-11-06 20:22:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412173003/310002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412173003/310002

[commit-bot: I haz the power, 2015-11-06 20:49:39]

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

[commit-bot: I haz the power, 2015-11-06 20:49:40]

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

[Irfan, 2015-11-06 22:31:09]

Apparently I need LGTM. Should I be just picking someone from OWNERS in
content/:

Missing LGTM from an OWNER for these files:
    content/browser/BUILD.gn
    content/browser/compositor/delegated_frame_host.cc
    content/browser/compositor/delegated_frame_host.h
    content/browser/frame_host/render_widget_host_view_child_frame.cc
    content/browser/frame_host/render_widget_host_view_child_frame.h
    content/public/browser/render_widget_host_view_frame_subscriber.h
    content/test/BUILD.gn
    content/test/test_render_view_host.cc
    content/test/test_render_view_host.h

?

[Irfan, 2015-11-09 16:43:02]

isheriff@chromium.org changed reviewers:
+ brettw@chromium.org

[Irfan, 2015-11-09 16:43:19]

Brett, can you help with LGTM ?

[Irfan, 2015-11-09 21:41:22]

isheriff@chromium.org changed reviewers:
+ avi@chromium.org
- brettw@chromium.org

[miu, 2015-11-10 01:26:50]

avi: Need OWNERS LGTM for these files:

    content/browser/BUILD.gn
    content/browser/compositor/delegated_frame_host.cc
    content/browser/compositor/delegated_frame_host.h
    content/browser/frame_host/render_widget_host_view_child_frame.cc
    content/browser/frame_host/render_widget_host_view_child_frame.h
    content/public/browser/render_widget_host_view_frame_subscriber.h
    content/test/BUILD.gn
    content/test/test_render_view_host.cc
    content/test/test_render_view_host.h

(Interface change to RWHVFrameSubscriber.)  Thanks!  :)

[Avi (use Gerrit), 2015-11-10 02:14:47]

lgtm

stampity stamp

[Irfan, 2015-11-10 05:35:33]

The CQ bit was checked by isheriff@chromium.org

[commit-bot: I haz the power, 2015-11-10 05:35:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412173003/310002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412173003/310002

[commit-bot: I haz the power, 2015-11-10 05:45:28]

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

[commit-bot: I haz the power, 2015-11-10 05:45:31]

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

[Irfan, 2015-11-10 19:50:07]

Patchset #12 (id:420001) has been deleted

[Irfan, 2015-11-10 21:16:59]

The CQ bit was checked by isheriff@chromium.org

[Irfan, 2015-11-10 21:17:00]

The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org,
avi@chromium.org, sadrul@chromium.org, miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1412173003/#ps440001
(title: "Disable cursor rendering on windows until resources are available")

[commit-bot: I haz the power, 2015-11-10 21:17:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412173003/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412173003/440001

[Irfan, 2015-11-10 21:17:38]

The CQ bit was unchecked by isheriff@chromium.org

[Irfan, 2015-11-10 21:18:33]

PTAL. I have turned off the feature on Windows until I can talk to the Windows
UX folks and figure out what needs to be done to get the resources be available.

[miu, 2015-11-10 23:15:58]

lgtm

[Irfan, 2015-11-10 23:31:32]

The CQ bit was checked by isheriff@chromium.org

[commit-bot: I haz the power, 2015-11-10 23:34:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412173003/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412173003/440001

[commit-bot: I haz the power, 2015-11-10 23:47:07]

Committed patchset #12 (id:440001)

[commit-bot: I haz the power, 2015-11-10 23:48:10]

Patchset 12 (id:??) landed as
https://crrev.com/727606cdcd89919585d5decf3579ef427adced98
Cr-Commit-Position: refs/heads/master@{#358958}

[thembrown, 2016-01-11 16:24:23]

On 2015/10/20 17:12:08, xjz wrote:
>
https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
> File content/browser/media/capture/cursor_renderer.h (right):
> 
>
https://codereview.chromium.org/1412173003/diff/20001/content/browser/media/c...
> content/browser/media/capture/cursor_renderer.h:24: ENABLE_MOUSE_EVENTS = 1
> On 2015/10/20 02:12:44, miu wrote:
> > I suggest removing this for a number of technical reasons (comments below). 
> > However, when it comes to the user experience, do you ever NOT want to
disable
> > the mouse cursor when there is no movement?
> 
> I think we want to disable the mouse cursor while playing video. But may want
to
> show cursor during presentation even when there is no mouse movement.

I agree. I might be biased here by our rather unusual use-case
(https://crbug.com/550555#c5), but I think the same concerns also apply for
presentations with Google Cast, whenever the cursor is literally used as a
pointer. 
In our case the new behavior makes it even harder to work around this, due to
the added unpredictability. I'd encourage to make the the mouse-hiding behavior
configurable or less aggressive (e.g. for full-screen videos only)

Sorry for bumping this old thread up again.