Extract kArcEnabled preference from ArcSessionManager part 2.

Extract kArcEnabled preference from ArcSessionManager part 2.

This extracts ArcPlayStoreEnabledPreferenceHandler from
ArcSessionManager.
The new class observes preference change, and notifies ArcSessionManager.
With this extraction, ArcSessionManager is no longer depends on
Google Play Store enabled preference.
The extraction makes ArcSessionManager's focus clearer. More clean up allow us for better testing.
Also, this is towards to ARC on login screen and pARC world.

This CL extracts the dependency to ArcAuthNotification from
ArcSessionManager and moved to ArcPlayStoreEnabledPreferenceHandler, too.
So, ArcAuthNotification::DisableForTesting is also extracted from
ArcSessionManager::DisableUIForTesting. Each test has responsibility
to call them individually if necessary.

BUG=657687
BUG=b/31079732
TEST=Ran trybots. Still boot ARC.

Review-Url: https://codereview.chromium.org/2723073002
Cr-Commit-Position: refs/heads/master@{#454441}
Committed: https://chromium.googlesource.com/chromium/src/+/f6f68f0cde9cde5b35b4f13bb5933d0166e64173

[hidehiko, 2017-03-01 11:49:54]

The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 11:50:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 12:51:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 12:51:24]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-03-01 12:56:04]

Description was changed from

==========
Extract kArcEnabled preference from ArcSessionManager part 2.

This extracts ArcPlayStoreEnabledPreferenceHandler from
ArcSessionManager.
The new class observes preference change, and notifies ArcSessionManager.
With this extraction, ArcSessionManager is no longer depends on
Google Play Store enabled preference.

BUG=657687
BUG=b/31079732
TEST=Ran trybots.
==========

to

==========
Extract kArcEnabled preference from ArcSessionManager part 2.

This extracts ArcPlayStoreEnabledPreferenceHandler from
ArcSessionManager.
The new class observes preference change, and notifies ArcSessionManager.
With this extraction, ArcSessionManager is no longer depends on
Google Play Store enabled preference.

BUG=657687
BUG=b/31079732
TEST=Ran trybots. Still boot ARC.
==========

[hidehiko, 2017-03-01 12:57:03]

hidehiko@chromium.org changed reviewers:
+ lhchavez@chromium.org, yusukes@chromium.org

[hidehiko, 2017-03-01 12:57:03]

This is the CL to split "arc.enabled" (= Google Play Store enabled) preference,
and ArcSessionManager conceptually.

PTAL.

[Yusuke Sato, 2017-03-01 18:09:27]

arc/ lgtm with some comments below:

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/BUI...
File chrome/browser/chromeos/BUILD.gn (right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/BUI...
chrome/browser/chromeos/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All
rights reserved.
[optional] If possible, can you elaborate a little bit more on the motivation of
the refactoring (in the CL description)? The reduced dependency allows us to do
...what?

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc
(right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc:133: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
qq: should we move this to c/a/arc_util.cc? (I'm not really sure as I'm not
familiar with the flag.)

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.h
(right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.h:13:
class Profile;
nit: As you know,
https://google.github.io/styleguide/cppguide.html#Forward_Declarations says "Try
to avoid forward declarations of entities defined in another project." Do you
know what "another project" usually means in Chromium? Depending on the
definition, we probably should start reducing forward declarations in new files.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:201: void StartArc();
While you're at it, can you give a function comment to this? We have both
RequestEnable/Disable() and Start/StopArc(), plus StopAndEnableArc() as a bonus.
I guess it's better to have a full documentation here.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:203: void
OnProvisioningFinished(ProvisioningResult result);
same

[hidehiko, 2017-03-01 19:20:34]

Description was changed from

==========
Extract kArcEnabled preference from ArcSessionManager part 2.

This extracts ArcPlayStoreEnabledPreferenceHandler from
ArcSessionManager.
The new class observes preference change, and notifies ArcSessionManager.
With this extraction, ArcSessionManager is no longer depends on
Google Play Store enabled preference.

BUG=657687
BUG=b/31079732
TEST=Ran trybots. Still boot ARC.
==========

to

==========
Extract kArcEnabled preference from ArcSessionManager part 2.

This extracts ArcPlayStoreEnabledPreferenceHandler from
ArcSessionManager.
The new class observes preference change, and notifies ArcSessionManager.
With this extraction, ArcSessionManager is no longer depends on
Google Play Store enabled preference.
The extraction makes ArcSessionManager's focus clearer.
Also, this is towards to ARC on login screen and pARC world.

BUG=657687
BUG=b/31079732
TEST=Ran trybots. Still boot ARC.
==========

[hidehiko, 2017-03-01 19:21:20]

Description was changed from

==========
Extract kArcEnabled preference from ArcSessionManager part 2.

This extracts ArcPlayStoreEnabledPreferenceHandler from
ArcSessionManager.
The new class observes preference change, and notifies ArcSessionManager.
With this extraction, ArcSessionManager is no longer depends on
Google Play Store enabled preference.
The extraction makes ArcSessionManager's focus clearer.
Also, this is towards to ARC on login screen and pARC world.

BUG=657687
BUG=b/31079732
TEST=Ran trybots. Still boot ARC.
==========

to

==========
Extract kArcEnabled preference from ArcSessionManager part 2.

This extracts ArcPlayStoreEnabledPreferenceHandler from
ArcSessionManager.
The new class observes preference change, and notifies ArcSessionManager.
With this extraction, ArcSessionManager is no longer depends on
Google Play Store enabled preference.
The extraction makes ArcSessionManager's focus clearer. More clean up allow us
for better testing.
Also, this is towards to ARC on login screen and pARC world.

BUG=657687
BUG=b/31079732
TEST=Ran trybots. Still boot ARC.
==========

[hidehiko, 2017-03-01 19:21:30]

hidehiko@chromium.org changed reviewers:
+ maxbogue@chromium.org, msw@chromium.org, skuhne@chromium.org,
xiyuan@chromium.org

[hidehiko, 2017-03-01 19:21:32]

Thank you for quick review, Yusuke.

R+=more OWNERS.

PTAL.
maxbogue@: c/b/sync
xiyuan@: c/b/ui/app_list
skuhne@: c/b/ui/ash/launcher
msw@: c/b/ui/views

Thank you for review in advance,
- hidehiko

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/BUI...
File chrome/browser/chromeos/BUILD.gn (right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/BUI...
chrome/browser/chromeos/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All
rights reserved.
On 2017/03/01 18:09:26, Yusuke Sato wrote:
> [optional] If possible, can you elaborate a little bit more on the motivation
of
> the refactoring (in the CL description)? The reduced dependency allows us to
do
> ...what?

Done.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc
(right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc:133: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2017/03/01 18:09:26, Yusuke Sato wrote:
> qq: should we move this to c/a/arc_util.cc? (I'm not really sure as I'm not
> familiar with the flag.)

I think it's nice to have. Let's do it in another CL, because
kEnableArcOOBEOptIn is used in several places.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.h
(right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.h:13:
class Profile;
On 2017/03/01 18:09:26, Yusuke Sato wrote:
> nit: As you know,
> https://google.github.io/styleguide/cppguide.html#Forward_Declarations says
"Try
> to avoid forward declarations of entities defined in another project." Do you
> know what "another project" usually means in Chromium? Depending on the
> definition, we probably should start reducing forward declarations in new
files.

AFAIK, in Chromium, the rule is overriden by
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:201: void StartArc();
On 2017/03/01 18:09:27, Yusuke Sato wrote:
> While you're at it, can you give a function comment to this? We have both
> RequestEnable/Disable() and Start/StopArc(), plus StopAndEnableArc() as a
bonus.
> I guess it's better to have a full documentation here.

Done in another CL.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:203: void
OnProvisioningFinished(ProvisioningResult result);
On 2017/03/01 18:09:27, Yusuke Sato wrote:
> same

Ditto.

[maxbogue, 2017-03-01 19:25:52]

c/b/sync lgtm with a question

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:155:
ArcSessionManager* arc_session_manager = ArcSessionManager::Get();
Is this line doing anything anymore?

[Yusuke Sato, 2017-03-01 19:50:19]

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc
(right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc:133: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2017/03/01 19:21:31, hidehiko wrote:
> On 2017/03/01 18:09:26, Yusuke Sato wrote:
> > qq: should we move this to c/a/arc_util.cc? (I'm not really sure as I'm not
> > familiar with the flag.)
> 
> I think it's nice to have. Let's do it in another CL, because
> kEnableArcOOBEOptIn is used in several places.

sgtm, please add a TODO unless you already have a CL.

[xiyuan, 2017-03-01 20:50:57]

c/b/ui/app_list lgtm

[Luis Héctor Chávez, 2017-03-02 00:09:17]

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc
(right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc:86:
const bool is_play_store_enabled = IsArcPlayStoreEnabledForProfile(profile_);
Is it possible to merge the implementations of Start() and this into a single
method? Probably take in a parameter to indicate whether it's the first start or
not to skip the skipping (ha!) of removing ARC data. That'll be less error-prone
in the future.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.h
(right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.h:13:
class Profile;
On 2017/03/01 19:21:31, hidehiko wrote:
> On 2017/03/01 18:09:26, Yusuke Sato wrote:
> > nit: As you know,
> > https://google.github.io/styleguide/cppguide.html#Forward_Declarations says
> "Try
> > to avoid forward declarations of entities defined in another project." Do
you
> > know what "another project" usually means in Chromium? Depending on the
> > definition, we probably should start reducing forward declarations in new
> files.
> 
> AFAIK, in Chromium, the rule is overriden by
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Yes, Chromium is considered to be a single project :)

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler_unittest.cc
(right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler_unittest.cc:78:
ArcPlayStoreEnabledPreferenceHandler* preference_handler() {
I know it's not your fault, but maybe make the constness of all the getters
consistent now that you're touching this file?

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:138: // services to subscribe
the handler instance directly. Instead, they can to
nit: Instead, they can subscribe to

[msw, 2017-03-02 00:25:43]

c/b/ui/views lgtm with a nit; I also have a related q.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:135:
ArcSessionManager::DisableUIForTesting();
q: Should this now call ArcAuthNotification::DisableForTesting() separately?

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/ui/views/arc...
File chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc (right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/ui/views/arc...
chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:43:
ArcAuthNotification::DisableForTesting();
nit: mention splitting out the auth disabling in your CL desc?

[hidehiko, 2017-03-02 02:20:26]

Description was changed from

==========
Extract kArcEnabled preference from ArcSessionManager part 2.

This extracts ArcPlayStoreEnabledPreferenceHandler from
ArcSessionManager.
The new class observes preference change, and notifies ArcSessionManager.
With this extraction, ArcSessionManager is no longer depends on
Google Play Store enabled preference.
The extraction makes ArcSessionManager's focus clearer. More clean up allow us
for better testing.
Also, this is towards to ARC on login screen and pARC world.

BUG=657687
BUG=b/31079732
TEST=Ran trybots. Still boot ARC.
==========

to

==========
Extract kArcEnabled preference from ArcSessionManager part 2.

This extracts ArcPlayStoreEnabledPreferenceHandler from
ArcSessionManager.
The new class observes preference change, and notifies ArcSessionManager.
With this extraction, ArcSessionManager is no longer depends on
Google Play Store enabled preference.
The extraction makes ArcSessionManager's focus clearer. More clean up allow us
for better testing.
Also, this is towards to ARC on login screen and pARC world.

This CL extracts the dependency to ArcAuthNotification from
ArcSessionManager and moved to ArcPlayStoreEnabledPreferenceHandler, too.
So, ArcAuthNotification::DisableForTesting is also extracted from
ArcSessionManager::DisableUIForTesting. Each test has responsibility
to call them individually if necessary.

BUG=657687
BUG=b/31079732
TEST=Ran trybots. Still boot ARC.
==========

[hidehiko, 2017-03-02 02:20:41]

The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 02:21:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hidehiko, 2017-03-02 02:21:57]

Thank you for review. PTAL, Luis. Friendly ping, Stefan.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc
(right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc:86:
const bool is_play_store_enabled = IsArcPlayStoreEnabledForProfile(profile_);
On 2017/03/02 00:09:17, Luis Héctor Chávez wrote:
> Is it possible to merge the implementations of Start() and this into a single
> method? Probably take in a parameter to indicate whether it's the first start
or
> not to skip the skipping (ha!) of removing ARC data. That'll be less
error-prone
> in the future.

Could you elaborate your intention a bit more?
IIUC, what we can share between them is just ArcSupportHost::SetArcManaged  and
ArcSessionManager::RequestEnable/Disable only, which looks small to me?

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc:133: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2017/03/01 19:50:19, Yusuke Sato wrote:
> On 2017/03/01 19:21:31, hidehiko wrote:
> > On 2017/03/01 18:09:26, Yusuke Sato wrote:
> > > qq: should we move this to c/a/arc_util.cc? (I'm not really sure as I'm
not
> > > familiar with the flag.)
> > 
> > I think it's nice to have. Let's do it in another CL, because
> > kEnableArcOOBEOptIn is used in several places.
> 
> sgtm, please add a TODO unless you already have a CL.

Done.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler_unittest.cc
(right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler_unittest.cc:78:
ArcPlayStoreEnabledPreferenceHandler* preference_handler() {
On 2017/03/02 00:09:17, Luis Héctor Chávez wrote:
> I know it's not your fault, but maybe make the constness of all the getters
> consistent now that you're touching this file?

Removed "const" from GetFakeUserManager(). Others should be mutable, and this is
test, so I do not think we need const T* foo() const { ... } methods.
Is it what you thought?

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:138: // services to subscribe
the handler instance directly. Instead, they can to
On 2017/03/02 00:09:17, Luis Héctor Chávez wrote:
> nit: Instead, they can subscribe to

Done.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:135:
ArcSessionManager::DisableUIForTesting();
On 2017/03/02 00:25:43, msw wrote:
> q: Should this now call ArcAuthNotification::DisableForTesting() separately?

It is intentionally dropped, because this test no longer depends on
ArcAuthNotification.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:155:
ArcSessionManager* arc_session_manager = ArcSessionManager::Get();
On 2017/03/01 19:25:51, maxbogue wrote:
> Is this line doing anything anymore?

Good catch. Removed.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/ui/views/arc...
File chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc (right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/ui/views/arc...
chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:43:
ArcAuthNotification::DisableForTesting();
On 2017/03/02 00:25:43, msw wrote:
> nit: mention splitting out the auth disabling in your CL desc?

Done.

[commit-bot: I haz the power, 2017-03-02 02:27:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 02:27:05]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  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_...)

[hidehiko, 2017-03-02 02:41:39]

FYI: will rebase when https://codereview.chromium.org/2722263002/ is landed (now
in CQ).

[Luis Héctor Chávez, 2017-03-02 03:21:09]

downgrading the nit to an optional request, so lgtm.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc
(right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc:86:
const bool is_play_store_enabled = IsArcPlayStoreEnabledForProfile(profile_);
On 2017/03/02 02:21:57, hidehiko wrote:
> On 2017/03/02 00:09:17, Luis Héctor Chávez wrote:
> > Is it possible to merge the implementations of Start() and this into a
single
> > method? Probably take in a parameter to indicate whether it's the first
start
> or
> > not to skip the skipping (ha!) of removing ARC data. That'll be less
> error-prone
> > in the future.
> 
> Could you elaborate your intention a bit more?
> IIUC, what we can share between them is just ArcSupportHost::SetArcManaged 
and
> ArcSessionManager::RequestEnable/Disable only, which looks small to me?

That's correct, but that's also the bulk of
ArcPlayStoreEnabledPreferenceHandler::Start() (sans actually observing the
pref). The main goal would be to minimize the number of places where
RequestEnable()/RequestDisable() are called from. And after thinking it more,
it'd be better to not pass the parameter whether it's the first boot or not:
just keep the started-at-profile-init/started-later case-specific code in their
own functions.

Having said that, I don't feel _too_ strongly about this because as you
mentioned, the LOC savings are low.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler_unittest.cc
(right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler_unittest.cc:78:
ArcPlayStoreEnabledPreferenceHandler* preference_handler() {
On 2017/03/02 02:21:57, hidehiko wrote:
> On 2017/03/02 00:09:17, Luis Héctor Chávez wrote:
> > I know it's not your fault, but maybe make the constness of all the getters
> > consistent now that you're touching this file?
> 
> Removed "const" from GetFakeUserManager(). Others should be mutable, and this
is
> test, so I do not think we need const T* foo() const { ... } methods.
> Is it what you thought?

Yes, that works.

[hidehiko, 2017-03-02 06:29:45]

The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 06:30:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 08:38:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 08:39:03]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-03-02 16:37:08]

Thank you for review.

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc
(right):

https://codereview.chromium.org/2723073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc:86:
const bool is_play_store_enabled = IsArcPlayStoreEnabledForProfile(profile_);
On 2017/03/02 03:21:09, Luis Héctor Chávez wrote:
> On 2017/03/02 02:21:57, hidehiko wrote:
> > On 2017/03/02 00:09:17, Luis Héctor Chávez wrote:
> > > Is it possible to merge the implementations of Start() and this into a
> single
> > > method? Probably take in a parameter to indicate whether it's the first
> start
> > or
> > > not to skip the skipping (ha!) of removing ARC data. That'll be less
> > error-prone
> > > in the future.
> > 
> > Could you elaborate your intention a bit more?
> > IIUC, what we can share between them is just ArcSupportHost::SetArcManaged 
> and
> > ArcSessionManager::RequestEnable/Disable only, which looks small to me?
> 
> That's correct, but that's also the bulk of
> ArcPlayStoreEnabledPreferenceHandler::Start() (sans actually observing the
> pref). The main goal would be to minimize the number of places where
> RequestEnable()/RequestDisable() are called from. And after thinking it more,
> it'd be better to not pass the parameter whether it's the first boot or not:
> just keep the started-at-profile-init/started-later case-specific code in
their
> own functions.
> 
> Having said that, I don't feel _too_ strongly about this because as you
> mentioned, the LOC savings are low.

Thank you for explanation. I believe I understand your intention. So done.

[Luis Héctor Chávez, 2017-03-02 16:39:26]

lgtm++.

[Mr4D (OOO till 08-26), 2017-03-02 19:45:59]

c/b/ui/ash/launcher lgtm

[hidehiko, 2017-03-02 23:48:54]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2017-03-02 23:48:55]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
msw@chromium.org, maxbogue@chromium.org, yusukes@chromium.org
Link to the patchset: https://codereview.chromium.org/2723073002/#ps60001
(title: "address comments")

[commit-bot: I haz the power, 2017-03-02 23:49:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 23:55:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488498534613960,
"parent_rev": "0e430dc72813ba8fb8086228ab1032863381f582", "commit_rev":
"f6f68f0cde9cde5b35b4f13bb5933d0166e64173"}

[commit-bot: I haz the power, 2017-03-02 23:56:08]

Description was changed from

==========
Extract kArcEnabled preference from ArcSessionManager part 2.

This extracts ArcPlayStoreEnabledPreferenceHandler from
ArcSessionManager.
The new class observes preference change, and notifies ArcSessionManager.
With this extraction, ArcSessionManager is no longer depends on
Google Play Store enabled preference.
The extraction makes ArcSessionManager's focus clearer. More clean up allow us
for better testing.
Also, this is towards to ARC on login screen and pARC world.

This CL extracts the dependency to ArcAuthNotification from
ArcSessionManager and moved to ArcPlayStoreEnabledPreferenceHandler, too.
So, ArcAuthNotification::DisableForTesting is also extracted from
ArcSessionManager::DisableUIForTesting. Each test has responsibility
to call them individually if necessary.

BUG=657687
BUG=b/31079732
TEST=Ran trybots. Still boot ARC.
==========

to

==========
Extract kArcEnabled preference from ArcSessionManager part 2.

This extracts ArcPlayStoreEnabledPreferenceHandler from
ArcSessionManager.
The new class observes preference change, and notifies ArcSessionManager.
With this extraction, ArcSessionManager is no longer depends on
Google Play Store enabled preference.
The extraction makes ArcSessionManager's focus clearer. More clean up allow us
for better testing.
Also, this is towards to ARC on login screen and pARC world.

This CL extracts the dependency to ArcAuthNotification from
ArcSessionManager and moved to ArcPlayStoreEnabledPreferenceHandler, too.
So, ArcAuthNotification::DisableForTesting is also extracted from
ArcSessionManager::DisableUIForTesting. Each test has responsibility
to call them individually if necessary.

BUG=657687
BUG=b/31079732
TEST=Ran trybots. Still boot ARC.

Review-Url: https://codereview.chromium.org/2723073002
Cr-Commit-Position: refs/heads/master@{#454441}
Committed:
https://chromium.googlesource.com/chromium/src/+/f6f68f0cde9cde5b35b4f13bb593...
==========

[commit-bot: I haz the power, 2017-03-02 23:56:09]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f6f68f0cde9cde5b35b4f13bb593...