ContentSettings now uses the JSON Schema Compiler.

ContentSettings now uses the JSON Schema Compiler.

BUG=121174
TEST=Old Tests Cover

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142763

[chebert, 2012-06-15 23:28:02]

https://chromiumcodereview.appspot.com/10553018/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://chromiumcodereview.appspot.com/10553018/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:43:
args->GetString(0, &content_type_str);
EXTENSION_FUNCTION_VALIDATE relies on a bad_message_ member variable, so I
removed it for this line. 

I'm not sure it is necessary in this case, since the ContentSettingsType param
gets added by the renderer.

https://chromiumcodereview.appspot.com/10553018/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:44:
args->Remove(0, NULL);
We need to remove the ContentSettingsType param since the compiled schema code
is expecting exactly 1 argument.

[battre, 2012-06-18 15:19:07]

LGTM

https://chromiumcodereview.appspot.com/10553018/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://chromiumcodereview.appspot.com/10553018/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:43:
args->GetString(0, &content_type_str);
On 2012/06/15 23:28:02, chebert wrote:
> EXTENSION_FUNCTION_VALIDATE relies on a bad_message_ member variable, so I
> removed it for this line. 
> 
> I'm not sure it is necessary in this case, since the ContentSettingsType param
> gets added by the renderer.

you could still return false here, right?

https://chromiumcodereview.appspot.com/10553018/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:44:
args->Remove(0, NULL);
On 2012/06/15 23:28:02, chebert wrote:
> We need to remove the ContentSettingsType param since the compiled schema code
> is expecting exactly 1 argument.

can you please make this a comment?

https://chromiumcodereview.appspot.com/10553018/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:66:
EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(1, &details));
This is not used any more.

[commit-bot: I haz the power, 2012-06-18 16:27:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/10553...

[commit-bot: I haz the power, 2012-06-18 16:27:52]

Presubmit check for 10553018-7001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/common/extensions/api

Add a description.

[not at google - send to devlin, 2012-06-18 16:48:57]

lgtm

https://chromiumcodereview.appspot.com/10553018/diff/7001/chrome/browser/exte...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://chromiumcodereview.appspot.com/10553018/diff/7001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:41: bool
ExtractContentType(ListValue* args, ContentSettingsType* content_type) {
nit: I actually think this should be called "RemoveContentType".

https://chromiumcodereview.appspot.com/10553018/diff/7001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:46: //
not expecting it.
nit: explain why... because it gets added by the renderer
(content_settings_custom_bindings.js) and isn't part of the JSON schema.

https://chromiumcodereview.appspot.com/10553018/diff/7001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:75: scope
== kExtensionPrefsScopeIncognitoSessionOnly);
Bit strange, "scope" can't possibly be kExtensionPrefsScopeIncognitoPersistent
because it's only either set to Regular or IncognitoSessionOnly above. Could you
rephrase stuff around here a bit?

Looks like this code is non-trivially different, but if battre says it's ok then
it's ok.

https://chromiumcodereview.appspot.com/10553018/diff/7001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:227:
scope == kExtensionPrefsScopeIncognitoSessionOnly);
same here

[battre, 2012-06-18 16:52:51]

https://chromiumcodereview.appspot.com/10553018/diff/7001/chrome/browser/exte...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://chromiumcodereview.appspot.com/10553018/diff/7001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:75: scope
== kExtensionPrefsScopeIncognitoSessionOnly);
On 2012/06/18 16:48:57, kalman wrote:
> Bit strange, "scope" can't possibly be kExtensionPrefsScopeIncognitoPersistent
> because it's only either set to Regular or IncognitoSessionOnly above. Could
you
> rephrase stuff around here a bit?
> 
> Looks like this code is non-trivially different, but if battre says it's ok
then
> it's ok.

Ah, that's a good point. This got simplified. We can delete the first case here.
-> I would add "bool incognito = false" after line 68 and assign it to false in
the body of the if statement.

[chebert, 2012-06-18 17:07:13]

https://chromiumcodereview.appspot.com/10553018/diff/7001/chrome/browser/exte...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://chromiumcodereview.appspot.com/10553018/diff/7001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:41: bool
ExtractContentType(ListValue* args, ContentSettingsType* content_type) {
On 2012/06/18 16:48:57, kalman wrote:
> nit: I actually think this should be called "RemoveContentType".

Done.

https://chromiumcodereview.appspot.com/10553018/diff/7001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:46: //
not expecting it.
On 2012/06/18 16:48:57, kalman wrote:
> nit: explain why... because it gets added by the renderer
> (content_settings_custom_bindings.js) and isn't part of the JSON schema.

Done.

https://chromiumcodereview.appspot.com/10553018/diff/7001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:75: scope
== kExtensionPrefsScopeIncognitoSessionOnly);
On 2012/06/18 16:48:57, kalman wrote:
> Bit strange, "scope" can't possibly be kExtensionPrefsScopeIncognitoPersistent
> because it's only either set to Regular or IncognitoSessionOnly above. Could
you
> rephrase stuff around here a bit?
> 
> Looks like this code is non-trivially different, but if battre says it's ok
then
> it's ok.

Done.

https://chromiumcodereview.appspot.com/10553018/diff/7001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:75: scope
== kExtensionPrefsScopeIncognitoSessionOnly);
On 2012/06/18 16:52:51, battre wrote:
> On 2012/06/18 16:48:57, kalman wrote:
> > Bit strange, "scope" can't possibly be
kExtensionPrefsScopeIncognitoPersistent
> > because it's only either set to Regular or IncognitoSessionOnly above. Could
> you
> > rephrase stuff around here a bit?
> > 
> > Looks like this code is non-trivially different, but if battre says it's ok
> then
> > it's ok.
> 
> Ah, that's a good point. This got simplified. We can delete the first case
here.
> -> I would add "bool incognito = false" after line 68 and assign it to false
in
> the body of the if statement.

Done.

https://chromiumcodereview.appspot.com/10553018/diff/7001/chrome/browser/exte...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:227:
scope == kExtensionPrefsScopeIncognitoSessionOnly);
On 2012/06/18 16:48:57, kalman wrote:
> same here

Done.

[commit-bot: I haz the power, 2012-06-18 17:07:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/10553...

[commit-bot: I haz the power, 2012-06-18 18:31:56]

Change committed as 142763