Initial support of large mouse cursor on Exosphere

Initial support of large mouse cursor on Exosphere

This patch adds support of large mouse cursor on Exosphere. If the large cursor set is selected, exosphere enlarges the cursor.

BUG=b/29169901
TEST=Run unit_tests, and test manualy

Committed: https://crrev.com/7aa92b14d0d99a8991856934a5ac32fcba0ad12b
Cr-Commit-Position: refs/heads/master@{#403141}

[yoshiki, 2016-06-15 17:03:10]

Description was changed from

==========
wip

BUG=
==========

to

==========
Initial support of large mouse cursor on Exosphere

This patch adds support of large mouse cursor on Exosphere. If the large cursor
set is selected, exosphere enlarges the cursor.

BUG=b/29169901
==========

[yoshiki, 2016-06-15 17:03:10]

yoshiki@chromium.org changed reviewers:
+ reveman@chromium.org

[yoshiki, 2016-06-15 17:03:45]

Description was changed from

==========
Initial support of large mouse cursor on Exosphere

This patch adds support of large mouse cursor on Exosphere. If the large cursor
set is selected, exosphere enlarges the cursor.

BUG=b/29169901
==========

to

==========
Initial support of large mouse cursor on Exosphere

This patch adds support of large mouse cursor on Exosphere. If the large cursor
set is selected, exosphere enlarges the cursor.

BUG=b/29169901
TEST=Run unit_tests, and do manual test
==========

[yoshiki, 2016-06-15 17:03:56]

Description was changed from

==========
Initial support of large mouse cursor on Exosphere

This patch adds support of large mouse cursor on Exosphere. If the large cursor
set is selected, exosphere enlarges the cursor.

BUG=b/29169901
TEST=Run unit_tests, and do manual test
==========

to

==========
Initial support of large mouse cursor on Exosphere

This patch adds support of large mouse cursor on Exosphere. If the large cursor
set is selected, exosphere enlarges the cursor.

BUG=b/29169901
TEST=Run unit_tests, and test manualy
==========

[yoshiki, 2016-06-15 17:16:53]

yoshiki@chromium.org changed reviewers:
+ oshima@chromium.org

[yoshiki, 2016-06-15 17:17:37]

yoshiki@chromium.org changed reviewers:
- oshima@chromium.org

[yoshiki, 2016-06-15 17:18:12]

David, could you take a first look (mostly at exo part)?

[reveman, 2016-06-15 17:48:30]

components/exo lgtm % nits

https://codereview.chromium.org/2071553002/diff/20001/components/exo/pointer.cc
File components/exo/pointer.cc (right):

https://codereview.chromium.org/2071553002/diff/20001/components/exo/pointer....
components/exo/pointer.cc:42: ash::Shell* ash_shell = ash::Shell::GetInstance();
nit: not sure this temporary variable is needed but if you add it here then
please do the same in dtor for consistency

https://codereview.chromium.org/2071553002/diff/20001/components/exo/pointer....
components/exo/pointer.cc:310: // TODO(yoshiki): Use large cursor asset instead
of scaling.
nit: Remove this TODO as we should still apply the scale after making the client
aware of it. If the client is aware of this scale, then it can provide a larger
resolution buffer and the result of this scale is not going to be blurred from
bi-linear filtering. This works exactly the same way as device scale factor for
non-cursor surfaces.

https://codereview.chromium.org/2071553002/diff/20001/components/exo/pointer.h
File components/exo/pointer.h (right):

https://codereview.chromium.org/2071553002/diff/20001/components/exo/pointer....
components/exo/pointer.h:68: void ResetCursor();
nit: UpdateCursorScale() seems like a better name for this function.

[yoshiki, 2016-06-20 02:40:54]

Patchset #3 (id:40001) has been deleted

[yoshiki, 2016-06-20 04:47:50]

Patchset #3 (id:60001) has been deleted

[yoshiki, 2016-06-20 04:52:48]

yoshiki@chromium.org changed reviewers:
+ sky@chromium.org

[yoshiki, 2016-06-20 04:52:48]

sky@, could you take a look at changes in /ui and /ash?

reveman@, could you take a look again just in case?

[reveman, 2016-06-20 05:01:34]

https://codereview.chromium.org/2071553002/diff/80001/components/exo/pointer.h
File components/exo/pointer.h (right):

https://codereview.chromium.org/2071553002/diff/80001/components/exo/pointer....
components/exo/pointer.h:92: bool use_large_cursor_;
This member state looks unnecessary. Can we just check
"cursor_manager->GetCursorSet() == ui::CURSOR_SET_LARGE" in UpdateCursorScale()
instead?

[yoshiki, 2016-06-20 07:12:35]

https://codereview.chromium.org/2071553002/diff/80001/components/exo/pointer.h
File components/exo/pointer.h (right):

https://codereview.chromium.org/2071553002/diff/80001/components/exo/pointer....
components/exo/pointer.h:92: bool use_large_cursor_;
On 2016/06/20 05:01:34, reveman wrote:
> This member state looks unnecessary. Can we just check
> "cursor_manager->GetCursorSet() == ui::CURSOR_SET_LARGE" in
UpdateCursorScale()
> instead?

Good point. Done.

[reveman, 2016-06-20 14:01:33]

components/exo lgtm

[sky, 2016-06-20 15:27:51]

https://codereview.chromium.org/2071553002/diff/100001/ash/wm/window_manager_...
File ash/wm/window_manager_unittest.cc (right):

https://codereview.chromium.org/2071553002/diff/100001/ash/wm/window_manager_...
ash/wm/window_manager_unittest.cc:38: cursor_visibility_ =
did_visibility_change_ = false;
Shouldn't you reset cursor_set_ too?

https://codereview.chromium.org/2071553002/diff/100001/ui/aura/client/cursor_...
File ui/aura/client/cursor_client_observer.h (right):

https://codereview.chromium.org/2071553002/diff/100001/ui/aura/client/cursor_...
ui/aura/client/cursor_client_observer.h:17: virtual void
OnCursorSetChanged(ui::CursorSetType cursor_set) {};
nit: no ';' here and previous line.

https://codereview.chromium.org/2071553002/diff/100001/ui/wm/core/cursor_mana...
File ui/wm/core/cursor_manager.cc (right):

https://codereview.chromium.org/2071553002/diff/100001/ui/wm/core/cursor_mana...
ui/wm/core/cursor_manager.cc:207: LOG(ERROR) <<
observers_.might_have_observers();
remove this.

https://codereview.chromium.org/2071553002/diff/100001/ui/wm/core/cursor_mana...
File ui/wm/core/cursor_manager_unittest.cc (right):

https://codereview.chromium.org/2071553002/diff/100001/ui/wm/core/cursor_mana...
ui/wm/core/cursor_manager_unittest.cc:55: class TestingCursorClientObserver :
public aura::client::CursorClientObserver {
How about moving this into testing_cursor_client_observer.h/.cc so that it isn't
duplicated?

https://codereview.chromium.org/2071553002/diff/100001/ui/wm/core/cursor_mana...
ui/wm/core/cursor_manager_unittest.cc:64: did_cursor_set_change_ = false;
Shouldn't you reset cursor_set_ here too?

[yoshiki, 2016-06-28 02:12:42]

sky@, PTAL. Thanks

https://codereview.chromium.org/2071553002/diff/100001/ash/wm/window_manager_...
File ash/wm/window_manager_unittest.cc (right):

https://codereview.chromium.org/2071553002/diff/100001/ash/wm/window_manager_...
ash/wm/window_manager_unittest.cc:38: cursor_visibility_ =
did_visibility_change_ = false;
On 2016/06/20 15:27:51, sky wrote:
> Shouldn't you reset cursor_set_ too?

Done.

https://codereview.chromium.org/2071553002/diff/100001/ui/aura/client/cursor_...
File ui/aura/client/cursor_client_observer.h (right):

https://codereview.chromium.org/2071553002/diff/100001/ui/aura/client/cursor_...
ui/aura/client/cursor_client_observer.h:17: virtual void
OnCursorSetChanged(ui::CursorSetType cursor_set) {};
On 2016/06/20 15:27:51, sky wrote:
> nit: no ';' here and previous line.

Done.

https://codereview.chromium.org/2071553002/diff/100001/ui/wm/core/cursor_mana...
File ui/wm/core/cursor_manager.cc (right):

https://codereview.chromium.org/2071553002/diff/100001/ui/wm/core/cursor_mana...
ui/wm/core/cursor_manager.cc:207: LOG(ERROR) <<
observers_.might_have_observers();
On 2016/06/20 15:27:51, sky wrote:
> remove this.

Done.

https://codereview.chromium.org/2071553002/diff/100001/ui/wm/core/cursor_mana...
File ui/wm/core/cursor_manager_unittest.cc (right):

https://codereview.chromium.org/2071553002/diff/100001/ui/wm/core/cursor_mana...
ui/wm/core/cursor_manager_unittest.cc:55: class TestingCursorClientObserver :
public aura::client::CursorClientObserver {
On 2016/06/20 15:27:51, sky wrote:
> How about moving this into testing_cursor_client_observer.h/.cc so that it
isn't
> duplicated?

Done.

https://codereview.chromium.org/2071553002/diff/100001/ui/wm/core/cursor_mana...
ui/wm/core/cursor_manager_unittest.cc:64: did_cursor_set_change_ = false;
On 2016/06/20 15:27:51, sky wrote:
> Shouldn't you reset cursor_set_ here too?

Done.

[sky, 2016-06-28 16:39:37]

LGTM with the following

https://codereview.chromium.org/2071553002/diff/160001/ui/wm/test/testing_cur...
File ui/wm/test/testing_cursor_client_observer.h (right):

https://codereview.chromium.org/2071553002/diff/160001/ui/wm/test/testing_cur...
ui/wm/test/testing_cursor_client_observer.h:10: class
TestingCursorClientObserver : public aura::client::CursorClientObserver {
Add description.

https://codereview.chromium.org/2071553002/diff/160001/ui/wm/wm.gyp
File ui/wm/wm.gyp (right):

https://codereview.chromium.org/2071553002/diff/160001/ui/wm/wm.gyp#newcode111
ui/wm/wm.gyp:111: 'test/testing_cursor_client_observer.cc',
You need to update BUILD.gn in this directory too.

[yoshiki, 2016-06-30 11:04:54]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2016-06-30 11:04:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2071553002/#ps220001
(title: "Add dependency to wm::test_support from ash::unittests")

[commit-bot: I haz the power, 2016-06-30 11:05:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 11:51:21]

Description was changed from

==========
Initial support of large mouse cursor on Exosphere

This patch adds support of large mouse cursor on Exosphere. If the large cursor
set is selected, exosphere enlarges the cursor.

BUG=b/29169901
TEST=Run unit_tests, and test manualy
==========

to

==========
Initial support of large mouse cursor on Exosphere

This patch adds support of large mouse cursor on Exosphere. If the large cursor
set is selected, exosphere enlarges the cursor.

BUG=b/29169901
TEST=Run unit_tests, and test manualy
==========

[commit-bot: I haz the power, 2016-06-30 11:51:22]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-06-30 11:53:11]

Description was changed from

==========
Initial support of large mouse cursor on Exosphere

This patch adds support of large mouse cursor on Exosphere. If the large cursor
set is selected, exosphere enlarges the cursor.

BUG=b/29169901
TEST=Run unit_tests, and test manualy
==========

to

==========
Initial support of large mouse cursor on Exosphere

This patch adds support of large mouse cursor on Exosphere. If the large cursor
set is selected, exosphere enlarges the cursor.

BUG=b/29169901
TEST=Run unit_tests, and test manualy

Committed: https://crrev.com/7aa92b14d0d99a8991856934a5ac32fcba0ad12b
Cr-Commit-Position: refs/heads/master@{#403141}
==========

[commit-bot: I haz the power, 2016-06-30 11:53:12]

Patchset 10 (id:??) landed as
https://crrev.com/7aa92b14d0d99a8991856934a5ac32fcba0ad12b
Cr-Commit-Position: refs/heads/master@{#403141}