arc: Consolidate IsArcAllowedForUser logic

arc: Consolidate IsArcAllowedForUser logic

BUG=722876
TEST=ArcUtilTest.IsArcAllowedForUser

Review-Url: https://codereview.chromium.org/2885933003
Cr-Commit-Position: refs/heads/master@{#474646}
Committed: https://chromium.googlesource.com/chromium/src/+/94a81dea3021d2e86673872e7b19ea0b684e614e

[xiyuan, 2017-05-16 21:05:31]

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

[xiyuan, 2017-05-16 21:06:05]

xiyuan@chromium.org changed reviewers:
+ hidehiko@chromium.org, kinaba@chromium.org

[xiyuan, 2017-05-16 21:06:06]

PTAL. Thanks.

[commit-bot: I haz the power, 2017-05-16 21:06:46]

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-05-17 00:39:55]

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

[commit-bot: I haz the power, 2017-05-17 00:39:56]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-05-17 02:09:09]

On 2017/05/16 21:06:06, xiyuan wrote:
> PTAL. Thanks.

Thank you very much for quick clean up!

https://chromium-review.googlesource.com/c/506129/ is cleaning specific
commandline flag and its utility function.
Could you kindly collaborate with it?

[hidehiko, 2017-05-17 02:09:45]

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

https://codereview.chromium.org/2885933003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.cc:146: if
(user_manager::UserManager::Get()
Clarification: Can this be merged into IsArcAllowedForUser, too?

https://codereview.chromium.org/2885933003/diff/20001/components/arc/arc_util.cc
File components/arc/arc_util.cc (right):

https://codereview.chromium.org/2885933003/diff/20001/components/arc/arc_util...
components/arc/arc_util.cc:98: return user &&
Instead of oneline, could you use if-stmt, and vlog for each not-allowed case?

E.g.;

if (!user->HasGaiaAccount()) {
  VLOG(1) << "User does not have gaia account.";
  return false;
}
  :

etc.?

https://codereview.chromium.org/2885933003/diff/20001/components/arc/arc_util.h
File components/arc/arc_util.h (right):

https://codereview.chromium.org/2885933003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:18: }
nit: // namespace user_manager
for consistency.

[xiyuan, 2017-05-17 21:34:28]

Patchset #3 (id:40001) has been deleted

[xiyuan, 2017-05-17 21:36:04]

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

[xiyuan, 2017-05-17 21:36:15]

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

https://codereview.chromium.org/2885933003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_util.cc:146: if
(user_manager::UserManager::Get()
On 2017/05/17 02:09:45, hidehiko wrote:
> Clarification: Can this be merged into IsArcAllowedForUser, too?

Done.

https://codereview.chromium.org/2885933003/diff/20001/components/arc/arc_util.cc
File components/arc/arc_util.cc (right):

https://codereview.chromium.org/2885933003/diff/20001/components/arc/arc_util...
components/arc/arc_util.cc:98: return user &&
On 2017/05/17 02:09:45, hidehiko wrote:
> Instead of oneline, could you use if-stmt, and vlog for each not-allowed case?
> 
> E.g.;
> 
> if (!user->HasGaiaAccount()) {
>   VLOG(1) << "User does not have gaia account.";
>   return false;
> }
>   :
> 
> etc.?

Done.

https://codereview.chromium.org/2885933003/diff/20001/components/arc/arc_util.h
File components/arc/arc_util.h (right):

https://codereview.chromium.org/2885933003/diff/20001/components/arc/arc_util...
components/arc/arc_util.h:18: }
On 2017/05/17 02:09:45, hidehiko wrote:
> nit: // namespace user_manager
> for consistency.

Done.

https://codereview.chromium.org/2885933003/diff/60001/components/user_manager...
File components/user_manager/fake_user_manager.h (left):

https://codereview.chromium.org/2885933003/diff/60001/components/user_manager...
components/user_manager/fake_user_manager.h:36: void LoginUser(const AccountId&
account_id);
Stale code with no impl.

https://codereview.chromium.org/2885933003/diff/60001/components/user_manager...
components/user_manager/fake_user_manager.h:41: const user_manager::UserList&
GetLoggedInUsers() const override;
Removed because it is the same as base class impl.

[commit-bot: I haz the power, 2017-05-17 21:37: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-05-17 21:43:45]

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

[commit-bot: I haz the power, 2017-05-17 21:43:46]

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

[xiyuan, 2017-05-17 21:47:57]

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

[commit-bot: I haz the power, 2017-05-17 21:49:18]

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-05-18 01:07:20]

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

[commit-bot: I haz the power, 2017-05-18 01:07:21]

Dry run: This issue passed the CQ dry run.

[kinaba, 2017-05-18 02:57:34]

for me lgtm, defer to hidehiko@

[hidehiko, 2017-05-18 18:52:50]

https://codereview.chromium.org/2885933003/diff/80001/components/arc/BUILD.gn
File components/arc/BUILD.gn (right):

https://codereview.chromium.org/2885933003/diff/80001/components/arc/BUILD.gn...
components/arc/BUILD.gn:81: "//components/signin/core/account_id",
Please add "//components/user_manager", here, too?

https://codereview.chromium.org/2885933003/diff/80001/components/arc/arc_util...
File components/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2885933003/diff/80001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:253: {"",
"--arc-availability=officially-supported-with-active-directory"});
This is flag is being deprecated (as you looks rebasing on top of the removing
CL?)

https://codereview.chromium.org/2885933003/diff/80001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:264: ASSERT_TRUE(ephemeral_user &&
fake_user_manager.IsUserCryptohomeDataEphemeral(
nit/style: How about splitting two ASSERT_TRUEs?
So that what happens can be easily detected.

[xiyuan, 2017-05-22 20:58:18]

https://codereview.chromium.org/2885933003/diff/80001/components/arc/BUILD.gn
File components/arc/BUILD.gn (right):

https://codereview.chromium.org/2885933003/diff/80001/components/arc/BUILD.gn...
components/arc/BUILD.gn:81: "//components/signin/core/account_id",
On 2017/05/18 18:52:49, hidehiko wrote:
> Please add "//components/user_manager", here, too?

Done.

https://codereview.chromium.org/2885933003/diff/80001/components/arc/arc_util...
File components/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2885933003/diff/80001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:253: {"",
"--arc-availability=officially-supported-with-active-directory"});
On 2017/05/18 18:52:50, hidehiko wrote:
> This is flag is being deprecated (as you looks rebasing on top of the removing
> CL?)

Removed after rebase.

https://codereview.chromium.org/2885933003/diff/80001/components/arc/arc_util...
components/arc/arc_util_unittest.cc:264: ASSERT_TRUE(ephemeral_user &&
fake_user_manager.IsUserCryptohomeDataEphemeral(
On 2017/05/18 18:52:50, hidehiko wrote:
> nit/style: How about splitting two ASSERT_TRUEs?
> So that what happens can be easily detected.

Done.

[xiyuan, 2017-05-22 20:58:24]

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

[commit-bot: I haz the power, 2017-05-22 20:58:51]

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-05-22 22:25:54]

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

[commit-bot: I haz the power, 2017-05-22 22:25:55]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-05-23 07:23:26]

LGTM with minor comments/clarifications.
Thank you very much for clean up!

https://codereview.chromium.org/2885933003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/2885933003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:156:
user_manager::UserManager::Get()->FindUser(account_id));
Clarification: Just in case, this change make behavior difference in case of
Kiosk, IIUC. Is it intentional?

https://codereview.chromium.org/2885933003/diff/100001/components/arc/arc_uti...
File components/arc/arc_util.cc (right):

https://codereview.chromium.org/2885933003/diff/100001/components/arc/arc_uti...
components/arc/arc_util.cc:102: user->GetType() !=
user_manager::USER_TYPE_ARC_KIOSK_APP) {
Could you comment that this check is compatible with IsArcKioskMode()'s?

[xiyuan, 2017-05-24 19:22:56]

https://codereview.chromium.org/2885933003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/2885933003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:156:
user_manager::UserManager::Get()->FindUser(account_id));
On 2017/05/23 07:23:26, hidehiko wrote:
> Clarification: Just in case, this change make behavior difference in case of
> Kiosk, IIUC. Is it intentional?

It is okay because the function is called for user pods banners. Kiosk users do
not have pods on the login screen. So the behvaior change should be no impact on
kiosk users.

https://codereview.chromium.org/2885933003/diff/100001/components/arc/arc_uti...
File components/arc/arc_util.cc (right):

https://codereview.chromium.org/2885933003/diff/100001/components/arc/arc_uti...
components/arc/arc_util.cc:102: user->GetType() !=
user_manager::USER_TYPE_ARC_KIOSK_APP) {
On 2017/05/23 07:23:26, hidehiko wrote:
> Could you comment that this check is compatible with IsArcKioskMode()'s?

Done.

[xiyuan, 2017-05-24 19:23:48]

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

[commit-bot: I haz the power, 2017-05-24 19:24:59]

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-05-24 23:57:11]

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

[commit-bot: I haz the power, 2017-05-24 23:57:12]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-05-25 14:27:13]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-05-25 14:27:14]

The patchset sent to the CQ was uploaded after l-g-t-m from kinaba@chromium.org,
hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/2885933003/#ps120001
(title: "add comment for arc kiosk user check")

[commit-bot: I haz the power, 2017-05-25 14:27:32]

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-05-25 14:31:07]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495722433920600,
"parent_rev": "27543b37fdfe82a4a1df1f0435969875b182585b", "commit_rev":
"17cc64a25c962adb11cb91f24bafd330ede17e10"}

[commit-bot: I haz the power, 2017-05-25 14:31:39]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495722433920600,
"parent_rev": "98b35aa5f07963d0528d0a5d1e4497675a22639b", "commit_rev":
"94a81dea3021d2e86673872e7b19ea0b684e614e"}

[commit-bot: I haz the power, 2017-05-25 14:31:55]

Description was changed from

==========
arc: Consolidate IsArcAllowedForUser logic

BUG=722876
TEST=ArcUtilTest.IsArcAllowedForUser
==========

to

==========
arc: Consolidate IsArcAllowedForUser logic

BUG=722876
TEST=ArcUtilTest.IsArcAllowedForUser

Review-Url: https://codereview.chromium.org/2885933003
Cr-Commit-Position: refs/heads/master@{#474646}
Committed:
https://chromium.googlesource.com/chromium/src/+/94a81dea3021d2e86673872e7b19...
==========

[commit-bot: I haz the power, 2017-05-25 14:31:56]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/94a81dea3021d2e86673872e7b19...