2x Cursor support.

2x Cursor support.

BUG=110394
TEST=manual

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

[sky, 2012-05-22 19:49:12]

LGTM

http://codereview.chromium.org/10399118/diff/7012/ui/aura/root_window_host_li...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10399118/diff/7012/ui/aura/root_window_host_li...
ui/aura/root_window_host_linux.cc:362: const SkBitmap& bitmap = 
image->GetBitmapForScale(scale_factor_,
nit: you have two spaces here.

[oshima, 2012-05-22 19:53:08]

http://codereview.chromium.org/10399118/diff/7012/ui/aura/root_window_host_li...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10399118/diff/7012/ui/aura/root_window_host_li...
ui/aura/root_window_host_linux.cc:362: const SkBitmap& bitmap = 
image->GetBitmapForScale(scale_factor_,
On 2012/05/22 19:49:12, sky wrote:
> nit: you have two spaces here.

oops, fixed. Thanks

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

http://codereview.chromium.org/10399118/diff/14001/ui/aura/root_window_host_l...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10399118/diff/14001/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:310: LoadImageCursor(ui::kCursorNull,
IDR_AURA_CURSOR_PTR, 9, 5);
when I was trying to include all the new cursors, I noticed that most of the
cursor assets had empty boundaries and needed to be cropped to get rid of it (I
think I noted that here:
http://code.google.com/p/chromium/issues/detail?id=123655#c18 ). Do these hot
points take care of that? I had assigned that bug to kuscher to fix the assets.
Should we wait for the assets to be fixed instead of fixing the issue with hot
points?

http://codereview.chromium.org/10399118/diff/14001/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:366: // because the method above coputes the
actual scale from the image
computes

[oshima, 2012-05-22 20:23:05]

http://codereview.chromium.org/10399118/diff/14001/ui/aura/root_window_host_l...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10399118/diff/14001/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:310: LoadImageCursor(ui::kCursorNull,
IDR_AURA_CURSOR_PTR, 9, 5);
On 2012/05/22 20:12:01, varunjain wrote:
> when I was trying to include all the new cursors, I noticed that most of the
> cursor assets had empty boundaries and needed to be cropped to get rid of it
(I
> think I noted that here:
> http://code.google.com/p/chromium/issues/detail?id=123655#c18 ). Do these hot
> points take care of that?
Yes,

> I had assigned that bug to kuscher to fix the assets.
> Should we wait for the assets to be fixed instead of fixing the issue with hot
> points?

hot point is the right way to deal with it. Some cursor has hot point in the
middle
of the image (like move, double arrow) so you can't fix by cropping :)

http://codereview.chromium.org/10399118/diff/14001/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:366: // because the method above coputes the
actual scale from the image
On 2012/05/22 20:12:01, varunjain wrote:
> computes

Fixed. thanks.

[varunjain, 2012-05-22 20:32:20]

http://codereview.chromium.org/10399118/diff/14001/ui/aura/root_window_host_l...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10399118/diff/14001/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:310: LoadImageCursor(ui::kCursorNull,
IDR_AURA_CURSOR_PTR, 9, 5);
On 2012/05/22 20:23:05, oshima wrote:
> On 2012/05/22 20:12:01, varunjain wrote:
> > when I was trying to include all the new cursors, I noticed that most of the
> > cursor assets had empty boundaries and needed to be cropped to get rid of it
> (I
> > think I noted that here:
> > http://code.google.com/p/chromium/issues/detail?id=123655#c18 ). Do these
hot
> > points take care of that?
> Yes,
> 
> > I had assigned that bug to kuscher to fix the assets.
> > Should we wait for the assets to be fixed instead of fixing the issue with
hot
> > points?
> 
> hot point is the right way to deal with it. Some cursor has hot point in the
> middle
> of the image (like move, double arrow) so you can't fix by cropping :)

I did not mean we should not have hot points at all. Obviously hot points are
different for a lot of cursors. But seems to me that we should not be making
adjustments in the code based on bad assets.  For instance, IDR_AURA_CURSOR_PTR
is simply a left arrow cursor and should have hot point (0,0) but is right now
set at (9,5) because of the bad asset. I think this is fine too but would have
liked it if the asset was fixed instead. In any case, if we go with this then I
should probably update that bug so that no one wastes their time fixing the
assets :)

[oshima, 2012-05-22 20:46:06]

http://codereview.chromium.org/10399118/diff/14001/ui/aura/root_window_host_l...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/10399118/diff/14001/ui/aura/root_window_host_l...
ui/aura/root_window_host_linux.cc:310: LoadImageCursor(ui::kCursorNull,
IDR_AURA_CURSOR_PTR, 9, 5);
On 2012/05/22 20:32:21, varunjain wrote:
> On 2012/05/22 20:23:05, oshima wrote:
> > On 2012/05/22 20:12:01, varunjain wrote:
> > > when I was trying to include all the new cursors, I noticed that most of
the
> > > cursor assets had empty boundaries and needed to be cropped to get rid of
it
> > (I
> > > think I noted that here:
> > > http://code.google.com/p/chromium/issues/detail?id=123655#c18 ). Do these
> hot
> > > points take care of that?
> > Yes,
> > 
> > > I had assigned that bug to kuscher to fix the assets.
> > > Should we wait for the assets to be fixed instead of fixing the issue with
> hot
> > > points?
> > 
> > hot point is the right way to deal with it. Some cursor has hot point in the
> > middle
> > of the image (like move, double arrow) so you can't fix by cropping :)
> 
> I did not mean we should not have hot points at all. Obviously hot points are
> different for a lot of cursors. But seems to me that we should not be making
> adjustments in the code based on bad assets.  For instance,
IDR_AURA_CURSOR_PTR
> is simply a left arrow cursor and should have hot point (0,0) but is right now
> set at (9,5) because of the bad asset. I think this is fine too but would have
> liked it if the asset was fixed instead. In any case, if we go with this then
I
> should probably update that bug so that no one wastes their time fixing the
> assets :)

I don't think these are necessarily bad assets. Some cursor MAY have hot
point at 0,0 but that's just coincident and specific to the current image.
There is no guarantee that IDR_AURA_CURSOR_IPTR is always 0,0.

The right way is to embed this info in the cursor image so that designer can
specify
exactly where the hot spot should be without modifying the code.

I'm planning to look into similar issue for 9 patch (or 9 slice, whatever people
call it)
and will try to address this issue as well. (post 21, though)

[pkotwicz, 2012-05-23 15:07:00]

Oshima, can you please wait till the end of the day till I land another patch to
land this? Adding the 2x resource pack right now will trigger a dcheck because
not all of the 2x resources in the data pack are actually 2x

[oshima, 2012-05-23 15:14:28]

On 2012/05/23 15:07:00, pkotwicz wrote:
> Oshima, can you please wait till the end of the day till I land another patch
to
> land this? Adding the 2x resource pack right now will trigger a dcheck because
> not all of the 2x resources in the data pack are actually 2x

Which DCHECK it will trigger?
It will take a while until we have all 2x assets, so we need to disable it
if such DCHECK exists.

[flackr, 2012-05-23 15:49:04]

On 2012/05/23 15:14:28, oshima wrote:
> On 2012/05/23 15:07:00, pkotwicz wrote:
> > Oshima, can you please wait till the end of the day till I land another
patch
> to
> > land this? Adding the 2x resource pack right now will trigger a dcheck
because
> > not all of the 2x resources in the data pack are actually 2x
> 
> Which DCHECK it will trigger?
> It will take a while until we have all 2x assets, so we need to disable it
> if such DCHECK exists.

The DCHECK on src/ui/gfx/image/image_skia.cc:99 would fail except that
resource_bundle.cc currently ignores the passed in scale factor when creating
the DataPack. My CL https://chromiumcodereview.appspot.com/10412004/ will fix
this but I can't land it right now because of the DCHECK.

[oshima, 2012-05-23 16:13:04]

On Wed, May 23, 2012 at 8:49 AM, <flackr@chromium.org> wrote:

> On 2012/05/23 15:14:28, oshima wrote:
>
>> On 2012/05/23 15:07:00, pkotwicz wrote:
>> > Oshima, can you please wait till the end of the day till I land another
>>
> patch
>
>> to
>> > land this? Adding the 2x resource pack right now will trigger a dcheck
>>
> because
>
>> > not all of the 2x resources in the data pack are actually 2x
>>
>
>  Which DCHECK it will trigger?
>> It will take a while until we have all 2x assets, so we need to disable it
>> if such DCHECK exists.
>>
>
> The DCHECK on src/ui/gfx/image/image_skia.**cc:99 would fail except that
> resource_bundle.cc currently ignores the passed in scale factor when
> creating
> the DataPack. My CL
https://chromiumcodereview.**appspot.com/10412004/<https://chromiumcodereview...
fix
> this but I can't land it right now because of the DCHECK.
>
>
This DCHECK is wrong. 2x resources contains resources for UI for 2x
density, but
it doesn't necessarily means the size of it is 2x of dip. It is possible
for HDA code
to use non 2x sized image. This is true only for HDC code.

- oshima


>
http://codereview.chromium.**org/10399118/<http://codereview.chromium.org/103...
>