|
|
Created:
3 years, 7 months ago by Roman Sorokin (ftl) Modified:
3 years, 6 months ago CC:
chromium-reviews, srahim+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromad: Create AuthPolicyCredentialsManager
AuthPolicyCredentialsManager KeyedService service is created for every
Active Directory user profile.
It calls GetUserStatus at the start of service, each hour and on every
network connection.
Tests are coming...
BUG=662400
Review-Url: https://codereview.chromium.org/2860443002
Cr-Commit-Position: refs/heads/master@{#476248}
Committed: https://chromium.googlesource.com/chromium/src/+/7ccdfaf7c1639a6f1e12d206fea9a608b6c33f44
Patch Set 1 #
Total comments: 55
Patch Set 2 : Rebase, Update after review #
Total comments: 4
Patch Set 3 : Update after review #
Total comments: 10
Patch Set 4 : More addressing comments #
Total comments: 35
Patch Set 5 : Address comments #Patch Set 6 : Fix notification id. #Patch Set 7 : Fix notification id. #
Total comments: 4
Patch Set 8 : Address comments #
Total comments: 10
Patch Set 9 : Update after review #
Total comments: 2
Patch Set 10 : nits #
Messages
Total messages: 45 (24 generated)
The CQ bit was checked by rsorokin@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...
rsorokin@chromium.org changed reviewers: + ljusten@chromium.org, tnagel@chromium.org
Hi Thiemo, Lutz, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:9566: Your password has expired. Please sign out then sign in again to change it. 'sign out AND sign in again'? https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:9572: Failed to get authentication token. Please sign out then sign in again to try again. Should we bother the user with this info? Maybe something more general like "Failed to authenticate with server"? Also see above. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:50: void FixSignIn(); Comment? From the class definition, shouldn't it be called TryLogOut or something like that? https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:94: StopObserveNetwork(); Cancel scheduled_get_user_status_call_? Note that it keeps a plain pointer to AuthPolicyCredentialsManager and the destructor doesn't cancel it afaik. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:125: DCHECK(error == authpolicy::ERROR_CONTACTING_KDC_FAILED); Suggest remove. This is dangerous. Even though ERROR_CONTACTING_KDC_FAILED is most likely, there could be other errors, too, e.g. when the machine gets removed from the server, you'll probably get ERROR_BAD_MACHINE_NAME. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:134: GetUserStatus(); Why do you update names if should_call_get_status_again_, but not notifications? https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:178: if (chromeos::NetworkHandler::IsInitialized()) Since this can fail, return bool? Or DCHECK? https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:184: if (chromeos::NetworkHandler::IsInitialized()) Since this can fail, return bool? Or DCHECK? https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:211: std::string notification_id = const https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:217: message_center::NotifierId notifier_id( const https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:282: KeyedService* AuthPolicyCredentialsManagerFactory::BuildServiceInstanceFor( Unused? https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:6: #define CHROME_BROWSER_CHROMEOS_AUTHPOLICY_AUTH_POLICY_CREDENTIALS_MANAGER_H_ Suggest to use 'authpolicy' instead of 'auth_policy' for filenames for consistency. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:27: Please add class-level comment. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:67: // failed. The comment contradicts the implementation. It calls GetUserStatus if the previous call succeeded. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:68: void CallGetStatusIfConnected(const chromeos::NetworkState* network_state); CallGetUserStatusIfConnected? https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:74: authpolicy::ErrorType last_error_ = authpolicy::ERROR_UNKNOWN; ERROR_NONE? ERROR_UNKNOWN is an actual error, default state should be clean. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:80: Please add class-level comment.
The CQ bit was checked by rsorokin@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...
Thanks Lutz, PTAL! https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:9566: Your password has expired. Please sign out then sign in again to change it. On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > 'sign out AND sign in again'? Why? Anyway let's ask David to look at it :) https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:50: void FixSignIn(); On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > Comment? From the class definition, shouldn't it be called TryLogOut or > something like that? Done. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:94: StopObserveNetwork(); On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > Cancel scheduled_get_user_status_call_? Note that it keeps a plain pointer to > AuthPolicyCredentialsManager and the destructor doesn't cancel it afaik. It does. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:125: DCHECK(error == authpolicy::ERROR_CONTACTING_KDC_FAILED); On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > Suggest remove. This is dangerous. Even though ERROR_CONTACTING_KDC_FAILED is > most likely, there could be other errors, too, e.g. when the machine gets > removed from the server, you'll probably get ERROR_BAD_MACHINE_NAME. Done. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:134: GetUserStatus(); On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > Why do you update names if should_call_get_status_again_, but not notifications? We don't want to spam notifications. Updating names is non-disruptive for the user. Also could be the case when we won't get names on the next call because of some error. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:178: if (chromeos::NetworkHandler::IsInitialized()) On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > Since this can fail, return bool? Or DCHECK? Done. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:184: if (chromeos::NetworkHandler::IsInitialized()) On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > Since this can fail, return bool? Or DCHECK? Done. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:211: std::string notification_id = On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > const Done. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:217: message_center::NotifierId notifier_id( On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > const Nope https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:282: KeyedService* AuthPolicyCredentialsManagerFactory::BuildServiceInstanceFor( On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > Unused? This is the factory method. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:6: #define CHROME_BROWSER_CHROMEOS_AUTHPOLICY_AUTH_POLICY_CREDENTIALS_MANAGER_H_ On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > Suggest to use 'authpolicy' instead of 'auth_policy' for filenames for > consistency. Yes, we don't have consistency now. But it should be auth_policy. So I leave it as is. And then we'll rename the files later. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:27: On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > Please add class-level comment. Done. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:67: // failed. On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > The comment contradicts the implementation. It calls GetUserStatus if the > previous call succeeded. Done. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:68: void CallGetStatusIfConnected(const chromeos::NetworkState* network_state); On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > CallGetUserStatusIfConnected? Done. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:74: authpolicy::ErrorType last_error_ = authpolicy::ERROR_UNKNOWN; On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > ERROR_NONE? ERROR_UNKNOWN is an actual error, default state should be clean. Not sure about it. Could be the case when network connected between the first call and response. It won't schedule another call in case last_error would be ERROR_NONE https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:80: On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > Please add class-level comment. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:9566: Your password has expired. Please sign out then sign in again to change it. On 2017/05/19 12:13:46, Roman Sorokin (ftl) wrote: > On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > > 'sign out AND sign in again'? > > Why? Anyway let's ask David to look at it :) Acknowledged. https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:9572: Failed to get authentication token. Please sign out then sign in again to try again. On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > Should we bother the user with this info? Maybe something more general like > "Failed to authenticate with server"? > > Also see above. Acknowledged. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:50: void FixSignIn(); On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > Comment? From the class definition, shouldn't it be called TryLogOut or > something like that? β https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:94: StopObserveNetwork(); On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > Cancel scheduled_get_user_status_call_? Note that it keeps a plain pointer to > AuthPolicyCredentialsManager and the destructor doesn't cancel it afaik. Nm, it actually is cancelled. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:134: GetUserStatus(); On 2017/05/19 12:13:47, Roman Sorokin (ftl) wrote: > On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > > Why do you update names if should_call_get_status_again_, but not > notifications? > > We don't want to spam notifications. Updating names is non-disruptive for the > user. Also could be the case when we won't get names on the next call because of > some error. But then the next call could fail and the user would never be notified. How about you only ever show the notification once per session? https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:140: if (user_status.has_password_status()) { DCHECK instead. It should always be present. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:154: if (user_status.has_tgt_status()) { DCHECK instead. It should always be present. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:217: message_center::NotifierId notifier_id( On 2017/05/19 12:13:47, Roman Sorokin (ftl) wrote: > On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > > const > > Nope Acknowledged. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:247: if (last_error_ != authpolicy::ERROR_NONE) Modify as discussed offline to handle the following race condition: 1) last_error is none, GetUserStatus() is run 2) Network disconnects 3) Network reconnects (since error is none, should_call_get_status_again_ is set to false) 4) GetUserStatus() call returns, failed because of network outage --> No new GetUserStatus() call is scheduled even though network came back online. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:6: #define CHROME_BROWSER_CHROMEOS_AUTHPOLICY_AUTH_POLICY_CREDENTIALS_MANAGER_H_ On 2017/05/19 12:13:48, Roman Sorokin (ftl) wrote: > On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > > Suggest to use 'authpolicy' instead of 'auth_policy' for filenames for > > consistency. > > Yes, we don't have consistency now. But it should be auth_policy. So I leave it > as is. And then we'll rename the files later. Meh. I'd rather rename the classes Authpolicy* than changing everything to auth_policy. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:74: authpolicy::ErrorType last_error_ = authpolicy::ERROR_UNKNOWN; On 2017/05/19 12:13:48, Roman Sorokin (ftl) wrote: > On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > > ERROR_NONE? ERROR_UNKNOWN is an actual error, default state should be clean. > > Not sure about it. Could be the case when network connected between the first > call and response. It won't schedule another call in case last_error would be > ERROR_NONE With what we discussed offline, this case will be handled and the default error can be ERROR_NONE. https://codereview.chromium.org/2860443002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:28: // A service responsible to track user credential status. Created for each "for tracking" - "to" can only be used with a person, e.g. "responsible to the president for cleaning his carpet" https://codereview.chromium.org/2860443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:68: // Call GetUserStatus if |network_state| is connected and previous call *the* previous call
The CQ bit was checked by rsorokin@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...
Thanks, Lutz! PTAL. https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2860443002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:9566: Your password has expired. Please sign out then sign in again to change it. On 2017/05/19 16:06:13, ljusten (instantaneous) wrote: > On 2017/05/19 12:13:46, Roman Sorokin (ftl) wrote: > > On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > > > 'sign out AND sign in again'? > > > > Why? Anyway let's ask David to look at it :) > > Acknowledged. Acknowledged. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:50: void FixSignIn(); On 2017/05/19 16:06:13, ljusten (instantaneous) wrote: > On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > > Comment? From the class definition, shouldn't it be called TryLogOut or > > something like that? > > β wat? https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:94: StopObserveNetwork(); On 2017/05/19 16:06:13, ljusten (instantaneous) wrote: > On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > > Cancel scheduled_get_user_status_call_? Note that it keeps a plain pointer to > > AuthPolicyCredentialsManager and the destructor doesn't cancel it afaik. > > Nm, it actually is cancelled. Acknowledged. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:134: GetUserStatus(); On 2017/05/19 16:06:13, ljusten (instantaneous) wrote: > On 2017/05/19 12:13:47, Roman Sorokin (ftl) wrote: > > On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > > > Why do you update names if should_call_get_status_again_, but not > > notifications? > > > > We don't want to spam notifications. Updating names is non-disruptive for the > > user. Also could be the case when we won't get names on the next call because > of > > some error. > > But then the next call could fail and the user would never be notified. How > about you only ever show the notification once per session? Yeah, it's a good idea https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:140: if (user_status.has_password_status()) { On 2017/05/19 16:06:13, ljusten (instantaneous) wrote: > DCHECK instead. It should always be present. Done. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:154: if (user_status.has_tgt_status()) { On 2017/05/19 16:06:13, ljusten (instantaneous) wrote: > DCHECK instead. It should always be present. Done. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:217: message_center::NotifierId notifier_id( On 2017/05/19 16:06:13, ljusten (instantaneous) wrote: > On 2017/05/19 12:13:47, Roman Sorokin (ftl) wrote: > > On 2017/05/03 10:12:16, ljusten (instantaneous) wrote: > > > const > > > > Nope > > Acknowledged. Acknowledged. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:247: if (last_error_ != authpolicy::ERROR_NONE) On 2017/05/19 16:06:13, ljusten (instantaneous) wrote: > Modify as discussed offline to handle the following race condition: > 1) last_error is none, GetUserStatus() is run > 2) Network disconnects > 3) Network reconnects (since error is none, should_call_get_status_again_ is set > to false) > 4) GetUserStatus() call returns, failed because of network outage > --> No new GetUserStatus() call is scheduled even though network came back > online. Done. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:6: #define CHROME_BROWSER_CHROMEOS_AUTHPOLICY_AUTH_POLICY_CREDENTIALS_MANAGER_H_ On 2017/05/19 16:06:13, ljusten (instantaneous) wrote: > On 2017/05/19 12:13:48, Roman Sorokin (ftl) wrote: > > On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > > > Suggest to use 'authpolicy' instead of 'auth_policy' for filenames for > > > consistency. > > > > Yes, we don't have consistency now. But it should be auth_policy. So I leave > it > > as is. And then we'll rename the files later. > > Meh. I'd rather rename the classes Authpolicy* than changing everything to > auth_policy. Acknowledged. https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:74: authpolicy::ErrorType last_error_ = authpolicy::ERROR_UNKNOWN; On 2017/05/19 16:06:13, ljusten (instantaneous) wrote: > On 2017/05/19 12:13:48, Roman Sorokin (ftl) wrote: > > On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > > > ERROR_NONE? ERROR_UNKNOWN is an actual error, default state should be clean. > > > > Not sure about it. Could be the case when network connected between the first > > call and response. It won't schedule another call in case last_error would be > > ERROR_NONE > > With what we discussed offline, this case will be handled and the default error > can be ERROR_NONE. Done. https://codereview.chromium.org/2860443002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:28: // A service responsible to track user credential status. Created for each On 2017/05/19 16:06:13, ljusten (instantaneous) wrote: > "for tracking" - "to" can only be used with a person, e.g. "responsible to the > president for cleaning his carpet" Done. https://codereview.chromium.org/2860443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:68: // Call GetUserStatus if |network_state| is connected and previous call On 2017/05/19 16:06:13, ljusten (instantaneous) wrote: > *the* previous call Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/1/chrome/browser/chromeos/aut... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:50: void FixSignIn(); On 2017/05/22 12:35:27, Roman Sorokin (ftl) wrote: > On 2017/05/19 16:06:13, ljusten (instantaneous) wrote: > > On 2017/05/03 10:12:17, ljusten (instantaneous) wrote: > > > Comment? From the class definition, shouldn't it be called TryLogOut or > > > something like that? > > > > β > > wat? beta = better (at least if you pronounce it like a German) https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:107: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); I'd suggest to cancel scheduled_get_user_status_call_ here, so that it doesn't fire while a callback is in-flight (it's not wrong, just unnecessary). https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:125: CHECK(user_status.account_info().account_id() == account_id_.GetObjGuid()); should_call_get_status_again_ = false; ? https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:129: ScheduleGetUserStatus(); Will this ever get rescheduled in case of error? https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:131: DCHECK(user_status.has_password_status()); I just realized that the code that sets this hasn't landed yet, so this will trigger in the meantime. https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:144: DCHECK(user_status.has_tgt_status()); Same here.
The CQ bit was checked by rsorokin@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...
Thanks Lutz, PTAL! https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:107: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2017/05/22 14:27:49, ljusten (instantaneous) wrote: > I'd suggest to cancel scheduled_get_user_status_call_ here, so that it doesn't > fire while a callback is in-flight (it's not wrong, just unnecessary). Done. https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:125: CHECK(user_status.account_info().account_id() == account_id_.GetObjGuid()); On 2017/05/22 14:27:49, ljusten (instantaneous) wrote: > should_call_get_status_again_ = false; ? Done. https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:129: ScheduleGetUserStatus(); On 2017/05/22 14:27:49, ljusten (instantaneous) wrote: > Will this ever get rescheduled in case of error? Done. https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:131: DCHECK(user_status.has_password_status()); On 2017/05/22 14:27:49, ljusten (instantaneous) wrote: > I just realized that the code that sets this hasn't landed yet, so this will > trigger in the meantime. Acknowledged. https://codereview.chromium.org/2860443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:144: DCHECK(user_status.has_tgt_status()); On 2017/05/22 14:27:49, ljusten (instantaneous) wrote: > Same here. Acknowledged.
Almost there... https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:122: last_error_ = error; I believe this will call GetUserStatus() over and over again if should_call_get_status_again_ is true and the calls keep failing. Do this instead: bool call_again = should_call_get_status_again_; should_call_get_status_again_ = false; ... if (call_again)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:9436: <message name="IDS_ACTIVE_DIRECTORY_PASSWORD_EXPIRED" desc="The text to display on the hyperlink when the user needs to sign out and sign in again to change their password"> Nit: End sentence with a period. https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:9439: <message name="IDS_ACTIVE_DIRECTORY_PASSWORD_CHANGED" desc="The text to display on the hyperlink when the user needs to sign out and sign in again to update local password to cryptohome"> Nit: End sentence with a period. https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:9440: Your password has been changed. Please sign out then sign in again to update it. Nit: Imho "to update it" should be dropped, because there's no way for the user to understand that "updating it" refers to the cryptohome. Also, I'd suggest to add "on the server" after "password has been changed" to make it clearer what has happened. https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:9442: <message name="IDS_ACTIVE_DIRECTORY_REFRESH_AUTH_TOKEN" desc="The text to display on the hyperlink when the user needs to sign out and sign in again to get authentication token"> Nit: End sentence with a period. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:32: const base::TimeDelta kGetUserStatusCallsInterval = Global objects must be constexpr. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:47: ~SigninNotificationDelegate() override; Are you sure this is necessary? Why not just use the default destructor? https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:76: : profile_(profile), weak_factory_(this) { Nit: For weak_factory_, I'd prefer in-class initialization: ... weak_factory_{this}; https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:77: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Nit: Any particular reason you're checking for this? Somehow I can't imagine someone trying to run this on a non-UI thread... (Same below.) https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:78: StartObserveNetwork(); Nit: I prefer to have CHECK()'s at the top of the function so that the assertions are easily glanceable. Could you please move StartObserveNetwork() below the CHECK()? https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:107: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Same as above. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:122: last_error_ = error; On 2017/05/22 16:52:00, ljusten (instantaneous) wrote: > I believe this will call GetUserStatus() over and over again if > should_call_get_status_again_ is true and the calls keep failing. Do this > instead: > bool call_again = should_call_get_status_again_; > should_call_get_status_again_ = false; > ... > if (call_again) Simply setting |should_call_get_status_again_| to false before the return should be sufficient, imho. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:163: &AuthPolicyCredentialsManager::GetUserStatus, base::Unretained(this))); Why is unretained safe here? Maybe add a comment? https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:164: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( Afaics, this does not re-schedule after wake from sleep (and thus the maximal interval between two calls can be (sleep time + kGetUserStatusCallsInterval)). No need to fix this right away, but I think it would be good to include a TODO. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:49: // See AuthPolicyClient::GetUserStatusCallback. Nit: Imho it's more readable to have a blank line above the comment. (Same below.) https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:75: bool should_call_get_status_again_ = false; The name of that bool imho doesn't do a great job describing what the variable does. What about e.g. |rerun_get_status_on_error_|? https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:76: bool was_notification_shown_ = false; Imho a single bool is not sufficient because we might want to show multiple notifications e.g. in case first the TGT expires and later the password changes on the server.
The CQ bit was checked by rsorokin@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...
Thiemo, Lutz, thanks for the review! PTAL https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:9436: <message name="IDS_ACTIVE_DIRECTORY_PASSWORD_EXPIRED" desc="The text to display on the hyperlink when the user needs to sign out and sign in again to change their password"> On 2017/05/24 10:11:04, Thiemo Nagel wrote: > Nit: End sentence with a period. Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:9439: <message name="IDS_ACTIVE_DIRECTORY_PASSWORD_CHANGED" desc="The text to display on the hyperlink when the user needs to sign out and sign in again to update local password to cryptohome"> On 2017/05/24 10:11:04, Thiemo Nagel wrote: > Nit: End sentence with a period. Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:9440: Your password has been changed. Please sign out then sign in again to update it. On 2017/05/24 10:11:04, Thiemo Nagel wrote: > Nit: Imho "to update it" should be dropped, because there's no way for the user > to understand that "updating it" refers to the cryptohome. Also, I'd suggest to > add "on the server" after "password has been changed" to make it clearer what > has happened. Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:9442: <message name="IDS_ACTIVE_DIRECTORY_REFRESH_AUTH_TOKEN" desc="The text to display on the hyperlink when the user needs to sign out and sign in again to get authentication token"> On 2017/05/24 10:11:04, Thiemo Nagel wrote: > Nit: End sentence with a period. Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:32: const base::TimeDelta kGetUserStatusCallsInterval = On 2017/05/24 10:11:05, Thiemo Nagel wrote: > Global objects must be constexpr. Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:47: ~SigninNotificationDelegate() override; On 2017/05/24 10:11:05, Thiemo Nagel wrote: > Are you sure this is necessary? Why not just use the default destructor? Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:76: : profile_(profile), weak_factory_(this) { On 2017/05/24 10:11:04, Thiemo Nagel wrote: > Nit: For weak_factory_, I'd prefer in-class initialization: > > ... weak_factory_{this}; Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:77: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2017/05/24 10:11:04, Thiemo Nagel wrote: > Nit: Any particular reason you're checking for this? Somehow I can't imagine > someone trying to run this on a non-UI thread... > > (Same below.) I dunno, just in case :) https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:78: StartObserveNetwork(); On 2017/05/24 10:11:04, Thiemo Nagel wrote: > Nit: I prefer to have CHECK()'s at the top of the function so that the > assertions are easily glanceable. Could you please move StartObserveNetwork() > below the CHECK()? Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:122: last_error_ = error; On 2017/05/22 16:52:00, ljusten (instantaneous) wrote: > I believe this will call GetUserStatus() over and over again if > should_call_get_status_again_ is true and the calls keep failing. Do this > instead: > bool call_again = should_call_get_status_again_; > should_call_get_status_again_ = false; > ... > if (call_again) Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:122: last_error_ = error; On 2017/05/24 10:11:04, Thiemo Nagel wrote: > On 2017/05/22 16:52:00, ljusten (instantaneous) wrote: > > I believe this will call GetUserStatus() over and over again if > > should_call_get_status_again_ is true and the calls keep failing. Do this > > instead: > > bool call_again = should_call_get_status_again_; > > should_call_get_status_again_ = false; > > ... > > if (call_again) > > Simply setting |should_call_get_status_again_| to false before the return should > be sufficient, imho. Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:163: &AuthPolicyCredentialsManager::GetUserStatus, base::Unretained(this))); On 2017/05/24 10:11:04, Thiemo Nagel wrote: > Why is unretained safe here? Maybe add a comment? Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:164: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( On 2017/05/24 10:11:04, Thiemo Nagel wrote: > Afaics, this does not re-schedule after wake from sleep (and thus the maximal > interval between two calls can be (sleep time + kGetUserStatusCallsInterval)). > No need to fix this right away, but I think it would be good to include a TODO. Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:49: // See AuthPolicyClient::GetUserStatusCallback. On 2017/05/24 10:11:05, Thiemo Nagel wrote: > Nit: Imho it's more readable to have a blank line above the comment. (Same > below.) Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:75: bool should_call_get_status_again_ = false; On 2017/05/24 10:11:05, Thiemo Nagel wrote: > The name of that bool imho doesn't do a great job describing what the variable > does. What about e.g. |rerun_get_status_on_error_|? Done. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:76: bool was_notification_shown_ = false; On 2017/05/24 10:11:05, Thiemo Nagel wrote: > Imho a single bool is not sufficient because we might want to show multiple > notifications e.g. in case first the TGT expires and later the password changes > on the server. Done.
Thank you! Almost done. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:77: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > I dunno, just in case :) What is your code doing that requires running on the UI thread? If you're calling into some UI-thread-specific code, shouldn't it be the job of that code to check for UI thread? Reason I keep going about this is that I'm afraid that enforcing specific threads makes servicification harder which afaiu might bring about more pervasive multithreading. Thus imho we shouldn't enforce running on a specific thread without good reason. https://codereview.chromium.org/2860443002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:162: // Unretained is safe here because the callback is owned by this object. As discussed offline: CancelableCallback is the reason. https://codereview.chromium.org/2860443002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:81: std::unordered_set<int> shown_notifications_; Hash imho doesn't seem to be a great choice to store at max 3 integers. Why not use std::set?
Thanks, Thiemo! PTAL. https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:77: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2017/05/26 12:46:21, Thiemo Nagel wrote: > > I dunno, just in case :) > > What is your code doing that requires running on the UI thread? If you're > calling into some UI-thread-specific code, shouldn't it be the job of that code > to check for UI thread? > > Reason I keep going about this is that I'm afraid that enforcing specific > threads makes servicification harder which afaiu might bring about more > pervasive multithreading. Thus imho we shouldn't enforce running on a specific > thread without good reason. make sense! thanks! https://codereview.chromium.org/2860443002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:162: // Unretained is safe here because the callback is owned by this object. On 2017/05/26 12:46:21, Thiemo Nagel wrote: > As discussed offline: CancelableCallback is the reason. Done. https://codereview.chromium.org/2860443002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:81: std::unordered_set<int> shown_notifications_; On 2017/05/26 12:46:21, Thiemo Nagel wrote: > Hash imho doesn't seem to be a great choice to store at max 3 integers. Why not > use std::set? Done.
Thank you! Lgtm!
rsorokin@chromium.org changed reviewers: + erg@chromium.org, xiyuan@chromium.org
Hi Elliot, PTAL at chrome/browser/profiles/profile_impl.cc Hi Xiyuan, PTAL.
https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:85: StopObserveNetwork(); What if this is called after OnShuttingDown() is called ? https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:74: Profile* profile_; nit: Profile* const profile_; https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:76: std::string display_name_, given_name_; nit: split this to declare each field on its own line https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:81: std::set<int> shown_notifications_; nit: #include <set>
lgtm
Thanks Xiyuan! PTAL. https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc (right): https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc:85: StopObserveNetwork(); On 2017/05/30 17:32:58, xiyuan wrote: > What if this is called after OnShuttingDown() is called ? Done. https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:74: Profile* profile_; On 2017/05/30 17:32:58, xiyuan wrote: > nit: Profile* const profile_; Unfortunately this function https://cs.chromium.org/chromium/src/chrome/browser/notifications/notificatio... takes non-const. https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:76: std::string display_name_, given_name_; On 2017/05/30 17:32:58, xiyuan wrote: > nit: split this to declare each field on its own line Done. https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:81: std::set<int> shown_notifications_; On 2017/05/30 17:32:58, xiyuan wrote: > nit: #include <set> Done.
lgtm with nits https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:74: Profile* profile_; On 2017/05/31 15:37:10, Roman Sorokin (ftl) wrote: > On 2017/05/30 17:32:58, xiyuan wrote: > > nit: Profile* const profile_; > > Unfortunately this function > https://cs.chromium.org/chromium/src/chrome/browser/notifications/notificatio... > takes non-const. I mean "Profile* const" since |profile_| always points to the same Profile instance for the lifetime of this class. It is different from "const Profile*", which declares a pointer to "const Profile". https://codereview.chromium.org/2860443002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:81: bool is_observing_network = false; is_observing_network -> is_observing_network_
The CQ bit was checked by rsorokin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org, erg@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2860443002/#ps180001 (title: "nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:74: Profile* profile_; On 2017/05/31 15:41:59, xiyuan wrote: > On 2017/05/31 15:37:10, Roman Sorokin (ftl) wrote: > > On 2017/05/30 17:32:58, xiyuan wrote: > > > nit: Profile* const profile_; > > > > Unfortunately this function > > > https://cs.chromium.org/chromium/src/chrome/browser/notifications/notificatio... > > takes non-const. > > I mean "Profile* const" since |profile_| always points to the same Profile > instance for the lifetime of this class. > > It is different from "const Profile*", which declares a pointer to "const > Profile". Ah, my bad. Thanks! https://codereview.chromium.org/2860443002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h (right): https://codereview.chromium.org/2860443002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h:81: bool is_observing_network = false; On 2017/05/31 15:41:59, xiyuan wrote: > is_observing_network -> is_observing_network_ Done.
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496309487766060, "parent_rev": "04ac4aeaf7f8dea29ec16e58c724daafd6aa6389", "commit_rev": "7ccdfaf7c1639a6f1e12d206fea9a608b6c33f44"}
Message was sent while issue was closed.
Description was changed from ========== Chromad: Create AuthPolicyCredentialsManager AuthPolicyCredentialsManager KeyedService service is created for every Active Directory user profile. It calls GetUserStatus at the start of service, each hour and on every network connection. Tests are coming... BUG=662400 ========== to ========== Chromad: Create AuthPolicyCredentialsManager AuthPolicyCredentialsManager KeyedService service is created for every Active Directory user profile. It calls GetUserStatus at the start of service, each hour and on every network connection. Tests are coming... BUG=662400 Review-Url: https://codereview.chromium.org/2860443002 Cr-Commit-Position: refs/heads/master@{#476248} Committed: https://chromium.googlesource.com/chromium/src/+/7ccdfaf7c1639a6f1e12d206fea9... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/7ccdfaf7c1639a6f1e12d206fea9... |