Do not let MagnificationController move cursor on login screen when cursor is invisible.

Do not let MagnificationController move cursor on login screen when cursor is invisible.

BUG=157918
TEST=Manually on device

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

[mazda, 2012-11-28 02:16:16]

Could you review this change?

derat: OWENERS review for ash
yoshiki: magnification_controller

Thanks,

[yoshiki, 2012-11-28 05:00:14]

https://codereview.chromium.org/11316214/diff/1/ash/magnifier/magnification_c...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/11316214/diff/1/ash/magnifier/magnification_c...
ash/magnifier/magnification_controller.cc:166:
scale_(std::numeric_limits<double>::min()) {
Could you change it from "std::numeric_limits<double>::min()" to "1.0f"? The
initial scale must be 1.0f.

https://codereview.chromium.org/11316214/diff/1/ash/magnifier/magnification_c...
ash/magnifier/magnification_controller.cc:361: if (!IsUserLoggedIn() &&
!cursor_client->IsCursorVisible())
I think you can remove "!IsUserLoggedIn()". We should keep the cursor hidden
after animation if it is hidden before animation, even when not on login screen.

[Daniel Erat, 2012-11-28 15:35:23]

https://codereview.chromium.org/11316214/diff/1/ash/magnifier/magnification_c...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/11316214/diff/1/ash/magnifier/magnification_c...
ash/magnifier/magnification_controller.cc:111:
Shell::GetInstance()->tray_delegate()->GetUserLoginStatus() :
it seems pretty strange to me how all of this code is going through the tray
delegate.  would the shell delegate make more sense?  it already has a
IsUserLoggedIn() method; does that do what you need?

GetUserLoginStatus() should probably also be moved from the tray delegate to the
shell delegate, but that's outside the scope of this change.

https://codereview.chromium.org/11316214/diff/1/ash/magnifier/magnification_c...
ash/magnifier/magnification_controller.cc:359: // When cursor is invisible on
login screen, do not move cursor nor show
nit: s/nor/or/, s/cursor/the cursor/g

("nor" would work in "... neither move the cursor nor show ...")

[mazda, 2012-11-28 19:27:15]

Please take another look.

https://codereview.chromium.org/11316214/diff/1/ash/magnifier/magnification_c...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/11316214/diff/1/ash/magnifier/magnification_c...
ash/magnifier/magnification_controller.cc:111:
Shell::GetInstance()->tray_delegate()->GetUserLoginStatus() :
On 2012/11/28 15:35:23, Daniel Erat wrote:
> it seems pretty strange to me how all of this code is going through the tray
> delegate.  would the shell delegate make more sense?  it already has a
> IsUserLoggedIn() method; does that do what you need?
> 
> GetUserLoginStatus() should probably also be moved from the tray delegate to
the
> shell delegate, but that's outside the scope of this change.

I filed a cleanup issue (http://crbug.com/163170) and will work on it in another
CL.
Thanks.

https://codereview.chromium.org/11316214/diff/1/ash/magnifier/magnification_c...
ash/magnifier/magnification_controller.cc:166:
scale_(std::numeric_limits<double>::min()) {
On 2012/11/28 05:00:14, yoshiki wrote:
> Could you change it from "std::numeric_limits<double>::min()" to "1.0f"? The
> initial scale must be 1.0f.

Done.

https://codereview.chromium.org/11316214/diff/1/ash/magnifier/magnification_c...
ash/magnifier/magnification_controller.cc:359: // When cursor is invisible on
login screen, do not move cursor nor show
On 2012/11/28 15:35:23, Daniel Erat wrote:
> nit: s/nor/or/, s/cursor/the cursor/g
> 
> ("nor" would work in "... neither move the cursor nor show ...")

Done.

https://codereview.chromium.org/11316214/diff/1/ash/magnifier/magnification_c...
ash/magnifier/magnification_controller.cc:361: if (!IsUserLoggedIn() &&
!cursor_client->IsCursorVisible())
On 2012/11/28 05:00:14, yoshiki wrote:
> I think you can remove "!IsUserLoggedIn()". We should keep the cursor hidden
> after animation if it is hidden before animation, even when not on login
screen.

Done.

[Daniel Erat, 2012-11-28 19:42:32]

lgtm

Thanks!

[yoshiki, 2012-11-30 06:59:56]

LGTM

[commit-bot: I haz the power, 2012-11-30 09:22:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/11316214/5001

[commit-bot: I haz the power, 2012-11-30 15:42:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/11316214/5001

[commit-bot: I haz the power, 2012-11-30 18:27:16]

Change committed as 170489