Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
907323002:
Implemented DeviceStatusCollector::GetDeviceSessionStatus() (Closed)
|
Created:
5 years, 10 months ago by Andrew T Wilson (Slow) Modified:
5 years, 9 months ago CC:
chromium-reviews, dzhioev+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplemented DeviceStatusCollector::GetDeviceSessionStatus()
GetDeviceSessionStatus() now generates information about the current active
kiosk session (if any).
BUG=430908
Committed: https://crrev.com/c6605bd30349ede3e1a9ea7657a338ec45036a0f
Cr-Commit-Position: refs/heads/master@{#316567}
Patch Set 1 #
Total comments: 43
Patch Set 2 : Review feedback #Patch Set 3 : Merge ToT #
Total comments: 29
Patch Set 4 : Review feedback. #
Messages
Total messages: 22 (7 generated)
atwilson@chromium.org changed reviewers: + bartfab@chromium.org
PTAL
I did not review includes yet. I will do that on the next pass, after the current comments are addressed. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/users/mock_user_manager.cc (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/users/mock_user_manager.cc:114: user_manager::User* user = user_manager::User::CreateKioskAppUser(email); Nit: s/user_manager::User* user/user_manager::User* const user/ https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/users/mock_user_manager.cc:115: user_list_.push_back(user); Nit: Why not push_back() the User object as it is created, and then refer to it via user_list_.back() throughout? This avoids the short stretch of code where |user| is owned via a raw pointer. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/users/mock_user_manager.h (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/users/mock_user_manager.h:130: user_manager::User* CreateKioskAppUser(const std::string& email); Nit: Please use |user_id| instead of |email|. We try to consistently use |user_id| in all new code. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:114: bool GetCurrentDeviceLocalAccount(chromeos::CrosSettings* settings, Nit: The name does not match what the method actually does. Device-local accounts are kiosk sessions and public sessions. This method handles kiosk sessions only. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:118: user_manager::User* user = user_manager::UserManager::Get()->GetActiveUser(); Nit: s/user_manager::User*/const user_manager::User* const/ https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:119: std::string user_id = user->GetUserID(); Nit: const. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:120: std::vector<policy::DeviceLocalAccount> accounts = Nit: const. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:121: policy::GetDeviceLocalAccounts(settings); It is possible to remove a device-local account from policy while a session using that account is running. You will run into a NOTREACHED() if that happens. Why do you not simply grab the kiosk app ID from the switches::kAppId command-line switch? You still need the account ID of course. This is available from the policy broker, even if the account was removed from policy. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:123: for (auto device_account : accounts) { Nit 1: s/device_account/device_local_account/ or s/device_account/account/. Let's not introduce new terminology ("device account"). Nit 2: s/auto/const auto&/ (no need for a copy where a reference will do) https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:129: NOTREACHED() << "Kiosk app user not found in list of local accounts"; Nit: s/local/device-local/ https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:418: DeviceLocalAccount account(DeviceLocalAccount::TYPE_KIOSK_APP); I think this implementation can be improved. The line above makes it look like passing TYPE_KIOSK_APP here is significant in some way while in reality, you just want a dummy DeviceLocalAccount you can write to. How about having GetCurrentDeviceLocalAccount() return a scoped_ptr instead? By doing it this way, you avoid the need to introduce a new DeviceLocalAccount constructor and prevent the TYPE_KIOSK_APP above from looking significant. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:425: return false; Is this not a NOTREACHED() case? https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:760: if (!GetCurrentDeviceLocalAccount(cros_settings_, &account)) You are doing all this fetching, parsing and searching of the device-local account list twice. It would be much nicer to hold on to the kiosk app ID you get the first time instead. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:767: std::string app_version = GetAppVersion(account.kiosk_app_id); Nit: const. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... File chrome/browser/chromeos/policy/device_status_collector.h (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.h:115: virtual std::string GetAppVersion(std::string app_id); Nit: Pass |app_id| by const ref. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:354: DeviceLocalAccount local_account( Nit: const. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:886: TEST_F(DeviceStatusCollectorTest, ReportSession) { Nit: s/ReportSession/ReportSessionStatus/ https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:890: // Setup a DeviceLocalAccount for a kiosk app. Nit: s/Setup DeviceLocalAccount for a kiosk app/Set up device-local account for single-app kiosk mode/ https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:892: em::SessionStatusReportRequest session_status; Nit: Add blank line above. The setup phase is done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:896: em::AppStatus app = session_status.installed_apps(0); Nit: const. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/sett... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/sett... chrome/browser/chromeos/settings/device_settings_provider.cc:315: if (reporting_policy.has_report_session_status()) { You need to add this new policy policy_templates.json. You also have to wire up decoding in device_policy_decoder_chromeos.cc so that the value shows up in chrome:// policy for debugging. While you are at it, please add |report_hardware_status| in those two places as well - it seems to have been missed.
PTAL https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/users/mock_user_manager.cc (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/users/mock_user_manager.cc:114: user_manager::User* user = user_manager::User::CreateKioskAppUser(email); On 2015/02/11 17:28:42, bartfab wrote: > Nit: s/user_manager::User* user/user_manager::User* const user/ Removed https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/users/mock_user_manager.cc:115: user_list_.push_back(user); On 2015/02/11 17:28:43, bartfab wrote: > Nit: Why not push_back() the User object as it is created, and then refer to it > via user_list_.back() throughout? This avoids the short stretch of code where > |user| is owned via a raw pointer. Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/users/mock_user_manager.h (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/users/mock_user_manager.h:130: user_manager::User* CreateKioskAppUser(const std::string& email); On 2015/02/11 17:28:43, bartfab wrote: > Nit: Please use |user_id| instead of |email|. We try to consistently use > |user_id| in all new code. Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:114: bool GetCurrentDeviceLocalAccount(chromeos::CrosSettings* settings, On 2015/02/11 17:28:43, bartfab wrote: > Nit: The name does not match what the method actually does. Device-local > accounts are kiosk sessions and public sessions. This method handles kiosk > sessions only. Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:114: bool GetCurrentDeviceLocalAccount(chromeos::CrosSettings* settings, On 2015/02/11 17:28:43, bartfab wrote: > Nit: The name does not match what the method actually does. Device-local > accounts are kiosk sessions and public sessions. This method handles kiosk > sessions only. Changed name to clarify the behavior and added a comment. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:118: user_manager::User* user = user_manager::UserManager::Get()->GetActiveUser(); On 2015/02/11 17:28:43, bartfab wrote: > Nit: s/user_manager::User*/const user_manager::User* const/ Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:119: std::string user_id = user->GetUserID(); On 2015/02/11 17:28:43, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:121: policy::GetDeviceLocalAccounts(settings); On 2015/02/11 17:28:43, bartfab wrote: > It is possible to remove a device-local account from policy while a session > using that account is running. You will run into a NOTREACHED() if that happens. > > Why do you not simply grab the kiosk app ID from the switches::kAppId > command-line switch? > > You still need the account ID of course. This is available from the policy > broker, even if the account was removed from policy. OK, good to know. I've changed the NOTREACHED into a LOG(WARNING) and now we just won't report on running kiosk sessions that are no longer part of policy. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:123: for (auto device_account : accounts) { On 2015/02/11 17:28:43, bartfab wrote: > Nit 1: s/device_account/device_local_account/ or s/device_account/account/. > Let's not introduce new terminology ("device account"). > Nit 2: s/auto/const auto&/ (no need for a copy where a reference will do) Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:129: NOTREACHED() << "Kiosk app user not found in list of local accounts"; On 2015/02/11 17:28:43, bartfab wrote: > Nit: s/local/device-local/ Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:418: DeviceLocalAccount account(DeviceLocalAccount::TYPE_KIOSK_APP); On 2015/02/11 17:28:43, bartfab wrote: > I think this implementation can be improved. The line above makes it look like > passing TYPE_KIOSK_APP here is significant in some way while in reality, you > just want a dummy DeviceLocalAccount you can write to. > > How about having GetCurrentDeviceLocalAccount() return a scoped_ptr instead? > > By doing it this way, you avoid the need to introduce a new DeviceLocalAccount > constructor and prevent the TYPE_KIOSK_APP above from looking significant. Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:425: return false; On 2015/02/11 17:28:43, bartfab wrote: > Is this not a NOTREACHED() case? Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:760: if (!GetCurrentDeviceLocalAccount(cros_settings_, &account)) On 2015/02/11 17:28:43, bartfab wrote: > You are doing all this fetching, parsing and searching of the device-local > account list twice. It would be much nicer to hold on to the kiosk app ID you > get the first time instead. Done. This cascaded into a yak shave that impacted the test code, but probably in a good way. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:767: std::string app_version = GetAppVersion(account.kiosk_app_id); On 2015/02/11 17:28:43, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... File chrome/browser/chromeos/policy/device_status_collector.h (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.h:115: virtual std::string GetAppVersion(std::string app_id); On 2015/02/11 17:28:43, bartfab wrote: > Nit: Pass |app_id| by const ref. Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:354: DeviceLocalAccount local_account( On 2015/02/11 17:28:43, bartfab wrote: > Nit: const. moot due to refactoring. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:886: TEST_F(DeviceStatusCollectorTest, ReportSession) { On 2015/02/11 17:28:43, bartfab wrote: > Nit: s/ReportSession/ReportSessionStatus/ Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:892: em::SessionStatusReportRequest session_status; On 2015/02/11 17:28:43, bartfab wrote: > Nit: Add blank line above. The setup phase is done. Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:896: em::AppStatus app = session_status.installed_apps(0); On 2015/02/11 17:28:43, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/sett... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/sett... chrome/browser/chromeos/settings/device_settings_provider.cc:315: if (reporting_policy.has_report_session_status()) { On 2015/02/11 17:28:43, bartfab wrote: > You need to add this new policy policy_templates.json. You also have to wire up > decoding in device_policy_decoder_chromeos.cc so that the value shows up in > chrome:// policy for debugging. > > While you are at it, please add |report_hardware_status| in those two places as > well - it seems to have been missed. This is part of the other CL which I will land before this.
atwilson@chromium.org changed reviewers: + ygorshenin@chromium.org
ygorshenin: PTAL at chromeos/login and comment-only change to chromeos/ownership.
https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:121: policy::GetDeviceLocalAccounts(settings); On 2015/02/11 17:28:43, bartfab wrote: > You still need the account ID of course. This is available from the policy > broker, even if the account was removed from policy. BTW, not using the policy broker because it's basically impossible to mock so testing is hugely problematic. Let me know if for some reason going through the active user to figure out which device-local account is active is bad. Otherwise I'm going to leave as-is.
lgtm https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/907323002/diff/1/chrome/browser/chromeos/poli... chrome/browser/chromeos/policy/device_status_collector.cc:121: policy::GetDeviceLocalAccounts(settings); On 2015/02/13 13:59:59, Andrew T Wilson wrote: > On 2015/02/11 17:28:43, bartfab wrote: > > You still need the account ID of course. This is available from the policy > > broker, even if the account was removed from policy. > > BTW, not using the policy broker because it's basically impossible to mock so > testing is hugely problematic. > > Let me know if for some reason going through the active user to figure out which > device-local account is active is bad. Otherwise I'm going to leave as-is. > The one advantage of going via the broker is that it will continue to work even if the currently logged-in account has been removed from policy. It makes sense that you would not care about reporting the status for a deleted account, so no problem with going the CrosSettings route. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:122: user_manager::User* const user = Nit 1: #include "components/user_manager/user.h" Nit 2: s/user_manager::User* const/const user_manager::User* const/ https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:124: const std::string user_id = user->GetUserID(); Nit: const. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:125: const std::vector<policy::DeviceLocalAccount> accounts = Nit: const. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:762: // enabled, and we are in an auto-launched kiosk session. Nit: You split the conditional into two. The comment should be split as well. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:766: scoped_ptr<DeviceLocalAccount> account = GetAutoLaunchedKioskSessionInfo(); Nit: const. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:788: Profile* profile = chromeos::ProfileHelper::Get()->GetProfileByUser( Nit: const. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:790: extensions::ExtensionRegistry* registry = Nit: const. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:792: const extensions::Extension* extension = registry->GetExtensionById( Nit: const. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/device_status_collector.h (right): https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.h:117: // Gets the version of the passed app. Virtual to allow mocking. Nit: Add a comment that this works for kiosk apps only. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (right): https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:250: std::string()) { Nit: /* kiosk_app_update_url */ https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:397: const policy::DeviceLocalAccount fake_local_account_; Nit: s/local/device_local/ https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:875: status_collector_->set_kiosk_account( Nit: This is unnecessary. status_collector_->kiosk_account_ initializes to an empty scoped_ptr automatically. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:886: // Setup a device-local account for single-app kiosk mode. Nit: s/Setup/Set up/ https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:898: // Setup a device-local account for single-app kiosk mode. Nit: s/Setup/Set up/
atwilson@chromium.org changed reviewers: + nkostylev@chromium.org - ygorshenin@chromium.org
+nkostylev for chromeos/login owners stamp.
lgtm
New patchsets have been uploaded after l-g-t-m from bartfab@chromium.org,nkostylev@chromium.org
https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:122: user_manager::User* const user = On 2015/02/16 13:30:33, bartfab wrote: > Nit 1: #include "components/user_manager/user.h" > Nit 2: s/user_manager::User* const/const user_manager::User* const/ Done. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:124: const std::string user_id = user->GetUserID(); On 2015/02/16 13:30:33, bartfab wrote: > Nit: const. Isn't it already const? https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:125: const std::vector<policy::DeviceLocalAccount> accounts = On 2015/02/16 13:30:33, bartfab wrote: > Nit: const. can't cast std::vector<policy::DeviceLocalAccount> to std::vector<const policy::DeviceLocalAccount>, for some reason. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:762: // enabled, and we are in an auto-launched kiosk session. On 2015/02/16 13:30:33, bartfab wrote: > Nit: You split the conditional into two. The comment should be split as well. Done. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:766: scoped_ptr<DeviceLocalAccount> account = GetAutoLaunchedKioskSessionInfo(); On 2015/02/16 13:30:33, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:788: Profile* profile = chromeos::ProfileHelper::Get()->GetProfileByUser( On 2015/02/16 13:30:33, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:790: extensions::ExtensionRegistry* registry = On 2015/02/16 13:30:33, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:792: const extensions::Extension* extension = registry->GetExtensionById( On 2015/02/16 13:30:33, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/device_status_collector.h (right): https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.h:117: // Gets the version of the passed app. Virtual to allow mocking. On 2015/02/16 13:30:33, bartfab wrote: > Nit: Add a comment that this works for kiosk apps only. This works for any app - we just grab the version from the extension registry. It turns out that currently it's only *called* for kiosk apps, but the code itself has no restrictions on its use. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (right): https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:250: std::string()) { On 2015/02/16 13:30:33, bartfab wrote: > Nit: /* kiosk_app_update_url */ Done. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:397: const policy::DeviceLocalAccount fake_local_account_; On 2015/02/16 13:30:33, bartfab wrote: > Nit: s/local/device_local/ Done. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:875: status_collector_->set_kiosk_account( On 2015/02/16 13:30:33, bartfab wrote: > Nit: This is unnecessary. status_collector_->kiosk_account_ initializes to an > empty scoped_ptr automatically. Done. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:886: // Setup a device-local account for single-app kiosk mode. On 2015/02/16 13:30:33, bartfab wrote: > Nit: s/Setup/Set up/ Done. https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:898: // Setup a device-local account for single-app kiosk mode. On 2015/02/16 13:30:33, bartfab wrote: > Nit: s/Setup/Set up/ Done.
The CQ bit was checked by atwilson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/907323002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by atwilson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/907323002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c6605bd30349ede3e1a9ea7657a338ec45036a0f Cr-Commit-Position: refs/heads/master@{#316567}
Message was sent while issue was closed.
https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/907323002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/device_status_collector.cc:125: const std::vector<policy::DeviceLocalAccount> accounts = On 2015/02/17 10:57:25, Andrew T Wilson wrote: > On 2015/02/16 13:30:33, bartfab wrote: > > Nit: const. > can't cast std::vector<policy::DeviceLocalAccount> to std::vector<const > policy::DeviceLocalAccount>, for some reason. Sorry, the three "const" comments were dupes of comments I wrote for your first patch set that you addressed already. no idea how they slipped in here again - I must have been looking at the wrong patch set version :(. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
