Save Play user id when ARC user gets a Managed Google Play account

Save Play user id when ARC user gets a Managed Google Play account

Chrome OS stores the user id it receives from DM Server in order to
later be able to use it for reporting the account activity.

BUG=694304
Review-Url: https://codereview.chromium.org/2704113003
Cr-Commit-Position: refs/heads/master@{#455065}
Committed: https://chromium.googlesource.com/chromium/src/+/d1d249ede7fe09c9d6fd2aacccec764987c323a0

[Marton Hunyady, 2017-02-20 17:48:11]

hunyadym@chromium.org changed reviewers:
+ achuith@chromium.org, battre@chromium.org, lhchavez@chromium.org,
tnagel@chromium.org

[Marton Hunyady, 2017-02-20 17:48:11]

tnagel@chromium.org: Please review changes in
components/policy/core/common/policy_pref_names.*

battre@chromium.org: Please review changes in
chrome/browser/prefs/browser_prefs.cc

lhchavez@chromium.org: Please review changes in
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher*

achuith@chromium.org: Please review changes in 
chrome/browser/chromeos/profiles/profile_helper.h

Thanks!

[achuithb, 2017-02-21 13:55:28]

On 2017/02/20 17:48:11, Marton Hunyady wrote:
> mailto:tnagel@chromium.org: Please review changes in
> components/policy/core/common/policy_pref_names.*
> 
> mailto:battre@chromium.org: Please review changes in
> chrome/browser/prefs/browser_prefs.cc
> 
> mailto:lhchavez@chromium.org: Please review changes in
>
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher*
> 
> mailto:achuith@chromium.org: Please review changes in 
> chrome/browser/chromeos/profiles/profile_helper.h

rubberstamp lgtm for profile_helper.h

[achuithb, 2017-02-21 13:55:34]

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

[commit-bot: I haz the power, 2017-02-21 13:55:58]

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-21 15:15:45]

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

[commit-bot: I haz the power, 2017-02-21 15:15:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[battre, 2017-02-21 16:03:17]

battre@chromium.org changed reviewers:
+ glevin@chromium.org

[battre, 2017-02-21 16:03:18]

Hi.

What about clearing the ID when the user disables ARC++?

+glevin for Chrome OS privacy review.

Please reach out to your PCounsel to get guidance on whether the privacy notice
needs to be updated. Please make sure the Cros PDD gets updated.

Best regards,
Dominic

[Greg Levin, 2017-02-21 17:35:47]

NOTE: I'm having an email conversation with hunyadym@ to get some background on
the feature and Chromad environment.

https://codereview.chromium.org/2704113003/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2704113003/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:18: #include
"chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h"
This should be moved down to the "#if defined(OS_CHROMEOS)" section of includes
(starting on line 175).

[Marton Hunyady, 2017-02-22 14:57:57]

On 2017/02/21 16:03:18, battre wrote:
> What about clearing the ID when the user disables ARC++?
Done.

[Marton Hunyady, 2017-02-22 15:21:21]

https://codereview.chromium.org/2704113003/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2704113003/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:18: #include
"chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h"
On 2017/02/21 17:35:47, Greg Levin wrote:
> This should be moved down to the "#if defined(OS_CHROMEOS)" section of
includes
> (starting on line 175).

Done.

[Marton Hunyady, 2017-02-22 15:21:54]

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

[commit-bot: I haz the power, 2017-02-22 15:22:24]

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-22 16:05:08]

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

[commit-bot: I haz the power, 2017-02-22 16:05:09]

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_...)

[Greg Levin, 2017-02-22 20:48:04]

LGTM

battre@ - I've included you on an email thread that includes a more detailed
summary of this feature and its context, if you want to take a look.

[Marton Hunyady, 2017-03-01 15:13:37]

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

[commit-bot: I haz the power, 2017-03-01 15:13:45]

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 15:59:55]

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

[commit-bot: I haz the power, 2017-03-01 15:59:56]

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_...)

[Marton Hunyady, 2017-03-01 17:06:38]

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

[commit-bot: I haz the power, 2017-03-01 17:06:54]

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 18:24:09]

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

[commit-bot: I haz the power, 2017-03-01 18:24:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marton Hunyady, 2017-03-02 11:34:53]

Description was changed from

==========
Save Play user id when ARC user gets a Managed Google Play account

Chrome OS stores the user id it receives from DM Server in order to
later be able to use it for reporting the account activity.

BUG=694304
==========

to

==========
Save Play user id when ARC user gets a Managed Google Play account

Chrome OS stores the user id it receives from DM Server in order to
later be able to use it for reporting the account activity.

BUG=694304
==========

[Marton Hunyady, 2017-03-02 11:34:53]

hunyadym@chromium.org changed reviewers:
+ pastarmovj@chromium.org
- tnagel@chromium.org

[Marton Hunyady, 2017-03-02 11:51:57]

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

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

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

[pastarmovj, 2017-03-02 12:28:08]

lgtm

https://codereview.chromium.org/2704113003/diff/100001/components/policy/core...
File components/policy/core/common/policy_pref_names.cc (right):

https://codereview.chromium.org/2704113003/diff/100001/components/policy/core...
components/policy/core/common/policy_pref_names.cc:13: const char
kActiveDirectoryPlayUserId[] =
nit: please move this closer to the code that needs it. Maybe chrome/common?

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

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

[commit-bot: I haz the power, 2017-03-02 13:09:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marton Hunyady, 2017-03-02 14:05:43]

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

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

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

[Marton Hunyady, 2017-03-02 14:17:17]

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

[Marton Hunyady, 2017-03-02 14:17:25]

https://codereview.chromium.org/2704113003/diff/100001/components/policy/core...
File components/policy/core/common/policy_pref_names.cc (right):

https://codereview.chromium.org/2704113003/diff/100001/components/policy/core...
components/policy/core/common/policy_pref_names.cc:13: const char
kActiveDirectoryPlayUserId[] =
On 2017/03/02 12:28:08, pastarmovj wrote:
> nit: please move this closer to the code that needs it. Maybe chrome/common?

Done.

[commit-bot: I haz the power, 2017-03-02 14:17:30]

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

[achuithb, 2017-03-02 14:41:01]

https://codereview.chromium.org/2704113003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2704113003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_session_manager.cc:119: std::string());
nit: "" is more concise

https://codereview.chromium.org/2704113003/diff/140001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.cc
(right):

https://codereview.chromium.org/2704113003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.cc:89:
Profile* GetProfile() {
You're replicating GetPrimaryUserProfile here: 
https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager....

The comment suggests that you should not be using GetPrimaryUserProfile, but
there's no hint of a replacement. Perhaps you can ask skuhne@?

I don't think writing your own function here is desirable?

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

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

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

Dry run: This issue passed the CQ dry run.

[Marton Hunyady, 2017-03-07 10:04:04]

On 2017/03/02 14:41:01, achuithb wrote:
>
https://codereview.chromium.org/2704113003/diff/140001/chrome/browser/chromeo...
> File chrome/browser/chromeos/arc/arc_session_manager.cc (right):
> 
>
https://codereview.chromium.org/2704113003/diff/140001/chrome/browser/chromeo...
> chrome/browser/chromeos/arc/arc_session_manager.cc:119: std::string());
> nit: "" is more concise
> 
>
https://codereview.chromium.org/2704113003/diff/140001/chrome/browser/chromeo...
> File
>
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.cc
> (right):
> 
>
https://codereview.chromium.org/2704113003/diff/140001/chrome/browser/chromeo...
>
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.cc:89:
> Profile* GetProfile() {
> You're replicating GetPrimaryUserProfile here: 
>
https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager....
> 
> The comment suggests that you should not be using GetPrimaryUserProfile, but
> there's no hint of a replacement. Perhaps you can ask skuhne@?
> 
> I don't think writing your own function here is desirable?

Thanks, after talking to you in person, I'm submitting this.

[Marton Hunyady, 2017-03-07 10:04:12]

The CQ bit was checked by hunyadym@chromium.org

[Marton Hunyady, 2017-03-07 10:04:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, glevin@chromium.org, pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2704113003/#ps140001
(title: "Move pref from policy_pref_names to chrome/common/pref_names")

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

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-07 10:51:23]

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

[commit-bot: I haz the power, 2017-03-07 10:51:24]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Marton Hunyady, 2017-03-07 11:11:55]

The CQ bit was checked by hunyadym@chromium.org

[commit-bot: I haz the power, 2017-03-07 11:12:14]

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-07 12:00:26]

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

[commit-bot: I haz the power, 2017-03-07 12:00:27]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Marton Hunyady, 2017-03-07 12:53:18]

The CQ bit was checked by hunyadym@chromium.org

[commit-bot: I haz the power, 2017-03-07 12:53:42]

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-07 12:57:52]

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

[commit-bot: I haz the power, 2017-03-07 12:57:53]

Failed to apply patch for chrome/browser/chromeos/profiles/profile_helper.h:
While running git apply --index -p1;
  error: patch failed: chrome/browser/chromeos/profiles/profile_helper.h:18
  error: chrome/browser/chromeos/profiles/profile_helper.h: patch does not apply

Patch:       chrome/browser/chromeos/profiles/profile_helper.h
Index: chrome/browser/chromeos/profiles/profile_helper.h
diff --git a/chrome/browser/chromeos/profiles/profile_helper.h
b/chrome/browser/chromeos/profiles/profile_helper.h
index
2104e1e187eb12d1715e6da67f8796d71d4f5457..858406614f0e9a2326dbc3db563b4007b0862b03
100644
--- a/chrome/browser/chromeos/profiles/profile_helper.h
+++ b/chrome/browser/chromeos/profiles/profile_helper.h
@@ -18,6 +18,7 @@
 #include "chrome/browser/chromeos/login/signin/oauth2_login_manager.h"
 #include "components/user_manager/user_manager.h"
 
+class ArcActiveDirectoryEnrollmentTokenFetcherBrowserTest;
 class ArcAppTest;
 class Profile;
 
@@ -171,6 +172,7 @@ class ProfileHelper
   friend class SystemTrayDelegateChromeOSTest;
   friend class arc::SyncArcPackageHelper;
   friend class ash::test::MultiUserWindowManagerChromeOSTest;
+  friend class ::ArcActiveDirectoryEnrollmentTokenFetcherBrowserTest;
   friend class ::ArcAppTest;
   friend class ::test::BrowserFinderChromeOSTest;

[Marton Hunyady, 2017-03-07 13:17:14]

The CQ bit was checked by hunyadym@chromium.org

[Marton Hunyady, 2017-03-07 13:17:14]

The patchset sent to the CQ was uploaded after l-g-t-m from glevin@chromium.org,
pastarmovj@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2704113003/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-07 13:17:42]

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-07 14:00:33]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488892634257360,
"parent_rev": "46d0c110e7c68b484bfc292cf030d599c9bb8251", "commit_rev":
"d1d249ede7fe09c9d6fd2aacccec764987c323a0"}

[commit-bot: I haz the power, 2017-03-07 14:01:35]

Description was changed from

==========
Save Play user id when ARC user gets a Managed Google Play account

Chrome OS stores the user id it receives from DM Server in order to
later be able to use it for reporting the account activity.

BUG=694304
==========

to

==========
Save Play user id when ARC user gets a Managed Google Play account

Chrome OS stores the user id it receives from DM Server in order to
later be able to use it for reporting the account activity.

BUG=694304

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

[commit-bot: I haz the power, 2017-03-07 14:01:37]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/d1d249ede7fe09c9d6fd2aacccec...