Adding Ozone CursorLoader and necessary API functions to allow setting the cursor in SoftwareFactor…

Adding Ozone CursorLoader and necessary API functions to allow setting the cursor in SoftwareFactoryOzone

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250929

[dnicoara, 2014-02-11 21:16:34]

PTAL

[rjkroege, 2014-02-11 21:57:55]

https://codereview.chromium.org/158023006/diff/1/ui/base/cursor/cursor_loader...
File ui/base/cursor/cursor_loader_ozone.cc (right):

https://codereview.chromium.org/158023006/diff/1/ui/base/cursor/cursor_loader...
ui/base/cursor/cursor_loader_ozone.cc:36: void
CursorLoaderOzone::LoadImageCursor(int id,
this code as written never removes cursors. how are custom cursors set by the
renderer cleaned up?

https://codereview.chromium.org/158023006/diff/1/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/158023006/diff/1/ui/ui.gyp#newcode552
ui/ui.gyp:552: ['OS=="android" or OS=="mac" or OS=="win" or use_ozone==1 or
use_x11==1', {
what platforms continue to use cursor_null* with this change? Any?

[dnicoara, 2014-02-12 16:18:18]

https://codereview.chromium.org/158023006/diff/1/ui/base/cursor/cursor_loader...
File ui/base/cursor/cursor_loader_ozone.cc (right):

https://codereview.chromium.org/158023006/diff/1/ui/base/cursor/cursor_loader...
ui/base/cursor/cursor_loader_ozone.cc:36: void
CursorLoaderOzone::LoadImageCursor(int id,
On 2014/02/11 21:57:55, rjkroege wrote:
> this code as written never removes cursors. how are custom cursors set by the
> renderer cleaned up?

I've added a call to clear in UnloadAll. It wasn't clear if this is required by
looking at the X11 impl. I think that we should probably clear the vectors in
the X11 impl as well otherwise it may reference destroyed resources.

https://codereview.chromium.org/158023006/diff/1/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/158023006/diff/1/ui/ui.gyp#newcode552
ui/ui.gyp:552: ['OS=="android" or OS=="mac" or OS=="win" or use_ozone==1 or
use_x11==1', {
On 2014/02/11 21:57:55, rjkroege wrote:
> what platforms continue to use cursor_null* with this change? Any?
> 

Not sure if there are any left. I've removed the *_null implementation and will
run try jobs to see if anything is still using them.

https://codereview.chromium.org/158023006/diff/140001/ui/aura/root_window_hos...
File ui/aura/root_window_host_ozone.cc (right):

https://codereview.chromium.org/158023006/diff/140001/ui/aura/root_window_hos...
ui/aura/root_window_host_ozone.cc:42: if (event->type() == ui::ET_MOUSE_MOVED) {
Removed as per our offline discussion.

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_ozone.cc (right):

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_ozone.cc:65:
cursor->SetPlatformCursor(cursor->platform());
This is where the custom webkit cursors are being set. I've opened issue
crbug.com/343155 since I need to add support for Ozone.

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_ozone.h (right):

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_ozone.h:39: std::map<int, const gfx::ImageSkia*>
cursors_;
Based on my discussion with sadrul@ my understanding is that ResourceBundles are
long lived. So rather than copy the SkBitmaps, just keep a reference to the
shared resources in ResourceBundle.

[rjkroege, 2014-02-12 18:03:17]

lgtm

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_ozone.cc (right):

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_ozone.cc:38: const gfx::ImageSkia* image =
ResourceBundle::GetSharedInstance()
Perhaps the point of maintaining the cursors_ cache in the x11 implementation
would be to avoid uploading the cursor image to the server on each call to
LoadImageCursor. So: in ozone, do we need to bother? In particular: if
GetSharedInstance is cheap, you could just always load the image from the
ResourceBundle below in SetPlatformCursor?

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_ozone.cc:65:
cursor->SetPlatformCursor(cursor->platform());
On 2014/02/12 16:18:20, dnicoara wrote:
> This is where the custom webkit cursors are being set. I've opened issue
> crbug.com/343155 since I need to add support for Ozone.

nit: can you add a TODO listing the bug. And add another bug for handling
animated cursors.

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_ozone.h (right):

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_ozone.h:19: class CursorLoaderOzone : public
CursorLoader {
you might still want this to be UI_BASE_EXPORT

[dnicoara, 2014-02-12 20:44:57]

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_ozone.cc (right):

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_ozone.cc:38: const gfx::ImageSkia* image =
ResourceBundle::GetSharedInstance()
On 2014/02/12 18:03:17, rjkroege wrote:
> Perhaps the point of maintaining the cursors_ cache in the x11 implementation
> would be to avoid uploading the cursor image to the server on each call to
> LoadImageCursor. So: in ozone, do we need to bother? In particular: if
> GetSharedInstance is cheap, you could just always load the image from the
> ResourceBundle below in SetPlatformCursor?

GetImageSkiaName may not be as cheap since it needs to load the resource. We'd
still need some sort of cache to keep track of id to resource_id mapping, so it
wouldn't change much.

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_ozone.cc:65:
cursor->SetPlatformCursor(cursor->platform());
On 2014/02/12 18:03:17, rjkroege wrote:
> On 2014/02/12 16:18:20, dnicoara wrote:
> > This is where the custom webkit cursors are being set. I've opened issue
> > crbug.com/343155 since I need to add support for Ozone.
> 
> nit: can you add a TODO listing the bug. And add another bug for handling
> animated cursors.

Done.

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_ozone.h (right):

https://codereview.chromium.org/158023006/diff/170001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_ozone.h:19: class CursorLoaderOzone : public
CursorLoader {
On 2014/02/12 18:03:17, rjkroege wrote:
> you might still want this to be UI_BASE_EXPORT

Done.

[dnicoara, 2014-02-12 20:45:19]

Adding sky@ for OWNERS review.

[sky, 2014-02-12 21:47:21]

LGTM

https://codereview.chromium.org/158023006/diff/260001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_ozone.cc (right):

https://codereview.chromium.org/158023006/diff/260001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_ozone.cc:41: cursors_[id] = image;
nit: do away with temporary (eg assign directly).

https://codereview.chromium.org/158023006/diff/260001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_ozone.h (right):

https://codereview.chromium.org/158023006/diff/260001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_ozone.h:27: 
note: Style on right side is correct here. Prefix section with where overrides
come from and no whitespace between overriden methods.

[dnicoara, 2014-02-12 21:57:40]

Thank you!

https://codereview.chromium.org/158023006/diff/260001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_ozone.cc (right):

https://codereview.chromium.org/158023006/diff/260001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_ozone.cc:41: cursors_[id] = image;
On 2014/02/12 21:47:22, sky wrote:
> nit: do away with temporary (eg assign directly).

Done.

https://codereview.chromium.org/158023006/diff/260001/ui/base/cursor/cursor_l...
File ui/base/cursor/cursor_loader_ozone.h (right):

https://codereview.chromium.org/158023006/diff/260001/ui/base/cursor/cursor_l...
ui/base/cursor/cursor_loader_ozone.h:27: 
On 2014/02/12 21:47:22, sky wrote:
> note: Style on right side is correct here. Prefix section with where overrides
> come from and no whitespace between overriden methods.

Done.

[dnicoara, 2014-02-12 21:58:20]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-02-12 21:58:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/158023006/390001

[commit-bot: I haz the power, 2014-02-13 00:19:18]

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

[commit-bot: I haz the power, 2014-02-13 00:19:19]

Retried try job too often on linux_chromeos for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[dnicoara, 2014-02-13 01:45:43]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-02-13 01:50:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/158023006/390001

[commit-bot: I haz the power, 2014-02-13 03:57:03]

Change committed as 250929