Extract kArcEnabled preference from ArcSessionManager part 1.

Extract kArcEnabled preference from ArcSessionManager part 1.

This CL extracts following methods:
- IsArcPlayStoreEnabled() -> IsArcPlayStoreEnabledForProfile()
- IsArcManaged() -> IsArcPlayStoreEnabledPreferenceManagedForProfile()
- SetArcPlayStoreEnabled() -> SetArcPlayStoreEnabledForProfile()

New functions takes Profile as its arguments explicitly, instead of
acting to the primary user Profile implicitly.

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

Review-Url: https://codereview.chromium.org/2702723002
Cr-Commit-Position: refs/heads/master@{#452400}
Committed: https://chromium.googlesource.com/chromium/src/+/0846083cfdbea5d902953540a4220599eb0fe9e1

[hidehiko, 2017-02-17 13:46:02]

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

[commit-bot: I haz the power, 2017-02-17 13:46:17]

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-02-17 14:28:25]

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

[commit-bot: I haz the power, 2017-02-17 14:28:26]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[hidehiko, 2017-02-17 14:56:24]

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

[hidehiko, 2017-02-17 14:56:25]

PTAL.

yusukes@: all as a main reviewer, specifically c/b/c/arc.
xiyuan@: c/b/c, c/b/ui/app_list OWNER.
benwells@: c/b/extensions, c/b/ui/views/apps OWNER.
maxbogue@: c/b/sync OWNER.
skuhne@: c/b/ui/ash/launcher OWNER
msw@: c/b/ui/views/ OWNER
stevenjb@: c/b/ui/webui/{options,settings} OWNER.

FYI: lhchavez@, khmel@, nya@ (for arc_file_system_operation_runner.cc change).

Note: android bot failure should be unrelated, because this is only for
ChromeOS.

Thank you for review in advance,
- hidehiko

[hidehiko, 2017-02-17 15:07:19]

Added note/comments for some changes.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc
(right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:167:
SetShouldDefer(
FYI (off-topic): yusukes@.
On ARC login screen work, this may be considered.
This module conceptually expects that ARC is tied to primary Profile, but on
login screen, primary profile is not prepared.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:273: DCHECK_EQ(profile_,
arc_session_manager->profile());
Note: If ArcSessionManager has profile, it should be as same as one this
instance has, because ArcAppListPrefsFactory creates an instance only if the
given Profile meets ARC's requirement. Anyway, just in case, check it here.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc
(right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc:417: if
(!arc::IsArcPlayStoreEnabledForProfile(profile) ||
Note: IsArcAllowedForProfile is no longer needed here, because
IsArcPlayStoreEnabledForProfile() contains it.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/chromeos/device_storage_handler.cc
(right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/chromeos/device_storage_handler.cc:349: if
(!arc::IsArcPlayStoreEnabledForProfile(profile) ||
Note: IsArcAllowedForProfile() is no longer needed here, because
IsArcPlayStoreEnabledForProfile() contains the check.

https://codereview.chromium.org/2702723002/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2702723002/diff/1/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:32: // SetArcPlayStoreEnabledForProfile()).
FYI: I haven't addressed this in this CL, so there still are several places
where kArcEnabled is directly used (specifically on testing).

A following CL will extract preference change handling from ArcSessionManager,
then several such code can/should be changed to
ArcSessionManager::Request{Enable/Disble} instead of current direct kArcEnabled
operation. I'll take a look at them, along with (or after) that change.

[skym, 2017-02-17 15:39:18]

skym@chromium.org changed reviewers:
+ skym@chromium.org
- maxbogue@chromium.org

[skym, 2017-02-17 15:39:18]

c/b/sync lgtm

[stevenjb, 2017-02-17 17:26:50]

c/b/ui/webui lgtm

[xiyuan, 2017-02-17 17:45:40]

lgtm

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

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_notification.cc:87: Profile* profile_;
nit: Profile* const profile_;

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

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_util.cc:114: VLOG(1) << "Whether Google Play
Store is enabled is managed. Do nothing.";
nit: Whether Google Play Store is enabled is managed. Do nothing.
=> Do nothing if Google-Play-Store-enabled pref is managed.

[msw, 2017-02-17 18:39:39]

c/b/ui/views lgtm

[Mr4D (OOO till 08-26), 2017-02-17 21:52:17]

c/b/ui/launcher lgtm

[Yusuke Sato, 2017-02-17 22:27:09]

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

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:421:
support_host_->SetArcManaged(IsArcPlayStoreEnabledForProfile(profile_));
This contradicts with the description of the CL you wrote. Did you mean
IsArcPlayStoreEnabledPreferenceManagedForProfile()?

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

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_util_unittest.cc:281:
SetArcPlayStoreEnabledForProfile(profile(), false);
nit: can you also test that SetArcPlayStoreEnabledForProfile(profile(), true)
does not change the pref from false to true when managed? That one would be more
important.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc
(right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:167:
SetShouldDefer(
On 2017/02/17 15:07:19, hidehiko wrote:
> FYI (off-topic): yusukes@.
> On ARC login screen work, this may be considered.
> This module conceptually expects that ARC is tied to primary Profile, but on
> login screen, primary profile is not prepared.

Acknowledged.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/not...
File chrome/browser/chromeos/note_taking_helper.cc (right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/not...
chrome/browser/chromeos/note_taking_helper.cc:230: //
IsArcPlayStoreEnabledForProfile() can return true only for the primary
Hmm.. but the primary profile thing is not mentioned in arc_util.h. Can you
mention it in the header? (or, check if the profile is primary here maybe?)

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:145: bool
IsArcEnabled(const Profile* profile) {
Shouldn't we rename this function?

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:148: // It is necessary to
check it here, in addition to whether Google Play Store
optional: Probably it's better to directly explain when IsAllowed() returns
false too.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:273: DCHECK_EQ(profile_,
arc_session_manager->profile());
On 2017/02/17 15:07:19, hidehiko wrote:
> Note: If ArcSessionManager has profile, it should be as same as one this
> instance has, because ArcAppListPrefsFactory creates an instance only if the
> given Profile meets ARC's requirement. Anyway, just in case, check it here.

Copy the comment to the code?

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/ash/launc...
File
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:5:
#include
"chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h"
nit: space between L5 and L6?

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc
(right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc:418:
arc::IsArcOptInVerificationDisabled()) {
Mostly OT: do you know why this check is necessary? code comment?

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/chromeos/device_storage_handler.cc
(right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/chromeos/device_storage_handler.cc:350:
arc::IsArcOptInVerificationDisabled()) {
same

https://codereview.chromium.org/2702723002/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2702723002/diff/1/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:31: // utility methods
(IsArcPlayStoreEnabledForProfile() and
Please also mention that the functions are in c/b/c/arc_util.h.

[hidehiko, 2017-02-20 16:37:50]

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

[commit-bot: I haz the power, 2017-02-20 16:38:03]

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-02-20 17:55:05]

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

[commit-bot: I haz the power, 2017-02-20 17:55:06]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-02-20 18:18:45]

Thank you for review. PTAL.

Note: bot failure in PS1 is;
- ShelfAppBrowserTest.MatchingShelfIDandActiveTab:
  - Flakiness issue. (crbug.com/694101)
- PermissionReporterBrowserTest.PermissionActionReporting
- PermissionReporterBrowserTest.PermissionActionReportingPriorIgnoreCount
  - Real failure, but addressed separately. crrev.com/	
2708693002

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

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_notification.cc:87: Profile* profile_;
On 2017/02/17 17:45:40, xiyuan wrote:
> nit: Profile* const profile_;

Done.

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

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:421:
support_host_->SetArcManaged(IsArcPlayStoreEnabledForProfile(profile_));
On 2017/02/17 22:27:08, Yusuke Sato wrote:
> This contradicts with the description of the CL you wrote. Did you mean
> IsArcPlayStoreEnabledPreferenceManagedForProfile()?

Good catch! Fixed.

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

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_util.cc:114: VLOG(1) << "Whether Google Play
Store is enabled is managed. Do nothing.";
On 2017/02/17 17:45:40, xiyuan wrote:
> nit: Whether Google Play Store is enabled is managed. Do nothing.
> => Do nothing if Google-Play-Store-enabled pref is managed.

Done.

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

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_util_unittest.cc:281:
SetArcPlayStoreEnabledForProfile(profile(), false);
On 2017/02/17 22:27:09, Yusuke Sato wrote:
> nit: can you also test that SetArcPlayStoreEnabledForProfile(profile(), true)
> does not change the pref from false to true when managed? That one would be
more
> important.

Done.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/not...
File chrome/browser/chromeos/note_taking_helper.cc (right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/not...
chrome/browser/chromeos/note_taking_helper.cc:230: //
IsArcPlayStoreEnabledForProfile() can return true only for the primary
On 2017/02/17 22:27:09, Yusuke Sato wrote:
> Hmm.. but the primary profile thing is not mentioned in arc_util.h. Can you
> mention it in the header? (or, check if the profile is primary here maybe?)

Hmm, I thought the combination of "if ARC is not allowed" in the function
comment, and "if the Profile is not for the primary user" for the
IsArcAllowedForProfile() described it... Anyway, added explicit deps from
IsArcPlayStoreEnabledForProfile() to IsArcAllowedForProfile(). Does it work for
you?

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:145: bool
IsArcEnabled(const Profile* profile) {
On 2017/02/17 22:27:09, Yusuke Sato wrote:
> Shouldn't we rename this function?

Changed. Also, moved IsArcPlayStoreEnabledForProfile() to caller, in order to
let this function focus on checking ARC aliveness.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:148: // It is necessary to
check it here, in addition to whether Google Play Store
On 2017/02/17 22:27:09, Yusuke Sato wrote:
> optional: Probably it's better to directly explain when IsAllowed() returns
> false too.

Done.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:273: DCHECK_EQ(profile_,
arc_session_manager->profile());
On 2017/02/17 22:27:09, Yusuke Sato wrote:
> On 2017/02/17 15:07:19, hidehiko wrote:
> > Note: If ArcSessionManager has profile, it should be as same as one this
> > instance has, because ArcAppListPrefsFactory creates an instance only if the
> > given Profile meets ARC's requirement. Anyway, just in case, check it here.
> 
> Copy the comment to the code?

Done, but in another CL.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/ash/launc...
File
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:5:
#include
"chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h"
On 2017/02/17 22:27:09, Yusuke Sato wrote:
> nit: space between L5 and L6?

Done.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc
(right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc:418:
arc::IsArcOptInVerificationDisabled()) {
On 2017/02/17 22:27:09, Yusuke Sato wrote:
> Mostly OT: do you know why this check is necessary? code comment?

I'm guessing we'd like to show the section only when ARC is running.
Let me confirm with the original author. At the moment, as this is off-topic,
let me work on it in a following CL soon.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/chromeos/device_storage_handler.cc
(right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/chromeos/device_storage_handler.cc:350:
arc::IsArcOptInVerificationDisabled()) {
On 2017/02/17 22:27:09, Yusuke Sato wrote:
> same

Acknowledged.

https://codereview.chromium.org/2702723002/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2702723002/diff/1/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:31: // utility methods
(IsArcPlayStoreEnabledForProfile() and
On 2017/02/17 22:27:09, Yusuke Sato wrote:
> Please also mention that the functions are in c/b/c/arc_util.h.

Done, but I'm not very sure if it is a good practice, considering we may want to
move them in future, TBH, so maybe cs is a tool for it? Anyway, done.

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_test.cc (left):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_test.cc:93: #include
"components/arc/arc_util.h"
Note: this should be a part of crrev.com/2708693002. If something needs to be
changed around it, could you comment in that CL?

[benwells, 2017-02-21 00:24:27]

c/b/extensions and c/b/ui/views/apps lgtm

[Luis Héctor Chávez, 2017-02-21 16:50:00]

lhchavez@chromium.org changed reviewers:
+ lhchavez@chromium.org

[Luis Héctor Chávez, 2017-02-21 16:50:02]

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util.cc (right):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.cc:107:
DCHECK(IsArcAllowedForProfile(profile));
Similar to the comments I left below: this is a probably aggressive behavioral
change because not all places that call this actually ensure one way or the
other that this is true since they rely on the old behavior of being a simple
if-not-allowed-return-false.

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.cc:114: VLOG(1) << "Do nothing, if the
Google-Play-Store-enabled pref is managed.";
nit: "Doing nothing since the Google-Play-Store-enabled pref is managed."

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:400: if (!(IsArcAlive() &&
arc::IsArcPlayStoreEnabledForProfile(profile_)))
I prefer to use de Morgan's law to simplify these kinds of checks.

Then again, you are using it in several places (and in L465 where de Morgan's
does not help :/), so maybe extract it into a function?

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:45:
playstore_launcher_.reset(
nit: = base::MakeUnique<..>(..);

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc
(right):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc:417: if
(!arc::IsArcPlayStoreEnabledForProfile(profile) ||
Can this trigger a DCHECK? |arc::IsArcAllowedForProfile()| was explicitly
checked for before.

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/device_storage_handler.cc
(right):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_storage_handler.cc:349: if
(!arc::IsArcPlayStoreEnabledForProfile(profile) ||
Can this trigger a DCHECK? |arc::IsArcAllowedForProfile()| was explicitly
checked for before.

[hidehiko, 2017-02-21 17:25:15]

Thank you for review. PTAL.

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util.cc (right):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.cc:107:
DCHECK(IsArcAllowedForProfile(profile));
On 2017/02/21 16:50:01, Luis Héctor Chávez wrote:
> Similar to the comments I left below: this is a probably aggressive behavioral
> change because not all places that call this actually ensure one way or the
> other that this is true since they rely on the old behavior of being a simple
> if-not-allowed-return-false.

This is not behavior change actually.
In the old code ArcSessionManager::IsArcManaged() has DCHECK(profile_), which is
corresponding to this DCHECK, because if IsArcAllowedForProfile(profile) is
false, ASM::profile_ won't be set.

Technicaly, we can change the behavior of the case where
IsArcAllowedForProfile() returns false, but at the moment, I think it's ok to
keep the DCHECK because it is more strict than returning false.

WDYT?

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.cc:114: VLOG(1) << "Do nothing, if the
Google-Play-Store-enabled pref is managed.";
On 2017/02/21 16:50:02, Luis Héctor Chávez wrote:
> nit: "Doing nothing since the Google-Play-Store-enabled pref is managed."

Done.

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:400: if (!(IsArcAlive() &&
arc::IsArcPlayStoreEnabledForProfile(profile_)))
On 2017/02/21 16:50:02, Luis Héctor Chávez wrote:
> I prefer to use de Morgan's law to simplify these kinds of checks.
> 
> Then again, you are using it in several places (and in L465 where de Morgan's
> does not help :/), so maybe extract it into a function?

Done for expansion for de Morgan's law.

As for wrapping function; indeed I actually thought about in the prev PS.
However, I couldn't come up with a good abstraction to wrap. Maybe very
straightforward name would be

IsArcAliveAndArcPlayStoreEnabledForProfile(profile).

Though, IMHO, the meaning looks complicated and actually the name looks too
long.
On the other hand, I do not want to re-define IsArcPlayStoreEnabledForProfile()
for different meaning (ASM check + Google Play Store pref check), because it
sounds just confusing.

Considering those reasons, I just kept them two as is.
If you have better abstraction and naming, please let me know. I'm happy to use
it :-).

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/ash/l...
File
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:45:
playstore_launcher_.reset(
On 2017/02/21 16:50:02, Luis Héctor Chávez wrote:
> nit: = base::MakeUnique<..>(..);

Done.

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc
(right):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc:417: if
(!arc::IsArcPlayStoreEnabledForProfile(profile) ||
On 2017/02/21 16:50:02, Luis Héctor Chávez wrote:
> Can this trigger a DCHECK? |arc::IsArcAllowedForProfile()| was explicitly
> checked for before.

No. IsArcAllowedForProfile() is called in IsArcPlayStoreEnabledForProfile(), and
if not allowed, IsArcPlayStoreEnabledForProfile() returns false (unlike
IsArcPlayStoreEnabledPreferenceManagedForProfile()).

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/device_storage_handler.cc
(right):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_storage_handler.cc:349: if
(!arc::IsArcPlayStoreEnabledForProfile(profile) ||
On 2017/02/21 16:50:02, Luis Héctor Chávez wrote:
> Can this trigger a DCHECK? |arc::IsArcAllowedForProfile()| was explicitly
> checked for before.

Acknowledged.

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

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util.cc (right):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.cc:107:
DCHECK(IsArcAllowedForProfile(profile));
On 2017/02/21 17:25:14, hidehiko wrote:
> On 2017/02/21 16:50:01, Luis Héctor Chávez wrote:
> > Similar to the comments I left below: this is a probably aggressive
behavioral
> > change because not all places that call this actually ensure one way or the
> > other that this is true since they rely on the old behavior of being a
simple
> > if-not-allowed-return-false.
> 
> This is not behavior change actually.
> In the old code ArcSessionManager::IsArcManaged() has DCHECK(profile_), which
is
> corresponding to this DCHECK, because if IsArcAllowedForProfile(profile) is
> false, ASM::profile_ won't be set.
> 
> Technicaly, we can change the behavior of the case where
> IsArcAllowedForProfile() returns false, but at the moment, I think it's ok to
> keep the DCHECK because it is more strict than returning false.
> 
> WDYT?

Makes sense, but it's still something that can be easy to miss for someone
that's not super careful.

How about we do

if (!IsArcAllowedForProfile(profile)) {
  DCHECK(false) << "ARC is not allowed for profile";
  return false;
}

that way we have the best of both worlds: we don't accidentally regress in
production builds and we get a nicer log.

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:400: if (!(IsArcAlive() &&
arc::IsArcPlayStoreEnabledForProfile(profile_)))
On 2017/02/21 17:25:14, hidehiko wrote:
> On 2017/02/21 16:50:02, Luis Héctor Chávez wrote:
> > I prefer to use de Morgan's law to simplify these kinds of checks.
> > 
> > Then again, you are using it in several places (and in L465 where de
Morgan's
> > does not help :/), so maybe extract it into a function?
> 
> Done for expansion for de Morgan's law.
> 
> As for wrapping function; indeed I actually thought about in the prev PS.
> However, I couldn't come up with a good abstraction to wrap. Maybe very
> straightforward name would be
> 
> IsArcAliveAndArcPlayStoreEnabledForProfile(profile).
> 
> Though, IMHO, the meaning looks complicated and actually the name looks too
> long.
> On the other hand, I do not want to re-define
IsArcPlayStoreEnabledForProfile()
> for different meaning (ASM check + Google Play Store pref check), because it
> sounds just confusing.
> 
> Considering those reasons, I just kept them two as is.
> If you have better abstraction and naming, please let me know. I'm happy to
use
> it :-).

It looks better in PS4, so I'm fine with the way you left it.

[Yusuke Sato, 2017-02-21 20:59:43]

lgtm

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/not...
File chrome/browser/chromeos/note_taking_helper.cc (right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/chromeos/not...
chrome/browser/chromeos/note_taking_helper.cc:230: //
IsArcPlayStoreEnabledForProfile() can return true only for the primary
On 2017/02/20 18:18:44, hidehiko wrote:
> On 2017/02/17 22:27:09, Yusuke Sato wrote:
> > Hmm.. but the primary profile thing is not mentioned in arc_util.h. Can you
> > mention it in the header? (or, check if the profile is primary here maybe?)
> 
> Hmm, I thought the combination of "if ARC is not allowed" in the function
> comment, and "if the Profile is not for the primary user" for the
> IsArcAllowedForProfile() described it... Anyway, added explicit deps from
> IsArcPlayStoreEnabledForProfile() to IsArcAllowedForProfile(). Does it work
for
> you?

Yes, thanks.

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2702723002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:273: DCHECK_EQ(profile_,
arc_session_manager->profile());
On 2017/02/20 18:18:44, hidehiko wrote:
> On 2017/02/17 22:27:09, Yusuke Sato wrote:
> > On 2017/02/17 15:07:19, hidehiko wrote:
> > > Note: If ArcSessionManager has profile, it should be as same as one this
> > > instance has, because ArcAppListPrefsFactory creates an instance only if
the
> > > given Profile meets ARC's requirement. Anyway, just in case, check it
here.
> > 
> > Copy the comment to the code?
> 
> Done, but in another CL.

Acknowledged.

https://codereview.chromium.org/2702723002/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2702723002/diff/1/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:31: // utility methods
(IsArcPlayStoreEnabledForProfile() and
On 2017/02/20 18:18:45, hidehiko wrote:
> On 2017/02/17 22:27:09, Yusuke Sato wrote:
> > Please also mention that the functions are in c/b/c/arc_util.h.
> 
> Done, but I'm not very sure if it is a good practice, considering we may want
to
> move them in future, TBH, so maybe cs is a tool for it? Anyway, done.

File names are not necessary especially when the method name in a comment is
long enough like ClassName::MethodName(). In this case, I felt like more
clarification might be helpful because the functions mentioned here are not in a
class, and we haven't fully cleaned up confusing function names in other places
just yet. I mean, we've changed method names and places for this type of things
probably too many times already and I wanted to avoid further confusion at some
cost.

https://codereview.chromium.org/2702723002/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2702723002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:149: bool IsArcAlive() {
nit: can you also git-grep the entire code with Arc and Enalbed to make sure we
no longer have confusing function names. (and please address it in a separate CL
if we still have any.)

[Yusuke Sato, 2017-02-22 08:51:00]

https://codereview.chromium.org/2702723002/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2702723002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:149: bool IsArcAlive() {
On 2017/02/21 20:59:43, Yusuke Sato wrote:
> nit: can you also git-grep the entire code with Arc and Enalbed to make sure
we
> no longer have confusing function names. (and please address it in a separate
CL
> if we still have any.)

s/Enalbed/Enabled/, sorry.

[Luis Héctor Chávez, 2017-02-23 02:18:35]

lgtm

[hidehiko, 2017-02-23 04:17:36]

+CC: those who have conflicting CL (afaik).

Thank you for review! Submitting.

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_util.cc (right):

https://codereview.chromium.org/2702723002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.cc:107:
DCHECK(IsArcAllowedForProfile(profile));
On 2017/02/21 18:18:08, Luis Héctor Chávez wrote:
> On 2017/02/21 17:25:14, hidehiko wrote:
> > On 2017/02/21 16:50:01, Luis Héctor Chávez wrote:
> > > Similar to the comments I left below: this is a probably aggressive
> behavioral
> > > change because not all places that call this actually ensure one way or
the
> > > other that this is true since they rely on the old behavior of being a
> simple
> > > if-not-allowed-return-false.
> > 
> > This is not behavior change actually.
> > In the old code ArcSessionManager::IsArcManaged() has DCHECK(profile_),
which
> is
> > corresponding to this DCHECK, because if IsArcAllowedForProfile(profile) is
> > false, ASM::profile_ won't be set.
> > 
> > Technicaly, we can change the behavior of the case where
> > IsArcAllowedForProfile() returns false, but at the moment, I think it's ok
to
> > keep the DCHECK because it is more strict than returning false.
> > 
> > WDYT?
> 
> Makes sense, but it's still something that can be easy to miss for someone
> that's not super careful.
> 
> How about we do
> 
> if (!IsArcAllowedForProfile(profile)) {
>   DCHECK(false) << "ARC is not allowed for profile";
>   return false;
> }
> 
> that way we have the best of both worlds: we don't accidentally regress in
> production builds and we get a nicer log.

Done. BTW, LOG(DFATAL) is the way in this case :-).

https://codereview.chromium.org/2702723002/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2702723002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:149: bool IsArcAlive() {
On 2017/02/22 08:51:00, Yusuke Sato wrote:
> On 2017/02/21 20:59:43, Yusuke Sato wrote:
> > nit: can you also git-grep the entire code with Arc and Enalbed to make sure
> we
> > no longer have confusing function names. (and please address it in a
separate
> CL
> > if we still have any.)
> 
> s/Enalbed/Enabled/, sorry.

Actually, there still is prefs::kArcEnabeld direct use.
AFAIK, mainly those are to enable ASM for testing purpose.
My next CL will split the pref observing from ASM, so we should be able to
remove some of them by replacing with ASM::RequestEnable().
So, yes, I'll work on them in following CLs.

[hidehiko, 2017-02-23 04:17:44]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2017-02-23 04:17:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, xiyuan@chromium.org, skuhne@chromium.org,
msw@chromium.org, skym@chromium.org, benwells@chromium.org,
lhchavez@chromium.org, yusukes@chromium.org
Link to the patchset: https://codereview.chromium.org/2702723002/#ps100001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-02-23 04:18:12]

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-02-23 05:10:22]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487823464407870,
"parent_rev": "ab9738f225331a756ec93dadef546130b7aa5fe2", "commit_rev":
"0846083cfdbea5d902953540a4220599eb0fe9e1"}

[commit-bot: I haz the power, 2017-02-23 05:11:26]

Description was changed from

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

This CL extracts following methods:
- IsArcPlayStoreEnabled() -> IsArcPlayStoreEnabledForProfile()
- IsArcManaged() -> IsArcPlayStoreEnabledPreferenceManagedForProfile()
- SetArcPlayStoreEnabled() -> SetArcPlayStoreEnabledForProfile()

New functions takes Profile as its arguments explicitly, instead of
acting to the primary user Profile implicitly.

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

to

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

This CL extracts following methods:
- IsArcPlayStoreEnabled() -> IsArcPlayStoreEnabledForProfile()
- IsArcManaged() -> IsArcPlayStoreEnabledPreferenceManagedForProfile()
- SetArcPlayStoreEnabled() -> SetArcPlayStoreEnabledForProfile()

New functions takes Profile as its arguments explicitly, instead of
acting to the primary user Profile implicitly.

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

Review-Url: https://codereview.chromium.org/2702723002
Cr-Commit-Position: refs/heads/master@{#452400}
Committed:
https://chromium.googlesource.com/chromium/src/+/0846083cfdbea5d902953540a422...
==========

[commit-bot: I haz the power, 2017-02-23 05:11:29]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0846083cfdbea5d902953540a422...