Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1892873002:
Add CheckAndroidManagement to ARC sign-in flow. (Closed)
|
Created:
4 years, 8 months ago by Polina Bondarenko Modified:
4 years, 7 months ago CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@check_android_management Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd CheckAndroidManagement to ARC sign-in flow.
ARC is not available for Chrome unmanaged and Android managed accounts.
Check whether account is managed by Android every time on ARC start.
Always allow ARC for managed by Chrome policy and well-known consumer accounts.
Uncheck ARC checkbox in case of any error.
BUG=602612
TEST=manual
Committed: https://crrev.com/b7ceb29f22004c344c982e48de1e436834b69274
Cr-Commit-Position: refs/heads/master@{#391482}
Patch Set 1 : #Patch Set 2 : Put CheckAndroidManagement before LSO. #
Total comments: 24
Patch Set 3 : Fixed to pass unit tests. #Patch Set 4 : Fixed comments. #Patch Set 5 : Added browser test for ArcAuthService. #
Total comments: 42
Patch Set 6 : Fixed comments, rebase. #
Total comments: 8
Patch Set 7 : Moved access token to AndroidManagementClient. #Patch Set 8 : scoped_ptr->unique_ptr in test. #
Total comments: 4
Patch Set 9 : Removed ptr_util.h from arc_auth_service.h #Patch Set 10 : Added comment, Unretained(this) -> weak_ptr_factory_.GetWeakPtr() #Patch Set 11 : Rebased. #Patch Set 12 : Added OnShutdownBridge to observer. #
Total comments: 52
Patch Set 13 : Fixed bartfab's comments. #
Total comments: 2
Patch Set 14 : Fixed nits. #Patch Set 15 : Added CheckAndroidManagement when OptIn is disabled. #
Total comments: 6
Patch Set 16 : Fixed comments. #Patch Set 17 : Rebased. #Patch Set 18 : Rebased. #
Messages
Total messages: 65 (27 generated)
Patchset #1 (id:1) has been deleted
pbond@chromium.org changed reviewers: + bartfab@chromium.org
PTAL
Description was changed from ========== Add CheckAndroidManagement to ARC sign-in flow. ARC is not available for Chrome unmanaged and Android managed accounts. Check whether account is managed by Android every time on ARC start. Always allow ARC for managed by Chrome policy and well-known consumer accounts. BUG=602612 ========== to ========== Add CheckAndroidManagement to ARC sign-in flow. ARC is not available for Chrome unmanaged and Android managed accounts. Check whether account is managed by Android every time on ARC start. Always allow ARC for managed by Chrome policy and well-known consumer accounts. Uncheck ARC checkbox in case of any error. BUG=602612 TEST=manual ==========
Patchset #2 (id:40001) has been deleted
Can we get an end-to-end test for this? https://codereview.chromium.org/1892873002/diff/60001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/1892873002/diff/60001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:6586: <message name="IDS_ARC_ANDROID_MANAGEMENT_ENABLED_ERROR" desc="Error message shown in case when the android management is enabled for user account."> Nit 1: s/in case when the android/when Android/ Nit 2: s/enabled for user account/required for an unmanaged Chrome OS user/ https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:524: policy::BrowserPolicyConnectorChromeOS* connector = Nit: const pointer. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:525: g_browser_process->platform_part()->browser_policy_connector_chromeos(); Nit: #include "chrome/browser/browser_process_platform_part.h" https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:526: policy::DeviceManagementService* service = Nit: const pointer. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:534: // No need to check Android management for Chrome OS managed users. Nit: Not just no need - you are only supposed to ping the server for unmanaged CrOS users. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:535: if (policy::ProfilePolicyConnectorFactory::GetForBrowserContext(profile_) Nit: #include "chrome/browser/policy/profile_policy_connector_factory.h" https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:536: ->IsManaged()) { Nit: Indent. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:543: profile_->GetProfileUserName())) { Nit: Indent. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:557: ShowUI(UIPage::LSO, base::string16()); Nit: #include "base/strings/string16.h" https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:190: std::string auth_token_; Nit: #include <string> https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:192: scoped_ptr<policy::AndroidManagementClient> android_management_client_; Nit: Use std::unique_ptr instead. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:30: ANDROID_MANAGEMENT_ENABLED = 7, // Android management is enabled for user. Nit 1: s/enabled/required/ Nit 2: consider s/ENABLED/REQUIRED/
Patchset #5 (id:120001) has been deleted
Hi Bartosz, Thanks for reviewing! I've fixed your comments and added browser test. PTAL. https://codereview.chromium.org/1892873002/diff/60001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/1892873002/diff/60001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:6586: <message name="IDS_ARC_ANDROID_MANAGEMENT_ENABLED_ERROR" desc="Error message shown in case when the android management is enabled for user account."> On 2016/04/18 15:06:14, bartfab (slow) wrote: > Nit 1: s/in case when the android/when Android/ > Nit 2: s/enabled for user account/required for an unmanaged Chrome OS user/ Done. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:524: policy::BrowserPolicyConnectorChromeOS* connector = On 2016/04/18 15:06:14, bartfab (slow) wrote: > Nit: const pointer. Done. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:525: g_browser_process->platform_part()->browser_policy_connector_chromeos(); On 2016/04/18 15:06:15, bartfab (slow) wrote: > Nit: #include "chrome/browser/browser_process_platform_part.h" Done. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:526: policy::DeviceManagementService* service = On 2016/04/18 15:06:14, bartfab (slow) wrote: > Nit: const pointer. Done. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:534: // No need to check Android management for Chrome OS managed users. On 2016/04/18 15:06:15, bartfab (slow) wrote: > Nit: Not just no need - you are only supposed to ping the server for unmanaged > CrOS users. Done. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:535: if (policy::ProfilePolicyConnectorFactory::GetForBrowserContext(profile_) On 2016/04/18 15:06:15, bartfab (slow) wrote: > Nit: #include "chrome/browser/policy/profile_policy_connector_factory.h" Done. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:536: ->IsManaged()) { On 2016/04/18 15:06:15, bartfab (slow) wrote: > Nit: Indent. Done. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:543: profile_->GetProfileUserName())) { On 2016/04/18 15:06:14, bartfab (slow) wrote: > Nit: Indent. Done. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:557: ShowUI(UIPage::LSO, base::string16()); On 2016/04/18 15:06:15, bartfab (slow) wrote: > Nit: #include "base/strings/string16.h" Done. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:190: std::string auth_token_; On 2016/04/18 15:06:15, bartfab (slow) wrote: > Nit: #include <string> Done. https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:30: ANDROID_MANAGEMENT_ENABLED = 7, // Android management is enabled for user. On 2016/04/18 15:06:15, bartfab (slow) wrote: > Nit 1: s/enabled/required/ > Nit 2: consider s/ENABLED/REQUIRED/ Done.
pbond@chromium.org changed reviewers: + xiyuan@chromium.org
Hi Xiyuan, I've added Android management check to ARC opt-in flow. It is a dependent CL on https://codereview.chromium.org/1892093002/ It doesn't work e2e with server because of a server-side issue, which will be resolved in a couple days (and this CL will not be changed much). I WILL NOT MERGE this code before e2e test will pass. please review changes in chrome/browser/chromeos/arc/* chrome/browser/resources/chromeos/arc_support/background.js
xiyuan@chromium.org changed reviewers: + khmel@chromium.org
+khmel https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:342: auth_token_ = token; |token| is uber token, not sure if it is what you can use with DM request. You might need a login scoped access token instead. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:548: service, g_browser_process->system_request_context())); Should we be using storage_partition_->GetURLRequestContext()? Or request context does not really matter since we don't send and set cookies? https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:554: ->IsManaged()) { Would ProfilePolicyConnectorFactory::GetForBrowserContext() return nullptr for an unmanaged user? https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:570: base::Unretained(this))); WeakPtr instead of base::Unretained. Or add a comment to explain why it is safe to use base::Unretained here. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:48: void OnOptInChanged(ArcAuthService::State state) override { nit: // ArcAuthService::Observer: https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:55: ArcAuthService::Get()->RemoveObserver(this); nit: RemoveObserver in dtor. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:59: void Wait() { To be more correct, we should check the current state of ArcAuthService before starting a runlook to wait. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:66: bool running_loop_; nit: Prefer to use the following to init member: bool running_loop_ = false; https://chromium-cpp.appspot.com/ "Non-Static Class Member Initializers" section. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:67: ArcAuthService::State state_; nit: const ArcAuthService::State state_; https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:68: std::unique_ptr<base::RunLoop> run_loop_; nit: #include <memory> https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:70: DISALLOW_COPY_AND_ASSIGN(ArcAuthServiceStateObserver); nit: #include "base/macros.h" https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:78: void SetUpInProcessBrowserTestFixture() override { nit: // InProcessBrowserTest: https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:124: void TearDownTest() { Can we override TearDown and move the boy there so that we don't need to explicitly call it on every test case? https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:141: IN_PROC_BROWSER_TEST_F(ArcAuthServiceTest, ConsumerAccountTest) { nit: ConsumerAccountTest -> ConsumerAccount, Test suffix feels redundant here. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:142: SetUpTest("test@example", kUnmanagedAuthToken); Should the username be "test@example.com"? https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/arc_support/background.js (left): https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/arc_support/background.js:192: showPage('lso'); This could break how the arc support app works. The current app kicks start webview loading when the current page is set to lso-loading and switch to lso page to show the webview when the load is finished. The CL changes to switch to lso page from C++ when check Android management finishes (started after a merge session). However, this does not cover all the paths to the lso-loading page. There are a few other places where StartUI could be called after merge session finishes. In such cases, we could stuck with lso-loading page indefinitely.
khmel@google.com changed reviewers: + khmel@google.com
https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:298: android_management_client_.reset(); Probably ArcAuthService::ShutdownBridge is better place https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:380: enable_check_android_management_for_testing) nit: 2+ lines, please use {} https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:579: ShowUI(UIPage::LSO, base::string16()); Please use UIPage::LSO_PROGRESS... instead UIPage::LSO https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:595: } nit: default: NOTREACHED(); ?
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
Thanks for reviewing! Fixed comments and rebased. Sorry, sent it in one patch because of compilation issues. Please take a look. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:298: android_management_client_.reset(); On 2016/04/20 19:13:31, khmel1 wrote: > Probably ArcAuthService::ShutdownBridge is better place If android_management_client_ is destroyed not at the same time with profile_, then the next scenario is possible: 1) Error is occurred during LSO. 2) That causes ShutdownBridge call. -> android_management_client_ is destroyed. 3) User presses "Retry" on error screen. 4) Start the flow again, but android_management_client_ has been already destroyed and OnPrimaryUserProfilePrepared hasn't been called since we re-do the flow for the current profile. 5) It causes failure:( https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:342: auth_token_ = token; On 2016/04/20 18:21:41, xiyuan wrote: > |token| is uber token, not sure if it is what you can use with DM request. You > might need a login scoped access token instead. Thanks, fixed to access token. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:380: enable_check_android_management_for_testing) On 2016/04/20 19:13:31, khmel1 wrote: > nit: 2+ lines, please use {} Done. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:548: service, g_browser_process->system_request_context())); On 2016/04/20 18:21:41, xiyuan wrote: > Should we be using storage_partition_->GetURLRequestContext()? Or request > context does not really matter since we don't send and set cookies? Done. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:554: ->IsManaged()) { On 2016/04/20 18:21:41, xiyuan wrote: > Would ProfilePolicyConnectorFactory::GetForBrowserContext() return nullptr for > an unmanaged user? No, it seems like it always returns a value. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:570: base::Unretained(this))); On 2016/04/20 18:21:41, xiyuan wrote: > WeakPtr instead of base::Unretained. Or add a comment to explain why it is safe > to use base::Unretained here. Done. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:579: ShowUI(UIPage::LSO, base::string16()); On 2016/04/20 19:13:31, khmel1 wrote: > Please use UIPage::LSO_PROGRESS... instead UIPage::LSO > Done. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:595: } On 2016/04/20 19:13:31, khmel1 wrote: > nit: > default: > NOTREACHED(); > ? Done. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:48: void OnOptInChanged(ArcAuthService::State state) override { On 2016/04/20 18:21:42, xiyuan wrote: > nit: // ArcAuthService::Observer: Done. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:55: ArcAuthService::Get()->RemoveObserver(this); On 2016/04/20 18:21:42, xiyuan wrote: > nit: RemoveObserver in dtor. Done. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:59: void Wait() { On 2016/04/20 18:21:41, xiyuan wrote: > To be more correct, we should check the current state of ArcAuthService before > starting a runlook to wait. Changed the observer and it waits only for the next state change now. E.g. a state changes from STOPPED to STOPPED in ManagedAndroidAccount test. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:66: bool running_loop_; On 2016/04/20 18:21:41, xiyuan wrote: > nit: Prefer to use the following to init member: > > bool running_loop_ = false; > > https://chromium-cpp.appspot.com/ "Non-Static Class Member Initializers" > section. Done. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:67: ArcAuthService::State state_; On 2016/04/20 18:21:41, xiyuan wrote: > nit: const ArcAuthService::State state_; Removed state_ at all. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:68: std::unique_ptr<base::RunLoop> run_loop_; On 2016/04/20 18:21:41, xiyuan wrote: > nit: #include <memory> Done. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:70: DISALLOW_COPY_AND_ASSIGN(ArcAuthServiceStateObserver); On 2016/04/20 18:21:42, xiyuan wrote: > nit: #include "base/macros.h" Done. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:78: void SetUpInProcessBrowserTestFixture() override { On 2016/04/20 18:21:41, xiyuan wrote: > nit: // InProcessBrowserTest: Done. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:124: void TearDownTest() { On 2016/04/20 18:21:41, xiyuan wrote: > Can we override TearDown and move the boy there so that we don't need to > explicitly call it on every test case? No, ArcAuthService should be stopped for each test/profile. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:141: IN_PROC_BROWSER_TEST_F(ArcAuthServiceTest, ConsumerAccountTest) { On 2016/04/20 18:21:41, xiyuan wrote: > nit: ConsumerAccountTest -> ConsumerAccount, Test suffix feels redundant here. Done. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:142: SetUpTest("test@example", kUnmanagedAuthToken); On 2016/04/20 18:21:41, xiyuan wrote: > Should the username be mailto:"test@example.com"? Done. https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/arc_support/background.js (left): https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/arc_support/background.js:192: showPage('lso'); On 2016/04/20 18:21:42, xiyuan wrote: > This could break how the arc support app works. The current app kicks start > webview loading when the current page is set to lso-loading and switch to lso > page to show the webview when the load is finished. > > The CL changes to switch to lso page from C++ when check Android management > finishes (started after a merge session). However, this does not cover all the > paths to the lso-loading page. There are a few other places where StartUI could > be called after merge session finishes. In such cases, we could stuck with > lso-loading page indefinitely. Thanks, removed the change, we don't really need it.
https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:233: if (state_ == state && !enable_check_android_management_for_testing) Why we need this? Could ArcAuthServiceStateObserver in test check ArcAuthService::state() before running its wait loop? https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:622: token_request_ = token_service_->StartRequest(account_id_, scopes, this); Think we should move access token fetching etc into AndroidManagementClient (or a helper class). The code does not seem belonging to ArcAuthService. https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:66: running_loop_ = true; running_loop_ seems redundant. Can we check whether run_loop_ holds an instance?
Patchset #7 (id:220001) has been deleted
I moved the access token fetch to AndroidManagementClient, see CL (https://codereview.chromium.org/1892093002/), removed running_loop_. Please take a look. https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:233: if (state_ == state && !enable_check_android_management_for_testing) On 2016/04/26 17:48:10, xiyuan wrote: > Why we need this? Could ArcAuthServiceStateObserver in test check > ArcAuthService::state() before running its wait loop? We need this to test ManagedAndroidAccount scenario: ARC is forbidden for accounts with the required Android management. The client should send CheckAndroidManagementRequest to DM server and receive a response from DM server (LocalPolicyTestServer in browsertest), which tells whether client's account management requires Android management or not, before start ARC. The observer's role is this case is to wait until the client receives DM server's response, and a state changes from STOPPED to STOPPED. If the observer checks the state before Wait(), it gets a right STOPPED state, but client has only sent a CheckAndroidManagementRequest and hasn't got a server's response at that time. https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:622: token_request_ = token_service_->StartRequest(account_id_, scopes, this); On 2016/04/26 17:48:09, xiyuan wrote: > Think we should move access token fetching etc into AndroidManagementClient (or > a helper class). The code does not seem belonging to ArcAuthService. Done. https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:66: running_loop_ = true; On 2016/04/26 17:48:10, xiyuan wrote: > running_loop_ seems redundant. Can we check whether run_loop_ holds an instance? Done.
https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:233: if (state_ == state && !enable_check_android_management_for_testing) On 2016/04/27 15:19:57, Polina Bondarenko wrote: > On 2016/04/26 17:48:10, xiyuan wrote: > > Why we need this? Could ArcAuthServiceStateObserver in test check > > ArcAuthService::state() before running its wait loop? > > We need this to test ManagedAndroidAccount scenario: > > ARC is forbidden for accounts with the required Android management. > The client should send CheckAndroidManagementRequest to DM server and receive a > response from DM server (LocalPolicyTestServer in browsertest), which tells > whether client's account management requires Android management or not, before > start ARC. > > The observer's role is this case is to wait until the client receives DM > server's response, and a state changes from STOPPED to STOPPED. > > If the observer checks the state before Wait(), it gets a right STOPPED state, > but client has only sent a CheckAndroidManagementRequest and hasn't got a > server's response at that time. It is strange to observe a state change which does not change the state. It looks like the test is more interested to know ShutdownBridge() is called. How about add that to Observer interface and make the test observe that? https://codereview.chromium.org/1892873002/diff/250001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/1892873002/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.h:12: #include "base/memory/ptr_util.h" nit: move this to cc file if WrapUnique is used there.
https://codereview.chromium.org/1892873002/diff/250001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/1892873002/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.h:12: #include "base/memory/ptr_util.h" On 2016/04/27 16:00:57, xiyuan wrote: > nit: move this to cc file if WrapUnique is used there. Sorry, removed, no need in this header.
https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:298: android_management_client_.reset(); On 2016/04/25 19:59:49, Polina Bondarenko wrote: > On 2016/04/20 19:13:31, khmel1 wrote: > > Probably ArcAuthService::ShutdownBridge is better place > > If android_management_client_ is destroyed not at the same time with profile_, > then the next scenario is possible: > > 1) Error is occurred during LSO. > 2) That causes ShutdownBridge call. -> android_management_client_ is destroyed. > 3) User presses "Retry" on error screen. > 4) Start the flow again, but android_management_client_ has been already > destroyed and OnPrimaryUserProfilePrepared hasn't been called since we re-do the > flow for the current profile. > 5) It causes failure:( Thanks for explanation. Short comment may be welcome here. https://codereview.chromium.org/1892873002/diff/250001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:276: base::Unretained(this))); Once we are using weak_ptr, would it make sense to change to weak_ptr_factory_.GetWeakPtr() here as well?
https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:298: android_management_client_.reset(); On 2016/04/27 16:07:42, khmel wrote: > On 2016/04/25 19:59:49, Polina Bondarenko wrote: > > On 2016/04/20 19:13:31, khmel1 wrote: > > > Probably ArcAuthService::ShutdownBridge is better place > > > > If android_management_client_ is destroyed not at the same time with profile_, > > then the next scenario is possible: > > > > 1) Error is occurred during LSO. > > 2) That causes ShutdownBridge call. -> android_management_client_ is > destroyed. > > 3) User presses "Retry" on error screen. > > 4) Start the flow again, but android_management_client_ has been already > > destroyed and OnPrimaryUserProfilePrepared hasn't been called since we re-do > the > > flow for the current profile. > > 5) It causes failure:( > > Thanks for explanation. Short comment may be welcome here. Added comment. https://codereview.chromium.org/1892873002/diff/250001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/250001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:276: base::Unretained(this))); On 2016/04/27 16:07:42, khmel wrote: > Once we are using weak_ptr, would it make sense to change to > weak_ptr_factory_.GetWeakPtr() here as well? Done.
The CQ bit was checked by pbond@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892873002/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892873002/310001
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 pbond@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892873002/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892873002/330001
Thanks a lot for your comments, sorry I've missed some, but I've already fixed them. Added OnShutdownBridge to Observer. The AndroidManagementClient CL has been merged. Please take a look! https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/1892873002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:192: scoped_ptr<policy::AndroidManagementClient> android_management_client_; On 2016/04/18 15:06:15, bartfab (slow) wrote: > Nit: Use std::unique_ptr instead. Done. https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:233: if (state_ == state && !enable_check_android_management_for_testing) On 2016/04/27 16:00:57, xiyuan wrote: > On 2016/04/27 15:19:57, Polina Bondarenko wrote: > > On 2016/04/26 17:48:10, xiyuan wrote: > > > Why we need this? Could ArcAuthServiceStateObserver in test check > > > ArcAuthService::state() before running its wait loop? > > > > We need this to test ManagedAndroidAccount scenario: > > > > ARC is forbidden for accounts with the required Android management. > > The client should send CheckAndroidManagementRequest to DM server and receive > a > > response from DM server (LocalPolicyTestServer in browsertest), which tells > > whether client's account management requires Android management or not, before > > start ARC. > > > > The observer's role is this case is to wait until the client receives DM > > server's response, and a state changes from STOPPED to STOPPED. > > > > If the observer checks the state before Wait(), it gets a right STOPPED state, > > but client has only sent a CheckAndroidManagementRequest and hasn't got a > > server's response at that time. > > It is strange to observe a state change which does not change the state. It > looks like the test is more interested to know ShutdownBridge() is called. How > about add that to Observer interface and make the test observe that? Sorry, I've missed this comment. Yes, that's much better. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1892873002/diff/330001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6587: Access to Android apps is disabled to the current user. Nit: s/to the/for the/ https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:10: #include "base/bind_helpers.h" Nit: No longer used. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:13: #include "base/strings/string16.h" Nit: Where is this used? https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:69: // Do check Android management requirement in browser tests. The comment says "do check" but the default value is |false|. That does not match. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:262: SigninManagerBase* signin_manager = Nit: const pointer https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:264: CHECK(token_service_ && signin_manager); Nit: #include "base/logging.h" https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:286: StartArc(); Will this not circumvent the management check? https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:315: // Destroy here, because |android_management_client_| is used for each ARC I do not understand what this comment is trying to tell me. What does "each connect" refer to? What does it mean for ARC to be signed-in? Why does the client have to be explicitly destroyed? https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:575: policy::AndroidManagementClient::Result::RESULT_UNMANAGED); It is a bit odd semantically that for a *managed* user, the result code we synthesize is "unmanaged." https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.h:81: // Called to notify that ARC bridge is shutdown. Nit: s/shutdown/shut down/ https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.h:200: std::unique_ptr<policy::AndroidManagementClient> android_management_client_; Nit: #include <memory> https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:14: #include "chrome/browser/browser_process.h" Nit: Not used. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:55: run_loop_.reset(); Why do you need to explicitly reset the |run_loop_| member? https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:115: ASSERT_TRUE(test_server_->Start()); Nit: #include "testing/gtest/include/gtest/gtest.h" https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:119: base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); Nit: const pointer https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:127: std::unique_ptr<chromeos::SessionManagerClient>( Nit: #include “chromeos/dbus/session_manager_client.h” https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:144: void SetUpTest(const std::string& username) { Why are SetUpTest() and TearDownTest() not part of the test fixture's regular setup flow? IIRC, browser tests are set up and torn down completely per test case. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:153: profile_builder.SetPath(temp_dir_.path().AppendASCII("TestArcProfile")); Nit: #include “base/files/file_path.h” https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:170: // Setup ARC for test profile. Nit: s/Setup/Set up/ https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:192: chromeos::FakeSessionManagerClient* fake_session_manager_client_; Nit: No need for this to be a member variable. It is only used in a single method. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:205: PrefService* pref = profile()->GetPrefs(); Nit 1: Here and in the other tests: s/pref/prefs/ Nit 2: Here and in the other tests: const pointer https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:228: policy::ProfilePolicyConnector* const connector = Nit: const pointer https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/policy/... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/policy/... chrome/browser/policy/test/policy_testserver.py:633: """Handles a check android management request. Nit: s/android/Android/
Patchset #13 (id:350001) has been deleted
Thanks for reviewing! Fixed comments. https://codereview.chromium.org/1892873002/diff/330001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6587: Access to Android apps is disabled to the current user. On 2016/05/02 14:55:48, bartfab (slow) wrote: > Nit: s/to the/for the/ Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:10: #include "base/bind_helpers.h" On 2016/05/02 14:55:48, bartfab (slow) wrote: > Nit: No longer used. Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:13: #include "base/strings/string16.h" On 2016/05/02 14:55:48, bartfab (slow) wrote: > Nit: Where is this used? It's used in ShowUI() and it's calls. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:69: // Do check Android management requirement in browser tests. On 2016/05/02 14:55:48, bartfab (slow) wrote: > The comment says "do check" but the default value is |false|. That does not > match. Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:262: SigninManagerBase* signin_manager = On 2016/05/02 14:55:48, bartfab (slow) wrote: > Nit: const pointer Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:264: CHECK(token_service_ && signin_manager); On 2016/05/02 14:55:48, bartfab (slow) wrote: > Nit: #include "base/logging.h" Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:286: StartArc(); On 2016/05/02 14:55:48, bartfab (slow) wrote: > Will this not circumvent the management check? Yes, it will. This branch of 'if' is used only for testing. When OptIn verification is disabled. I think, that is better to disable Android management check in this case too. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:315: // Destroy here, because |android_management_client_| is used for each ARC On 2016/05/02 14:55:48, bartfab (slow) wrote: > I do not understand what this comment is trying to tell me. What does "each > connect" refer to? What does it mean for ARC to be signed-in? Why does the > client have to be explicitly destroyed? It doesn't have to, removed. I tried to show that CheckAndroidManagement is called every time, even if ARC is signed-in and it can't be destroyed in ShutdownBridge. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:575: policy::AndroidManagementClient::Result::RESULT_UNMANAGED); On 2016/05/02 14:55:48, bartfab (slow) wrote: > It is a bit odd semantically that for a *managed* user, the result code we > synthesize is "unmanaged." Yes, fixed to a plain code. Hopefully it doesn't look like copy-paste here. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.h:81: // Called to notify that ARC bridge is shutdown. On 2016/05/02 14:55:48, bartfab (slow) wrote: > Nit: s/shutdown/shut down/ Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.h:200: std::unique_ptr<policy::AndroidManagementClient> android_management_client_; On 2016/05/02 14:55:48, bartfab (slow) wrote: > Nit: #include <memory> Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:14: #include "chrome/browser/browser_process.h" On 2016/05/02 14:55:49, bartfab (slow) wrote: > Nit: Not used. Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:55: run_loop_.reset(); On 2016/05/02 14:55:49, bartfab (slow) wrote: > Why do you need to explicitly reset the |run_loop_| member? No need to reset it. Removed. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:115: ASSERT_TRUE(test_server_->Start()); On 2016/05/02 14:55:48, bartfab (slow) wrote: > Nit: #include "testing/gtest/include/gtest/gtest.h" Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:119: base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); On 2016/05/02 14:55:49, bartfab (slow) wrote: > Nit: const pointer Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:127: std::unique_ptr<chromeos::SessionManagerClient>( On 2016/05/02 14:55:49, bartfab (slow) wrote: > Nit: #include “chromeos/dbus/session_manager_client.h” Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:144: void SetUpTest(const std::string& username) { On 2016/05/02 14:55:49, bartfab (slow) wrote: > Why are SetUpTest() and TearDownTest() not part of the test fixture's regular > setup flow? IIRC, browser tests are set up and torn down completely per test > case. Fixed, thanks, didn't know that. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:153: profile_builder.SetPath(temp_dir_.path().AppendASCII("TestArcProfile")); On 2016/05/02 14:55:49, bartfab (slow) wrote: > Nit: #include “base/files/file_path.h” Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:170: // Setup ARC for test profile. On 2016/05/02 14:55:48, bartfab (slow) wrote: > Nit: s/Setup/Set up/ Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:192: chromeos::FakeSessionManagerClient* fake_session_manager_client_; On 2016/05/02 14:55:49, bartfab (slow) wrote: > Nit: No need for this to be a member variable. It is only used in a single > method. Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:205: PrefService* pref = profile()->GetPrefs(); On 2016/05/02 14:55:49, bartfab (slow) wrote: > Nit 1: Here and in the other tests: s/pref/prefs/ > Nit 2: Here and in the other tests: const pointer Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:228: policy::ProfilePolicyConnector* const connector = On 2016/05/02 14:55:49, bartfab (slow) wrote: > Nit: const pointer Done. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/policy/... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/policy/... chrome/browser/policy/test/policy_testserver.py:633: """Handles a check android management request. On 2016/05/02 14:55:49, bartfab (slow) wrote: > Nit: s/android/Android/ Done.
The CQ bit was checked by pbond@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892873002/370001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892873002/370001
https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:286: StartArc(); On 2016/05/03 13:41:54, Polina Bondarenko wrote: > On 2016/05/02 14:55:48, bartfab (slow) wrote: > > Will this not circumvent the management check? > > Yes, it will. This branch of 'if' is used only for testing. When OptIn > verification is disabled. I think, that is better to disable Android management > check in this case too. According to the documentation, that switch "disables ARC Opt-in verification process and ARC is enabled by default." I am concerned that someone may decide in the future to make this the default (no more opt-in needed) and then suddenly your check is skipped. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:575: policy::AndroidManagementClient::Result::RESULT_UNMANAGED); On 2016/05/03 13:41:54, Polina Bondarenko wrote: > On 2016/05/02 14:55:48, bartfab (slow) wrote: > > It is a bit odd semantically that for a *managed* user, the result code we > > synthesize is "unmanaged." > > Yes, fixed to a plain code. Hopefully it doesn't look like copy-paste here. You could always extract these four lines to a helper, StartArcIfSignedIn() or some such. https://codereview.chromium.org/1892873002/diff/370001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/1892873002/diff/370001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:131: chromeos::FakeSessionManagerClient* fake_session_manager_client( Nit 1: That is an unusual way to initialize a pointer. Why not set it more traditionally, via "="? Nit 2: const pointer
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:286: StartArc(); On 2016/05/03 14:25:18, bartfab (slow) wrote: > On 2016/05/03 13:41:54, Polina Bondarenko wrote: > > On 2016/05/02 14:55:48, bartfab (slow) wrote: > > > Will this not circumvent the management check? > > > > Yes, it will. This branch of 'if' is used only for testing. When OptIn > > verification is disabled. I think, that is better to disable Android > management > > check in this case too. > > According to the documentation, that switch "disables ARC Opt-in verification > process and ARC is enabled by default." I am concerned that someone may decide > in the future to make this the default (no more opt-in needed) and then suddenly > your check is skipped. Would you recommend to add check here too? This branch is used only for testing right now, not sure which accounts do they use. https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:575: policy::AndroidManagementClient::Result::RESULT_UNMANAGED); On 2016/05/03 14:25:18, bartfab (slow) wrote: > On 2016/05/03 13:41:54, Polina Bondarenko wrote: > > On 2016/05/02 14:55:48, bartfab (slow) wrote: > > > It is a bit odd semantically that for a *managed* user, the result code we > > > synthesize is "unmanaged." > > > > Yes, fixed to a plain code. Hopefully it doesn't look like copy-paste here. > > You could always extract these four lines to a helper, StartArcIfSignedIn() or > some such. Done. https://codereview.chromium.org/1892873002/diff/370001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/1892873002/diff/370001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:131: chromeos::FakeSessionManagerClient* fake_session_manager_client( On 2016/05/03 14:25:18, bartfab (slow) wrote: > Nit 1: That is an unusual way to initialize a pointer. Why not set it more > traditionally, via "="? > Nit 2: const pointer Done.
https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:286: StartArc(); On 2016/05/03 14:55:44, Polina Bondarenko wrote: > On 2016/05/03 14:25:18, bartfab (slow) wrote: > > On 2016/05/03 13:41:54, Polina Bondarenko wrote: > > > On 2016/05/02 14:55:48, bartfab (slow) wrote: > > > > Will this not circumvent the management check? > > > > > > Yes, it will. This branch of 'if' is used only for testing. When OptIn > > > verification is disabled. I think, that is better to disable Android > > management > > > check in this case too. > > > > According to the documentation, that switch "disables ARC Opt-in verification > > process and ARC is enabled by default." I am concerned that someone may decide > > in the future to make this the default (no more opt-in needed) and then > suddenly > > your check is skipped. > > Would you recommend to add check here too? This branch is used only for testing > right now, not sure which accounts do they use. Yes, I think we should add the check to that code path too. If it breaks manual testing, someone will come and complain to us. We can always adjust the check in that case.
https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/330001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:286: StartArc(); On 2016/05/03 15:26:38, bartfab (slow) wrote: > On 2016/05/03 14:55:44, Polina Bondarenko wrote: > > On 2016/05/03 14:25:18, bartfab (slow) wrote: > > > On 2016/05/03 13:41:54, Polina Bondarenko wrote: > > > > On 2016/05/02 14:55:48, bartfab (slow) wrote: > > > > > Will this not circumvent the management check? > > > > > > > > Yes, it will. This branch of 'if' is used only for testing. When OptIn > > > > verification is disabled. I think, that is better to disable Android > > > management > > > > check in this case too. > > > > > > According to the documentation, that switch "disables ARC Opt-in > verification > > > process and ARC is enabled by default." I am concerned that someone may > decide > > > in the future to make this the default (no more opt-in needed) and then > > suddenly > > > your check is skipped. > > > > Would you recommend to add check here too? This branch is used only for > testing > > right now, not sure which accounts do they use. > > Yes, I think we should add the check to that code path too. If it breaks manual > testing, someone will come and complain to us. We can always adjust the check in > that case. Ok, added check. It seems like nothing's failed for now.
lgtm https://codereview.chromium.org/1892873002/diff/410001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/410001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:613: if (profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn) || Nit: Multi-line conditionals require curly braces.
lgtm https://codereview.chromium.org/1892873002/diff/410001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/410001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:415: FOR_EACH_OBSERVER(Observer, observer_list_, OnShutdownBridge()); nit: move this to the end of the function? https://codereview.chromium.org/1892873002/diff/410001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/1892873002/diff/410001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:76: DISALLOW_COPY_AND_ASSIGN(ArcAuthServiceObserver); nit: This and the other two DISALLOW_COPY_AND_ASSIG should be in a private section.
The CQ bit was checked by pbond@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892873002/430001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892873002/430001
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 pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/1892873002/#ps450001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892873002/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892873002/450001
Thanks a lot for reviewing! https://codereview.chromium.org/1892873002/diff/410001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1892873002/diff/410001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:415: FOR_EACH_OBSERVER(Observer, observer_list_, OnShutdownBridge()); On 2016/05/03 16:57:23, xiyuan wrote: > nit: move this to the end of the function? Done. https://codereview.chromium.org/1892873002/diff/410001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:613: if (profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn) || On 2016/05/03 16:53:44, bartfab (slow) wrote: > Nit: Multi-line conditionals require curly braces. Done. https://codereview.chromium.org/1892873002/diff/410001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/1892873002/diff/410001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:76: DISALLOW_COPY_AND_ASSIGN(ArcAuthServiceObserver); On 2016/05/03 16:57:23, xiyuan wrote: > nit: This and the other two DISALLOW_COPY_AND_ASSIG should be in a private > section. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/1892873002/#ps470001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892873002/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892873002/470001
Message was sent while issue was closed.
Description was changed from ========== Add CheckAndroidManagement to ARC sign-in flow. ARC is not available for Chrome unmanaged and Android managed accounts. Check whether account is managed by Android every time on ARC start. Always allow ARC for managed by Chrome policy and well-known consumer accounts. Uncheck ARC checkbox in case of any error. BUG=602612 TEST=manual ========== to ========== Add CheckAndroidManagement to ARC sign-in flow. ARC is not available for Chrome unmanaged and Android managed accounts. Check whether account is managed by Android every time on ARC start. Always allow ARC for managed by Chrome policy and well-known consumer accounts. Uncheck ARC checkbox in case of any error. BUG=602612 TEST=manual ==========
Message was sent while issue was closed.
Committed patchset #18 (id:470001)
Message was sent while issue was closed.
Description was changed from ========== Add CheckAndroidManagement to ARC sign-in flow. ARC is not available for Chrome unmanaged and Android managed accounts. Check whether account is managed by Android every time on ARC start. Always allow ARC for managed by Chrome policy and well-known consumer accounts. Uncheck ARC checkbox in case of any error. BUG=602612 TEST=manual ========== to ========== Add CheckAndroidManagement to ARC sign-in flow. ARC is not available for Chrome unmanaged and Android managed accounts. Check whether account is managed by Android every time on ARC start. Always allow ARC for managed by Chrome policy and well-known consumer accounts. Uncheck ARC checkbox in case of any error. BUG=602612 TEST=manual Committed: https://crrev.com/b7ceb29f22004c344c982e48de1e436834b69274 Cr-Commit-Position: refs/heads/master@{#391482} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/b7ceb29f22004c344c982e48de1e436834b69274 Cr-Commit-Position: refs/heads/master@{#391482}
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:470001) has been created in https://codereview.chromium.org/1951343002/ by lgcheng@google.com. The reason for reverting is: This Patch blocks android boot. Will contact the author to resolve the issue and resubmit..
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:470001) has been created in https://codereview.chromium.org/1948383002/ by elijahtaylor@chromium.org. The reason for reverting is: broken ARC builders. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
