Re: Add support for the Android WebView policy configuration (issue 1372953002 by dgn@chromium.org)

[d...@chromium.org]

PTAL

https://codereview.chromium.org/1372953002/

[bau...@chromium.org]

lgtm



https://codereview.chromium.org/1372953002/

[tha...@chromium.org]

code lgtm except a few nits

I don't really understand what the bug is about though. It sounds like we
explicitly want to have different policy configurations for Chromium and
WebView
– I would've thought we would want to keep them identical so that WebView
requires less maintenance. If there's some good reason why we wouldn't want
it
and it doesn't take too long to explain, I'd find that interesting :-)


https://codereview.chromium.org/1372953002/diff/60001/grit/format/policy_templates/writers/template_writer.py
File grit/format/policy_templates/writers/template_writer.py (left):

https://codereview.chromium.org/1372953002/diff/60001/grit/format/policy_templates/writers/template_writer.py#oldcode102
grit/format/policy_templates/writers/template_writer.py:102:
is_supported = lambda x: platform in x['platforms']
This returns a bool…

https://codereview.chromium.org/1372953002/diff/60001/grit/format/policy_templates/writers/template_writer.py
File grit/format/policy_templates/writers/template_writer.py (right):

https://codereview.chromium.org/1372953002/diff/60001/grit/format/policy_templates/writers/template_writer.py#newcode104
grit/format/policy_templates/writers/template_writer.py:104: if not
platform in supported_on['platforms']:
nit: `platform not in` instead of `not platform in`

https://codereview.chromium.org/1372953002/diff/60001/grit/format/policy_templates/writers/template_writer.py#newcode106
grit/format/policy_templates/writers/template_writer.py:106: if product
and not product in supported_on['product']:
same nit

https://codereview.chromium.org/1372953002/diff/60001/grit/format/policy_templates/writers/template_writer.py#newcode108
grit/format/policy_templates/writers/template_writer.py:108: return
supported_on
…but this returns an object. Since it's used with filter() which talks
about boolean values, I think returning False and True instead of None
and supported_on is easier to understand.

Is this function body equivalent to

return platform in so[] and (!product or product in so[])

? (Technically, that also needs an `and bool(supported_on)`, but you
already use `in supported_on`, so if supported_on was None or empty
that'd already fail)

https://codereview.chromium.org/1372953002/

[d...@chromium.org]

> I don't really understand what the bug is about though. It sounds like
> we explicitly want to have different policy configurations for Chromium
> and WebView – I would've thought we would want to keep them identical so
> that WebView requires less maintenance. If there's some good reason why
> we wouldn't want it and it doesn't take too long to explain, I'd find
> that interesting :-)

When we added policy support to WebView, we prefixed the app restiction
names
to reduce the chances of conflicting with restrictions declared by the
embedding app. Because of that, the policy "Foo" has the app restriction
name
"Foo" in Chrome and "com.android.browser:Foo" in Webview.
The CL is to display an extra line that says from which version of webview
the
policy is supported and show the full restriction name that has to be
declared:
https://screenshot.googleplex.com/rUTBrLEcZ3d.png

https://codereview.chromium.org/1368373002/ introduces the visible changes
to the policy list, enabled by the current CL

https://codereview.chromium.org/1372953002/diff/60001/grit/format/policy_templates/writers/template_writer.py
File grit/format/policy_templates/writers/template_writer.py (right):

https://codereview.chromium.org/1372953002/diff/60001/grit/format/policy_templates/writers/template_writer.py#newcode104
grit/format/policy_templates/writers/template_writer.py:104: if not
platform in supported_on['platforms']:

On 2015/11/02 16:41:10, Nico wrote:
> nit: `platform not in` instead of `not platform in`

Acknowledged.

https://codereview.chromium.org/1372953002/diff/60001/grit/format/policy_templates/writers/template_writer.py#newcode106
grit/format/policy_templates/writers/template_writer.py:106: if product
and not product in supported_on['product']:

On 2015/11/02 16:41:10, Nico wrote:
> same nit

Acknowledged.

https://codereview.chromium.org/1372953002/diff/60001/grit/format/policy_templates/writers/template_writer.py#newcode108
grit/format/policy_templates/writers/template_writer.py:108: return
supported_on

On 2015/11/02 16:41:10, Nico wrote:
> …but this returns an object. Since it's used with filter() which talks
about
> boolean values, I think returning False and True instead of None and
> supported_on is easier to understand.

> Is this function body equivalent to

> return platform in so[] and (!product or product in so[])

> ? (Technically, that also needs an `and bool(supported_on)`, but you
already use
> `in supported_on`, so if supported_on was None or empty that'd already
fail)

Done.

https://codereview.chromium.org/1372953002/

[tha...@chromium.org]

landed in grit r201

https://codereview.chromium.org/1372953002/

[d...@chromium.org]

On 2015/11/03 19:20:57, Nico wrote:
> landed in grit r201

Thanks a lot!

https://codereview.chromium.org/1372953002/

[d...@chromium.org]

On 2015/11/03 21:24:32, dgn wrote:
> On 2015/11/03 19:20:57, Nico wrote:
> > landed in grit r201

> Thanks a lot!

Do I abandon the CL now?

https://codereview.chromium.org/1372953002/

[tha...@chromium.org]

Probably enough to close it ("edit issue" -> check "closed"). I just did
that.

https://codereview.chromium.org/1372953002/

[d...@chromium.org]

On 2015/11/03 21:26:11, Nico wrote:
> Probably enough to close it ("edit issue" -> check "closed"). I just did
> that.

Thanks

https://codereview.chromium.org/1372953002/