Aura: Adds custom cursors for drag and drop.

Aura: Adds custom cursors for drag and drop.

BUG=121135
TEST=none

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

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135426

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135533

[varunjain, 2012-05-03 18:00:24]

+sail,flackr to check that resources are added properly in the right place.

[sail, 2012-05-03 18:11:05]

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
File ui/aura/root_window_host_linux.cc (right):

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:315: XFreeCursor(xdisplay_, it->second.first);
instead of doing your own reference counting can you use
x11_util.cc/XCustomCursor instead. You'll have to make that class public.

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:347: int height = bitmap->height(), width =
bitmap->width();
width,height should be on separate lines

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:350: uint32 pixel = bitmap->getAddr32(0,
y)[x];
this code seems to duplicate WebCursor::GetPlatformCursor(). Can you refactor
it? 
http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/webcursor_aura...

[sail, 2012-05-03 18:11:29]

LGTM! The GRD stuff looks good to me by the way.

[flackr, 2012-05-03 18:12:25]

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/resources/ui_res...
File ui/resources/ui_resources_2x.grd (right):

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/resources/ui_res...
ui/resources/ui_resources_2x.grd:82: <include name="IDR_AURA_CURSOR_COPY"
file="aura/left_ptr_copy_2x.png" type="BINDATA" />
You must also keep ui_resources_touch.grd in sync.

[sky, 2012-05-03 19:12:52]

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
File ui/aura/root_window_host_linux.cc (right):

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:312: virtual ~ImageCursors() {
no virtual.

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:314: for (it = cursors_.begin(); it !=
cursors_.end(); it++) {
++it

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:335:
ui::ResourceBundle::GetSharedInstance().GetBitmapNamed(resource_id);
GetBitmapNamed is deprecated, and as far as I know never returns null.

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:353: pixels[i] =
SkUnPreMultiply::PMColorToColor(pixel);
Don't we have code like this some where already?

[varunjain, 2012-05-03 19:48:54]

+jamesr for webkit/ OWNERS

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
File ui/aura/root_window_host_linux.cc (right):

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:312: virtual ~ImageCursors() {
On 2012/05/03 19:12:52, sky wrote:
> no virtual.

Done.

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:314: for (it = cursors_.begin(); it !=
cursors_.end(); it++) {
On 2012/05/03 19:12:52, sky wrote:
> ++it

Done.

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:315: XFreeCursor(xdisplay_, it->second.first);
On 2012/05/03 18:11:05, sail wrote:
> instead of doing your own reference counting can you use
> x11_util.cc/XCustomCursor instead. You'll have to make that class public.

Done with ui::CreateReffedXCursor()

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:335:
ui::ResourceBundle::GetSharedInstance().GetBitmapNamed(resource_id);
On 2012/05/03 19:12:52, sky wrote:
> GetBitmapNamed is deprecated, and as far as I know never returns null.

Done.

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:347: int height = bitmap->height(), width =
bitmap->width();
On 2012/05/03 18:11:05, sail wrote:
> width,height should be on separate lines

Done.

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:350: uint32 pixel = bitmap->getAddr32(0,
y)[x];
On 2012/05/03 18:11:05, sail wrote:
> this code seems to duplicate WebCursor::GetPlatformCursor(). Can you refactor
> it? 
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/webcursor_aura...

Done.

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/aura/root_window...
ui/aura/root_window_host_linux.cc:353: pixels[i] =
SkUnPreMultiply::PMColorToColor(pixel);
On 2012/05/03 19:12:52, sky wrote:
> Don't we have code like this some where already?

Done.

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/resources/ui_res...
File ui/resources/ui_resources_2x.grd (right):

https://chromiumcodereview.appspot.com/10316019/diff/3002/ui/resources/ui_res...
ui/resources/ui_resources_2x.grd:82: <include name="IDR_AURA_CURSOR_COPY"
file="aura/left_ptr_copy_2x.png" type="BINDATA" />
On 2012/05/03 18:12:25, flackr wrote:
> You must also keep ui_resources_touch.grd in sync.

Done.

[sail, 2012-05-03 20:00:46]

lgtm

https://chromiumcodereview.appspot.com/10316019/diff/17/ui/aura/root_window_h...
File ui/aura/root_window_host_linux.cc (right):

https://chromiumcodereview.appspot.com/10316019/diff/17/ui/aura/root_window_h...
ui/aura/root_window_host_linux.cc:313: ui::UnrefCustomXCursor(it->second);
I was thinking that you could just use XCustomCursor directly instead of
manually keeping track of ::Cursor references.
That said this seems ok to me.

[flackr, 2012-05-03 20:33:28]

LGTM with nit

http://codereview.chromium.org/10316019/diff/17/ui/aura/aura.gyp
File ui/aura/aura.gyp (right):

http://codereview.chromium.org/10316019/diff/17/ui/aura/aura.gyp#newcode22
ui/aura/aura.gyp:22: '../ui.gyp:ui_resources_2x',
I don't think this actually depends on ui_resources_2x (or ui_resources_touch
which it can also use) since it gets the resource id from the
ui_resources_standard grit header file.

[sky, 2012-05-03 21:09:44]

https://chromiumcodereview.appspot.com/10316019/diff/17/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

https://chromiumcodereview.appspot.com/10316019/diff/17/ui/base/x/x11_util.cc...
ui/base/x/x11_util.cc:420: XcursorImage* image =
XcursorImageCreate(bitmap->width(), bitmap->height());
Can you move ConvertSkiatoRGBA to a common place?

[varunjain, 2012-05-03 22:12:20]

http://codereview.chromium.org/10316019/diff/17/ui/aura/aura.gyp
File ui/aura/aura.gyp (right):

http://codereview.chromium.org/10316019/diff/17/ui/aura/aura.gyp#newcode22
ui/aura/aura.gyp:22: '../ui.gyp:ui_resources_2x',
On 2012/05/03 20:33:28, flackr wrote:
> I don't think this actually depends on ui_resources_2x (or ui_resources_touch
> which it can also use) since it gets the resource id from the
> ui_resources_standard grit header file.

According to sail, I need to include both _2x and _standard in order for it to
pick up the right size resource.

http://codereview.chromium.org/10316019/diff/17/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/10316019/diff/17/ui/base/x/x11_util.cc#newcode420
ui/base/x/x11_util.cc:420: XcursorImage* image =
XcursorImageCreate(bitmap->width(), bitmap->height());
On 2012/05/03 21:09:44, sky wrote:
> Can you move ConvertSkiatoRGBA to a common place?

Done.

[sky, 2012-05-03 22:46:49]

LGTM

http://codereview.chromium.org/10316019/diff/23001/ui/gfx/skia_util.h
File ui/gfx/skia_util.h (right):

http://codereview.chromium.org/10316019/diff/23001/ui/gfx/skia_util.h#newcode54
ui/gfx/skia_util.h:54: UI_EXPORT void ConvertSkiatoRGBA(const unsigned char*
skia,
To

[varunjain, 2012-05-04 16:15:38]

+ darin since jamesr is away

[darin (slow to review), 2012-05-04 16:32:36]

LGTM for src/webkit

[varunjain, 2012-05-04 17:06:52]

http://codereview.chromium.org/10316019/diff/23001/ui/gfx/skia_util.h
File ui/gfx/skia_util.h (right):

http://codereview.chromium.org/10316019/diff/23001/ui/gfx/skia_util.h#newcode54
ui/gfx/skia_util.h:54: UI_EXPORT void ConvertSkiatoRGBA(const unsigned char*
skia,
On 2012/05/03 22:46:49, sky wrote:
> To

Done.

[commit-bot: I haz the power, 2012-05-04 17:08:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10316019/33001

[commit-bot: I haz the power, 2012-05-04 17:28:05]

Try job failure for 10316019-33001 (retry) on win_rel for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-04 19:17:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10316019/38003

[commit-bot: I haz the power, 2012-05-04 21:02:17]

Change committed as 135426

[sky, 2012-05-04 22:30:52]

LGTM Are you sure don't need to update any subclasses?

[varunjain, 2012-05-04 22:34:25]

On 2012/05/04 22:30:52, sky wrote:
> LGTM Are you sure don't need to update any subclasses?

I dont think so... running through trybots to be sure.

[jam, 2012-05-04 23:42:53]

lgtm as discussed, all the aura intilization should move out in a future change
similar to how https://chromiumcodereview.appspot.com/10191010/ did it for
content wrt aura

[commit-bot: I haz the power, 2012-05-05 01:19:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10316019/43023

[commit-bot: I haz the power, 2012-05-05 04:55:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10316019/32016

[commit-bot: I haz the power, 2012-05-05 09:43:18]

Change committed as 135533

[jochen (gone - plz use gerrit), 2012-05-07 23:21:29]

http://codereview.chromium.org/10316019/diff/32016/content/test/DEPS
File content/test/DEPS (right):

http://codereview.chromium.org/10316019/diff/32016/content/test/DEPS#newcode8
content/test/DEPS:8: "+ui/base/resource/resource_bundle.h",
please revert this CL:

- this is a layering violation, ui/base/resource depends on chrome (see
content/DEPS)
- on Mac, the resource bundle is part of the Chrome framework, but content
itself doesn't build a framework

[Nico, 2012-05-07 23:34:51]

On 2012/05/07 23:21:29, jochen wrote:
> http://codereview.chromium.org/10316019/diff/32016/content/test/DEPS
> File content/test/DEPS (right):
> 
> http://codereview.chromium.org/10316019/diff/32016/content/test/DEPS#newcode8
> content/test/DEPS:8: "+ui/base/resource/resource_bundle.h",
> please revert this CL:
> 
> - this is a layering violation, ui/base/resource depends on chrome (see
> content/DEPS)
> - on Mac, the resource bundle is part of the Chrome framework, but content
> itself doesn't build a framework

Because of this second reason, this CL made all content_unittests fail in debug
builds on mac. That's not visible on main and try waterfalls due to
http://crbug.com/126558 , but bots that don't shard tests have been red since
this landed (e.g.
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Mac%20Ninja )

[varunjain, 2012-05-08 15:17:57]

Hi guys... apologies for breaking the build... I am not sure why I did not
receive any email about this buildbot failure.
I am aware of the layering violation. But the agreement with jam was to submit
this for now since this is fixing a m20 p1 and work on fixing the violation in
another CL.


On 2012/05/07 23:34:51, Nico wrote:
> On 2012/05/07 23:21:29, jochen wrote:
> > http://codereview.chromium.org/10316019/diff/32016/content/test/DEPS
> > File content/test/DEPS (right):
> > 
> >
http://codereview.chromium.org/10316019/diff/32016/content/test/DEPS#newcode8
> > content/test/DEPS:8: "+ui/base/resource/resource_bundle.h",
> > please revert this CL:
> > 
> > - this is a layering violation, ui/base/resource depends on chrome (see
> > content/DEPS)
> > - on Mac, the resource bundle is part of the Chrome framework, but content
> > itself doesn't build a framework
> 
> Because of this second reason, this CL made all content_unittests fail in
debug
> builds on mac. That's not visible on main and try waterfalls due to
> http://crbug.com/126558 , but bots that don't shard tests have been red since
> this landed (e.g.
> http://build.chromium.org/p/chromium.fyi/builders/Chromium%2520Mac%2520Ninja )

[jochen (gone - plz use gerrit), 2012-05-08 15:27:38]

On Tue, May 8, 2012 at 8:17 AM, <varunjain@chromium.org> wrote:

> Hi guys... apologies for breaking the build... I am not sure why I did not
> receive any email about this buildbot failure.
>

yeah, that's a bug :-/



> I am aware of the layering violation. But the agreement with jam was to
> submit
> this for now since this is fixing a m20 p1 and work on fixing the
> violation in
> another CL.


The problem is that it's not just a style thing, but it just won't work.


>
>
>
> On 2012/05/07 23:34:51, Nico wrote:
>
>> On 2012/05/07 23:21:29, jochen wrote:
>> > http://codereview.chromium.**org/10316019/diff/32016/**
>>
content/test/DEPS<http://codereview.chromium.org/10316019/diff/32016/content/test/DEPS>
>> > File content/test/DEPS (right):
>> >
>> >
>>
> http://codereview.chromium.**org/10316019/diff/32016/**
>
content/test/DEPS#newcode8<http://codereview.chromium.org/10316019/diff/32016/content/test/DEPS#newcode8>
>
>> > content/test/DEPS:8: "+ui/base/resource/resource_**bundle.h",
>> > please revert this CL:
>> >
>> > - this is a layering violation, ui/base/resource depends on chrome (see
>> > content/DEPS)
>> > - on Mac, the resource bundle is part of the Chrome framework, but
>> content
>> > itself doesn't build a framework
>>
>
>  Because of this second reason, this CL made all content_unittests fail in
>>
> debug
>
>> builds on mac. That's not visible on main and try waterfalls due to
>> http://crbug.com/126558 , but bots that don't shard tests have been red
>> since
>> this landed (e.g.
>> http://build.chromium.org/p/**chromium.fyi/builders/**
>>
Chromium%2520Mac%2520Ninja<http://build.chromium.org/p/chromium.fyi/builders/Chromium%2520Mac%2520Ninja>)
>>
>
>
>
>
http://codereview.chromium.**org/10316019/<http://codereview.chromium.org/103...
>