Issue 1754803003: Implement Stub for policy bridge to ARC

Issue 1754803003: Implement Stub for policy bridge to ARC (Closed)

Created:
4 years, 9 months ago by phweiss

Modified:
4 years, 9 months ago

Reviewers:
Thiemo Nagel, Luis Héctor Chávez, mdempsky

CC:
chromium-reviews, qsr+mojo_chromium.org, droger+watchlist_chromium.org, viettrungluu+watch_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, oshima+watch_chromium.org, darin (slow to review), ben+mojo_chromium.org, davemoore+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Implement Stub for policy bridge to ARC BUG=b/26001755 Committed: https://crrev.com/83a17b105b9d218b8b79762f681bad062198a7d6 Cr-Commit-Position: refs/heads/master@{#381951}

Patch Set 1 #

Patch Set 2 : change build rules for gyp as well #

Patch Set 3 : rebase, change logs #

Patch Set 4 : Fix logging #

Patch Set 5 : Rebase: auth.mojom was changed #

Patch Set 6 : Fixing gn dependencies #

Patch Set 7 : Remove functionality for OnPolicyChanged due to dependency issues #

Total comments: 10

Patch Set 8 : Fix dependencies by moving files, implement OnPolicyChanged again #

Total comments: 4

Patch Set 9 : Why do we still have two build systems? #

Patch Set 10 : Includes, Comments, and Formatting adjusted #

Total comments: 14

Patch Set 11 : Clarify comments #

Total comments: 2

Patch Set 12 : Use primary user instead of active user #

Patch Set 13 : Renaming to GetPolicies and OnPolicyUpdated #

Total comments: 6

Patch Set 14 : Ordering and ordinal numbers in .mojom files, nits adressed #

Created: 4 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+200 lines, -0 lines)			Patch
M	chrome/browser/chromeos/arc/arc_auth_service.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/arc/arc_auth_service.cc	View	1 2 3 4	1 chunk	+7 lines, -0 lines	0 comments	Download
A	chrome/browser/chromeos/arc/arc_policy_bridge.h	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+44 lines, -0 lines	0 comments	Download
A	chrome/browser/chromeos/arc/arc_policy_bridge.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+74 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/arc/arc_service_launcher.cc	View	1 2 3 4 5 6 7	2 chunks	+3 lines, -0 lines	0 comments	Download
M	chrome/chrome_browser_chromeos.gypi	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -0 lines	0 comments	Download
M	components/arc.gypi	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -0 lines	0 comments	Download
M	components/arc/BUILD.gn	View	1 2 3 4 5 6 7	1 chunk	+1 line, -0 lines	0 comments	Download
M	components/arc/arc_bridge_service.h	View	1 2 3 4 5 6 7 8 9	8 chunks	+11 lines, -0 lines	0 comments	Download
M	components/arc/arc_bridge_service.cc	View		3 chunks	+27 lines, -0 lines	0 comments	Download
M	components/arc/common/arc_bridge.mojom	View		2 chunks	+4 lines, -0 lines	0 comments	Download
M	components/arc/common/auth.mojom	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+2 lines, -0 lines	0 comments	Download
A	components/arc/common/policy.mojom	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+20 lines, -0 lines	0 comments	Download

Messages

Total messages: 20 (7 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Thiemo Nagel

Sgtm from policy perspective. https://codereview.chromium.org/1754803003/diff/120001/chrome/browser/chromeos/arc/arc_auth_service.h File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/1754803003/diff/120001/chrome/browser/chromeos/arc/arc_auth_service.h#newcode102 chrome/browser/chromeos/arc/arc_auth_service.h:102: void GetIsAccountManaged( Please add comment. ...

4 years, 9 months ago (2016-03-15 16:49:23 UTC) #4

phweiss

Hi, please take a look. lhchavez: chrome/browser/chromeos/arc/arc_auth_service.cc chrome/browser/chromeos/arc/arc_auth_service.h chrome/browser/chromeos/arc/arc_policy_bridge.cc chrome/browser/chromeos/arc/arc_policy_bridge.h chrome/browser/chromeos/arc/arc_service_launcher.cc components/arc.gypi components/arc/BUILD.gn components/arc/arc_bridge_service.cc components/arc/arc_bridge_service.h ...

4 years, 9 months ago (2016-03-15 17:38:23 UTC) #6

Luis Héctor Chávez

https://codereview.chromium.org/1754803003/diff/180001/chrome/browser/chromeos/arc/arc_policy_bridge.cc File chrome/browser/chromeos/arc/arc_policy_bridge.cc (right): https://codereview.chromium.org/1754803003/diff/180001/chrome/browser/chromeos/arc/arc_policy_bridge.cc#newcode33 chrome/browser/chromeos/arc/arc_policy_bridge.cc:33: DCHECK(user_list.size() == 1); Is it possible to change this ...

4 years, 9 months ago (2016-03-15 18:01:56 UTC) #7

phweiss

4 years, 9 months ago (2016-03-16 18:45:13 UTC) #8

https://codereview.chromium.org/1754803003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_policy_bridge.cc (right):

https://codereview.chromium.org/1754803003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_policy_bridge.cc:33: DCHECK(user_list.size() ==
1);
On 2016/03/15 18:01:56, Luis Héctor Chávez wrote:
> Is it possible to change this for
> user_manager::UserManager::Get()->GetActiveUser()/GetPrimaryUser();? This
DCHECK
> looks worrisome.
> 
> If you can't change it, please add a comment explaining why adding this DCHECK
> is the right choice and how it will be guaranteed that it will never trigger
> spuriously.

I changed this to using the active user.

https://codereview.chromium.org/1754803003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_policy_bridge.cc:71:
arc_bridge_service()->policy_instance()->OnPolicyChanged();
On 2016/03/15 18:01:56, Luis Héctor Chávez wrote:
> It's probably a good idea to add DCHECKs that
> arc_bridge_service()->policy_instance() is not null in all places you access
it.

Done.

https://codereview.chromium.org/1754803003/diff/180001/components/arc/common/...
File components/arc/common/auth.mojom (right):

https://codereview.chromium.org/1754803003/diff/180001/components/arc/common/...
components/arc/common/auth.mojom:28: // Get is account managed from Chrome OS.
On 2016/03/15 18:01:56, Luis Héctor Chávez wrote:
> How about something along the lines of "Gets whether the account is managed
from
> Chrome OS"?

Done.

https://codereview.chromium.org/1754803003/diff/180001/components/arc/common/...
File components/arc/common/policy.mojom (right):

https://codereview.chromium.org/1754803003/diff/180001/components/arc/common/...
components/arc/common/policy.mojom:8: // Get policies from Chrome OS.
On 2016/03/15 18:01:56, Luis Héctor Chávez wrote:
> You are currently returning a single JSON-encoded policy, but it looks like
you
> might eventually return multiple policies (making a wild guess based on the
fact
> that PolicyMap holds multiple policies).
> 
> If the long-term plan is to only send one, update the comment to read "Get the
> policy from Chrome OS", otherwise change the callback type to string[] policy.
> In any case, please expand this comment so that it documents more about the
> encoding, the content of the policy, and the cardinality of policies.

The "Policy" in GetPolicy and PolicyHost refers to the whole collection of
decisions by the admin, which is a list of values for our "policies" in the
technical sense. The string will be a single JSON-object that can contain values
for multiple policies. I added some more details to the comments.

Thiemo Nagel

4 years, 9 months ago (2016-03-16 19:59:33 UTC) #9

phweiss

ptal, comments inline https://codereview.chromium.org/1754803003/diff/180001/chrome/browser/chromeos/arc/arc_policy_bridge.cc File chrome/browser/chromeos/arc/arc_policy_bridge.cc (right): https://codereview.chromium.org/1754803003/diff/180001/chrome/browser/chromeos/arc/arc_policy_bridge.cc#newcode33 chrome/browser/chromeos/arc/arc_policy_bridge.cc:33: DCHECK(user_list.size() == 1); On 2016/03/16 19:59:33, ...

4 years, 9 months ago (2016-03-17 15:52:59 UTC) #10

ptal, comments inline

https://codereview.chromium.org/1754803003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_policy_bridge.cc (right):

https://codereview.chromium.org/1754803003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_policy_bridge.cc:33: DCHECK(user_list.size() ==
1);
On 2016/03/16 19:59:33, Thiemo Nagel wrote:
> > I changed this to using the active user.
> 
> Primary user would make more sense to me but in practice it should not make a
> difference.

I dont have a preference, changed it to primary.

https://codereview.chromium.org/1754803003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_policy_bridge.cc:59: // TODO(phweiss): Get
actual policies
On 2016/03/15 18:01:56, Luis Héctor Chávez wrote:
> Regardless of the decision mentioned in policy.mojom, please be consistent
with
> the number. Either only use "policy" or "policies".

Renamed the function to GetPolicies, also for consistency with the PolicyService
class.

https://codereview.chromium.org/1754803003/diff/180001/components/arc/common/...
File components/arc/common/policy.mojom (right):

https://codereview.chromium.org/1754803003/diff/180001/components/arc/common/...
components/arc/common/policy.mojom:8: // Get policies from Chrome OS.
On 2016/03/16 18:45:12, phweiss wrote:
> On 2016/03/15 18:01:56, Luis Héctor Chávez wrote:
> > You are currently returning a single JSON-encoded policy, but it looks like
> you
> > might eventually return multiple policies (making a wild guess based on the
> fact
> > that PolicyMap holds multiple policies).
> > 
> > If the long-term plan is to only send one, update the comment to read "Get
the
> > policy from Chrome OS", otherwise change the callback type to string[]
policy.
> > In any case, please expand this comment so that it documents more about the
> > encoding, the content of the policy, and the cardinality of policies.
> 
> The "Policy" in GetPolicy and PolicyHost refers to the whole collection of
> decisions by the admin, which is a list of values for our "policies" in the
> technical sense. The string will be a single JSON-object that can contain
values
> for multiple policies. I added some more details to the comments.

I reconsidered this, and renamed the function now to plural.

https://codereview.chromium.org/1754803003/diff/180001/components/arc/common/...
components/arc/common/policy.mojom:17: OnPolicyChanged();
This function is now renamed to OnPolicyUpdated for consistency with
PolicyService.

Luis Héctor Chávez

lgtm with nits. https://codereview.chromium.org/1754803003/diff/200001/chrome/browser/chromeos/arc/arc_policy_bridge.cc File chrome/browser/chromeos/arc/arc_policy_bridge.cc (right): https://codereview.chromium.org/1754803003/diff/200001/chrome/browser/chromeos/arc/arc_policy_bridge.cc#newcode70 chrome/browser/chromeos/arc/arc_policy_bridge.cc:70: DCHECK(arc_bridge_service()->policy_instance() != NULL); nit: prefer |nullptr| ...

4 years, 9 months ago (2016-03-17 15:57:49 UTC) #11

phweiss

https://codereview.chromium.org/1754803003/diff/200001/chrome/browser/chromeos/arc/arc_policy_bridge.cc File chrome/browser/chromeos/arc/arc_policy_bridge.cc (right): https://codereview.chromium.org/1754803003/diff/200001/chrome/browser/chromeos/arc/arc_policy_bridge.cc#newcode70 chrome/browser/chromeos/arc/arc_policy_bridge.cc:70: DCHECK(arc_bridge_service()->policy_instance() != NULL); On 2016/03/17 15:57:49, Luis Héctor Chávez ...

4 years, 9 months ago (2016-03-17 16:07:41 UTC) #12

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1754803003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1754803003/260001

4 years, 9 months ago (2016-03-18 13:30:39 UTC) #16

commit-bot: I haz the power

4 years, 9 months ago (2016-03-18 13:37:14 UTC) #20

Message was sent while issue was closed.

Patchset 14 (id:??) landed as
https://crrev.com/83a17b105b9d218b8b79762f681bad062198a7d6
Cr-Commit-Position: refs/heads/master@{#381951}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages