Do not scale cursor image for 2x image

Do not scale cursor image for 2x image

It's regressed in  https://codereview.chromium.org/226293005.

BUG=368365
TEST=CursorLoaderX11Test.Scale, also tested manually
TBR=sky@chromium.org

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

[tdanderson, 2014-05-12 13:53:08]

https://codereview.chromium.org/280133002/diff/2/ui/base/cursor/cursor_loader...
File ui/base/cursor/cursor_loader_x11.cc (right):

https://codereview.chromium.org/280133002/diff/2/ui/base/cursor/cursor_loader...
ui/base/cursor/cursor_loader_x11.cc:168: scale() / image_rep.scale(),
rotation(), &bitmap, &hotpoint);
What caused this regression in the first place?

[oshima, 2014-05-12 16:48:02]

On 2014/05/12 13:53:08, tdanderson wrote:
>
https://codereview.chromium.org/280133002/diff/2/ui/base/cursor/cursor_loader...
> File ui/base/cursor/cursor_loader_x11.cc (right):
> 
>
https://codereview.chromium.org/280133002/diff/2/ui/base/cursor/cursor_loader...
> ui/base/cursor/cursor_loader_x11.cc:168: scale() / image_rep.scale(),
> rotation(), &bitmap, &hotpoint);
> What caused this regression in the first place?

It's regressed in https://codereview.chromium.org/226293005. I updated the bug
description.

[tdanderson, 2014-05-12 17:30:24]

Thanks, lgtm with one small typo mentioned below:

https://codereview.chromium.org/280133002/diff/50001/ui/base/cursor/cursor_lo...
File ui/base/cursor/cursor_loader_x11.cc (right):

https://codereview.chromium.org/280133002/diff/50001/ui/base/cursor/cursor_lo...
ui/base/cursor/cursor_loader_x11.cc:179: // TODO(oshima|tdanderson): Support
rotation and fractional scale factor.
Note that I will likely not have time to work on this in the near future.

https://codereview.chromium.org/280133002/diff/50001/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

https://codereview.chromium.org/280133002/diff/50001/ui/base/x/x11_util.h#new...
ui/base/x/x11_util.h:69: // caller temust not free/modify the image. The
refcount of the newly created
typo?

[oshima, 2014-05-12 20:42:08]

I had to move the test to ash due to the lack of 2x assets in ui_unittests.

https://codereview.chromium.org/280133002/diff/50001/ui/base/cursor/cursor_lo...
File ui/base/cursor/cursor_loader_x11.cc (right):

https://codereview.chromium.org/280133002/diff/50001/ui/base/cursor/cursor_lo...
ui/base/cursor/cursor_loader_x11.cc:179: // TODO(oshima|tdanderson): Support
rotation and fractional scale factor.
On 2014/05/12 17:30:24, tdanderson wrote:
> Note that I will likely not have time to work on this in the near future.

No worry, the name in TODO does not mean the person who should work on it. It
simply means they're most familiar with the issue.

https://codereview.chromium.org/280133002/diff/50001/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

https://codereview.chromium.org/280133002/diff/50001/ui/base/x/x11_util.h#new...
ui/base/x/x11_util.h:69: // caller temust not free/modify the image. The
refcount of the newly created
On 2014/05/12 17:30:24, tdanderson wrote:
> typo?

oops, fixed. thank you for the catch.

[oshima, 2014-05-12 21:00:11]

derat -> x related change in ui/base

[Daniel Erat, 2014-05-12 21:19:48]

lgtm for ui/base/x

https://codereview.chromium.org/280133002/diff/130001/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

https://codereview.chromium.org/280133002/diff/130001/ui/base/x/x11_util.h#ne...
ui/base/x/x11_util.h:339: // Resets the cache used by GetXCursor(). Only useful
for tests that may delete
put this in an "#if defined(UNIT_TEST)" section instead?

[oshima, 2014-05-12 22:23:41]

https://codereview.chromium.org/280133002/diff/130001/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

https://codereview.chromium.org/280133002/diff/130001/ui/base/x/x11_util.h#ne...
ui/base/x/x11_util.h:339: // Resets the cache used by GetXCursor(). Only useful
for tests that may delete
On 2014/05/12 21:19:48, Daniel Erat wrote:
> put this in an "#if defined(UNIT_TEST)" section instead?

I don't think this works because this is used in non unit test cc file
(aura_test_helper.cc and cursor_loader_x11.cc) . I just tried and got
link/compile errors.

I can change the CursorLoaderX11's test method to return ::Cursor
and call this from unittest though. Let me know if you like this change, or have
another suggestion.

(Note that reset XCursorCache still have to be outside of UNIT_TEST as it's
called from AuraTestHelper)

[Daniel Erat, 2014-05-12 23:02:22]

https://codereview.chromium.org/280133002/diff/130001/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

https://codereview.chromium.org/280133002/diff/130001/ui/base/x/x11_util.h#ne...
ui/base/x/x11_util.h:339: // Resets the cache used by GetXCursor(). Only useful
for tests that may delete
On 2014/05/12 22:23:42, oshima wrote:
> On 2014/05/12 21:19:48, Daniel Erat wrote:
> > put this in an "#if defined(UNIT_TEST)" section instead?
> 
> I don't think this works because this is used in non unit test cc file
> (aura_test_helper.cc and cursor_loader_x11.cc) . I just tried and got
> link/compile errors.
> 
> I can change the CursorLoaderX11's test method to return ::Cursor
> and call this from unittest though. Let me know if you like this change, or
have
> another suggestion.
> 
> (Note that reset XCursorCache still have to be outside of UNIT_TEST as it's
> called from AuraTestHelper)

ah, never mind then. i was hoping that everything would get compiled with
UNIT_TEST defined when building tests, but maybe that's not how it works. this
seems fine to me, in that case (although i've maybe seen *ForTest() suffixes
added to testing-only method names more often than "test" namespaces being used
-- do you know if there's a presubmit check that things in the test namespace
don't get called from non-test code?)

[oshima, 2014-05-13 00:57:04]

On 2014/05/12 23:02:22, Daniel Erat wrote:
> https://codereview.chromium.org/280133002/diff/130001/ui/base/x/x11_util.h
> File ui/base/x/x11_util.h (right):
> 
>
https://codereview.chromium.org/280133002/diff/130001/ui/base/x/x11_util.h#ne...
> ui/base/x/x11_util.h:339: // Resets the cache used by GetXCursor(). Only
useful
> for tests that may delete
> On 2014/05/12 22:23:42, oshima wrote:
> > On 2014/05/12 21:19:48, Daniel Erat wrote:
> > > put this in an "#if defined(UNIT_TEST)" section instead?
> > 
> > I don't think this works because this is used in non unit test cc file
> > (aura_test_helper.cc and cursor_loader_x11.cc) . I just tried and got
> > link/compile errors.
> > 
> > I can change the CursorLoaderX11's test method to return ::Cursor
> > and call this from unittest though. Let me know if you like this change, or
> have
> > another suggestion.
> > 
> > (Note that reset XCursorCache still have to be outside of UNIT_TEST as it's
> > called from AuraTestHelper)
> 
> ah, never mind then. i was hoping that everything would get compiled with
> UNIT_TEST defined when building tests, but maybe that's not how it works. this
> seems fine to me, in that case (although i've maybe seen *ForTest() suffixes
> added to testing-only method names more often than "test" namespaces being
used
> -- do you know if there's a presubmit check that things in the test namespace
> don't get called from non-test code?)

https://codereview.chromium.org/285623004, but I'm also fine to rename functions
to XxxForTest. Please
let me know.

[Daniel Erat, 2014-05-13 00:57:28]

either way is fine with me; still lgtm

[oshima_google, 2014-05-13 00:58:52]

On 2014/05/13 00:57:28, Daniel Erat wrote:
> either way is fine with me; still lgtm

ok, i'll land this and presubmit change. thanks

[oshima_google, 2014-05-13 00:59:23]

tbr'ing sky for trivial change in ui/aura/test/aura_test_helper.cc

[oshima_google, 2014-05-13 00:59:27]

The CQ bit was checked by oshima@google.com

[commit-bot: I haz the power, 2014-05-13 01:01:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/280133002/130001

[commit-bot: I haz the power, 2014-05-13 05:25:27]

Change committed as 270028