hidpi support for custom cursors in windows

hidpi support for custom cursors in windows

BUG=161614
Committed: https://crrev.com/027fb1e25f5d850f6ba479315212262287300e3a
Cr-Commit-Position: refs/heads/master@{#365723}

[Bret, 2015-12-15 22:27:00]

bsep@chromium.org changed reviewers:
+ anantha@chromium.org, jam@chromium.org

[Bret, 2015-12-15 23:11:06]

Description was changed from

==========
hidpi support for custom cursors in windows

BUG=161614
==========

to

==========
hidpi support for custom cursors in windows

BUG=161614
==========

[Bret, 2015-12-15 23:11:06]

bsep@chromium.org changed reviewers:
+ ananta@chromium.org
- anantha@chromium.org

[ananta, 2015-12-15 23:28:17]

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
File content/common/cursors/webcursor.cc (right):

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.cc:229: std::vector<char>& custom_data,
Fix alignment for the remaining parameters to be on the same column.

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
File content/common/cursors/webcursor.h (right):

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.h:114: void
CreateScaledBitmapAndHotspotFromCustomData(
Newline between functions

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.h:115: SkBitmap* bitmap,
Fix indent to 4 spaces.

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.h:154: void CreateCustomData(const SkBitmap&
bitmap,
Newline between functions

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.h:154: void CreateCustomData(const SkBitmap&
bitmap,
Some comments for this function would be helpful.

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.h:155: std::vector<char>& custom_data,
We don't use reference parameters for output as per convention. Please change to
pointers.

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.h:187: #if defined(USE_AURA)
USE_AURA is no longer optional. So remove the ifdef

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
File content/common/cursors/webcursor_aura.cc (right):

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor_aura.cc:116: return device_scale_factor_ /
custom_scale_;
do we need a dcheck for custom_scale_ == 0?

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
File content/common/cursors/webcursor_aurawin.cc (right):

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor_aurawin.cc:21: if (custom_data_.size() == 0)
should this be an error condition? and hence a dcheck?

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
File content/common/cursors/webcursor_unittest.cc (right):

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor_unittest.cc:283:
display.set_device_scale_factor(2.0f);
It would be good to add more test cases like 1.25/1.5, etc.

[Bret, 2015-12-16 19:03:59]

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
File content/common/cursors/webcursor.cc (right):

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.cc:229: std::vector<char>& custom_data,
On 2015/12/15 23:28:17, ananta wrote:
> Fix alignment for the remaining parameters to be on the same column.

Done.

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
File content/common/cursors/webcursor.h (right):

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.h:114: void
CreateScaledBitmapAndHotspotFromCustomData(
On 2015/12/15 23:28:17, ananta wrote:
> Newline between functions

Done.

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.h:115: SkBitmap* bitmap,
On 2015/12/15 23:28:17, ananta wrote:
> Fix indent to 4 spaces.

Done.

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.h:154: void CreateCustomData(const SkBitmap&
bitmap,
On 2015/12/15 23:28:17, ananta wrote:
> Newline between functions

Done.

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.h:154: void CreateCustomData(const SkBitmap&
bitmap,
On 2015/12/15 23:28:17, ananta wrote:
> Some comments for this function would be helpful.

Done.

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.h:155: std::vector<char>& custom_data,
On 2015/12/15 23:28:17, ananta wrote:
> We don't use reference parameters for output as per convention. Please change
to
> pointers.

Done.

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor.h:187: #if defined(USE_AURA)
On 2015/12/15 23:28:17, ananta wrote:
> USE_AURA is no longer optional. So remove the ifdef

This file is used by mac so the check needs to stay.

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
File content/common/cursors/webcursor_aura.cc (right):

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor_aura.cc:116: return device_scale_factor_ /
custom_scale_;
On 2015/12/15 23:28:17, ananta wrote:
> do we need a dcheck for custom_scale_ == 0?

Done.

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
File content/common/cursors/webcursor_aurawin.cc (right):

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor_aurawin.cc:21: if (custom_data_.size() == 0)
On 2015/12/15 23:28:17, ananta wrote:
> should this be an error condition? and hence a dcheck?

Done.

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
File content/common/cursors/webcursor_unittest.cc (right):

https://codereview.chromium.org/1525263004/diff/1/content/common/cursors/webc...
content/common/cursors/webcursor_unittest.cc:283:
display.set_device_scale_factor(2.0f);
On 2015/12/15 23:28:17, ananta wrote:
> It would be good to add more test cases like 1.25/1.5, etc.

Done.

[ananta, 2015-12-16 20:47:54]

lgtm % nits

https://codereview.chromium.org/1525263004/diff/20001/content/common/cursors/...
File content/common/cursors/webcursor.h (right):

https://codereview.chromium.org/1525263004/diff/20001/content/common/cursors/...
content/common/cursors/webcursor.h:193: float device_scale_factor_;
Please initialize this in the ctor

https://codereview.chromium.org/1525263004/diff/20001/content/common/cursors/...
File content/common/cursors/webcursor_unittest.cc (right):

https://codereview.chromium.org/1525263004/diff/20001/content/common/cursors/...
content/common/cursors/webcursor_unittest.cc:281: void ScaleCursor(float
scaleFactor, int initialX, int initialY) {
Please rename these to hotspot_x or something on those lines.

[Bret, 2015-12-16 22:27:52]

https://codereview.chromium.org/1525263004/diff/20001/content/common/cursors/...
File content/common/cursors/webcursor.h (right):

https://codereview.chromium.org/1525263004/diff/20001/content/common/cursors/...
content/common/cursors/webcursor.h:193: float device_scale_factor_;
On 2015/12/16 20:47:54, ananta wrote:
> Please initialize this in the ctor

Done.

https://codereview.chromium.org/1525263004/diff/20001/content/common/cursors/...
File content/common/cursors/webcursor_unittest.cc (right):

https://codereview.chromium.org/1525263004/diff/20001/content/common/cursors/...
content/common/cursors/webcursor_unittest.cc:281: void ScaleCursor(float
scaleFactor, int initialX, int initialY) {
On 2015/12/16 20:47:54, ananta wrote:
> Please rename these to hotspot_x or something on those lines.

Done.

[jam, 2015-12-16 23:08:34]

rubberstamp lgtm

[Bret, 2015-12-17 00:10:44]

The CQ bit was checked by bsep@chromium.org

[Bret, 2015-12-17 00:10:45]

The patchset sent to the CQ was uploaded after l-g-t-m from ananta@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1525263004/#ps60001
(title: ".")

[commit-bot: I haz the power, 2015-12-17 00:33:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1525263004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1525263004/60001

[commit-bot: I haz the power, 2015-12-17 03:48:38]

Description was changed from

==========
hidpi support for custom cursors in windows

BUG=161614
==========

to

==========
hidpi support for custom cursors in windows

BUG=161614
==========

[commit-bot: I haz the power, 2015-12-17 03:48:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-17 03:50:14]

Description was changed from

==========
hidpi support for custom cursors in windows

BUG=161614
==========

to

==========
hidpi support for custom cursors in windows

BUG=161614

Committed: https://crrev.com/027fb1e25f5d850f6ba479315212262287300e3a
Cr-Commit-Position: refs/heads/master@{#365723}
==========

[commit-bot: I haz the power, 2015-12-17 03:50:15]

Patchset 4 (id:??) landed as
https://crrev.com/027fb1e25f5d850f6ba479315212262287300e3a
Cr-Commit-Position: refs/heads/master@{#365723}

[Nico, 2015-12-17 12:04:15]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-12-17 12:04:16]

https://codereview.chromium.org/1525263004/diff/60001/content/common/cursors/...
File content/common/cursors/webcursor_unittest.cc (right):

https://codereview.chromium.org/1525263004/diff/60001/content/common/cursors/...
content/common/cursors/webcursor_unittest.cc:301:
EXPECT_EQ(static_cast<int>(scaleFactor * hotspotX), windowsIconInfo.xHotspot);
Does this intentionally not round? For the third call, scaleFactor is 1.25 and
hotspotX is 3, so the product is 3.75. Should that really be converted to 3 and
not 4?

[Bret, 2015-12-17 19:28:43]

On 2015/12/17 12:04:16, Nico (office move Wed Thu Fri) wrote:
>
https://codereview.chromium.org/1525263004/diff/60001/content/common/cursors/...
> File content/common/cursors/webcursor_unittest.cc (right):
> 
>
https://codereview.chromium.org/1525263004/diff/60001/content/common/cursors/...
> content/common/cursors/webcursor_unittest.cc:301:
> EXPECT_EQ(static_cast<int>(scaleFactor * hotspotX), windowsIconInfo.xHotspot);
> Does this intentionally not round? For the third call, scaleFactor is 1.25 and
> hotspotX is 3, so the product is 3.75. Should that really be converted to 3
and
> not 4?

I didn't modify the scaling function. If this is the wrong behavior we should
investigate ui::ScaleAndRotateCursorBitmapAndHotpoint.

[chris.araman, 2016-05-11 01:19:52]

1) device_scale_factor_ is initialized in the default ctor, but not in the copy
ctor.
2) device_scale_factor_ is not initialized in the webcursor_aurawin.cc
implementation of InitPlatformData.
3) device_scale_factor_ is not copied in the webcursor_aurawin.cc implementation
of CopyPlatformData.

These defects cause an out of memory exception in CEF, tracked here:
http://magpcss.org/ceforum/viewtopic.php?f=6&t=14130