Fix cursor missing in tabCapture on OSX Sierra

Fix cursor missing in tabCapture on OSX Sierra

Since OSX Sierra, when we ask for the CGImage of a cursor image, it will return
a double-size CGImage with Retina display, which can't pass the sanity check.

=========================(deprecated)==================================
In this cl, we try to pass a half-size Rect hint to get CGImage with original
cursor size on both Retina and non-Retina display.
Also it fixs another issue that we render the cursor based on window coordinate
to a target in physical coordinate.
======================================================================

In this cl, we implemented a new common base class, CursorRenderer , to
merge the two implementations. Then we can handle the scaling and
rendering of cursor same on all platforms, also with other advantages:
caching scaled cursor image for better performance and only caring
about view coordinates (same handling to Retina and non-Retina). 

BUG=659183
Review-Url: https://codereview.chromium.org/2553763002
Cr-Commit-Position: refs/heads/master@{#441975}
Committed: https://chromium.googlesource.com/chromium/src/+/2f9c8630f029d251af62445f88357d0edbf0543c

[braveyao, 2016-12-05 18:31:51]

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

[commit-bot: I haz the power, 2016-12-05 18:32:50]

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-05 19:36:06]

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

[commit-bot: I haz the power, 2016-12-05 19:36:07]

Dry run: This issue passed the CQ dry run.

[braveyao, 2016-12-05 19:42:47]

braveyao@chromium.org changed reviewers:
+ miu@chromium.org

[braveyao, 2016-12-05 19:42:48]

Hi Miu, please take a look.

[miu, 2016-12-06 22:06:23]

I went through reviewing the changes, but then I realized something: There's a
ton of duplicated code between CursorRendererAura and CursorRendererMac. That
should be rectified first, since we want SnapshotCursorState() and the rendering
of the cursor in the VideoFrame to be identical; and we don't want to have to
maintain two copies of the same code (the problem we have now).

Therefore, before proceeding with this change, I would prefer you to merge the
two implementations into a common base class. Then, each subclass would only
need to implement methods that provide platform-specific functionality.

FWICT, CursorRendererAura looks like it's in a much cleaner state, so I would
suggest moving all its non-platform-specific implementation into CursorRenderer.
Then, CursorRenderer would require subclasses to implement the platform-specific
functionality. Here's what I think this would look like:

  class CursorRenderer {
   public:
    CursorRenderer(gfx::NativeView captured_view,
                   CursorDisplaySetting cursor_display);

    // Note: These are not overridden in the subclasses anymore.
    void Clear();
    void SnapshotCursorState(...);
    void RenderOnVideoFrame(...);

   protected:
    gfx::NativeView captured_view() const { return captured_view_; }

    // Returns true if the captured view is a part of an active application
window.
    virtual bool IsCapturedViewActive() = 0;

    // Returns the size of the captured view (view coordinates).
    virtual gfx::Size GetCapturedViewSize() = 0;

    // Returns the cursor's position within the captured view (view
coordinates).
    virtual gfx::Point GetCursorPositionInView() = 0;

    // Returns the last-known mouse cursor.
    virtual gfx::NativeCursor GetLastKnownCursor() = 0;

    // Returns the image of the last-known mouse cursor.
    virtual SkBitmap GetLastKnownCursorImage() = 0;

    // Returns the hot point (in the cursor image) of the last-known mouse
cursor.
    virtual gfx::Point GetLastKnownCursorHotPoint() = 0;

    // Called by subclasses to report mouse events within the captured view.
    void OnMouseMoved(const gfx::Point& location, base::TimeTicks timestamp);
    void OnMouseClicked(const gfx::Point& location, base::TimeTicks timestamp);

   private:
    ...private members that were in CursorRendererAura go here...
  };

Note: gfx::NativeView can be used in the common base class implementation
because it is typedef'ed to the correct windowing toolkit data types on Aura
versus Mac (see ui/gfx/native_widget_types.h).

For the record, here are comments I made on patch set 1:

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
File content/browser/media/capture/cursor_renderer_mac.mm (right):

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
content/browser/media/capture/cursor_renderer_mac.mm:85: NSPoint
mouse_tab_location =
This variable must be renamed. Code in src/content does not know of the concept
of "browser tabs."

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
content/browser/media/capture/cursor_renderer_mac.mm:93: float x_scale =
region_in_frame.width() / frame_rect.size.width;
Please add divide-by-zero checks here, since it's not uncommon for a view to be
an empty size. If this happens, there probably should be an early return.

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
content/browser/media/capture/cursor_renderer_mac.mm:95: 
Instead of multiplying by these scaling factors everywhere below, how about just
creating a point here:

  const NSPoint mouse_region_location =
      NSMakePoint(mouse_tab_location.x * region_in_frame.width() /
frame_rect.size.width,
                  mouse_tab_location.y * region_in_frame.height() /
frame_rect.size.height);

...and using that instead of mouse_tab_location everywhere below.

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
content/browser/media/capture/cursor_renderer_mac.mm:133: last_cursor_width_ =
nssize.width * x_scale;
To simplify the code after this point, let's make last_cursor_width_ and
last_cursor_height_ be in terms of the video frame's coordinate system.

  const float device_scale_factor = ui::GetScaleFactorForNativeView(view_);
  last_cursor_width_ = nssize.width / device_scale_factor * x_scale;
  last_cursor_height_ = nssize.height / device_scale_factor * y_scale;

In fact, doing this would seem to automatically fix a bug with how these values
are used in RenderOnVideoFrame().

To help code readability, feel free to rename these to
last_cursor_[width|height]_in_frame_.

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
content/browser/media/capture/cursor_renderer_mac.mm:145: NSRect image_rect =
NSMakeRect(0, 0, nssize.width / 2, nssize.height / 2);
Instead of doing this here, just allow the higher-resolution cursor to be
grabbed. The ResizeCGImage() call below will always do the right thing if it
scales down to last_cursor_width_ and height_.

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
content/browser/media/capture/cursor_renderer_mac.mm:158: if (x_scale != 1.0f ||
y_scale != 1.0f) {
Now, the only reason to resize is if the cursor image size does not match
last_cursor_width_ and last_cursor_height_:

  if (nssize.width != last_cursor_width_ || nssize.height !=
last_cursor_height_) {
    ...ResizeCGImage...
  }

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
content/browser/media/capture/cursor_renderer_mac.mm:160: cg_image =
ResizeCGImage(cg_image, last_cursor_width_, last_cursor_height_);
This is not super-expensive, but not cheap either. How about caching, just like
in cursor_renderer_aura.h/.cc?

[braveyao, 2016-12-07 00:56:03]

Hi Miu,

Huge thanks for the review comments! Learn a lot from them. Will give a try to
the refactoring.

Have questions to two of the comments, could you please help clarify? Thanks
again!

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
File content/browser/media/capture/cursor_renderer_mac.mm (right):

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
content/browser/media/capture/cursor_renderer_mac.mm:133: last_cursor_width_ =
nssize.width * x_scale;
On 2016/12/06 22:06:23, miu wrote:
> To simplify the code after this point, let's make last_cursor_width_ and
> last_cursor_height_ be in terms of the video frame's coordinate system.
> 
>   const float device_scale_factor = ui::GetScaleFactorForNativeView(view_);
>   last_cursor_width_ = nssize.width / device_scale_factor * x_scale;
>   last_cursor_height_ = nssize.height / device_scale_factor * y_scale;
> 
> In fact, doing this would seem to automatically fix a bug with how these
values
> are used in RenderOnVideoFrame().
> 
> To help code readability, feel free to rename these to
> last_cursor_[width|height]_in_frame_.

I don't think we should divide the |nssize| by |device_scale_factor| here. The
problem of this issue is 
 - |nssize| is always the original size on any Mac device, i.e. 17x23 on my MBP.
 - the corresponding CGImage of |nsimage| is 2X size on Sierra+Retina, and still
1X size on others.
So the cursor size in frame should be nssize x scale.

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
content/browser/media/capture/cursor_renderer_mac.mm:160: cg_image =
ResizeCGImage(cg_image, last_cursor_width_, last_cursor_height_);
On 2016/12/06 22:06:23, miu wrote:
> This is not super-expensive, but not cheap either. How about caching, just
like
> in cursor_renderer_aura.h/.cc?

What does 'caching' mean?

I suppose what I should do for the new class is to convert CGImage to SKBitmap
and call SKBitmap.Resize() method to scale. Right?

[miu, 2016-12-07 20:53:44]

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
File content/browser/media/capture/cursor_renderer_mac.mm (right):

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
content/browser/media/capture/cursor_renderer_mac.mm:133: last_cursor_width_ =
nssize.width * x_scale;
On 2016/12/07 00:56:03, braveyao wrote:
> On 2016/12/06 22:06:23, miu wrote:
> > To simplify the code after this point, let's make last_cursor_width_ and
> > last_cursor_height_ be in terms of the video frame's coordinate system.
> > 
> >   const float device_scale_factor = ui::GetScaleFactorForNativeView(view_);
> >   last_cursor_width_ = nssize.width / device_scale_factor * x_scale;
> >   last_cursor_height_ = nssize.height / device_scale_factor * y_scale;
> > 
> > In fact, doing this would seem to automatically fix a bug with how these
> values
> > are used in RenderOnVideoFrame().
> > 
> > To help code readability, feel free to rename these to
> > last_cursor_[width|height]_in_frame_.
> 
> I don't think we should divide the |nssize| by |device_scale_factor| here. The
> problem of this issue is 
>  - |nssize| is always the original size on any Mac device, i.e. 17x23 on my
MBP.
>  - the corresponding CGImage of |nsimage| is 2X size on Sierra+Retina, and
still
> 1X size on others.
> So the cursor size in frame should be nssize x scale. 

Sounds good. I didn't realize nssize was in view coordinates.

https://codereview.chromium.org/2553763002/diff/1/content/browser/media/captu...
content/browser/media/capture/cursor_renderer_mac.mm:160: cg_image =
ResizeCGImage(cg_image, last_cursor_width_, last_cursor_height_);
On 2016/12/07 00:56:03, braveyao wrote:
> On 2016/12/06 22:06:23, miu wrote:
> > This is not super-expensive, but not cheap either. How about caching, just
> like
> > in cursor_renderer_aura.h/.cc?
> 
> What does 'caching' mean?

I mean 'memoize': Only resize the image whenever the cursor or the required size
changes. Hold a reference to the scaled cursor image and re-use it instead of
re-computing it every time.

> I suppose what I should do for the new class is to convert CGImage to SKBitmap
> and call SKBitmap.Resize() method to scale. Right? 

Yes. Something like:

#include "ui/gfx/image/image.h"

class CursorRendererMac {
 private:
  gfx::Image cursor_image_;
};

SkBitmap CursorRendererMac::GetLastKnownCursorImage() {
  ...
  if (cursor image changed) {
    cursor_image_ = gfx::Image(nsimage);
  }
  return *(cursor_image.ToSkBitmap());
}

...and then the base class would handle the scaling of the SkBitmap (prob with
this code:
https://cs.chromium.org/chromium/src/content/browser/media/capture/cursor_ren...).

[braveyao, 2016-12-14 22:18:35]

Patchset #2 (id:20001) has been deleted

[braveyao, 2016-12-20 21:30:12]

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

[commit-bot: I haz the power, 2016-12-20 21:30:40]

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

[braveyao, 2016-12-20 21:40:51]

Description was changed from

==========
Fix cursor missing in tabCapture on OSX Sierra

Since OSX Sierra, when we ask for the CGImage of a cursor image, it will return
a double-size CGImage with Retina display, which can't pass the sanity check.

In this cl, we try to pass a half-size Rect hint to get CGImage with original
cursor size on both Retina and non-Retina display.
Also it fixs another issue that we render the cursor based on window coordinate
to a target in physical coordinate.

BUG=659183
==========

to

==========
Fix cursor missing in tabCapture on OSX Sierra

Since OSX Sierra, when we ask for the CGImage of a cursor image, it will return
a double-size CGImage with Retina display, which can't pass the sanity check.

=========================(deprecated)==================================
In this cl, we try to pass a half-size Rect hint to get CGImage with original
cursor size on both Retina and non-Retina display.
Also it fixs another issue that we render the cursor based on window coordinate
to a target in physical coordinate.
======================================================================

In this cl, we implemented a new common base class, CursorRenderer , to
merge the two implementations. Then we can handle the scaling and
rendering of cursor same on all platforms, also with other advantages: caching
scaled cursor image for better performance and only caring
about view coordinates (same handling to Retina and non-Retina). 

BUG=659183
==========

[braveyao, 2016-12-20 22:32:07]

Hi miu@, thanks for the advice! Now have CursorRenderer done and it's really
better than before. PTAL.

BTW: the problem that cursur hotspot is not accurate in Aura will be followed up
in a separate cl.

[commit-bot: I haz the power, 2016-12-20 22:43:34]

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

[commit-bot: I haz the power, 2016-12-20 22:43:35]

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

[braveyao, 2016-12-21 20:31:00]

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

[braveyao, 2016-12-21 20:31:11]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-12-21 20:31:56]

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-21 22:18:37]

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

[commit-bot: I haz the power, 2016-12-21 22:18:38]

Dry run: This issue passed the CQ dry run.

[braveyao, 2016-12-22 01:25:02]

Description was changed from

==========
Fix cursor missing in tabCapture on OSX Sierra

Since OSX Sierra, when we ask for the CGImage of a cursor image, it will return
a double-size CGImage with Retina display, which can't pass the sanity check.

=========================(deprecated)==================================
In this cl, we try to pass a half-size Rect hint to get CGImage with original
cursor size on both Retina and non-Retina display.
Also it fixs another issue that we render the cursor based on window coordinate
to a target in physical coordinate.
======================================================================

In this cl, we implemented a new common base class, CursorRenderer , to
merge the two implementations. Then we can handle the scaling and
rendering of cursor same on all platforms, also with other advantages: caching
scaled cursor image for better performance and only caring
about view coordinates (same handling to Retina and non-Retina). 

BUG=659183
==========

to

==========
Fix cursor missing in tabCapture on OSX Sierra

Since OSX Sierra, when we ask for the CGImage of a cursor image, it will return
a double-size CGImage with Retina display, which can't pass the sanity check.

=========================(deprecated)==================================
In this cl, we try to pass a half-size Rect hint to get CGImage with original
cursor size on both Retina and non-Retina display.
Also it fixs another issue that we render the cursor based on window coordinate
to a target in physical coordinate.
======================================================================

In this cl, we implemented a new common base class, CursorRenderer , to
merge the two implementations. Then we can handle the scaling and
rendering of cursor same on all platforms, also with other advantages:
caching scaled cursor image for better performance and only caring
about view coordinates (same handling to Retina and non-Retina). 

BUG=659183
==========

[braveyao, 2016-12-22 18:02:39]

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

[commit-bot: I haz the power, 2016-12-22 18:02:54]

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-22 18:57:20]

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

[commit-bot: I haz the power, 2016-12-22 18:57:21]

Dry run: This issue passed the CQ dry run.

[braveyao, 2016-12-22 19:00:21]

Patchset #3 (id:80001) has been deleted

[miu, 2016-12-27 23:21:03]

Looks great!

Comments on PS3: A bunch of small things, plus we seem to need mouse movement
tracking on Mac:

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

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.cc:11: #if defined(USE_AURA)
It seems these platform-specific includes should be removed.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.cc:63: DVLOG(2) << "Skipping
update with no window being tracked";
nit: For consistency, let's call it a "view" instead of a "window" here and
everywhere else. The "window" terminology referred to an Aura Window (which
corresponds to a view). I know, confusing. ;-)

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.cc:77: gfx::NativeCursor cursor =
GetLastKnownCursor();
Please move this down to just before it will first be used. (Since we will only
use this once we know the cursor should be displayed.)

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.cc:78: gfx::Point cursor_position
= GetCursorPositionInView();
Please move this down to just before it will first be used. (Since there is no
reason to invoke the method if |view_size| is empty.)

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.cc:87: if (cursor_position.x() < 0
|| cursor_position.y() < 0 ||
nit: A trick to make this simpler:

  if (!gfx::Rect(view_size).Contains(cursor_position)) {
    ...
    return false;
  }

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.cc:115:
window_size_when_cursor_last_updated_ != view_size) {
The scaled size also depends on |region_in_frame.size()|, so it seems this
should be:

  if (last_cursor_ != cursor ||
      window_size_when_cursor_last_updated_ != view_size ||
      last_region_in_frame_size_ != region_in_frame.size()) {
    ...
  }

However, from the code within the then-block, only |x_scale| and |y_scale|
affect the result, so this would be simpler:

  if (last_cursor_ != cursor || last_x_scale_ != x_scale ||
      last_y_scale_ != y_scale) {
    ...
  }

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

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.h:28: // CursorRenderer is an
interface that can be implememented to do cursor
This class comment needs to be updated, since this class is now an abstract base
class that handles all the non-platform-specific common cursor rendering
functionality.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.h:53: void
RenderOnVideoFrame(const scoped_refptr<media::VideoFrame>& target) const;
style nit (per smart pointer guidelines): The old code did this wrong. Could you
fix the 1st argument to just be a raw pointer type (media::VideoFrame*), since
the method will not hold a reference to |target|?

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.h:92: gfx::NativeView
captured_view_;
nit: Use const since this member doesn't change after construction:

  const gfx::NativeView captured_view_;

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.h:99: gfx::Size
window_size_when_cursor_last_updated_;
naming nit: How about |view_size_when_cursor_last_updated_|?

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.h:117:
DISALLOW_COPY_AND_ASSIGN(CursorRenderer);
nit: Please add a blank line above this line.

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

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.cc:7: #include
"ui/aura/env.h"
Please add:

#include "base/memory/ptr_util.h"

(since you use base::MakeUnique in this module)

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.cc:39: if
(cursor_display_setting_ == kCursorEnabledOnMouseMovement)
It's acceptable to just always call window_->RemovePreTargetHandler(this). Then,
you wouldn't need |cursor_display_setting_| anymore (see comment in .h file).

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.cc:44: bool
CursorRendererAura::IsCapturedViewActive() {
Please add a null-check for |window_| (and return false if null).

Also, this seems to be needed for all the other methods.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.cc:87: return
last_cursor_hot_point_;
The result of this method depends on whether GetLastKnownCursorImage() is called
first. Since they are intricately linked (because of the single call to
ui::GetCursorBitmap()), I'm thinking maybe the base class should declare only
one method to get the image+hot_point from the subclasses. So, instead of this
(in the base class):

  virtual SkBitmap GetLastKnownCursorImage() = 0;
  virtual gfx::Point GetLastKnownCursorHotPoint() = 0;

Just combine these into a single method:

  virtual SkBitmap GetLastKnownCursorImage(gfx::Point* hot_point) = 0;

(Then, you can delete the |last_cursor_hot_point_| member from this subclass.)

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

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.h:14: // Tracks state for
making decisions on cursor display on a captured video
This class comment is incomplete. However, all the functionality should be
documented in the new base class. So, here, maybe something as simple as "Aura
platform-specific implementation of CursorRenderer." or no class comment needed
at all?

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.h:40: gfx::Point
last_cursor_hot_point_;
You can remove this member (see comments in .cc file).

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.h:43: const
CursorDisplaySetting cursor_display_setting_;
ditto: You can remove this member too.

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

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.h:12: // Tracks state for
making decisions on cursor display on a captured video
ditto: Class comment needs updating. (or just remove it, like
CursorRendererAura?)

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
File content/browser/media/capture/cursor_renderer_mac.mm (right):

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:45: // Mouse location with
respect to the web contents.
nit: s/the web contents/the view within the window/

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:49: // Revert y coordinate
to unify with Aura.
s/Revert/Invert/

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:54: // Update cursor
movement info to CursorRenderer.
This is an unexpected side-effect of calling GetCursorPositionInView(). Could
you instead use a mouse event listener, like in CursorRendererAura? To do this
in the Mac/Cocoa world, you'll need to add a "tracking area" to the
|captured_view_| in CursorRendererMac's ctor.

References:

https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Ev...

https://developer.apple.com/reference/appkit/nstrackingarea

I think, for our purposes, the NSTrackingArea would be created/added this way.
In the ctor:

  CursorRendererMac(...) : ...,
    // Note: All of |captured_view_|, |tracker_| and |tracking_area_| should be
base::scoped_nsobject<T> to ensure these objects remain alive until the dtor is
called.
    tracker_([[CursorRendererMouseTracker alloc] initWithOwner:this]),
    tracking_area_(
        [[NSTrackingArea alloc] initWithRect:NSZeroRect
             options: (NSTrackingMouseMoved | NSTrackingActiveAlways |
                       NSTrackingInVisibleRect)
             owner:tracker_.get()
             userInfo:nil]) {
    [captured_view_ addTrackingArea:tracking_area_.get()];
  }

And, in the dtor:

  [captured_view_ removeTrackingArea:tracking_area_.get()];

Also, you'll need to create an Objective-C class that will receive the
|-mouseMoved| method calls for the tracking area and pass them back to
CursorRendererMac:

  @interface CursorRendererMouseTracker
  - (id)initWithOwner:(CursorRendererMac* owner);
  - (void)mouseMoved:(NSEvent*)event;
  @private
    CursorRendererMac* owner_;
  @end

  @implementation CursorRendererMouseTracker

  - (id)initWithOwner:(CursorRendererMac* owner) {
    self = [super init];
    if (self)
      owner_ = owner;
    return self;
  }

  - (void)mouseMoved:(NSEvent*)theEvent {
    NSPoint locationInView = [captured_view_ convertPoint:[theEvent
locationInWindow] fromView:nil];
    // Invert Y coord too???
    owner_->OnMouseMoved(gfx::Point(locationInView.x, locationInView.y),
                         ui::EventTimeFromNative(theEvent));
  }

  @end  // @implementation CursorRendererMouseTracker

[braveyao, 2016-12-29 01:15:54]

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

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

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

[braveyao, 2016-12-29 01:28:09]

Patchset #4 (id:120001) has been deleted

[braveyao, 2016-12-29 01:28:36]

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

[commit-bot: I haz the power, 2016-12-29 01:28:51]

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

[braveyao, 2016-12-29 01:55:30]

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

[commit-bot: I haz the power, 2016-12-29 01:55:40]

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-29 02:23:03]

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

[commit-bot: I haz the power, 2016-12-29 02:23:04]

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...)

[braveyao, 2016-12-29 04:17:12]

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

[commit-bot: I haz the power, 2016-12-29 04:17:28]

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

[braveyao, 2016-12-29 04:17:49]

Patchset #4 (id:140001) has been deleted

[braveyao, 2016-12-29 04:18:02]

Patchset #4 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-12-29 04:39:48]

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

[commit-bot: I haz the power, 2016-12-29 04:39:49]

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

[braveyao, 2016-12-29 04:48:23]

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

[commit-bot: I haz the power, 2016-12-29 04:48:32]

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

[braveyao, 2016-12-29 04:51:30]

Patchset #4 (id:180001) has been deleted

[commit-bot: I haz the power, 2016-12-29 05:21:23]

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

[commit-bot: I haz the power, 2016-12-29 05:21:24]

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

[braveyao, 2016-12-29 18:18:58]

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

[commit-bot: I haz the power, 2016-12-29 18:19:07]

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

[braveyao, 2016-12-29 18:19:10]

Patchset #4 (id:200001) has been deleted

[commit-bot: I haz the power, 2016-12-29 20:08:30]

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

[commit-bot: I haz the power, 2016-12-29 20:08:31]

Dry run: This issue passed the CQ dry run.

[braveyao, 2017-01-04 00:51:19]

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

[commit-bot: I haz the power, 2017-01-04 00:51:44]

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

[braveyao, 2017-01-04 00:52:38]

Patchset #4 (id:220001) has been deleted

[commit-bot: I haz the power, 2017-01-04 01:55:39]

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

[commit-bot: I haz the power, 2017-01-04 01:55:40]

Dry run: This issue passed the CQ dry run.

[braveyao, 2017-01-04 01:57:49]

Hi Miu, all comments are addressed. PTAL.

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

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.cc:11: #if defined(USE_AURA)
On 2016/12/27 23:21:03, miu wrote:
> It seems these platform-specific includes should be removed.

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.cc:63: DVLOG(2) << "Skipping
update with no window being tracked";
On 2016/12/27 23:21:02, miu wrote:
> nit: For consistency, let's call it a "view" instead of a "window" here and
> everywhere else. The "window" terminology referred to an Aura Window (which
> corresponds to a view). I know, confusing. ;-)

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.cc:77: gfx::NativeCursor cursor =
GetLastKnownCursor();
On 2016/12/27 23:21:03, miu wrote:
> Please move this down to just before it will first be used. (Since we will
only
> use this once we know the cursor should be displayed.)

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.cc:78: gfx::Point cursor_position
= GetCursorPositionInView();
On 2016/12/27 23:21:03, miu wrote:
> Please move this down to just before it will first be used. (Since there is no
> reason to invoke the method if |view_size| is empty.)

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.cc:87: if (cursor_position.x() < 0
|| cursor_position.y() < 0 ||
On 2016/12/27 23:21:02, miu wrote:
> nit: A trick to make this simpler:
> 
>   if (!gfx::Rect(view_size).Contains(cursor_position)) {
>     ...
>     return false;
>   }

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.cc:115:
window_size_when_cursor_last_updated_ != view_size) {
On 2016/12/27 23:21:03, miu wrote:
> The scaled size also depends on |region_in_frame.size()|, so it seems this
> should be:
> 
>   if (last_cursor_ != cursor ||
>       window_size_when_cursor_last_updated_ != view_size ||
>       last_region_in_frame_size_ != region_in_frame.size()) {
>     ...
>   }
> 
> However, from the code within the then-block, only |x_scale| and |y_scale|
> affect the result, so this would be simpler:
> 
>   if (last_cursor_ != cursor || last_x_scale_ != x_scale ||
>       last_y_scale_ != y_scale) {
>     ...
>   }
> 

Done.

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

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.h:28: // CursorRenderer is an
interface that can be implememented to do cursor
On 2016/12/27 23:21:03, miu wrote:
> This class comment needs to be updated, since this class is now an abstract
base
> class that handles all the non-platform-specific common cursor rendering
> functionality.

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.h:53: void
RenderOnVideoFrame(const scoped_refptr<media::VideoFrame>& target) const;
On 2016/12/27 23:21:03, miu wrote:
> style nit (per smart pointer guidelines): The old code did this wrong. Could
you
> fix the 1st argument to just be a raw pointer type (media::VideoFrame*), since
> the method will not hold a reference to |target|?

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.h:92: gfx::NativeView
captured_view_;
On 2016/12/27 23:21:03, miu wrote:
> nit: Use const since this member doesn't change after construction:
> 
>   const gfx::NativeView captured_view_;

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.h:99: gfx::Size
window_size_when_cursor_last_updated_;
On 2016/12/27 23:21:03, miu wrote:
> naming nit: How about |view_size_when_cursor_last_updated_|?

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer.h:117:
DISALLOW_COPY_AND_ASSIGN(CursorRenderer);
On 2016/12/27 23:21:03, miu wrote:
> nit: Please add a blank line above this line.

Done.

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

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.cc:7: #include
"ui/aura/env.h"
On 2016/12/27 23:21:03, miu wrote:
> Please add:
> 
> #include "base/memory/ptr_util.h"
> 
> (since you use base::MakeUnique in this module)

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.cc:39: if
(cursor_display_setting_ == kCursorEnabledOnMouseMovement)
On 2016/12/27 23:21:03, miu wrote:
> It's acceptable to just always call window_->RemovePreTargetHandler(this).
Then,
> you wouldn't need |cursor_display_setting_| anymore (see comment in .h file).

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.cc:44: bool
CursorRendererAura::IsCapturedViewActive() {
On 2016/12/27 23:21:03, miu wrote:
> Please add a null-check for |window_| (and return false if null).
> 
> Also, this seems to be needed for all the other methods.

Done.

There is such a check in the beginning of CursorRenderer::SnapshotCursorState().
So I wonder if it's needed for all the methods here? Or just add it in this
method only?

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.cc:87: return
last_cursor_hot_point_;
On 2016/12/27 23:21:03, miu wrote:
> The result of this method depends on whether GetLastKnownCursorImage() is
called
> first. Since they are intricately linked (because of the single call to
> ui::GetCursorBitmap()), I'm thinking maybe the base class should declare only
> one method to get the image+hot_point from the subclasses. So, instead of this
> (in the base class):
> 
>   virtual SkBitmap GetLastKnownCursorImage() = 0;
>   virtual gfx::Point GetLastKnownCursorHotPoint() = 0;
> 
> Just combine these into a single method:
> 
>   virtual SkBitmap GetLastKnownCursorImage(gfx::Point* hot_point) = 0;
> 
> (Then, you can delete the |last_cursor_hot_point_| member from this subclass.)

Done.

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

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.h:14: // Tracks state for
making decisions on cursor display on a captured video
On 2016/12/27 23:21:03, miu wrote:
> This class comment is incomplete. However, all the functionality should be
> documented in the new base class. So, here, maybe something as simple as "Aura
> platform-specific implementation of CursorRenderer." or no class comment
needed
> at all?

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.h:40: gfx::Point
last_cursor_hot_point_;
On 2016/12/27 23:21:03, miu wrote:
> You can remove this member (see comments in .cc file).

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_aura.h:43: const
CursorDisplaySetting cursor_display_setting_;
On 2016/12/27 23:21:03, miu wrote:
> ditto: You can remove this member too.

Done.

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

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.h:12: // Tracks state for
making decisions on cursor display on a captured video
On 2016/12/27 23:21:03, miu wrote:
> ditto: Class comment needs updating. (or just remove it, like
> CursorRendererAura?)

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
File content/browser/media/capture/cursor_renderer_mac.mm (right):

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:45: // Mouse location with
respect to the web contents.
On 2016/12/27 23:21:03, miu wrote:
> nit: s/the web contents/the view within the window/

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:49: // Revert y coordinate
to unify with Aura.
On 2016/12/27 23:21:03, miu wrote:
> s/Revert/Invert/

Done.

https://codereview.chromium.org/2553763002/diff/100001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:54: // Update cursor
movement info to CursorRenderer.
On 2016/12/27 23:21:03, miu wrote:
> This is an unexpected side-effect of calling GetCursorPositionInView(). Could
> you instead use a mouse event listener, like in CursorRendererAura? To do this
> in the Mac/Cocoa world, you'll need to add a "tracking area" to the
> |captured_view_| in CursorRendererMac's ctor.
> 
> References:
> 
>
https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Ev...
> 
> https://developer.apple.com/reference/appkit/nstrackingarea
> 
> I think, for our purposes, the NSTrackingArea would be created/added this way.
> In the ctor:
> 
>   CursorRendererMac(...) : ...,
>     // Note: All of |captured_view_|, |tracker_| and |tracking_area_| should
be
> base::scoped_nsobject<T> to ensure these objects remain alive until the dtor
is
> called.
>     tracker_([[CursorRendererMouseTracker alloc] initWithOwner:this]),
>     tracking_area_(
>         [[NSTrackingArea alloc] initWithRect:NSZeroRect
>              options: (NSTrackingMouseMoved | NSTrackingActiveAlways |
>                        NSTrackingInVisibleRect)
>              owner:tracker_.get()
>              userInfo:nil]) {
>     [captured_view_ addTrackingArea:tracking_area_.get()];
>   }
> 
> And, in the dtor:
> 
>   [captured_view_ removeTrackingArea:tracking_area_.get()];
> 
> Also, you'll need to create an Objective-C class that will receive the
> |-mouseMoved| method calls for the tracking area and pass them back to
> CursorRendererMac:
> 
>   @interface CursorRendererMouseTracker
>   - (id)initWithOwner:(CursorRendererMac* owner);
>   - (void)mouseMoved:(NSEvent*)event;
>   @private
>     CursorRendererMac* owner_;
>   @end
> 
>   @implementation CursorRendererMouseTracker
> 
>   - (id)initWithOwner:(CursorRendererMac* owner) {
>     self = [super init];
>     if (self)
>       owner_ = owner;
>     return self;
>   }
> 
>   - (void)mouseMoved:(NSEvent*)theEvent {
>     NSPoint locationInView = [captured_view_ convertPoint:[theEvent
> locationInWindow] fromView:nil];
>     // Invert Y coord too???
>     owner_->OnMouseMoved(gfx::Point(locationInView.x, locationInView.y),
>                          ui::EventTimeFromNative(theEvent));
>   }
> 
>   @end  // @implementation CursorRendererMouseTracker

Done.

[Robert Sesek, 2017-01-04 16:10:15]

rsesek@chromium.org changed reviewers:
+ rsesek@chromium.org

[Robert Sesek, 2017-01-04 16:10:17]

Some additional comments on cursor_renderer_mac.mm. Also, can we get a unittest
for the new code?

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
File content/browser/media/capture/cursor_renderer_mac.h (right):

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.h:16: ui::ScopedCrTrackingArea
tracking_area_;
nit: use camelCase_ for instance variable names in ObjC

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
File content/browser/media/capture/cursor_renderer_mac.mm (right):

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:18: self = [super init];
Style is:

if ((self = [super init])) {
  // initialization logic
}
return self

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:52: @end
nit: blank line before

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:74: DVLOG(2) << "Window
currently inactive";
Remove this log message?

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:113: gfx::Image
cursor_image = gfx::Image([nsimage retain]);
Use skia::NSImageToSkBitmapWithColorSpace instead since you're not really using
the gfx::Image here.

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:118: // Update cursor
movement info to CursorRenderer
nit: punctuation

[braveyao, 2017-01-04 18:05:07]

All comments are addressed. PTAL.
As to the unittest, we have one for Aura, in which we can manipulate the cursor.
But how to do same thing on Mac?

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
File content/browser/media/capture/cursor_renderer_mac.h (right):

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.h:16: ui::ScopedCrTrackingArea
tracking_area_;
On 2017/01/04 16:10:16, Robert Sesek wrote:
> nit: use camelCase_ for instance variable names in ObjC

Done.

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
File content/browser/media/capture/cursor_renderer_mac.mm (right):

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:18: self = [super init];
On 2017/01/04 16:10:16, Robert Sesek wrote:
> Style is:
> 
> if ((self = [super init])) {
>   // initialization logic
> }
> return self

Done.

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:52: @end
On 2017/01/04 16:10:16, Robert Sesek wrote:
> nit: blank line before

Done.

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:74: DVLOG(2) << "Window
currently inactive";
On 2017/01/04 16:10:16, Robert Sesek wrote:
> Remove this log message?

Done.

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:113: gfx::Image
cursor_image = gfx::Image([nsimage retain]);
On 2017/01/04 16:10:16, Robert Sesek wrote:
> Use skia::NSImageToSkBitmapWithColorSpace instead since you're not really
using
> the gfx::Image here.

Done.

https://codereview.chromium.org/2553763002/diff/240001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:118: // Update cursor
movement info to CursorRenderer
On 2017/01/04 16:10:16, Robert Sesek wrote:
> nit: punctuation

Done.

[Robert Sesek, 2017-01-04 21:27:43]

On 2017/01/04 18:05:07, braveyao wrote:
> All comments are addressed. PTAL.
> As to the unittest, we have one for Aura, in which we can manipulate the
cursor.
> But how to do same thing on Mac?

[NSCursor push] and -pop. As well as testing the results of the various mouse
events.

[miu, 2017-01-05 01:26:57]

PS5 lgtm % one concern:

https://codereview.chromium.org/2553763002/diff/260001/content/browser/media/...
File content/browser/media/capture/cursor_renderer_mac.mm (right):

https://codereview.chromium.org/2553763002/diff/260001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:36: [capturedView_
removeTrackingArea:trackingArea_.get()];
It seems possible for |capturedView_| could sometimes be invalid at this point.
I'd suggest making it a scoped_nsobject to retain it so this method call doesn't
cause a crash.

[miu, 2017-01-05 01:28:49]

And, thanks rsesek, for helping us out with the Mac specifics! :)

[braveyao, 2017-01-05 02:17:13]

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

[commit-bot: I haz the power, 2017-01-05 02:17: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-01-05 02:20:00]

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

[commit-bot: I haz the power, 2017-01-05 02:20:01]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[braveyao, 2017-01-05 21:50:20]

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

[commit-bot: I haz the power, 2017-01-05 21:50:55]

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

[braveyao, 2017-01-05 22:34:49]

Thanks rsesek@ for the great hints. Filed a crbug/678783 for following up the
unittest on OSX. (In a separate cl as I don't think I can make it soon. Sorry.)

And addressed the concern, by making |capturedView_| a scoped_nsobject.

https://codereview.chromium.org/2553763002/diff/260001/content/browser/media/...
File content/browser/media/capture/cursor_renderer_mac.mm (right):

https://codereview.chromium.org/2553763002/diff/260001/content/browser/media/...
content/browser/media/capture/cursor_renderer_mac.mm:36: [capturedView_
removeTrackingArea:trackingArea_.get()];
On 2017/01/05 01:26:57, miu wrote:
> It seems possible for |capturedView_| could sometimes be invalid at this
point.
> I'd suggest making it a scoped_nsobject to retain it so this method call
doesn't
> cause a crash.

Done.

[braveyao, 2017-01-05 22:38:48]

braveyao@chromium.org changed reviewers:
+ boliu@chromium.org

[braveyao, 2017-01-05 22:38:49]

+ boliu@, for the owner's review for content/browser/BUILD.gn  (note: patchset 7
is a rebase). Thanks!

[Robert Sesek, 2017-01-05 23:07:38]

On 2017/01/05 22:34:49, braveyao wrote:
> Thanks rsesek@ for the great hints. Filed a crbug/678783 for following up the
> unittest on OSX. (In a separate cl as I don't think I can make it soon.
Sorry.)

What does that mean? Why can't you add a test now?

[boliu, 2017-01-05 23:26:50]

content/browser/BUILD.gn lgtm

[commit-bot: I haz the power, 2017-01-05 23:33:52]

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

[commit-bot: I haz the power, 2017-01-05 23:33:53]

Dry run: This issue passed the CQ dry run.

[braveyao, 2017-01-05 23:44:54]

On 2017/01/05 23:26:50, boliu wrote:
> content/browser/BUILD.gn lgtm

rsesek@, I just don't have confidence to add it now, lacking experience of
object-c and content capture(this is the first time I touch this two fields.).
But I really want to practice in this area so I filed the crbug to remind me.
Would you excuse me for doing it later? Thanks!

[braveyao, 2017-01-06 17:59:41]

The CQ bit was checked by braveyao@chromium.org

[braveyao, 2017-01-06 17:59:42]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2553763002/#ps300001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-06 18:00:16]

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

[Robert Sesek, 2017-01-06 18:06:29]

On 2017/01/05 23:44:54, braveyao wrote:
> On 2017/01/05 23:26:50, boliu wrote:
> > content/browser/BUILD.gn lgtm
> 
> rsesek@, I just don't have confidence to add it now, lacking experience of
> object-c and content capture(this is the first time I touch this two fields.).
> But I really want to practice in this area so I filed the crbug to remind me.
> Would you excuse me for doing it later? Thanks!

In my experience bugs to write tests later don't typically get addressed. But
I'm not going to press the point.

[commit-bot: I haz the power, 2017-01-06 18:06:35]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1483725581866300,
"parent_rev": "2d69d55423fa357963f4e5bbaf8b5ab44075a340", "commit_rev":
"2f9c8630f029d251af62445f88357d0edbf0543c"}

[commit-bot: I haz the power, 2017-01-06 18:07:05]

Description was changed from

==========
Fix cursor missing in tabCapture on OSX Sierra

Since OSX Sierra, when we ask for the CGImage of a cursor image, it will return
a double-size CGImage with Retina display, which can't pass the sanity check.

=========================(deprecated)==================================
In this cl, we try to pass a half-size Rect hint to get CGImage with original
cursor size on both Retina and non-Retina display.
Also it fixs another issue that we render the cursor based on window coordinate
to a target in physical coordinate.
======================================================================

In this cl, we implemented a new common base class, CursorRenderer , to
merge the two implementations. Then we can handle the scaling and
rendering of cursor same on all platforms, also with other advantages:
caching scaled cursor image for better performance and only caring
about view coordinates (same handling to Retina and non-Retina). 

BUG=659183
==========

to

==========
Fix cursor missing in tabCapture on OSX Sierra

Since OSX Sierra, when we ask for the CGImage of a cursor image, it will return
a double-size CGImage with Retina display, which can't pass the sanity check.

=========================(deprecated)==================================
In this cl, we try to pass a half-size Rect hint to get CGImage with original
cursor size on both Retina and non-Retina display.
Also it fixs another issue that we render the cursor based on window coordinate
to a target in physical coordinate.
======================================================================

In this cl, we implemented a new common base class, CursorRenderer , to
merge the two implementations. Then we can handle the scaling and
rendering of cursor same on all platforms, also with other advantages:
caching scaled cursor image for better performance and only caring
about view coordinates (same handling to Retina and non-Retina). 

BUG=659183

Review-Url: https://codereview.chromium.org/2553763002
Cr-Commit-Position: refs/heads/master@{#441975}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f9c8630f029d251af62445f8835...
==========

[commit-bot: I haz the power, 2017-01-06 18:07:06]

Committed patchset #7 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/2f9c8630f029d251af62445f8835...

[braveyao, 2017-01-06 18:26:03]

On 2017/01/06 18:06:29, Robert Sesek wrote:
> On 2017/01/05 23:44:54, braveyao wrote:
> > On 2017/01/05 23:26:50, boliu wrote:
> > > content/browser/BUILD.gn lgtm
> > 
> > rsesek@, I just don't have confidence to add it now, lacking experience of
> > object-c and content capture(this is the first time I touch this two
fields.).
> > But I really want to practice in this area so I filed the crbug to remind
me.
> > Would you excuse me for doing it later? Thanks!
> 
> In my experience bugs to write tests later don't typically get addressed. But
> I'm not going to press the point.

rsesek@, "TODO" is a typical lie, but not the crbugs (especially to me) ^_^
I definitely need your help on this. So I suppose you'll get the updates soon.