Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2923773003:
Adding mojo calls for several lock screen related operations. (Closed)
|
Created:
3 years, 6 months ago by xiaoyinh(OOO Sep 11-29) Modified:
3 years, 6 months ago CC:
chromium-reviews, alemate+watch_chromium.org, sadrul, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, achuith+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, oshima+watch_chromium.org, kalyank, darin (slow to review), davemoore+watch_chromium.org, rkc Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding mojo calls for several lock screen related operations.
This CL does the following:
1. Adding Mojo calls to:
a. Notify chrome when user pod is focused/no user pod is focused,
and when user has reached maximum incorrect password attempts.
b. Send request to chrome to load wallpaper and sign out current user.
c. Notify lockscreen views when pin unlock is enabled/disabled.
2. Move common helper functions out of SignInScreenHandler into
lock_screen_utils so both WebUI handlers and Views based handlers
(ViewsScreenLocker) can use.
BUG=721524
Review-Url: https://codereview.chromium.org/2923773003
Cr-Commit-Position: refs/heads/master@{#479142}
Committed: https://chromium.googlesource.com/chromium/src/+/f534c4f1aef85310aed3ca0ab432eb1bd1ae9d73
Patch Set 1 #Patch Set 2 : trybot #Patch Set 3 : clean up and rebase #Patch Set 4 : clean up #
Total comments: 16
Patch Set 5 : comments and rebase #
Total comments: 16
Patch Set 6 : comments+rebase #
Total comments: 6
Patch Set 7 : comments and rebase #Messages
Total messages: 64 (44 generated)
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== covert JS calls in SigninScreenHandler into mojo. BUG=721524 ========== to ========== convert JS calls in SigninScreenHandler into mojo. BUG=721524 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== convert JS calls in SigninScreenHandler into mojo. BUG=721524 ========== to ========== add mojo calls for user pod. BUG=721524 ==========
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xiaoyinh@chromium.org changed reviewers: + jdufault@chromium.org
jdufault@, could you take a look first? Thanks!
https://codereview.chromium.org/2923773003/diff/60001/ash/public/interfaces/l... File ash/public/interfaces/lock_screen.mojom (right): https://codereview.chromium.org/2923773003/diff/60001/ash/public/interfaces/l... ash/public/interfaces/lock_screen.mojom:78: // Requests to enable pin unlock for user in the user pod. nit: // Notification if pin is enabled or disabled for the given user. https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/views_screen_locker.cc (right): https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/views_screen_locker.cc:135: // If pin storage is unavailable, authenticated by PIN must be false. nit: authenticated by PIN -> |authenticated_by_pin_| https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/views_screen_locker.cc:172: // |user| may be nullptr in kiosk mode or unit tests. nit: null is preferred in comments https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/views_screen_locker.h (right): https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/views_screen_locker.h:61: void UpdatePinKeyboardState(const AccountId& account_id); These methods should go above the overrides. https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/views_screen_locker.h:73: std::unique_ptr<AccountId> focused_pod_account_id_; base::Optional - avoids heap allocation - I'm guessing it conveys intent more accurately https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock_screen_utils.cc (right): https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock_screen_utils.cc:34: DVLOG(0) << "SetUserInputMethod('" << username I don't think we should be logging username (PII) https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock_screen_utils.cc:79: if (users_last_input_methods != nullptr) { drop != nullptr drop {} https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock_screen_utils.h (right): https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock_screen_utils.h:36: bool Contains(const std::vector<std::string>& container, Replace all instances of this with base::ContainsValue
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2923773003/diff/60001/ash/public/interfaces/l... File ash/public/interfaces/lock_screen.mojom (right): https://codereview.chromium.org/2923773003/diff/60001/ash/public/interfaces/l... ash/public/interfaces/lock_screen.mojom:78: // Requests to enable pin unlock for user in the user pod. On 2017/06/08 21:12:06, jdufault wrote: > nit: // Notification if pin is enabled or disabled for the given user. Thanks! Done. https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/views_screen_locker.cc (right): https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/views_screen_locker.cc:135: // If pin storage is unavailable, authenticated by PIN must be false. On 2017/06/08 21:12:06, jdufault wrote: > nit: authenticated by PIN -> |authenticated_by_pin_| Done. https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/views_screen_locker.cc:172: // |user| may be nullptr in kiosk mode or unit tests. On 2017/06/08 21:12:06, jdufault wrote: > nit: null is preferred in comments Done. https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/views_screen_locker.h (right): https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/views_screen_locker.h:61: void UpdatePinKeyboardState(const AccountId& account_id); On 2017/06/08 21:12:06, jdufault wrote: > These methods should go above the overrides. Done. https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/views_screen_locker.h:73: std::unique_ptr<AccountId> focused_pod_account_id_; On 2017/06/08 21:12:06, jdufault wrote: > base::Optional > > - avoids heap allocation > - I'm guessing it conveys intent more accurately Done. https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock_screen_utils.cc (right): https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock_screen_utils.cc:34: DVLOG(0) << "SetUserInputMethod('" << username On 2017/06/08 21:12:06, jdufault wrote: > I don't think we should be logging username (PII) Removed, thanks! https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock_screen_utils.cc:79: if (users_last_input_methods != nullptr) { On 2017/06/08 21:12:06, jdufault wrote: > drop != nullptr > drop {} Done. https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock_screen_utils.h (right): https://codereview.chromium.org/2923773003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock_screen_utils.h:36: bool Contains(const std::vector<std::string>& container, On 2017/06/08 21:12:06, jdufault wrote: > Replace all instances of this with base::ContainsValue Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
xiaoyinh@chromium.org changed reviewers: + xiyuan@chromium.org
xiyuan@, could you help to take a look?
lgtm https://codereview.chromium.org/2923773003/diff/80001/ash/public/interfaces/l... File ash/public/interfaces/lock_screen.mojom (right): https://codereview.chromium.org/2923773003/diff/80001/ash/public/interfaces/l... ash/public/interfaces/lock_screen.mojom:130: MaxIncorrectPasswordAttempts(signin.mojom.AccountId account_id); nit: OnMaxIncorrectPasswordAttemptsReached? https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/views_screen_locker.cc (right): https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/views_screen_locker.cc:145: user_context.SetKey(key); nit: construct the key inline without a temporary variable? https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock_screen_utils.cc (right): https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock_screen_utils.cc:47: DLOG(WARNING) << "GetUserLastInputMethod('" << username don't log username https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock_screen_utils.cc:70: LOG(WARNING) << "SetUserInputMethod('" << username don't log username https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock_screen_utils.cc:86: DLOG(ERROR) << "SigninScreenHandler::SetUserInputMethod('" << username don't log username
https://codereview.chromium.org/2923773003/diff/80001/ash/public/interfaces/l... File ash/public/interfaces/lock_screen.mojom (right): https://codereview.chromium.org/2923773003/diff/80001/ash/public/interfaces/l... ash/public/interfaces/lock_screen.mojom:118: FocusPod(signin.mojom.AccountId account_id); nit: This and NoPodFocused are more like notifications/events of what happened on UI. Prefer to call them OnPodFocused and OnNoPodFocused. https://codereview.chromium.org/2923773003/diff/80001/ash/public/interfaces/l... ash/public/interfaces/lock_screen.mojom:130: MaxIncorrectPasswordAttempts(signin.mojom.AccountId account_id); nit: MaxIncorrectPasswordAttempts -> OnMaxIncorrectPasswordAttempted https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/views_screen_locker.cc (right): https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/views_screen_locker.cc:197: lock_screen_utils::EnforcePolicyInputMethods(std::string()); reset |focused_pod_account_id_| ?
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2923773003/diff/80001/ash/public/interfaces/l... File ash/public/interfaces/lock_screen.mojom (right): https://codereview.chromium.org/2923773003/diff/80001/ash/public/interfaces/l... ash/public/interfaces/lock_screen.mojom:118: FocusPod(signin.mojom.AccountId account_id); On 2017/06/12 18:17:13, xiyuan wrote: > nit: This and NoPodFocused are more like notifications/events of what happened > on UI. Prefer to call them OnPodFocused and OnNoPodFocused. Done. https://codereview.chromium.org/2923773003/diff/80001/ash/public/interfaces/l... ash/public/interfaces/lock_screen.mojom:130: MaxIncorrectPasswordAttempts(signin.mojom.AccountId account_id); On 2017/06/12 18:17:12, xiyuan wrote: > nit: MaxIncorrectPasswordAttempts -> OnMaxIncorrectPasswordAttempted Done. https://codereview.chromium.org/2923773003/diff/80001/ash/public/interfaces/l... ash/public/interfaces/lock_screen.mojom:130: MaxIncorrectPasswordAttempts(signin.mojom.AccountId account_id); On 2017/06/12 18:05:49, jdufault wrote: > nit: OnMaxIncorrectPasswordAttemptsReached? Thanks! Changed to OnMaxIncorrectPasswordAttempted. https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/views_screen_locker.cc (right): https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/views_screen_locker.cc:145: user_context.SetKey(key); On 2017/06/12 18:05:49, jdufault wrote: > nit: construct the key inline without a temporary variable? Done. https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/views_screen_locker.cc:197: lock_screen_utils::EnforcePolicyInputMethods(std::string()); On 2017/06/12 18:17:13, xiyuan wrote: > reset |focused_pod_account_id_| ? Missed this... Thanks! https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock_screen_utils.cc (right): https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock_screen_utils.cc:47: DLOG(WARNING) << "GetUserLastInputMethod('" << username On 2017/06/12 18:05:49, jdufault wrote: > don't log username Done. https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock_screen_utils.cc:70: LOG(WARNING) << "SetUserInputMethod('" << username On 2017/06/12 18:05:49, jdufault wrote: > don't log username Done. https://codereview.chromium.org/2923773003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock_screen_utils.cc:86: DLOG(ERROR) << "SigninScreenHandler::SetUserInputMethod('" << username On 2017/06/12 18:05:49, jdufault wrote: > don't log username Done.
lgtm
On 2017/06/12 22:16:24, xiyuan wrote: > lgtm Can you add more details to the CL description? It is not helpful to know what is changed in the current form. Subject line is a sentence and should start with an upper case letter.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== add mojo calls for user pod. BUG=721524 ========== to ========== This CL does the following: 1. Adding Mojo calls to: a. Notify chrome when user pod is focused/no user pod is focused, and when user has reached maximum incorrect password attempts. b. Send request to chrome to load wallpaper and sign out current user. c. Notify lockscreen views when pin unlock is enabled/disabled. 2. Move common helper functions out of SignInScreenHandler into lock_screen_utils so both WebUI handlers and Views based handlers(ViewsScreenLocker) can use. BUG=721524 ==========
On 2017/06/12 22:17:47, xiyuan wrote: > On 2017/06/12 22:16:24, xiyuan wrote: > > lgtm > > Can you add more details to the CL description? It is not helpful to know what > is changed in the current form. > > Subject line is a sentence and should start with an upper case letter. Updated. Please let me know if this looks good.
xiaoyinh@chromium.org changed reviewers: + jamescook@chromium.org, rsesek@chromium.org
+jamescook@ for chrome/browser/ui/ash +rsesek@ for ash/public/interfaces/lock_screen.mojom Could you help to take a look? Thanks!
LGTM with nits. Please change the first line of the description to match the "subject" in reitveld -- I don't think you've actually given it the title you want. https://codereview.chromium.org/2923773003/diff/100001/ash/public/interfaces/... File ash/public/interfaces/lock_screen.mojom (right): https://codereview.chromium.org/2923773003/diff/100001/ash/public/interfaces/... ash/public/interfaces/lock_screen.mojom:118: OnFocusPod(signin.mojom.AccountId account_id); Aside, nothing to do: It's unfortunate that ash and chrome have to communicate about such low-level details like which pod has focus. https://codereview.chromium.org/2923773003/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/lock_screen_client.cc (right): https://codereview.chromium.org/2923773003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/lock_screen_client.cc:80: if (!delegate_) optional: If all these methods do is forward things to the delegate then if (delegate_) delegate_->DoFoo(); is one line shorter. :-) https://codereview.chromium.org/2923773003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/lock_screen_client.cc:89: } It's a little odd that every mojo method here just calls a method with the same name on the delegate. For a future CL, consider making the delegate (ViewsScreenLocker?) directly implement the mojo interface.
Description was changed from ========== This CL does the following: 1. Adding Mojo calls to: a. Notify chrome when user pod is focused/no user pod is focused, and when user has reached maximum incorrect password attempts. b. Send request to chrome to load wallpaper and sign out current user. c. Notify lockscreen views when pin unlock is enabled/disabled. 2. Move common helper functions out of SignInScreenHandler into lock_screen_utils so both WebUI handlers and Views based handlers(ViewsScreenLocker) can use. BUG=721524 ========== to ========== Adding mojo calls for several lock screen related operations. This CL does the following: 1. Adding Mojo calls to: a. Notify chrome when user pod is focused/no user pod is focused, and when user has reached maximum incorrect password attempts. b. Send request to chrome to load wallpaper and sign out current user. c. Notify lockscreen views when pin unlock is enabled/disabled. 2. Move common helper functions out of SignInScreenHandler into lock_screen_utils so both WebUI handlers and Views based handlers(ViewsScreenLocker) can use. BUG=721524 ==========
On 2017/06/12 23:37:28, xiaoyinh wrote: > On 2017/06/12 22:17:47, xiyuan wrote: > > On 2017/06/12 22:16:24, xiyuan wrote: > > > lgtm > > > > Can you add more details to the CL description? It is not helpful to know what > > is changed in the current form. > > > > Subject line is a sentence and should start with an upper case letter. > > Updated. Please let me know if this looks good. One last request: please wrap lines around 75-80 chars. Looks good otherwise. Thanks.
lgtm
Description was changed from
==========
Adding mojo calls for several lock screen related operations.
This CL does the following:
1. Adding Mojo calls to:
a. Notify chrome when user pod is focused/no user pod is focused, and when
user has reached maximum incorrect password attempts.
b. Send request to chrome to load wallpaper and sign out current user.
c. Notify lockscreen views when pin unlock is enabled/disabled.
2. Move common helper functions out of SignInScreenHandler into
lock_screen_utils so both WebUI handlers and Views based
handlers(ViewsScreenLocker) can use.
BUG=721524
==========
to
==========
Adding mojo calls for several lock screen related operations.---------1--------0
This CL does the following:
1. Adding Mojo calls to:
a. Notify chrome when user pod is focused/no user pod is focused,
and when user has reached maximum incorrect password attempts.
b. Send request to chrome to load wallpaper and sign out current user.
c. Notify lockscreen views when pin unlock is enabled/disabled.
2. Move common helper functions out of SignInScreenHandler into
lock_screen_utils so both WebUI handlers and Views based
handlers(ViewsScreenLocker) can use.
BUG=721524
==========
Description was changed from
==========
Adding mojo calls for several lock screen related operations.---------1--------0
This CL does the following:
1. Adding Mojo calls to:
a. Notify chrome when user pod is focused/no user pod is focused,
and when user has reached maximum incorrect password attempts.
b. Send request to chrome to load wallpaper and sign out current user.
c. Notify lockscreen views when pin unlock is enabled/disabled.
2. Move common helper functions out of SignInScreenHandler into
lock_screen_utils so both WebUI handlers and Views based
handlers(ViewsScreenLocker) can use.
BUG=721524
==========
to
==========
Adding mojo calls for several lock screen related operations.
This CL does the following:
1. Adding Mojo calls to:
a. Notify chrome when user pod is focused/no user pod is focused,
and when user has reached maximum incorrect password attempts.
b. Send request to chrome to load wallpaper and sign out current user.
c. Notify lockscreen views when pin unlock is enabled/disabled.
2. Move common helper functions out of SignInScreenHandler into
lock_screen_utils so both WebUI handlers and Views based
handlers(ViewsScreenLocker) can use.
BUG=721524
==========
Description was changed from
==========
Adding mojo calls for several lock screen related operations.
This CL does the following:
1. Adding Mojo calls to:
a. Notify chrome when user pod is focused/no user pod is focused,
and when user has reached maximum incorrect password attempts.
b. Send request to chrome to load wallpaper and sign out current user.
c. Notify lockscreen views when pin unlock is enabled/disabled.
2. Move common helper functions out of SignInScreenHandler into
lock_screen_utils so both WebUI handlers and Views based
handlers(ViewsScreenLocker) can use.
BUG=721524
==========
to
==========
Adding mojo calls for several lock screen related operations.
This CL does the following:
1. Adding Mojo calls to:
a. Notify chrome when user pod is focused/no user pod is focused,
and when user has reached maximum incorrect password attempts.
b. Send request to chrome to load wallpaper and sign out current user.
c. Notify lockscreen views when pin unlock is enabled/disabled.
2. Move common helper functions out of SignInScreenHandler into
lock_screen_utils
so both WebUI handlers and Views based handlers(ViewsScreenLocker) can use.
BUG=721524
==========
Description was changed from
==========
Adding mojo calls for several lock screen related operations.
This CL does the following:
1. Adding Mojo calls to:
a. Notify chrome when user pod is focused/no user pod is focused,
and when user has reached maximum incorrect password attempts.
b. Send request to chrome to load wallpaper and sign out current user.
c. Notify lockscreen views when pin unlock is enabled/disabled.
2. Move common helper functions out of SignInScreenHandler into
lock_screen_utils
so both WebUI handlers and Views based handlers(ViewsScreenLocker) can use.
BUG=721524
==========
to
==========
Adding mojo calls for several lock screen related operations.
This CL does the following:
1. Adding Mojo calls to:
a. Notify chrome when user pod is focused/no user pod is focused,
and when user has reached maximum incorrect password attempts.
b. Send request to chrome to load wallpaper and sign out current user.
c. Notify lockscreen views when pin unlock is enabled/disabled.
2. Move common helper functions out of SignInScreenHandler into
lock_screen_utils so both WebUI handlers and Views based
handlers(ViewsScreenLocker) can use.
BUG=721524
==========
Description was changed from
==========
Adding mojo calls for several lock screen related operations.
This CL does the following:
1. Adding Mojo calls to:
a. Notify chrome when user pod is focused/no user pod is focused,
and when user has reached maximum incorrect password attempts.
b. Send request to chrome to load wallpaper and sign out current user.
c. Notify lockscreen views when pin unlock is enabled/disabled.
2. Move common helper functions out of SignInScreenHandler into
lock_screen_utils so both WebUI handlers and Views based
handlers(ViewsScreenLocker) can use.
BUG=721524
==========
to
==========
Adding mojo calls for several lock screen related operations.
This CL does the following:
1. Adding Mojo calls to:
a. Notify chrome when user pod is focused/no user pod is focused,
and when user has reached maximum incorrect password attempts.
b. Send request to chrome to load wallpaper and sign out current user.
c. Notify lockscreen views when pin unlock is enabled/disabled.
2. Move common helper functions out of SignInScreenHandler into
lock_screen_utils so both WebUI handlers and Views based handlers
(ViewsScreenLocker) can use.
BUG=721524
==========
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2923773003/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/lock_screen_client.cc (right): https://codereview.chromium.org/2923773003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/lock_screen_client.cc:80: if (!delegate_) On 2017/06/13 00:58:04, James Cook wrote: > optional: If all these methods do is forward things to the delegate then > > if (delegate_) > delegate_->DoFoo(); > > is one line shorter. :-) Done. https://codereview.chromium.org/2923773003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/lock_screen_client.cc:89: } On 2017/06/13 00:58:04, James Cook wrote: > It's a little odd that every mojo method here just calls a method with the same > name on the delegate. For a future CL, consider making the delegate > (ViewsScreenLocker?) directly implement the mojo interface. Thanks for the suggestion. Do you mean to have 2 separate code paths for calls ash->chrome(LockScreenController->ViewsScreenLocker) and chrome->ash(LockScreenClient->LockScreenController)? Right now LockScreenClient will handle mojo method directly(like LoadWallpaper, SignOutUser etc.) if they don't need to store some internal state. ViewsScreenLocker in the other hand will store/update data like focused_pod_account_id_ for mojo method OnFocusPod and OnNoPodFocused.
On 2017/06/13 17:42:33, xiyuan wrote: > On 2017/06/12 23:37:28, xiaoyinh wrote: > > On 2017/06/12 22:17:47, xiyuan wrote: > > > On 2017/06/12 22:16:24, xiyuan wrote: > > > > lgtm > > > > > > Can you add more details to the CL description? It is not helpful to know > what > > > is changed in the current form. > > > > > > Subject line is a sentence and should start with an upper case letter. > > > > Updated. Please let me know if this looks good. > > One last request: please wrap lines around 75-80 chars. Looks good otherwise. > Thanks. Done. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2923773003/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/lock_screen_client.cc (right): https://codereview.chromium.org/2923773003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/lock_screen_client.cc:89: } On 2017/06/13 18:36:56, xiaoyinh wrote: > On 2017/06/13 00:58:04, James Cook wrote: > > It's a little odd that every mojo method here just calls a method with the > same > > name on the delegate. For a future CL, consider making the delegate > > (ViewsScreenLocker?) directly implement the mojo interface. > > Thanks for the suggestion. Do you mean to have 2 separate code paths for calls > ash->chrome(LockScreenController->ViewsScreenLocker) and > chrome->ash(LockScreenClient->LockScreenController)? > > Right now LockScreenClient will handle mojo method directly(like LoadWallpaper, > SignOutUser etc.) if they don't need to store some internal state. > ViewsScreenLocker in the other hand will store/update data like > focused_pod_account_id_ for mojo method OnFocusPod and OnNoPodFocused. It's just something to think about. I established this FooController/FooControllerClient pattern for mojo in ash, but it's not necessary to use it everywhere. It's most useful when both sides (Controller and ControllerClient) both need to cache state.
On 2017/06/13 20:38:34, James Cook wrote: > https://codereview.chromium.org/2923773003/diff/100001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/lock_screen_client.cc (right): > > https://codereview.chromium.org/2923773003/diff/100001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/lock_screen_client.cc:89: } > On 2017/06/13 18:36:56, xiaoyinh wrote: > > On 2017/06/13 00:58:04, James Cook wrote: > > > It's a little odd that every mojo method here just calls a method with the > > same > > > name on the delegate. For a future CL, consider making the delegate > > > (ViewsScreenLocker?) directly implement the mojo interface. > > > > Thanks for the suggestion. Do you mean to have 2 separate code paths for calls > > ash->chrome(LockScreenController->ViewsScreenLocker) and > > chrome->ash(LockScreenClient->LockScreenController)? > > > > Right now LockScreenClient will handle mojo method directly(like > LoadWallpaper, > > SignOutUser etc.) if they don't need to store some internal state. > > ViewsScreenLocker in the other hand will store/update data like > > focused_pod_account_id_ for mojo method OnFocusPod and OnNoPodFocused. > > It's just something to think about. I established this > FooController/FooControllerClient pattern for mojo in ash, but it's not > necessary to use it everywhere. It's most useful when both sides (Controller and > ControllerClient) both need to cache state. Thanks! I will think about this in a later CL.
The CQ bit was checked by xiaoyinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org, rsesek@chromium.org, xiyuan@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2923773003/#ps120001 (title: "comments and rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497386642301800,
"parent_rev": "fc3a18b2ecfaec67f409b94d3832fa49c8dbd81e", "commit_rev":
"f534c4f1aef85310aed3ca0ab432eb1bd1ae9d73"}
Message was sent while issue was closed.
Description was changed from
==========
Adding mojo calls for several lock screen related operations.
This CL does the following:
1. Adding Mojo calls to:
a. Notify chrome when user pod is focused/no user pod is focused,
and when user has reached maximum incorrect password attempts.
b. Send request to chrome to load wallpaper and sign out current user.
c. Notify lockscreen views when pin unlock is enabled/disabled.
2. Move common helper functions out of SignInScreenHandler into
lock_screen_utils so both WebUI handlers and Views based handlers
(ViewsScreenLocker) can use.
BUG=721524
==========
to
==========
Adding mojo calls for several lock screen related operations.
This CL does the following:
1. Adding Mojo calls to:
a. Notify chrome when user pod is focused/no user pod is focused,
and when user has reached maximum incorrect password attempts.
b. Send request to chrome to load wallpaper and sign out current user.
c. Notify lockscreen views when pin unlock is enabled/disabled.
2. Move common helper functions out of SignInScreenHandler into
lock_screen_utils so both WebUI handlers and Views based handlers
(ViewsScreenLocker) can use.
BUG=721524
Review-Url: https://codereview.chromium.org/2923773003
Cr-Commit-Position: refs/heads/master@{#479142}
Committed:
https://chromium.googlesource.com/chromium/src/+/f534c4f1aef85310aed3ca0ab432...
==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f534c4f1aef85310aed3ca0ab432... |
