Allow content embedder to set a cursor for RenderWidgetHost

Allow content embedder to set a cursor for RenderWidgetHost
    
One example is DevTools eye dropper feature which changes
cursor to custom image built from the rendered page contents.
    
content::CursorInfo is introduced to pass the data through
public API.
    
BUG=711412
TBR=inferno@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2808763009
Cr-Commit-Position: refs/heads/master@{#464633}
Committed: https://chromium.googlesource.com/chromium/src/+/9adbfb97089e255303e640084c0eb45481766457

[dgozman, 2017-04-12 23:10:50]

Description was changed from

==========
Expose content::WebCursor in public/common/webcursor.h

This is needed for RenderWidgetHost::SetCursor method, which
allows embedder to manipulate cursor if needed.

One example is DevTools eye dropper feature.

BUG=none
==========

to

==========
Expose content::WebCursor in public/common/webcursor.h

This is needed for RenderWidgetHost::SetCursor method, which
allows embedder to manipulate cursor if needed.

One example is DevTools eye dropper feature.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dgozman, 2017-04-12 23:12:50]

Description was changed from

==========
Expose content::WebCursor in public/common/webcursor.h

This is needed for RenderWidgetHost::SetCursor method, which
allows embedder to manipulate cursor if needed.

One example is DevTools eye dropper feature.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Expose content::WebCursor in public/common/webcursor.h

This is needed for RenderWidgetHost::SetCursor method
allowing embedder to manipulate cursor if needed.

One example is DevTools eye dropper feature which changes
cursor to custom image built from the rendered page contents.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dgozman, 2017-04-12 23:19:10]

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

[commit-bot: I haz the power, 2017-04-12 23:20:15]

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

[dgozman, 2017-04-12 23:21:02]

dgozman@chromium.org changed reviewers:
+ avi@chromium.org, bsep@chromium.org

[dgozman, 2017-04-12 23:21:02]

Hi Avi and Bret,

Could you please take a look?
I stumbled upon this while working on this patch:
https://codereview.chromium.org/2817593004/
I think it makes sense to allow embedder to manipulate cursor, but you should
have more insights into the area.

Thanks,
Dmitry

[commit-bot: I haz the power, 2017-04-12 23:25:41]

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

[commit-bot: I haz the power, 2017-04-12 23:25:43]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Bret, 2017-04-12 23:35:15]

I'm not very familiar with embedding, but this change LGTM.

[dgozman, 2017-04-13 01:07:58]

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

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

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-04-13 01:15:52]

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

[commit-bot: I haz the power, 2017-04-13 01:15:53]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Avi (use Gerrit), 2017-04-13 14:30:34]

1. You need a bug.
2. The content API is interfaces, not implementations. Leave webcursor.h where
it is, rename it to _impl, and make a new webcursor.h. See all the
Foo.h/FooImpl.h classes in the content API as an example. (see
https://www.chromium.org/developers/content-module/content-api : "content/public
should contain only interfaces, structs and (rarely) static functions")

[dgozman, 2017-04-13 17:18:37]

> 2. The content API is interfaces, not implementations. Leave webcursor.h where
> it is, rename it to _impl, and make a new webcursor.h. See all the
> Foo.h/FooImpl.h classes in the content API as an example. (see
> https://www.chromium.org/developers/content-module/content-api :
"content/public
> should contain only interfaces, structs and (rarely) static functions")

That's what I planned to do. But then I stumbled upon multiple struct-like
classes without impl counterpart, e.g. CdmInfo, DropData, PageState, and wanted
to get another person's opinion. I'll try to only expose CursorInfo struct to
not introduce extra impls if we can avoid it.

[Avi (use Gerrit), 2017-04-13 17:30:27]

On 2017/04/13 17:18:37, dgozman wrote:
> > 2. The content API is interfaces, not implementations. Leave webcursor.h
where
> > it is, rename it to _impl, and make a new webcursor.h. See all the
> > Foo.h/FooImpl.h classes in the content API as an example. (see
> > https://www.chromium.org/developers/content-module/content-api :
> "content/public
> > should contain only interfaces, structs and (rarely) static functions")
> 
> That's what I planned to do. But then I stumbled upon multiple struct-like
> classes without impl counterpart, e.g. CdmInfo, DropData, PageState, and
wanted
> to get another person's opinion. I'll try to only expose CursorInfo struct to
> not introduce extra impls if we can avoid it.

This seems to hinge on whether WebCursor is "struct-like". With the .cc file at
200 lines, I'm tending towards no. I would say that CdmInfo and DropData don't
help your argument; they are literally structs with maybe one function aside
from the constructors and destructors. I really wouldn't have allowed PageState
as it is, but even its cpp file is shorter than WebCursor's.

[dgozman, 2017-04-13 17:38:56]

On 2017/04/13 17:30:27, Avi wrote:
> On 2017/04/13 17:18:37, dgozman wrote:
> > > 2. The content API is interfaces, not implementations. Leave webcursor.h
> where
> > > it is, rename it to _impl, and make a new webcursor.h. See all the
> > > Foo.h/FooImpl.h classes in the content API as an example. (see
> > > https://www.chromium.org/developers/content-module/content-api :
> > "content/public
> > > should contain only interfaces, structs and (rarely) static functions")
> > 
> > That's what I planned to do. But then I stumbled upon multiple struct-like
> > classes without impl counterpart, e.g. CdmInfo, DropData, PageState, and
> wanted
> > to get another person's opinion. I'll try to only expose CursorInfo struct
to
> > not introduce extra impls if we can avoid it.
> 
> This seems to hinge on whether WebCursor is "struct-like". With the .cc file
at
> 200 lines, I'm tending towards no. I would say that CdmInfo and DropData don't
> help your argument; they are literally structs with maybe one function aside
> from the constructors and destructors. I really wouldn't have allowed
PageState
> as it is, but even its cpp file is shorter than WebCursor's.

That's fair.

[dgozman, 2017-04-13 19:38:51]

Description was changed from

==========
Expose content::WebCursor in public/common/webcursor.h

This is needed for RenderWidgetHost::SetCursor method
allowing embedder to manipulate cursor if needed.

One example is DevTools eye dropper feature which changes
cursor to custom image built from the rendered page contents.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Allow content embedder to set a cursor for RenderWidgetHost
    
One example is DevTools eye dropper feature which changes
cursor to custom image built from the rendered page contents.
    
content::CursorInfo is introduced to pass the data through
public API.
    
BUG=711412
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dgozman, 2017-04-13 19:38:58]

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

[commit-bot: I haz the power, 2017-04-13 19:39:33]

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-04-13 21:19:28]

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

[commit-bot: I haz the power, 2017-04-13 21:19:29]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-04-13 21:52:03]

Avi, Bret, could you please take another look? I think this is ready.

[Avi (use Gerrit), 2017-04-13 22:12:31]

lgtm

minor grammar nits

https://codereview.chromium.org/2808763009/diff/60001/content/public/common/c...
File content/public/common/cursor_info.h (right):

https://codereview.chromium.org/2808763009/diff/60001/content/public/common/c...
content/public/common/cursor_info.h:33: // The scale factor of custom image,
used to possible re-scale the image for
... used to possibly re-scale ... for a different ...

[Bret, 2017-04-13 22:23:04]

lgtm

[dgozman, 2017-04-13 23:21:09]

Thank you for review.

https://codereview.chromium.org/2808763009/diff/60001/content/public/common/c...
File content/public/common/cursor_info.h (right):

https://codereview.chromium.org/2808763009/diff/60001/content/public/common/c...
content/public/common/cursor_info.h:33: // The scale factor of custom image,
used to possible re-scale the image for
On 2017/04/13 22:12:31, Avi wrote:
> ... used to possibly re-scale ... for a different ...

Done.

[dgozman, 2017-04-13 23:22:49]

Description was changed from

==========
Allow content embedder to set a cursor for RenderWidgetHost
    
One example is DevTools eye dropper feature which changes
cursor to custom image built from the rendered page contents.
    
content::CursorInfo is introduced to pass the data through
public API.
    
BUG=711412
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Allow content embedder to set a cursor for RenderWidgetHost
    
One example is DevTools eye dropper feature which changes
cursor to custom image built from the rendered page contents.
    
content::CursorInfo is introduced to pass the data through
public API.
    
BUG=711412
TBR=inferno@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dgozman, 2017-04-13 23:23:11]

TBR'ing inferno@chromium.org for tools/ipc_fuzzer (simple rename).

[dgozman, 2017-04-13 23:23:16]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2017-04-13 23:23:16]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
bsep@chromium.org
Link to the patchset: https://codereview.chromium.org/2808763009/#ps80001
(title: "comment")

[commit-bot: I haz the power, 2017-04-13 23:23:47]

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-04-14 00:52:42]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492125796442130,
"parent_rev": "18a9048f37ca12fa560fd989d837a0de0974f88e", "commit_rev":
"9adbfb97089e255303e640084c0eb45481766457"}

[commit-bot: I haz the power, 2017-04-14 00:53:31]

Description was changed from

==========
Allow content embedder to set a cursor for RenderWidgetHost
    
One example is DevTools eye dropper feature which changes
cursor to custom image built from the rendered page contents.
    
content::CursorInfo is introduced to pass the data through
public API.
    
BUG=711412
TBR=inferno@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Allow content embedder to set a cursor for RenderWidgetHost
    
One example is DevTools eye dropper feature which changes
cursor to custom image built from the rendered page contents.
    
content::CursorInfo is introduced to pass the data through
public API.
    
BUG=711412
TBR=inferno@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2808763009
Cr-Commit-Position: refs/heads/master@{#464633}
Committed:
https://chromium.googlesource.com/chromium/src/+/9adbfb97089e255303e640084c0e...
==========

[commit-bot: I haz the power, 2017-04-14 00:53:32]

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