Pass policy VideoCaptureAllowed to ARC

Fix syntax of JSON string, include cameraDisabled

Instead of passing ArcApplicationPolicy as entry in the
json dictionary, it is now parsed and used as the top level
dictionary, with other policies added to it. The
VideoCaptureAllowed policy is mapped to cameraDisabled.

Bug: b/25985338

Committed: https://crrev.com/e939d0314bc6ae82b04def09c58972ab121de51e
Cr-Commit-Position: refs/heads/master@{#385712}

[phweiss, 2016-04-05 15:20:29]

phweiss@chromium.org changed reviewers:
+ bartfab@chromium.org

[phweiss, 2016-04-05 15:20:29]

ptal

[bartfab (slow), 2016-04-06 11:42:44]

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

https://codereview.chromium.org/1864523002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_policy_bridge.h:8: #include <string>
Nit: No longer used.

[phweiss, 2016-04-06 13:23:03]

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

https://codereview.chromium.org/1864523002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_policy_bridge.h:8: #include <string>
On 2016/04/06 11:42:44, bartfab (slow) wrote:
> Nit: No longer used.

Done.

[bartfab (slow), 2016-04-06 13:26:49]

lgtm

[phweiss, 2016-04-06 14:16:29]

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

[phweiss, 2016-04-06 14:16:29]

Hi, ptal

[Luis Héctor Chávez, 2016-04-06 15:36:15]

lgtm

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

https://codereview.chromium.org/1864523002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_policy_bridge.cc:47: }  // anonymous namespace
nit: "}  // namespace" is more commonly used (7x in all .cc files)

[phweiss, 2016-04-06 17:35:48]

Description was changed from

==========
Pass policy VideoCaptureAllowed to ARC

Includes small refactoring to make it easier to add other policies
in the future.

BUG= b/25985338
==========

to

==========
Fix syntax of JSON string, include cameraDisabled
    
Instead of passing ArcApplicationPolicy as entry in the
json dictionary, it is now parsed and used as the top level
dictionary, with other policies added to it. The
VideoCaptureAllowed policy is mapped to cameraDisabled.
    
Bug: b/25985338
==========

[phweiss, 2016-04-06 17:39:24]

ptal again, the json syntax was wrong, ArcApplicationPolicy is now parsed and
used as dictionary instead of as string, VideoCaptureAllowed is then added to
this dictionary.

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

https://codereview.chromium.org/1864523002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_policy_bridge.cc:47: }  // anonymous namespace
On 2016/04/06 15:36:15, Luis Héctor Chávez wrote:
> nit: "}  // namespace" is more commonly used (7x in all .cc files)

Done.

[Nikita (slow), 2016-04-06 17:52:42]

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

[Nikita (slow), 2016-04-06 17:52:42]

lgtm

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

https://codereview.chromium.org/1864523002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_policy_bridge_unittest.cc:87: "}]"
nit: add extra top level policy
"defaultPermissionPolicy": "GRANT"

[phweiss, 2016-04-06 18:47:27]

The CQ bit was checked by phweiss@chromium.org

[phweiss, 2016-04-06 18:47:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, lhchavez@chromium.org, nkostylev@chromium.org
Link to the patchset: https://codereview.chromium.org/1864523002/#ps60001
(title: "Modify testcases")

[phweiss, 2016-04-06 18:47:33]

The CQ bit was unchecked by phweiss@chromium.org

[phweiss, 2016-04-07 11:30:26]

The CQ bit was checked by phweiss@chromium.org

[commit-bot: I haz the power, 2016-04-07 11:30:46]

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

[commit-bot: I haz the power, 2016-04-07 11:36:03]

Description was changed from

==========
Fix syntax of JSON string, include cameraDisabled
    
Instead of passing ArcApplicationPolicy as entry in the
json dictionary, it is now parsed and used as the top level
dictionary, with other policies added to it. The
VideoCaptureAllowed policy is mapped to cameraDisabled.
    
Bug: b/25985338
==========

to

==========
Fix syntax of JSON string, include cameraDisabled
    
Instead of passing ArcApplicationPolicy as entry in the
json dictionary, it is now parsed and used as the top level
dictionary, with other policies added to it. The
VideoCaptureAllowed policy is mapped to cameraDisabled.
    
Bug: b/25985338
==========

[commit-bot: I haz the power, 2016-04-07 11:36:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-07 11:37:35]

Description was changed from

==========
Fix syntax of JSON string, include cameraDisabled
    
Instead of passing ArcApplicationPolicy as entry in the
json dictionary, it is now parsed and used as the top level
dictionary, with other policies added to it. The
VideoCaptureAllowed policy is mapped to cameraDisabled.
    
Bug: b/25985338
==========

to

==========
Fix syntax of JSON string, include cameraDisabled

Instead of passing ArcApplicationPolicy as entry in the
json dictionary, it is now parsed and used as the top level
dictionary, with other policies added to it. The
VideoCaptureAllowed policy is mapped to cameraDisabled.

Bug: b/25985338

Committed: https://crrev.com/e939d0314bc6ae82b04def09c58972ab121de51e
Cr-Commit-Position: refs/heads/master@{#385712}
==========

[commit-bot: I haz the power, 2016-04-07 11:37:36]

Patchset 4 (id:??) landed as
https://crrev.com/e939d0314bc6ae82b04def09c58972ab121de51e
Cr-Commit-Position: refs/heads/master@{#385712}

[bartfab (slow), 2016-04-07 12:46:29]

LGTM

https://codereview.chromium.org/1864523002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_policy_bridge.cc (right):

https://codereview.chromium.org/1864523002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_policy_bridge.cc:56:
std::unique_ptr<base::DictionaryValue> app_policy_dict =
Nit: #include <memory>

https://codereview.chromium.org/1864523002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_policy_bridge.cc:59:
filtered_policies.Swap(app_policy_dict.get());
This is correct but dangerous: The Swap() throws away the current contents of
|filtered_policies|. Yes, that dict is empty right now, but what if someone does
not realize that this is happening here and tries to parse some other policy
before the app policy?

It would be much safer to use MergeDictionary() instead.

https://codereview.chromium.org/1864523002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_policy_bridge.cc:61: LOG(ERROR) << "Value of
ArcApplicationPolicy has invalid format: "
We would ideally surface this on the chrome://policy page as well.

[phweiss, 2016-04-07 19:18:54]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1869783006/ by phweiss@chromium.org.

The reason for reverting is: Unit tests are failing, see 
https://bugs.chromium.org/p/chromium/issues/detail?id=601542.

[Nikita (slow), 2016-04-07 20:09:45]

Well that's unfortunate :(

On Thu, 7 Apr 2016 at 20:18 <phweiss@chromium.org> wrote:

> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/1869783006/ by phweiss@chromium.org.
>
> The reason for reverting is: Unit tests are failing, see
> https://bugs.chromium.org/p/chromium/issues/detail?id=601542.
>
> https://codereview.chromium.org/1864523002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bartfab (slow), 2016-04-08 08:55:05]

I am sure Philipp has the power to fix this and re-land :).

- Bartosz

On 04/07/2016 10:09 PM, Nikita Kostylev wrote:
> Well that's unfortunate :(
>
> On Thu, 7 Apr 2016 at 20:18 <phweiss@chromium.org
> <mailto:phweiss@chromium.org>> wrote:
>
>     A revert of this CL (patchset #4 id:60001) has been created in
>     https://codereview.chromium.org/1869783006/ by phweiss@chromium.org
>     <mailto:phweiss@chromium.org>.
>
>     The reason for reverting is: Unit tests are failing, see
>     https://bugs.chromium.org/p/chromium/issues/detail?id=601542.
>
>     https://codereview.chromium.org/1864523002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.