Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2707133006:
Start ARC and sign in after Chrome OS login (Closed)
|
Created:
3 years, 10 months ago by victorhsieh Modified:
3 years, 9 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, sadrul, yusukes+watch_chromium.org, michaelpg+watch-md-settings_chromium.org, tfarina, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, kalyank, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, davemoore+watch_chromium.org, Matt Giuca Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStart ARC and sign in after Chrome OS login
This feature is guarded by --arc-always-start.
Goal of this change include:
- Always start ARC after Chrome OS login
- Start sign-in flow automatically
- Rename Arc to Play in the code when appropriate
Non-goal and next plans:
- Support Play Store opt-in
- Support Play Store opt-out
- Support non-primary-profile use cases
This change means to be the very first step of Persistent ARC.
Once landed, each individual "next plan" can be started.
TEST=unit_tests
TEST=add --arc-always-start to /etc/chrome_dev.conf, login new
account on chrome os. As expected, no opt-in window showed up,
Android was running, gmail account was set up correctly. Able to
launch Settings, Play Movies and Play Store from the launcher
(though some of them should be disabled later).
BUG=b:32746126
Review-Url: https://codereview.chromium.org/2707133006
Cr-Commit-Position: refs/heads/master@{#455174}
Committed: https://chromium.googlesource.com/chromium/src/+/ec70785d809d34826ed4803537dc64885f7d8d13
Patch Set 1 #
Total comments: 38
Patch Set 2 : rebase to fix patch conflict #Patch Set 3 : address review comments #Patch Set 4 : test coverage in chrome_launcher_controller_impl_unittest.cc #
Total comments: 15
Patch Set 5 : rename flag #
Total comments: 8
Patch Set 6 : . #
Total comments: 2
Patch Set 7 : LaunchApp change #Patch Set 8 : LaunchApp #
Total comments: 11
Patch Set 9 : rebase + a few comment change per review #
Total comments: 22
Patch Set 10 : Start ARC and sign in after Chrome OS login #
Total comments: 6
Patch Set 11 : enum, comments #
Total comments: 11
Patch Set 12 : rebase then add test; NOTREACHED -> DCHECK #
Total comments: 4
Patch Set 13 : update comment #
Total comments: 6
Patch Set 14 : remove parameterization #Patch Set 15 : remove parameterization #Patch Set 16 : minimize test diff #
Total comments: 4
Patch Set 17 : Start ARC and sign in after Chrome OS login #Patch Set 18 : rebase to ToT #Messages
Total messages: 80 (40 generated)
The CQ bit was checked by victorhsieh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
victorhsieh@chromium.org changed reviewers: + hidehiko@chromium.org, khmel@chromium.org, yusukes@chromium.org
PTAL This is the first step. Once landed, we can start implementing opt-in, opt-out, kisok mode, guest mode, etc. And of course connect to Yusuke's work when ready.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
still reviewing, but some initial comments below: https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. In case you missed https://codereview.chromium.org/2702723002/ by hidehiko, you might want to rebase this on top of hidehiko's once it lands. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:125: static bool IsPersistentArc(); I haven't reviewed this CL in detail yet, but the word Persistent ARC seems a bit too broad here because it is a project name that consists of a series of features. Also, developers who are not familiar with the project won't immediately get the meaning. The word "persistent" may also give a wrong impression that the instance will outlive the user session when IsPersistentArc() is true. Could you use a little bit more specific function name (and flag name) for this? As we chatted off line, what about something like ShouldAlwaysStartArc()? hidehiko@, WDYT? Any preference on this?
The CQ bit was checked by victorhsieh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Could you describe the goal and non-goal of this CL a bit more clearer in CL description? I know you added TODOs, but there are many edge cases in ArcSessionManager for historical reason, so, for better review, I'd like to understand your intention first. Anyway, here are my initial review. Note: bot failure looks real one. Could you take a look into them, too? https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:523: const bool play_enabled = IsArcPlayStoreEnabled(); Could you use "play_store" instead of "play" for consistency? Ditto for all this CL. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:549: if (IsPersistentArc()) { Clarification: Several services expect OnArcPlayStoreEnabledChanged() callback below. Any plans about that? https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:744: if (IsPersistentArc()) { Maybe merge this into L757? https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:747: if (!profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn)) IIUC, background management check is always needed? If there is special reason here, could you comment? https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:775: StartArcAndroidManagementCheckedInBackground(); Maybe: "Check" rather than "Checked"? https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:889: android_management_checker_ = base::MakeUnique<ArcAndroidManagementChecker>( Could you add DCHECKs similar to StartArcAndroidManagementCheck() https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:125: static bool IsPersistentArc(); On 2017/02/23 00:47:13, Yusuke Sato wrote: > I haven't reviewed this CL in detail yet, but the word Persistent ARC seems a > bit too broad here because it is a project name that consists of a series of > features. Also, developers who are not familiar with the project won't > immediately get the meaning. The word "persistent" may also give a wrong > impression that the instance will outlive the user session when > IsPersistentArc() is true. > > Could you use a little bit more specific function name (and flag name) for this? > As we chatted off line, what about something like ShouldAlwaysStartArc()? > > hidehiko@, WDYT? Any preference on this? Makes sense. Another candidate is ShouldAlwaysRunArc()? Also, IMHO, it'd be nice if the flag name is also consistent with the function name somehow. Could you move this into components/arc/arc_util? https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:239: INSTANTIATE_TEST_CASE_P(, Any name? https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:241: ::testing::Values(false, true)); ::testing::Bool() can be used? https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:730: bool IsPersistentArcEnabled() override { return false; } This also should be tested with and without PARC? https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/not... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/not... chrome/browser/chromeos/note_taking_helper.cc:245: if (play_enabled_ && If PARC work is done, IntentHelper should be available even if Play Store is not yet enabled? https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_test.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_test.cc:36: void ArcAppTest::Init(bool is_persistent_arc) { Could you move this to components/arc/arc_util, too? Maybe SetArcAlways{Run,Start}ForTesting() etc.? https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_utils.cc:272: if (arc_session_manager->IsPersistentArc()) { nit: Could you elide the brace? https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_utils.cc:272: if (arc_session_manager->IsPersistentArc()) { If Play Store is not yet enabled, I think it is still needed to enable it even under PARC, otherwise apps on Play Store cannot be launched. That means, apps need to be categorized into two groups, ones can run with PARC, and ones need Play Store. Am I correct? If so any comments and/or plans? https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:826: void EnablePlay(bool enabled) { Maybe TODO to test with PARC? https://codereview.chromium.org/2707133006/diff/1/chromeos/chromeos_switches.cc File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/2707133006/diff/1/chromeos/chromeos_switches.... chromeos/chromeos_switches.cc:240: // Enables ARC for all users excluding Play. This sounds confusing, because this is still only for Primary user. Then, maybe Runs ARC for all primary users regardless of whether they have opted-in to use Google Play Store or not.
https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:125: static bool IsPersistentArc(); On 2017/02/23 10:29:16, hidehiko wrote: > On 2017/02/23 00:47:13, Yusuke Sato wrote: > > I haven't reviewed this CL in detail yet, but the word Persistent ARC seems a > > bit too broad here because it is a project name that consists of a series of > > features. Also, developers who are not familiar with the project won't > > immediately get the meaning. The word "persistent" may also give a wrong > > impression that the instance will outlive the user session when > > IsPersistentArc() is true. > > > > Could you use a little bit more specific function name (and flag name) for > this? > > As we chatted off line, what about something like ShouldAlwaysStartArc()? > > > > hidehiko@, WDYT? Any preference on this? > > Makes sense. Another candidate is ShouldAlwaysRunArc()? > Also, IMHO, it'd be nice if the flag name is also consistent with the function > name somehow. > > > Could you move this into components/arc/arc_util? a bit bikeshedding: To be consistent with helper functions in arc_util, maybe ShouldArc...() looks better?
PTAL https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:523: const bool play_enabled = IsArcPlayStoreEnabled(); On 2017/02/23 10:29:16, hidehiko wrote: > Could you use "play_store" instead of "play" for consistency? Ditto for all this > CL. Done. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:549: if (IsPersistentArc()) { On 2017/02/23 10:29:16, hidehiko wrote: > Clarification: Several services expect OnArcPlayStoreEnabledChanged() callback > below. > Any plans about that? Fixed. It's not intentional. Thanks. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:744: if (IsPersistentArc()) { On 2017/02/23 10:29:16, hidehiko wrote: > Maybe merge this into L757? Done. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:747: if (!profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn)) On 2017/02/23 10:29:16, hidehiko wrote: > IIUC, background management check is always needed? > If there is special reason here, could you comment? Done. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:775: StartArcAndroidManagementCheckedInBackground(); On 2017/02/23 10:29:16, hidehiko wrote: > Maybe: "Check" rather than "Checked"? Done. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:889: android_management_checker_ = base::MakeUnique<ArcAndroidManagementChecker>( On 2017/02/23 10:29:16, hidehiko wrote: > Could you add DCHECKs similar to StartArcAndroidManagementCheck() Reverted this change. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/23 00:47:13, Yusuke Sato wrote: > In case you missed https://codereview.chromium.org/2702723002/ by hidehiko, you > might want to rebase this on top of hidehiko's once it lands. Thanks. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:125: static bool IsPersistentArc(); On 2017/02/23 10:29:16, hidehiko wrote: > On 2017/02/23 00:47:13, Yusuke Sato wrote: > > I haven't reviewed this CL in detail yet, but the word Persistent ARC seems a > > bit too broad here because it is a project name that consists of a series of > > features. Also, developers who are not familiar with the project won't > > immediately get the meaning. The word "persistent" may also give a wrong > > impression that the instance will outlive the user session when > > IsPersistentArc() is true. > > > > Could you use a little bit more specific function name (and flag name) for > this? > > As we chatted off line, what about something like ShouldAlwaysStartArc()? > > > > hidehiko@, WDYT? Any preference on this? > > Makes sense. Another candidate is ShouldAlwaysRunArc()? > Also, IMHO, it'd be nice if the flag name is also consistent with the function > name somehow. > > > Could you move this into components/arc/arc_util? Done. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:239: INSTANTIATE_TEST_CASE_P(, On 2017/02/23 10:29:16, hidehiko wrote: > Any name? I prefer not to. It will change the test name from FooTest to FooTest/FooTest without benefit in this case. Many existing code does the same thing. Actually, perhaps let me remove the name from the other case in this file. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:241: ::testing::Values(false, true)); On 2017/02/23 10:29:16, hidehiko wrote: > ::testing::Bool() can be used? Done. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:730: bool IsPersistentArcEnabled() override { return false; } On 2017/02/23 10:29:16, hidehiko wrote: > This also should be tested with and without PARC? Done. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/not... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/not... chrome/browser/chromeos/note_taking_helper.cc:245: if (play_enabled_ && On 2017/02/23 10:29:16, hidehiko wrote: > If PARC work is done, IntentHelper should be available even if Play Store is not > yet enabled? IIUC, this app is installed from Play automatically to the supported devices, so the logic should be correct. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_test.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_test.cc:36: void ArcAppTest::Init(bool is_persistent_arc) { On 2017/02/23 10:29:16, hidehiko wrote: > Could you move this to components/arc/arc_util, too? > > Maybe SetArcAlways{Run,Start}ForTesting() etc.? Done. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_utils.cc:272: if (arc_session_manager->IsPersistentArc()) { On 2017/02/23 10:29:16, hidehiko wrote: > If Play Store is not yet enabled, I think it is still needed to enable it even > under PARC, otherwise apps on Play Store cannot be launched. > That means, apps need to be categorized into two groups, ones can run with PARC, > and ones need Play Store. Am I correct? If so any comments and/or plans? Right. There won't be apps from Play Store in opt-out mode, but default apps like the note taking one is the exception that conceptually linked to Play Store. Clicking on them should launch the opt-in flow just like the opt out case today. This should be already the case and pARC shouldn't change that from user's perspective. Consider the use cases (line number based on the original copy): 1) launching app in the system image (L267 false -> L313) 2) launching default apps that needs to be installed from Play Store (L267 true -> L272 -> L278 -> L294) Now it seems to me that there is no difference between pARC and today here. I'm removing my change here, but please let me know if I missed something. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:826: void EnablePlay(bool enabled) { On 2017/02/23 10:29:17, hidehiko wrote: > Maybe TODO to test with PARC? Done. I didn't realize many of these tests need rework. Will work on that in parallel. https://codereview.chromium.org/2707133006/diff/1/chromeos/chromeos_switches.cc File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/2707133006/diff/1/chromeos/chromeos_switches.... chromeos/chromeos_switches.cc:240: // Enables ARC for all users excluding Play. On 2017/02/23 10:29:17, hidehiko wrote: > This sounds confusing, because this is still only for Primary user. > Then, maybe > Runs ARC for all primary users regardless of whether they have opted-in to use > Google Play Store or not. Re-write the comments. Hope it's less confusing.
The CQ bit was checked by victorhsieh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
PTAL. I'm refraining to rebase (thus red trybots) until the review is close to finish. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_utils.cc:272: if (arc_session_manager->IsPersistentArc()) { On 2017/02/24 00:45:00, victorhsieh wrote: > On 2017/02/23 10:29:16, hidehiko wrote: > > If Play Store is not yet enabled, I think it is still needed to enable it even > > under PARC, otherwise apps on Play Store cannot be launched. > > That means, apps need to be categorized into two groups, ones can run with > PARC, > > and ones need Play Store. Am I correct? If so any comments and/or plans? > > Right. There won't be apps from Play Store in opt-out mode, but default apps > like the note taking one is the exception that conceptually linked to Play > Store. Clicking on them should launch the opt-in flow just like the opt out > case today. This should be already the case and pARC shouldn't change that from > user's perspective. > > Consider the use cases (line number based on the original copy): > 1) launching app in the system image (L267 false -> L313) > 2) launching default apps that needs to be installed from Play Store (L267 true > -> L272 -> L278 -> L294) > > Now it seems to me that there is no difference between pARC and today here. I'm > removing my change here, but please let me know if I missed something. Seems like there is an unlikely edge case when apps are registered, but the instance is not ready. Modify to bypass the NOTREACHED for pARC. https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:301: // FIXME: what's the best way/place to detect and cancel this? Please advise.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
more bikeshedding drive-by. https://codereview.chromium.org/2707133006/diff/60001/chromeos/chromeos_switc... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/2707133006/diff/60001/chromeos/chromeos_switc... chromeos/chromeos_switches.cc:58: // Always starts ARC after login screen without Play Store, not only for primary nit: we can make the wording stronger: "Always starts ARC after login screen without Play Store. This still requires that both the hardware and account/profile supports starting ARC." that way we tie it to normal ARC support and don't have to update the wording every time we add support for more modes (like AD auth). https://codereview.chromium.org/2707133006/diff/60001/chromeos/chromeos_switc... chromeos/chromeos_switches.cc:62: const char kAlwaysStartArc[] = "always-start-arc"; for consistency with all the other ARC flags, can this be "arc-start-always"? (even other non-ARC flags also seem to follow the convention of kComponentFlag... or kDisableComponentFlag...) https://codereview.chromium.org/2707133006/diff/60001/components/arc/arc_util.h File components/arc/arc_util.h (right): https://codereview.chromium.org/2707133006/diff/60001/components/arc/arc_util... components/arc/arc_util.h:32: bool ShouldAlwaysStartArc(); same here, ShouldArcStartAlways
https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:610: if (enabled || arc::ShouldAlwaysStartArc()) Top level comment and thoughts. I think that is might be better to put control logic to Android side only and here only rely on events from Android. For example if user Opts out from using Play Store. IIUC we should automatically delete all non-system packages on Android, right? If so we can try to handle Package added/removed/updated events and drop logic here to analyze if ARC is enabled or not. RemoveAllApps here is to handle event ARC stopped as opt-out, which, it seems no longer the case for pARC. This potentially may seriously simplify impl here. Yet another point. I am not sure if you aware or not about secondary user profile case. At this moment we don't create ArcAppListPrefs for such profile. We probably need to revise ArcAppListPrefsFactory. I am not forcing to do in this CL, just keep in mind. WDYT?
PTAL https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:610: if (enabled || arc::ShouldAlwaysStartArc()) On 2017/02/28 00:10:10, khmel wrote: > Top level comment and thoughts. > > I think that is might be better to put control logic to Android side only and > here only rely on events from Android. > > For example if user Opts out from using Play Store. IIUC we should automatically > delete all non-system packages on Android, right? If so we can try to handle > Package added/removed/updated events and drop logic here to analyze if ARC is > enabled or not. RemoveAllApps here is to handle event ARC stopped as opt-out, > which, it seems no longer the case for pARC. This potentially may seriously > simplify impl here. Yes, that's also my plan, too. This CL should pretty much make this happen after login, excluding unimplemented opt-in mode. In fact, this line of change should not be part of this CL, but the future CL that implements opt-in. So reverting. > > Yet another point. > I am not sure if you aware or not about secondary user profile case. At this > moment we don't create ArcAppListPrefs for such profile. We probably need to > revise ArcAppListPrefsFactory. I am not forcing to do in this CL, just keep in > mind. Secondary profile is very unlikely to be supported in pARC. In fact, there is a discussion to rethink the UX. > > WDYT? Yes https://codereview.chromium.org/2707133006/diff/60001/chromeos/chromeos_switc... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/2707133006/diff/60001/chromeos/chromeos_switc... chromeos/chromeos_switches.cc:58: // Always starts ARC after login screen without Play Store, not only for primary On 2017/02/27 17:13:15, Luis Héctor Chávez wrote: > nit: we can make the wording stronger: "Always starts ARC after login screen > without Play Store. This still requires that both the hardware and > account/profile supports starting ARC." that way we tie it to normal ARC support > and don't have to update the wording every time we add support for more modes > (like AD auth). Actually, ARC will run without account/profile. We will only have exception case when ARC won't run, possibly including secondary profile. Let me reword. https://codereview.chromium.org/2707133006/diff/60001/chromeos/chromeos_switc... chromeos/chromeos_switches.cc:62: const char kAlwaysStartArc[] = "always-start-arc"; On 2017/02/27 17:13:15, Luis Héctor Chávez wrote: > for consistency with all the other ARC flags, can this be "arc-start-always"? > (even other non-ARC flags also seem to follow the convention of > kComponentFlag... or kDisableComponentFlag...) Done. Also renamed the functions accordingly. https://codereview.chromium.org/2707133006/diff/60001/components/arc/arc_util.h File components/arc/arc_util.h (right): https://codereview.chromium.org/2707133006/diff/60001/components/arc/arc_util... components/arc/arc_util.h:32: bool ShouldAlwaysStartArc(); On 2017/02/27 17:13:15, Luis Héctor Chávez wrote: > same here, ShouldArcStartAlways Done.
Sorry for delay. I think this is almost ok as a first step of pARC. https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:523: const bool play_enabled = IsArcPlayStoreEnabled(); On 2017/02/24 00:44:59, victorhsieh wrote: > On 2017/02/23 10:29:16, hidehiko wrote: > > Could you use "play_store" instead of "play" for consistency? Ditto for all > this > > CL. > > Done. There are still many "play" (not "play store") usage in this CL. Could you fix all of them for consistency? https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:301: // FIXME: what's the best way/place to detect and cancel this? On 2017/02/25 00:21:33, victorhsieh wrote: > Please advise. Could you share which test is problematic and its stacktrace? In real use case, the ASM is destroyed after UI message loop is stopped, then ArcAppListPrefs is destroyed. Between them we do not expect this is called. FYI: crbug.com/672829 Slightly related off-topic. For historical reason, shutdown operation is not in the reverse order of the construction. To make the situation simpler, it's nice to fix it, IMO. Considering the current plan, I think early next milestone could be a good timing for this fix, at the moment. https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:207: class AbstractArcSessionManagerTest : public ArcSessionManagerTestBase { As I CC'ed you, I'm extracting the "Google Play Store enabled preference" part from ASM. (Will send the main CL maybe tomorrow in my TZ). Thus, the main change/test will be for the new class. Then, is it still needed to test all combination with and without pARC flag? Because this is unittest, is it possible to minimize the parameterized testing? https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_utils.cc:282: if (arc::ShouldArcAlwaysStart()) { nit: "guard" is preferred in ARC code. So, how about; if (!arc::ShouldArcAlwaysStart()) { NOTREACHED(); return false; } arc_activated = true;
https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. This is almost the same request as #12 (by hidehiko), but could you also mention non-goals and future plans (i.e. why this first step is important for b:32746126 and how later steps will look like) in the CL description?
Description was changed from
==========
Start ARC and sign in after Chrome OS login
This feature is guarded by --enable-arc-persistently.
Goal of this change include:
- Always start ARC after Chrome OS login
- Start sign-in flow automatically
- Rename Arc to Play in the code when appropriate
TEST=unit_tests
TEST=add --enable-arc-persistently to /etc/chrome_dev.conf, login new
account on chrome os. As expected, no opt-in window showed up,
Android was running, gmail account was set up correctly. Able to
launch Settings, Play Movies and Play Store from the launcher
(though some of them should be disabled later).
BUG=b:32746126
==========
to
==========
Start ARC and sign in after Chrome OS login
This feature is guarded by --enable-arc-persistently.
Goal of this change include:
- Always start ARC after Chrome OS login
- Start sign-in flow automatically
- Rename Arc to Play in the code when appropriate
Non-goal and next plans:
- Support Play Store opt-in
- Support Play Store opt-out
- Support non-primary-profile use cases
This change means to be the first step, and will unblock each individual "next
plans".
TEST=unit_tests
TEST=add --enable-arc-persistently to /etc/chrome_dev.conf, login new
account on chrome os. As expected, no opt-in window showed up,
Android was running, gmail account was set up correctly. Able to
launch Settings, Play Movies and Play Store from the launcher
(though some of them should be disabled later).
BUG=b:32746126
==========
Description was changed from
==========
Start ARC and sign in after Chrome OS login
This feature is guarded by --enable-arc-persistently.
Goal of this change include:
- Always start ARC after Chrome OS login
- Start sign-in flow automatically
- Rename Arc to Play in the code when appropriate
Non-goal and next plans:
- Support Play Store opt-in
- Support Play Store opt-out
- Support non-primary-profile use cases
This change means to be the first step, and will unblock each individual "next
plans".
TEST=unit_tests
TEST=add --enable-arc-persistently to /etc/chrome_dev.conf, login new
account on chrome os. As expected, no opt-in window showed up,
Android was running, gmail account was set up correctly. Able to
launch Settings, Play Movies and Play Store from the launcher
(though some of them should be disabled later).
BUG=b:32746126
==========
to
==========
Start ARC and sign in after Chrome OS login
This feature is guarded by --enable-arc-persistently.
Goal of this change include:
- Always start ARC after Chrome OS login
- Start sign-in flow automatically
- Rename Arc to Play in the code when appropriate
Non-goal and next plans:
- Support Play Store opt-in
- Support Play Store opt-out
- Support non-primary-profile use cases
This change means to be the very first step of Persistent ARC. Once landed,
each individual "next plan" can be started.
TEST=unit_tests
TEST=add --enable-arc-persistently to /etc/chrome_dev.conf, login new
account on chrome os. As expected, no opt-in window showed up,
Android was running, gmail account was set up correctly. Able to
launch Settings, Play Movies and Play Store from the launcher
(though some of them should be disabled later).
BUG=b:32746126
==========
https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2707133006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:523: const bool play_enabled = IsArcPlayStoreEnabled(); On 2017/02/28 19:26:06, hidehiko wrote: > On 2017/02/24 00:44:59, victorhsieh wrote: > > On 2017/02/23 10:29:16, hidehiko wrote: > > > Could you use "play_store" instead of "play" for consistency? Ditto for all > > this > > > CL. > > > > Done. > > There are still many "play" (not "play store") usage in this CL. Could you fix > all of them for consistency? Really done. Double checked with `git show | grep play -i |grep -v -i 'play[ _]*store' |grep -v display -i`. https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:301: // FIXME: what's the best way/place to detect and cancel this? On 2017/02/28 19:26:06, hidehiko wrote: > On 2017/02/25 00:21:33, victorhsieh wrote: > > Please advise. > > Could you share which test is problematic and its stacktrace? ChromeLauncherControllerImplWithArcTest.ArcAppPin/1 is an example. https://paste.googleplex.com/5898263773315072 > > In real use case, the ASM is destroyed after UI message loop is stopped, > then ArcAppListPrefs is destroyed. Between them we do not expect this is called. It does look like in this failing case, ASM is destroyed (in TearDown) before message loop. So perhaps option 1) keep this check or something similar/better, option 2) reorder ChromeLauncherControllerImplTest::TearDown (not sure if it's safe), or option 3) stub out some code to prevent the test going that deep if possible. WDYT? BTW, this problem only surfaces now because without the new flag, this function returns early at L310. That is, the callback still runs, but didn't access the deleted object. https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/28 20:18:12, Yusuke Sato wrote: > This is almost the same request as #12 (by hidehiko), but could you also mention > non-goals and future plans (i.e. why this first step is important for b:32746126 > and how later steps will look like) in the CL description? Done. https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:207: class AbstractArcSessionManagerTest : public ArcSessionManagerTestBase { On 2017/02/28 19:26:07, hidehiko wrote: > As I CC'ed you, I'm extracting the "Google Play Store enabled preference" part > from ASM. (Will send the main CL maybe tomorrow in my TZ). > Thus, the main change/test will be for the new class. OK, I can wait until your refactoring is done, and rebase my change. > > Then, is it still needed to test all combination with and without pARC flag? > Because this is unittest, is it possible to minimize the parameterized testing? Yes, I like to minimize the TEST_P if possible. Sounds like your change will allow this with better modulization. Thanks! https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_utils.cc:282: if (arc::ShouldArcAlwaysStart()) { On 2017/02/28 19:26:07, hidehiko wrote: > nit: "guard" is preferred in ARC code. So, how about; > > if (!arc::ShouldArcAlwaysStart()) { > NOTREACHED(); > return false; > } > arc_activated = true; Done.
https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:610: if (enabled || arc::ShouldAlwaysStartArc()) On 2017/02/28 18:57:33, victorhsieh wrote: > On 2017/02/28 00:10:10, khmel wrote: > > Top level comment and thoughts. > > > > I think that is might be better to put control logic to Android side only and > > here only rely on events from Android. > > > > For example if user Opts out from using Play Store. IIUC we should > automatically > > delete all non-system packages on Android, right? If so we can try to handle > > Package added/removed/updated events and drop logic here to analyze if ARC is > > enabled or not. RemoveAllApps here is to handle event ARC stopped as opt-out, > > which, it seems no longer the case for pARC. This potentially may seriously > > simplify impl here. > Yes, that's also my plan, too. This CL should pretty much make this happen > after login, excluding unimplemented opt-in mode. In fact, this line of change > should not be part of this CL, but the future CL that implements opt-in. So > reverting. > > > > > Yet another point. > > I am not sure if you aware or not about secondary user profile case. At this > > moment we don't create ArcAppListPrefs for such profile. We probably need to > > revise ArcAppListPrefsFactory. I am not forcing to do in this CL, just keep in > > mind. > Secondary profile is very unlikely to be supported in pARC. In fact, there is a > discussion to rethink the UX. Even system apps? > > > > > WDYT? > > Yes https://codereview.chromium.org/2707133006/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:272: bool arc_activated = false; Please add TODO or rename to something like play_store_enabled
https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:610: if (enabled || arc::ShouldAlwaysStartArc()) On 2017/02/28 21:58:44, khmel wrote: > On 2017/02/28 18:57:33, victorhsieh wrote: > > On 2017/02/28 00:10:10, khmel wrote: > > > Top level comment and thoughts. > > > > > > I think that is might be better to put control logic to Android side only > and > > > here only rely on events from Android. > > > > > > For example if user Opts out from using Play Store. IIUC we should > > automatically > > > delete all non-system packages on Android, right? If so we can try to handle > > > Package added/removed/updated events and drop logic here to analyze if ARC > is > > > enabled or not. RemoveAllApps here is to handle event ARC stopped as > opt-out, > > > which, it seems no longer the case for pARC. This potentially may seriously > > > simplify impl here. > > Yes, that's also my plan, too. This CL should pretty much make this happen > > after login, excluding unimplemented opt-in mode. In fact, this line of > change > > should not be part of this CL, but the future CL that implements opt-in. So > > reverting. > > > > > > > > Yet another point. > > > I am not sure if you aware or not about secondary user profile case. At this > > > moment we don't create ArcAppListPrefs for such profile. We probably need to > > > revise ArcAppListPrefsFactory. I am not forcing to do in this CL, just keep > in > > > mind. > > Secondary profile is very unlikely to be supported in pARC. In fact, there is > a > > discussion to rethink the UX. > > Even system apps? Yes. But the plan is still TBD at the moment. https://codereview.chromium.org/2707133006/diff/100001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/100001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:272: bool arc_activated = false; On 2017/02/28 21:58:44, khmel wrote: > Please add TODO or rename to something like play_store_enabled Revised to make these more clear as discussed offline.
lgtm, deffer to other reviewers
defer to hidehiko@ c/b/c/arc/ and c/arc/ lgtm https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.h (left): https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.h:124: // Returns true if ARC is allowed to run for the current session. Any reason to remove the comment? Your CL doesn't touch the function at all. https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. > This feature is guarded by --enable-arc-persistently. Please update the flag name in the description. > This change means to be the very first step of Persistent ARC. Once landed, each individual "next plan" can be started. This probably exceeds 67 col? https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.h:2: // Use of this source code is governed by a BSD-style license that can be Please run git cl format and git cl lint if you haven't. https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.h:3: // found in the LICENSE file. Please rebase and run git cl try to make sure all tests pass. https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:300: // FIXME: what's the best way/place to detect and cancel this? FIXME is unusual in Chromium code. TODO(your_ldap) would be better. https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:827: // TODO(victorhsieh): Add test coverage for Persistent ARC nit: can you rephrase this to make it more specific and actionable? as I mentioned before, Persistent ARC seems to be too broad and vague to use in code.
Description was changed from
==========
Start ARC and sign in after Chrome OS login
This feature is guarded by --enable-arc-persistently.
Goal of this change include:
- Always start ARC after Chrome OS login
- Start sign-in flow automatically
- Rename Arc to Play in the code when appropriate
Non-goal and next plans:
- Support Play Store opt-in
- Support Play Store opt-out
- Support non-primary-profile use cases
This change means to be the very first step of Persistent ARC. Once landed,
each individual "next plan" can be started.
TEST=unit_tests
TEST=add --enable-arc-persistently to /etc/chrome_dev.conf, login new
account on chrome os. As expected, no opt-in window showed up,
Android was running, gmail account was set up correctly. Able to
launch Settings, Play Movies and Play Store from the launcher
(though some of them should be disabled later).
BUG=b:32746126
==========
to
==========
Start ARC and sign in after Chrome OS login
This feature is guarded by --arc-always-start.
Goal of this change include:
- Always start ARC after Chrome OS login
- Start sign-in flow automatically
- Rename Arc to Play in the code when appropriate
Non-goal and next plans:
- Support Play Store opt-in
- Support Play Store opt-out
- Support non-primary-profile use cases
This change means to be the very first step of Persistent ARC.
Once landed, each individual "next plan" can be started.
TEST=unit_tests
TEST=add --arc-always-start to /etc/chrome_dev.conf, login new
account on chrome os. As expected, no opt-in window showed up,
Android was running, gmail account was set up correctly. Able to
launch Settings, Play Movies and Play Store from the launcher
(though some of them should be disabled later).
BUG=b:32746126
==========
The CQ bit was checked by victorhsieh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Rebase + minor comments change as suggested. Trybot passed. Sorry I happened to just finish the rebase. Since the change to address the comment is small enough, I didn't try to split for clearness. https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.h (left): https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.h:124: // Returns true if ARC is allowed to run for the current session. On 2017/03/01 20:43:57, Yusuke Sato wrote: > Any reason to remove the comment? Your CL doesn't touch the function at all. Done. Thanks. I lost in rebase. https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/01 20:43:57, Yusuke Sato wrote: > > This feature is guarded by --enable-arc-persistently. > > Please update the flag name in the description. > > > This change means to be the very first step of Persistent ARC. Once landed, > each individual "next plan" can be started. > > This probably exceeds 67 col? Done. https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.h:2: // Use of this source code is governed by a BSD-style license that can be On 2017/03/01 20:43:57, Yusuke Sato wrote: > Please run git cl format and git cl lint if you haven't. I did. There are just a few unrelated errors in c/b/ui/webui. I can send out separate CL to fix them. https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:300: // FIXME: what's the best way/place to detect and cancel this? On 2017/03/01 20:43:57, Yusuke Sato wrote: > FIXME is unusual in Chromium code. TODO(your_ldap) would be better. Sorry, I meant to ask if the reviewers have any thoughts. Let me switch to TODO for now, until we figure out the right fix. https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:827: // TODO(victorhsieh): Add test coverage for Persistent ARC On 2017/03/01 20:43:57, Yusuke Sato wrote: > nit: can you rephrase this to make it more specific and actionable? as I > mentioned before, Persistent ARC seems to be too broad and vague to use in code. Done.
high-level test comment: is it possible to parameterize all tests with an enum class to avoid having magic bools? Especially for the multi-parameterized ones, which should make the meaning clearer. https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:555: // nothing but keep the existing ARC instance running. nit: return; to avoid the else. https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:720: // If it is marked that sign in has been successfully done, then directly nit: "successfully done or if ARC has been set up to always start" https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:677: // This class takes two test parameters because the both itself and its child nit: s/because the both/because both/ https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper.h (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.h:205: bool play_store_enabled_ = false; Will there ever be a default note-taking app that will be pre-bundled with ARC? If so, this needs a TODO to switch this back to mean "ARC" instead of "Play Store". https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper_unittest.cc:165: enum InitFlags { nit: enum InitFlags : uint32_t https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper_unittest.cc:483: // TODO(victorhsieh): Imeplement opt-in. nit: Implement https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:445: (!IsArcAlive() || !IsArcAndroidEnabledForProfile(profile_))) { consider refactoring (!IsArcAlive() || !IsArcAndroidEnabledForProfile(profile_)) to its own function because it is used in several places. https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_unittest.cc:1382: INSTANTIATE_TEST_CASE_P(, Given that you're not exhaustively going through all combinations of ::testing::Values, can you mention what each entry in the tuple mean? https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:275: if (!IsArcPlayStoreEnabledForProfile(profile)) { This is the same condition as L272, so this will always be true if you hit this. https://codereview.chromium.org/2707133006/diff/160001/chromeos/chromeos_swit... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chromeos/chromeos_swit... chromeos/chromeos_switches.cc:67: // But there may be some exception that ARC will not run. Is there a way to make this a bit more exhaustive? Like kArcAvailability.
re converting double boolean to enum, I *think* it's not trivial to translate the current double boolean to enum, because I'm relying on gtest to generate the permutation. https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:555: // nothing but keep the existing ARC instance running. On 2017/03/01 22:25:38, Luis Héctor Chávez wrote: > nit: return; to avoid the else. L563-564 should still run. https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:720: // If it is marked that sign in has been successfully done, then directly On 2017/03/01 22:25:38, Luis Héctor Chávez wrote: > nit: "successfully done or if ARC has been set up to always start" Done. https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:677: // This class takes two test parameters because the both itself and its child On 2017/03/01 22:25:38, Luis Héctor Chávez wrote: > nit: s/because the both/because both/ Done. https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper.h (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.h:205: bool play_store_enabled_ = false; On 2017/03/01 22:25:38, Luis Héctor Chávez wrote: > Will there ever be a default note-taking app that will be pre-bundled with ARC? > If so, this needs a TODO to switch this back to mean "ARC" instead of "Play > Store". No. I think there is no such plan in the foreseeable future. https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper_unittest.cc:165: enum InitFlags { On 2017/03/01 22:25:38, Luis Héctor Chávez wrote: > nit: enum InitFlags : uint32_t Done. https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper_unittest.cc:483: // TODO(victorhsieh): Imeplement opt-in. On 2017/03/01 22:25:38, Luis Héctor Chávez wrote: > nit: Implement Done. https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:445: (!IsArcAlive() || !IsArcAndroidEnabledForProfile(profile_))) { On 2017/03/01 22:25:38, Luis Héctor Chávez wrote: > consider refactoring (!IsArcAlive() || !IsArcAndroidEnabledForProfile(profile_)) > to its own function because it is used in several places. As you mentioned offline about the same comment in the original change, I'll just leave this as is. But we should consider rename IsArcAlive (which doesn't mean IsArcInstanceAlive). https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_unittest.cc:1382: INSTANTIATE_TEST_CASE_P(, On 2017/03/01 22:25:38, Luis Héctor Chávez wrote: > Given that you're not exhaustively going through all combinations of > ::testing::Values, can you mention what each entry in the tuple mean? Actually this is just one switch. Change to ::testing::Bool(). https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:275: if (!IsArcPlayStoreEnabledForProfile(profile)) { On 2017/03/01 22:25:39, Luis Héctor Chávez wrote: > This is the same condition as L272, so this will always be true if you hit this. No, the previous line switches it. I don't know why we need to double check though. https://codereview.chromium.org/2707133006/diff/160001/chromeos/chromeos_swit... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chromeos/chromeos_swit... chromeos/chromeos_switches.cc:67: // But there may be some exception that ARC will not run. On 2017/03/01 22:25:39, Luis Héctor Chávez wrote: > Is there a way to make this a bit more exhaustive? Like kArcAvailability. Done.
turns out using an enum shouldn't be too complicated :D https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_unittest.cc:1382: INSTANTIATE_TEST_CASE_P(, On 2017/03/02 00:28:12, victorhsieh wrote: > On 2017/03/01 22:25:38, Luis Héctor Chávez wrote: > > Given that you're not exhaustively going through all combinations of > > ::testing::Values, can you mention what each entry in the tuple mean? > > Actually this is just one switch. Change to ::testing::Bool(). Ah! Misread that as those being the cases. re: substituting the boolean with an enum class, you can actually use this ::testing::Values(enum_val_1, enum_val_2) to iterate through the values of an enum: https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:301: // FIXME: what's the best way/place to detect and cancel this? On 2017/02/28 21:47:06, victorhsieh wrote: > On 2017/02/28 19:26:06, hidehiko wrote: > > On 2017/02/25 00:21:33, victorhsieh wrote: > > > Please advise. > > > > Could you share which test is problematic and its stacktrace? > > ChromeLauncherControllerImplWithArcTest.ArcAppPin/1 is an example. > https://paste.googleplex.com/5898263773315072 > > > > > In real use case, the ASM is destroyed after UI message loop is stopped, > > then ArcAppListPrefs is destroyed. Between them we do not expect this is > called. > > It does look like in this failing case, ASM is destroyed (in TearDown) before > message loop. So perhaps option 1) keep this check or something similar/better, > option 2) reorder ChromeLauncherControllerImplTest::TearDown (not sure if it's > safe), or option 3) stub out some code to prevent the test going that deep if > possible. WDYT? > > BTW, this problem only surfaces now because without the new flag, this function > returns early at L310. That is, the callback still runs, but didn't access the > deleted object. Great to know. Thank you for explanation. My two cents, 2) looks better to adapt the real usage. However, I'm sure existing ArcSessionManager's usage in tests is unexpected, unfortunately, and this is not the regression. So, as long as TODO is there, I think it is ok in either way for now. https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:207: class AbstractArcSessionManagerTest : public ArcSessionManagerTestBase { On 2017/02/28 21:47:06, victorhsieh wrote: > On 2017/02/28 19:26:07, hidehiko wrote: > > As I CC'ed you, I'm extracting the "Google Play Store enabled preference" part > > from ASM. (Will send the main CL maybe tomorrow in my TZ). > > Thus, the main change/test will be for the new class. > OK, I can wait until your refactoring is done, and rebase my change. > > > > > Then, is it still needed to test all combination with and without pARC flag? > > Because this is unittest, is it possible to minimize the parameterized > testing? > > Yes, I like to minimize the TEST_P if possible. Sounds like your change will > allow this with better modulization. Thanks! Thank you! I'll let you know when it's landed. I hope it's done within a day. https://codereview.chromium.org/2707133006/diff/180001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:155: bool IsArcAndroidEnabledForProfile(const Profile* profile) { Could you add detailed document about what is "AndroidEnabled" state? https://codereview.chromium.org/2707133006/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:444: if (!arc::ShouldArcAlwaysStart() && The condition looks complicated, because IsArcAndroidEnabledForProfile also contains ShouldArcAlwaysStart(). Could you simplify? https://codereview.chromium.org/2707133006/diff/180001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:271: Dropping this changes the behavior in case Google Play Store is enabled, but not yet ready?
https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:301: // FIXME: what's the best way/place to detect and cancel this? On 2017/03/02 15:57:51, hidehiko wrote: > On 2017/02/28 21:47:06, victorhsieh wrote: > > On 2017/02/28 19:26:06, hidehiko wrote: > > > On 2017/02/25 00:21:33, victorhsieh wrote: > > > > Please advise. > > > > > > Could you share which test is problematic and its stacktrace? > > > > ChromeLauncherControllerImplWithArcTest.ArcAppPin/1 is an example. > > https://paste.googleplex.com/5898263773315072 > > > > > > > > In real use case, the ASM is destroyed after UI message loop is stopped, > > > then ArcAppListPrefs is destroyed. Between them we do not expect this is > > called. > > > > It does look like in this failing case, ASM is destroyed (in TearDown) before > > message loop. So perhaps option 1) keep this check or something > similar/better, > > option 2) reorder ChromeLauncherControllerImplTest::TearDown (not sure if it's > > safe), or option 3) stub out some code to prevent the test going that deep if > > possible. WDYT? > > > > BTW, this problem only surfaces now because without the new flag, this > function > > returns early at L310. That is, the callback still runs, but didn't access > the > > deleted object. > > Great to know. Thank you for explanation. > > My two cents, 2) looks better to adapt the real usage. > However, I'm sure existing ArcSessionManager's usage in tests is unexpected, > unfortunately, and this is not the regression. So, as long as TODO is there, I > think it is ok in either way for now. Acknowledged. https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_unittest.cc:1382: INSTANTIATE_TEST_CASE_P(, On 2017/03/02 04:34:22, Luis Héctor Chávez wrote: > On 2017/03/02 00:28:12, victorhsieh wrote: > > On 2017/03/01 22:25:38, Luis Héctor Chávez wrote: > > > Given that you're not exhaustively going through all combinations of > > > ::testing::Values, can you mention what each entry in the tuple mean? > > > > Actually this is just one switch. Change to ::testing::Bool(). > > Ah! Misread that as those being the cases. > > re: substituting the boolean with an enum class, you can actually use this > ::testing::Values(enum_val_1, enum_val_2) to iterate through the values of an > enum: Ok, I also misread your comments :P Done. https://codereview.chromium.org/2707133006/diff/180001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2707133006/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:155: bool IsArcAndroidEnabledForProfile(const Profile* profile) { On 2017/03/02 15:57:51, hidehiko wrote: > Could you add detailed document about what is "AndroidEnabled" state? Done. https://codereview.chromium.org/2707133006/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:444: if (!arc::ShouldArcAlwaysStart() && On 2017/03/02 15:57:51, hidehiko wrote: > The condition looks complicated, because IsArcAndroidEnabledForProfile also > contains ShouldArcAlwaysStart(). Could you simplify? Now I shortcut ShouldArcAlwaysStart to return early. The rest of the condition is semantically more correct despite of duplicated call to ShouldArcAlwaysStart. I hope it's fine for now. Also, at the end of the work, we will remove --arc-always-start. https://codereview.chromium.org/2707133006/diff/180001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:271: On 2017/03/02 15:57:51, hidehiko wrote: > Dropping this changes the behavior in case Google Play Store is enabled, but not > yet ready? Yury and I had an offline discussion, then realized that variable is redundant, thus we reduced the code to the current way.
victorhsieh@chromium.org changed reviewers: + stevenjb@chromium.org
Steven, could you review the following files as owner? chrome/browser/chromeos/note_taking_helper.cc chrome/browser/chromeos/note_taking_helper.h chrome/browser/chromeos/note_taking_helper_unittest.cc chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc chrome/browser/ui/webui/options/browser_options_handler.cc chrome/browser/ui/webui/options/chromeos/options_stylus_handler.cc chrome/browser/ui/webui/settings/chromeos/device_stylus_handler.cc And these. I'm surprised khmel is not the OWNER. chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc chrome/browser/ui/app_list/arc/arc_app_test.cc chrome/browser/ui/app_list/arc/arc_app_unittest.cc chrome/browser/ui/app_list/arc/arc_app_utils.cc Thanks!
lgtm! https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:695: std::tuple<ArcAlwaysStartOption, bool>> { Much better :D
Looks good! Could you rebase? https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:207: class AbstractArcSessionManagerTest : public ArcSessionManagerTestBase { On 2017/03/02 15:57:51, hidehiko wrote: > On 2017/02/28 21:47:06, victorhsieh wrote: > > On 2017/02/28 19:26:07, hidehiko wrote: > > > As I CC'ed you, I'm extracting the "Google Play Store enabled preference" > part > > > from ASM. (Will send the main CL maybe tomorrow in my TZ). > > > Thus, the main change/test will be for the new class. > > OK, I can wait until your refactoring is done, and rebase my change. > > > > > > > > Then, is it still needed to test all combination with and without pARC flag? > > > Because this is unittest, is it possible to minimize the parameterized > > testing? > > > > Yes, I like to minimize the TEST_P if possible. Sounds like your change will > > allow this with better modulization. Thanks! > > Thank you! I'll let you know when it's landed. I hope it's done within a day. So, landed. After rebasing, I'd expect adding one new case in this file from ToT, which is checking RequestEnable() skips ToS negotiation etc. if --arc-always-start is set. Other tests will be moved into arc_play_store_enabled_preference_handler_unittests.cc, I think. https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:499: if (ShouldArcAlwaysStart() || IsArcPlayStoreEnabledForProfile(profile_)) { FYI: I landed a CL to extract this part. The rebasing should be straightforward. Thank you for your cooperation.
Also, FYI: crbug.com/696425 https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:555: // nothing but keep the existing ARC instance running. nit: Avoid an empty if clause; invert and move the comment above the test. https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:253: UpdateAndroidApps(); {} https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:276: NOTREACHED(); If this is logically impossibe: {D}CHECK(IsArcPlayStoreEnabledForProfile(profile)); Otherwise LOG(ERROR) or LOG(FATAL) here instead. https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:290: NOTREACHED(); Same here.
Also, FYI: crbug.com/696425 https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:555: // nothing but keep the existing ARC instance running. nit: Avoid an empty if clause; invert and move the comment above the test. https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:253: UpdateAndroidApps(); {} https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:276: NOTREACHED(); If this is logically impossibe: {D}CHECK(IsArcPlayStoreEnabledForProfile(profile)); Otherwise LOG(ERROR) or LOG(FATAL) here instead. https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:290: NOTREACHED(); Same here.
https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:499: if (ShouldArcAlwaysStart() || IsArcPlayStoreEnabledForProfile(profile_)) { On 2017/03/03 12:52:47, hidehiko wrote: > FYI: I landed a CL to extract this part. The rebasing should be straightforward. > Thank you for your cooperation. Done. https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:555: // nothing but keep the existing ARC instance running. On 2017/03/03 18:25:34, stevenjb wrote: > nit: Avoid an empty if clause; invert and move the comment above the test. Acknowledged. This is no longer the case after rebasing. https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper.cc (right): https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper.cc:253: UpdateAndroidApps(); On 2017/03/03 18:25:34, stevenjb wrote: > {} Done. https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:276: NOTREACHED(); On 2017/03/03 18:25:34, stevenjb wrote: > If this is logically impossibe: > {D}CHECK(IsArcPlayStoreEnabledForProfile(profile)); > > Otherwise LOG(ERROR) or LOG(FATAL) here instead. Done with more comments about the rationale. https://codereview.chromium.org/2707133006/diff/200001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:290: NOTREACHED(); On 2017/03/03 18:25:34, stevenjb wrote: > Same here. Done.
The CQ bit was checked by victorhsieh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
owner lgtm with one clarification request https://codereview.chromium.org/2707133006/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:276: // other logics prevent us from reaching here in that case. I'm not sure I follow. Isn't managed part of 'reality'? If it is the case that this code should never be called if SetArcPlayStoreEnabledForProfile() would not succeed, say that. If that is not the case, we should handle it and log a warning or error.
https://codereview.chromium.org/2707133006/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:276: // other logics prevent us from reaching here in that case. On 2017/03/03 18:59:06, stevenjb wrote: > I'm not sure I follow. Isn't managed part of 'reality'? If it is the case that > this code should never be called if SetArcPlayStoreEnabledForProfile() would not > succeed, say that. > If that is not the case, we should handle it and log a warning or error. IIUC, this code is never called because other code checks if it's managed before calling this function. This DCHECK just helps to verify the assumption.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2707133006/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:276: // other logics prevent us from reaching here in that case. On 2017/03/03 19:04:18, victorhsieh wrote: > On 2017/03/03 18:59:06, stevenjb wrote: > > I'm not sure I follow. Isn't managed part of 'reality'? If it is the case that > > this code should never be called if SetArcPlayStoreEnabledForProfile() would > not > > succeed, say that. > > If that is not the case, we should handle it and log a warning or error. > > IIUC, this code is never called because other code checks if it's managed before > calling this function. This DCHECK just helps to verify the assumption. If that is true, great, it just wasn't entirely clear to me from the comment.
https://codereview.chromium.org/2707133006/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2707133006/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:276: // other logics prevent us from reaching here in that case. On 2017/03/03 20:00:10, stevenjb wrote: > On 2017/03/03 19:04:18, victorhsieh wrote: > > On 2017/03/03 18:59:06, stevenjb wrote: > > > I'm not sure I follow. Isn't managed part of 'reality'? If it is the case > that > > > this code should never be called if SetArcPlayStoreEnabledForProfile() would > > not > > > succeed, say that. > > > If that is not the case, we should handle it and log a warning or error. > > > > IIUC, this code is never called because other code checks if it's managed > before > > calling this function. This DCHECK just helps to verify the assumption. > > If that is true, great, it just wasn't entirely clear to me from the comment. Re-worded. Hopefully it's more clear.
https://codereview.chromium.org/2707133006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler_unittest.cc:36: : public testing::TestWithParam<bool> { Optional: As you skip all test cases, how about post-pone toe add "WithParam" until some of is actually added? Because, in future, if the test scenario is actually much different, you may want to create another test fixture, instead of parametric. WDYT? https://codereview.chromium.org/2707133006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:213: class AbstractArcSessionManagerTest : public ArcSessionManagerTestBase { IIUC, except skipping ToS, ASM should run same for (current) ARC and pARC, right? If so, could you get rid of param test now, instead, set the flag in BaseWorkflowWithArcAlwaysStart only? Also, even in future, it'd be nice if ASM works (almost) same for ARC and pARC. If something big diff is needed, maybe doing it in a (conceptually delegated) another class will be better maintenance, IMHO. So, no need of param test, in future, too? WDYT? (off topic) Just a random idea for future work. Also, if ToS is the only matter, we may want to extract ToS negotiation stuff from ASM, so that ASM works exactly same for both ARC and pARC? I guess this is something we chatted offline last weekend. WDYT?
https://codereview.chromium.org/2707133006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler_unittest.cc:36: : public testing::TestWithParam<bool> { On 2017/03/06 05:37:20, hidehiko wrote: > Optional: As you skip all test cases, how about post-pone toe add "WithParam" > until some of is actually added? Done. Thanks for catching. Didn't realize there is no change yet. https://codereview.chromium.org/2707133006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:213: class AbstractArcSessionManagerTest : public ArcSessionManagerTestBase { On 2017/03/06 05:37:20, hidehiko wrote: > IIUC, except skipping ToS, ASM should run same for (current) ARC and pARC, > right? > If so, could you get rid of param test now, instead, set the flag in > BaseWorkflowWithArcAlwaysStart only? Yes in the future, but no for now. No because I haven't implemented full ASM related logic like opt-in, opt-out (including data removal). I'm fine if you prefer to only test BaseWorkflowWithArcAlwaysStart, and ignore other test cases, until all features are implemented. WDYT? > > Also, even in future, it'd be nice if ASM works (almost) same for ARC and pARC. > If something big diff is needed, maybe doing it in a (conceptually delegated) > another class will be better maintenance, IMHO. So, no need of param test, in > future, too? Given that we will eventually turn the flag on by default and make this the standard ARC, I'm not sure if delegating to another class would be an over kill at this point. But we'll see. > > WDYT? > > (off topic) Just a random idea for future work. Also, if ToS is the only matter, > we may want to extract ToS negotiation stuff from ASM, so that ASM works exactly > same for both ARC and pARC? I guess this is something we chatted offline last > weekend. WDYT? Yes, I'm working on this now. Data removal would be another difference, btw.
The CQ bit was checked by victorhsieh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PS 13 and PS 14 look same. Could you give it another try to upload? https://codereview.chromium.org/2707133006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:213: class AbstractArcSessionManagerTest : public ArcSessionManagerTestBase { On 2017/03/06 15:46:56, victorhsieh wrote: > On 2017/03/06 05:37:20, hidehiko wrote: > > IIUC, except skipping ToS, ASM should run same for (current) ARC and pARC, > > right? > > If so, could you get rid of param test now, instead, set the flag in > > BaseWorkflowWithArcAlwaysStart only? > Yes in the future, but no for now. No because I haven't implemented full ASM > related logic like opt-in, opt-out (including data removal). > > I'm fine if you prefer to only test BaseWorkflowWithArcAlwaysStart, and ignore > other test cases, until all features are implemented. WDYT? > I personally prefer testing BaseWorkflowWithArcAlwaysStart only now. And, adding tests for opt-in/-out cases with its implementation on that ToT, considering you, Yusuke, and I will touch the code at the same time. > > > > Also, even in future, it'd be nice if ASM works (almost) same for ARC and > pARC. > > If something big diff is needed, maybe doing it in a (conceptually delegated) > > another class will be better maintenance, IMHO. So, no need of param test, in > > future, too? > Given that we will eventually turn the flag on by default and make this the > standard ARC, I'm not sure if delegating to another class would be an over kill > at this point. But we'll see. > Yes, it depends on the change. Let's see the coming change. > > > > WDYT? > > > > (off topic) Just a random idea for future work. Also, if ToS is the only > matter, > > we may want to extract ToS negotiation stuff from ASM, so that ASM works > exactly > > same for both ARC and pARC? I guess this is something we chatted offline last > > weekend. WDYT? > Yes, I'm working on this now. Data removal would be another difference, btw. >
https://codereview.chromium.org/2707133006/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:213: class AbstractArcSessionManagerTest : public ArcSessionManagerTestBase { On 2017/03/06 17:10:43, hidehiko wrote: > On 2017/03/06 15:46:56, victorhsieh wrote: > > On 2017/03/06 05:37:20, hidehiko wrote: > > > IIUC, except skipping ToS, ASM should run same for (current) ARC and pARC, > > > right? > > > If so, could you get rid of param test now, instead, set the flag in > > > BaseWorkflowWithArcAlwaysStart only? > > Yes in the future, but no for now. No because I haven't implemented full ASM > > related logic like opt-in, opt-out (including data removal). > > > > I'm fine if you prefer to only test BaseWorkflowWithArcAlwaysStart, and ignore > > other test cases, until all features are implemented. WDYT? > > > > I personally prefer testing BaseWorkflowWithArcAlwaysStart only now. > And, adding tests for opt-in/-out cases with its implementation on that ToT, > considering you, Yusuke, and I will touch the code at the same time. SG. Removed many others, too.
The CQ bit was checked by victorhsieh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Excellent. LGTM! Thank you for clean up. https://codereview.chromium.org/2707133006/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper_unittest.cc:298: INSTANTIATE_TEST_CASE_P(, NoteTakingHelperTest, ::testing::Values(false, true)); nit: maybe, ::testing::Bool() https://codereview.chromium.org/2707133006/diff/300001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_test.cc (right): https://codereview.chromium.org/2707133006/diff/300001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_test.cc:21: #include "chromeos/chromeos_switches.h" nit: unused, now.
The CQ bit was checked by victorhsieh@chromium.org to run a CQ dry run
The CQ bit was unchecked by victorhsieh@chromium.org
https://codereview.chromium.org/2707133006/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_helper_unittest.cc (right): https://codereview.chromium.org/2707133006/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_helper_unittest.cc:298: INSTANTIATE_TEST_CASE_P(, NoteTakingHelperTest, ::testing::Values(false, true)); On 2017/03/07 13:31:35, hidehiko wrote: > nit: maybe, ::testing::Bool() Done. https://codereview.chromium.org/2707133006/diff/300001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_test.cc (right): https://codereview.chromium.org/2707133006/diff/300001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_test.cc:21: #include "chromeos/chromeos_switches.h" On 2017/03/07 13:31:35, hidehiko wrote: > nit: unused, now. Done.
The CQ bit was checked by victorhsieh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org, khmel@chromium.org, lhchavez@chromium.org, stevenjb@chromium.org, hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/2707133006/#ps340001 (title: "rebase to ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1488910257083450,
"parent_rev": "c4adf5ff6dc1cb76bd0fd956de3affeeb14f9060", "commit_rev":
"ec70785d809d34826ed4803537dc64885f7d8d13"}
Message was sent while issue was closed.
Description was changed from
==========
Start ARC and sign in after Chrome OS login
This feature is guarded by --arc-always-start.
Goal of this change include:
- Always start ARC after Chrome OS login
- Start sign-in flow automatically
- Rename Arc to Play in the code when appropriate
Non-goal and next plans:
- Support Play Store opt-in
- Support Play Store opt-out
- Support non-primary-profile use cases
This change means to be the very first step of Persistent ARC.
Once landed, each individual "next plan" can be started.
TEST=unit_tests
TEST=add --arc-always-start to /etc/chrome_dev.conf, login new
account on chrome os. As expected, no opt-in window showed up,
Android was running, gmail account was set up correctly. Able to
launch Settings, Play Movies and Play Store from the launcher
(though some of them should be disabled later).
BUG=b:32746126
==========
to
==========
Start ARC and sign in after Chrome OS login
This feature is guarded by --arc-always-start.
Goal of this change include:
- Always start ARC after Chrome OS login
- Start sign-in flow automatically
- Rename Arc to Play in the code when appropriate
Non-goal and next plans:
- Support Play Store opt-in
- Support Play Store opt-out
- Support non-primary-profile use cases
This change means to be the very first step of Persistent ARC.
Once landed, each individual "next plan" can be started.
TEST=unit_tests
TEST=add --arc-always-start to /etc/chrome_dev.conf, login new
account on chrome os. As expected, no opt-in window showed up,
Android was running, gmail account was set up correctly. Able to
launch Settings, Play Movies and Play Store from the launcher
(though some of them should be disabled later).
BUG=b:32746126
Review-Url: https://codereview.chromium.org/2707133006
Cr-Commit-Position: refs/heads/master@{#455174}
Committed:
https://chromium.googlesource.com/chromium/src/+/ec70785d809d34826ed4803537dc...
==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/ec70785d809d34826ed4803537dc... |
