Show policy allowed URLs as policy exceptions in the media settings.

Show policy allowed URLs as policy exceptions in the media settings.

Show the policy-set URLs which are allowed to use mic and camera without asking (kAudioCaptureAllowedUrls, kVideoCaptureAllowedUrls) as if they were policy-set camera and microphone exceptions.

BASE=1243723003
BUG=509962
Committed: https://crrev.com/957baf4c0d8985666ff817dd67c5c31cbb186375
Cr-Commit-Position: refs/heads/master@{#341093}

[msramek, 2015-07-23 19:50:11]

Patchset #1 (id:1) has been deleted

[msramek, 2015-07-23 19:51:54]

msramek@chromium.org changed reviewers:
+ bauerb@chromium.org, raymes@chromium.org

[msramek, 2015-07-23 19:51:54]

Hi again Bernhard and Raymes,

this is the last and smallest of the trio of CLs about the media UI split.

PTAL :)

Thanks!
Martin

[Bernhard Bauer, 2015-07-24 09:13:25]

https://codereview.chromium.org/1251543002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.h (right):

https://codereview.chromium.org/1251543002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.h:232:
std::vector<base::Value*>* exceptions);
Use ScopedVector instead of a vector containing raw pointers. Or actually, why
doesn't this return a base::ListValue?

[msramek, 2015-07-24 10:55:07]

Patchset #2 (id:40001) has been deleted

[msramek, 2015-07-24 10:57:32]

https://codereview.chromium.org/1251543002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.h (right):

https://codereview.chromium.org/1251543002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.h:232:
std::vector<base::Value*>* exceptions);
On 2015/07/24 09:13:25, Bernhard Bauer wrote:
> Use ScopedVector instead of a vector containing raw pointers. Or actually, why
> doesn't this return a base::ListValue?

I agree that base::ListValue is much better than std::vector<base::Value>. I was
just trying to spare the conversion code, since the
|GetExceptionsFromHostContentSettingsMap|, the only callsite, works internally
with std::vector<base::Value>.

[Bernhard Bauer, 2015-07-28 13:06:49]

Sorry, I missed this CL. Feel free to ping me if I'm not responding to code
reviews.

https://codereview.chromium.org/1251543002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.h (right):

https://codereview.chromium.org/1251543002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.h:232:
std::vector<base::Value*>* exceptions);
On 2015/07/24 10:57:32, msramek wrote:
> On 2015/07/24 09:13:25, Bernhard Bauer wrote:
> > Use ScopedVector instead of a vector containing raw pointers. Or actually,
why
> > doesn't this return a base::ListValue?
> 
> I agree that base::ListValue is much better than std::vector<base::Value>. I
was
> just trying to spare the conversion code, since the
> |GetExceptionsFromHostContentSettingsMap|, the only callsite, works internally
> with std::vector<base::Value>.

You could make |all_provider_exceptions| a ScopedVector<base::ListValue> to
avoid those conversions.

https://codereview.chromium.org/1251543002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.h (right):

https://codereview.chromium.org/1251543002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.h:230: base::ListValue*
GetPolicyAllowedUrls(ContentSettingsType type);
Return this as scoped_ptr?

[msramek, 2015-07-28 15:05:38]

No worries, the two dependencies aren't landed yet, so there's no hurry with
this CL.

https://codereview.chromium.org/1251543002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.h (right):

https://codereview.chromium.org/1251543002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.h:232:
std::vector<base::Value*>* exceptions);
On 2015/07/28 13:06:48, Bernhard Bauer wrote:
> On 2015/07/24 10:57:32, msramek wrote:
> > On 2015/07/24 09:13:25, Bernhard Bauer wrote:
> > > Use ScopedVector instead of a vector containing raw pointers. Or actually,
> why
> > > doesn't this return a base::ListValue?
> > 
> > I agree that base::ListValue is much better than std::vector<base::Value>. I
> was
> > just trying to spare the conversion code, since the
> > |GetExceptionsFromHostContentSettingsMap|, the only callsite, works
internally
> > with std::vector<base::Value>.
> 
> You could make |all_provider_exceptions| a ScopedVector<base::ListValue> to
> avoid those conversions.

Done. Although I'm not sure that it increased readability in the end.

https://codereview.chromium.org/1251543002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.h (right):

https://codereview.chromium.org/1251543002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.h:230: base::ListValue*
GetPolicyAllowedUrls(ContentSettingsType type);
On 2015/07/28 13:06:49, Bernhard Bauer wrote:
> Return this as scoped_ptr?

Done. Also change GetExceptionForPage() et al. to be consistent.

[Bernhard Bauer, 2015-07-28 15:35:31]

https://codereview.chromium.org/1251543002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/1251543002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:1208:
all_provider_exceptions.get().at(
Could you just use the [] operator? The bounds checking that at() does might be
disabled in our builds anyway (because we don't support exceptions), and you
could in any case do it yourself.

https://codereview.chromium.org/1251543002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:1244:
policy_exceptions->Swap(GetPolicyAllowedUrls(type).release());
Doesn't this leak the ListValue object that is returned by GetPolicyAllowedUrls
(which is now empty, but still a heap-allocated object)?

https://codereview.chromium.org/1251543002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:1247: for (auto&
one_provider_exceptions : all_provider_exceptions) {
Can you use const auto& here?

https://codereview.chromium.org/1251543002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:1249:
exceptions->Append(exception->DeepCopy());
Hm... it's a pity that ListValue doesn't support releasing its contents. What
you could do to avoid the DeepCopy is call Remove() with index 0 repeatedly.

[msramek, 2015-07-29 13:07:09]

https://codereview.chromium.org/1251543002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/1251543002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:1208:
all_provider_exceptions.get().at(
On 2015/07/28 15:35:31, Bernhard Bauer wrote:
> Could you just use the [] operator? The bounds checking that at() does might
be
> disabled in our builds anyway (because we don't support exceptions), and you
> could in any case do it yourself.

Done. Since GetProviderTypeFromSource returns ProviderType and we allocate
NUM_PROVIDER_TYPES, the only thing to check is an invalid source argument, and
that will be handled by the NOTREACHED in GetProviderTypeFromSource.

https://codereview.chromium.org/1251543002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:1244:
policy_exceptions->Swap(GetPolicyAllowedUrls(type).release());
On 2015/07/28 15:35:31, Bernhard Bauer wrote:
> Doesn't this leak the ListValue object that is returned by
GetPolicyAllowedUrls
> (which is now empty, but still a heap-allocated object)?

Good catch!

https://codereview.chromium.org/1251543002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:1247: for (auto&
one_provider_exceptions : all_provider_exceptions) {
On 2015/07/28 15:35:31, Bernhard Bauer wrote:
> Can you use const auto& here?

Done.

https://codereview.chromium.org/1251543002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:1249:
exceptions->Append(exception->DeepCopy());
On 2015/07/28 15:35:31, Bernhard Bauer wrote:
> Hm... it's a pity that ListValue doesn't support releasing its contents. What
> you could do to avoid the DeepCopy is call Remove() with index 0 repeatedly.

But ListValue uses std::vector (not std::list) under the hood, so that would be
quadratic.

Alternatively, we can Remove objects from the back, which will be linear, but
then we'll have to push them to a vector or stack to reverse them, and push them
back to |exceptions|. But that seems to me like a lot of boilerplate code for a
small performance gain.

So a third idea - don't reverse the exceptions while processing them, then
explain the reversion when appending (implemented in the last patchset).

[Bernhard Bauer, 2015-07-29 13:19:24]

LGTM!

https://codereview.chromium.org/1251543002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/1251543002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:1249:
exceptions->Append(exception->DeepCopy());
On 2015/07/29 13:07:09, msramek wrote:
> On 2015/07/28 15:35:31, Bernhard Bauer wrote:
> > Hm... it's a pity that ListValue doesn't support releasing its contents.
What
> > you could do to avoid the DeepCopy is call Remove() with index 0 repeatedly.
> 
> But ListValue uses std::vector (not std::list) under the hood, so that would
be
> quadratic.
> 
> Alternatively, we can Remove objects from the back, which will be linear, but
> then we'll have to push them to a vector or stack to reverse them, and push
them
> back to |exceptions|. But that seems to me like a lot of boilerplate code for
a
> small performance gain.
> 
> So a third idea - don't reverse the exceptions while processing them, then
> explain the reversion when appending (implemented in the last patchset).

Heh. I like that ☺

[msramek, 2015-07-29 14:06:36]

Thanks :)

[raymes, 2015-07-29 23:36:44]

btw I defer to Bernhard again :)

[msramek, 2015-07-30 09:09:20]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-07-30 09:09:21]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1251543002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-07-30 09:09:26]

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

[commit-bot: I haz the power, 2015-07-30 10:37:32]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-07-30 10:38:03]

Patchset 6 (id:??) landed as
https://crrev.com/957baf4c0d8985666ff817dd67c5c31cbb186375
Cr-Commit-Position: refs/heads/master@{#341093}