Content settings extension API

Content settings extension API

BUG=71067
TEST=

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

[Bernhard Bauer, 2011-06-03 23:38:27]

Please review.

Note that we don't allow persistent incognito content settings yet, as that will
most likely require some refactoring of the HostContentSettingsMap first.

Also, some of the things discussed in the extension API review aren't
implemented yet (pattern format, names of the embedded/main-frame/what-have-you
URLs and patterns), as it's not fully clear yet what we will do there.

[battre, 2011-06-06 23:45:12]

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:152: ContentSetting
setting);
Nit: Given the name of the function, change the order of parameters?

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_content_settings_api.cc (right):

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:26:
args_->GetString(0, &content_type_str));
nit: single line?

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:31: DictionaryValue*
details;
nit: initialize with NULL

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:54: error_ = "Can't
modify regular settings from an incognito context.";
nit: export this like keys::kInvalidUrlError?

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:69:
args_->GetString(0, &content_type_str));
nit: single line?

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:124: "when no
incognito window is open.";
nit: export error message?

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:134: //
TODO(jochen): Do we return the value for setting or for reading cookies?
add an optional parameter?

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:155:
args_->GetString(0, &content_type_str));
nit: single line?

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:175:
DictionaryValue* embedded_pattern_dict;
nit: initialize with NULL

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:214: scope !=
extension_prefs_scope::kIncognitoPersistent);
So we do not want to allow setting default ContentSettings for incognito windows
(e.g. no Flash/Java in my incognito sessions)?

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:229: error_ = "Can't
modify regular settings from an incognito context.";
nit: externalize error message?

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:242:
store->SetExtensionContentSetting(extension_id(), top_level_pattern,
no distinction between setting and reading cookies here?

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_preference_api_constants.cc (right):

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api_constants.cc:24: }
nit: // namespace extension_preference_api_constants

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:55: "inIncognitoContext": {
Can we change this? This is exposed to extensions. I don't know whether it is
documented anywhere.

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5682: "description": {
This looks wired. Does it mean that if I want to block something, I need to pass
a human readable description of the resource to Chrome?

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5695: "description": "The
pattern string. The pattern should never be constructed directly, but should
always be derived from an URL using the methods in the
<var>contentSettings.patterns</var> module."
How about making this an "any" or "object" instead of a string in order to not
encourage the developer to guess the format? Just an idea.

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5695: "description": "The
pattern string. The pattern should never be constructed directly, but should
always be derived from an URL using the methods in the
<var>contentSettings.patterns</var> module."
I could not find contentSettings.patterns in extension_api.json. Is this
documented/implemented? The browser test just passes "*" w/o calling
contentSettings.patterns.

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5718: "description": "The
setting applied by this rule."
add description to look at concrete content setting for allowed values?

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5789: "type": "any",
same here?

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5821: "type": "any",
same here?

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/do...
File chrome/common/extensions/docs/examples/api/contentSettings/manifest.json
(right):

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/do...
chrome/common/extensions/docs/examples/api/contentSettings/manifest.json:7:
"default_icon": "penguin.jpg",
do we have the copyright for this image?

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/do...
File chrome/common/extensions/docs/examples/api/contentSettings/popup.html
(right):

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/do...
chrome/common/extensions/docs/examples/api/contentSettings/popup.html:55:
<table>
Do this properly with a
<fieldset>
  <legend>Content Settings</legend>
  <dl>
    <dt><label for="cookies">Cookies</label></dt>
    <dd><select id="cookies" onchange="...">...</select>
    <dt><label for="images">...
?

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/do...
chrome/common/extensions/docs/examples/api/contentSettings/popup.html:64:
</select></li>
This </li> is not correct, also some more cases below

http://codereview.chromium.org/7029031/diff/31001/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/7029031/diff/31001/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:350: function
ChromeSetting(prefKey, valueSchema) {
nit: start with lowercase?

http://codereview.chromium.org/7029031/diff/31001/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:381: function
ContentSetting(content_type, settingSchema) {
same here?

[Bernhard Bauer, 2011-06-07 13:48:13]

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:152: ContentSetting
setting);
On 2011/06/06 23:45:13, battre wrote:
> Nit: Given the name of the function, change the order of parameters?

Done.

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_content_settings_api.cc (right):

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:26:
args_->GetString(0, &content_type_str));
On 2011/06/06 23:45:13, battre wrote:
> nit: single line?

Done.

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:31: DictionaryValue*
details;
On 2011/06/06 23:45:13, battre wrote:
> nit: initialize with NULL

Done.

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:54: error_ = "Can't
modify regular settings from an incognito context.";
On 2011/06/06 23:45:13, battre wrote:
> nit: export this like keys::kInvalidUrlError?

Done.

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:69:
args_->GetString(0, &content_type_str));
On 2011/06/06 23:45:13, battre wrote:
> nit: single line?

Done.

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:124: "when no
incognito window is open.";
On 2011/06/06 23:45:13, battre wrote:
> nit: export error message?

Done.

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:134: //
TODO(jochen): Do we return the value for setting or for reading cookies?
On 2011/06/06 23:45:13, battre wrote:
> add an optional parameter?

Hm. I'm not sure adding an additional parameter for cookies would be easy to
implement in a generic ContentSetting object. Maybe we could use the resource
identifier.

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:155:
args_->GetString(0, &content_type_str));
On 2011/06/06 23:45:13, battre wrote:
> nit: single line?

Done.

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:175:
DictionaryValue* embedded_pattern_dict;
On 2011/06/06 23:45:13, battre wrote:
> nit: initialize with NULL

Done.

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:214: scope !=
extension_prefs_scope::kIncognitoPersistent);
On 2011/06/06 23:45:13, battre wrote:
> So we do not want to allow setting default ContentSettings for incognito
windows
> (e.g. no Flash/Java in my incognito sessions)?

Not yet. Supporting this requires the incognito HostContentSettingsMap to exist
independent of the OffTheRecordProfile, which depends on some (already planned)
refactoring.

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:229: error_ = "Can't
modify regular settings from an incognito context.";
On 2011/06/06 23:45:13, battre wrote:
> nit: externalize error message?

Done.

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:242:
store->SetExtensionContentSetting(extension_id(), top_level_pattern,
On 2011/06/06 23:45:13, battre wrote:
> no distinction between setting and reading cookies here?

For getting the content setting, HostContentSettingsMap requires the flag,
because the behavior w/r/t third-party cookie blocking depends on it (we block
setting third-party cookies, not reading them), but setting a content setting
only changes the entry in the HostContentSettingsMap. We'll probably need to
think about how to properly integrate third-party cookie blocking with content
settings based on two URLs.

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_preference_api_constants.cc (right):

http://codereview.chromium.org/7029031/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api_constants.cc:24: }
On 2011/06/06 23:45:13, battre wrote:
> nit: // namespace extension_preference_api_constants

Done.

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:55: "inIncognitoContext": {
On 2011/06/06 23:45:13, battre wrote:
> Can we change this? This is exposed to extensions. I don't know whether it is
> documented anywhere.

This reflects the actual interface: chrome.InitExtension sets this as a boolean,
not an object, we just never validated it against the way it's declared in the
schema here.

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5682: "description": {
On 2011/06/06 23:45:13, battre wrote:
> This looks wired. Does it mean that if I want to block something, I need to
pass
> a human readable description of the resource to Chrome?

You're supposed to get the resource identifier from an additional (not yet
added) API method.

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5695: "description": "The
pattern string. The pattern should never be constructed directly, but should
always be derived from an URL using the methods in the
<var>contentSettings.patterns</var> module."
On 2011/06/06 23:45:13, battre wrote:
> I could not find contentSettings.patterns in extension_api.json. Is this
> documented/implemented? The browser test just passes "*" w/o calling
> contentSettings.patterns.

These methods aren't implemented yet.

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5695: "description": "The
pattern string. The pattern should never be constructed directly, but should
always be derived from an URL using the methods in the
<var>contentSettings.patterns</var> module."
On 2011/06/06 23:45:13, battre wrote:
> How about making this an "any" or "object" instead of a string in order to not
> encourage the developer to guess the format? Just an idea.

OTOH, "string" allows us to verify that the extension really passes a string,
otherwise we might get problems extracting the information on the browser side.

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5718: "description": "The
setting applied by this rule."
On 2011/06/06 23:45:13, battre wrote:
> add description to look at concrete content setting for allowed values?

Done.

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/do...
File chrome/common/extensions/docs/examples/api/contentSettings/manifest.json
(right):

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/do...
chrome/common/extensions/docs/examples/api/contentSettings/manifest.json:7:
"default_icon": "penguin.jpg",
On 2011/06/06 23:45:13, battre wrote:
> do we have the copyright for this image?

Given the number of times this image has appeared on the internet, I wouldn't
really be worried about copyright issues, but yeah, I put together another icon.

I still liked the socially awkward penguin better, though :-p

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/do...
File chrome/common/extensions/docs/examples/api/contentSettings/popup.html
(right):

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/do...
chrome/common/extensions/docs/examples/api/contentSettings/popup.html:55:
<table>
On 2011/06/06 23:45:13, battre wrote:
> Do this properly with a
> <fieldset>
>   <legend>Content Settings</legend>
>   <dl>
>     <dt><label for="cookies">Cookies</label></dt>
>     <dd><select id="cookies" onchange="...">...</select>
>     <dt><label for="images">...
> ?

Done.

http://codereview.chromium.org/7029031/diff/31001/chrome/common/extensions/do...
chrome/common/extensions/docs/examples/api/contentSettings/popup.html:64:
</select></li>
On 2011/06/06 23:45:13, battre wrote:
> This </li> is not correct, also some more cases below

Whoops. Done.

http://codereview.chromium.org/7029031/diff/31001/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/7029031/diff/31001/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:350: function
ChromeSetting(prefKey, valueSchema) {
On 2011/06/06 23:45:13, battre wrote:
> nit: start with lowercase?

It's supposed to be the constructor.

[battre, 2011-06-07 18:16:14]

LGTM

http://codereview.chromium.org/7029031/diff/34032/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_content_settings_api.cc (right):

http://codereview.chromium.org/7029031/diff/34032/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:192: }
nit: newline

http://codereview.chromium.org/7029031/diff/34032/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:202:
extension_prefs_scope::Scope scope = extension_prefs_scope::kRegular;
Please note that the extension_prefs_scope is gone on ToT

http://codereview.chromium.org/7029031/diff/34032/chrome/common/extensions/do...
File chrome/common/extensions/docs/examples/api/contentSettings/popup.html
(right):

http://codereview.chromium.org/7029031/diff/34032/chrome/common/extensions/do...
chrome/common/extensions/docs/examples/api/contentSettings/popup.html:67:
<dt><label for="cookies">Cookies: </label></dt><dd><select id="cookies"
onchange="settingChanged(this);">
nit: split before <dd> to have <80 character lines? (also below)

[Bernhard Bauer, 2011-06-08 14:20:15]

http://codereview.chromium.org/7029031/diff/34032/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_content_settings_api.cc (right):

http://codereview.chromium.org/7029031/diff/34032/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:192: }
On 2011/06/07 18:16:14, battre wrote:
> nit: newline

Done.

http://codereview.chromium.org/7029031/diff/34032/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:202:
extension_prefs_scope::Scope scope = extension_prefs_scope::kRegular;
On 2011/06/07 18:16:14, battre wrote:
> Please note that the extension_prefs_scope is gone on ToT

Yup, rebasing.

http://codereview.chromium.org/7029031/diff/34032/chrome/common/extensions/do...
File chrome/common/extensions/docs/examples/api/contentSettings/popup.html
(right):

http://codereview.chromium.org/7029031/diff/34032/chrome/common/extensions/do...
chrome/common/extensions/docs/examples/api/contentSettings/popup.html:67:
<dt><label for="cookies">Cookies: </label></dt><dd><select id="cookies"
onchange="settingChanged(this);">
On 2011/06/07 18:16:14, battre wrote:
> nit: split before <dd> to have <80 character lines? (also below)

Done.

[Bernhard Bauer, 2011-06-08 14:26:13]

+mpcomplete, erikkay for API changes (note that this still uses the content
settings pattern format and topLevelUrl/embeddedUrl, as we haven't decided what
exactly to do there).

[Matt Perry, 2011-06-08 19:30:18]

api.json LGTM

http://codereview.chromium.org/7029031/diff/40038/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7029031/diff/40038/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5844: "global": {
Grouping these settings this way feels weird to me. Any reason not to just have
the 3 properties below at the top level?

http://codereview.chromium.org/7029031/diff/40038/chrome/renderer/resources/e...
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/7029031/diff/40038/chrome/renderer/resources/e...
chrome/renderer/resources/event_bindings.js:184: "': " + e.stack);
we should print the error message as well. does 'stack' include that?

http://codereview.chromium.org/7029031/diff/40038/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/7029031/diff/40038/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:381: function
ContentSetting(content_type, settingSchema) {
use javascriptStyle

[Bernhard Bauer, 2011-06-09 13:57:48]

http://codereview.chromium.org/7029031/diff/40038/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7029031/diff/40038/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5844: "global": {
On 2011/06/08 19:30:18, Matt Perry wrote:
> Grouping these settings this way feels weird to me. Any reason not to just
have
> the 3 properties below at the top level?

I think that having them at the top level would be more confusing, as they have
slightly different interfaces (conceptually, they're somewhat like global
content settings, hence the name).

http://codereview.chromium.org/7029031/diff/40038/chrome/renderer/resources/e...
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/7029031/diff/40038/chrome/renderer/resources/e...
chrome/renderer/resources/event_bindings.js:184: "': " + e.stack);
On 2011/06/08 19:30:18, Matt Perry wrote:
> we should print the error message as well. does 'stack' include that?

Yes, that's the topmost part.

http://codereview.chromium.org/7029031/diff/40038/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/7029031/diff/40038/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:381: function
ContentSetting(content_type, settingSchema) {
On 2011/06/08 19:30:18, Matt Perry wrote:
> use javascriptStyle

Done.

[Matt Perry, 2011-06-09 20:57:53]

http://codereview.chromium.org/7029031/diff/40038/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7029031/diff/40038/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5844: "global": {
On 2011/06/09 13:57:49, Bernhard Bauer wrote:
> On 2011/06/08 19:30:18, Matt Perry wrote:
> > Grouping these settings this way feels weird to me. Any reason not to just
> have
> > the 3 properties below at the top level?
> 
> I think that having them at the top level would be more confusing, as they
have
> slightly different interfaces (conceptually, they're somewhat like global
> content settings, hence the name).

I'm not convinced that having a different interface warrants them being in a
different namespace. We have different interfaces for all functions in every
other module. What makes this module different?

[Bernhard Bauer, 2011-06-11 16:16:19]

On Thursday, June 9, 2011,  <mpcomplete@chromium.org> wrote:
>
>
http://codereview.chromium.org/7029031/diff/40038/chrome/common/extensions/ap...
> File chrome/common/extensions/api/extension_api.json (right):
>
>
http://codereview.chromium.org/7029031/diff/40038/chrome/common/extensions/ap...
> chrome/common/extensions/api/extension_api.json:5844: "global": {
> On 2011/06/09 13:57:49, Bernhard Bauer wrote:
>
> On 2011/06/08 19:30:18, Matt Perry wrote:
>> Grouping these settings this way feels weird to me. Any reason not
>
> to just
>
> have
>> the 3 properties below at the top level?
>
>
>
> I think that having them at the top level would be more confusing, as
>
> they have
>
> slightly different interfaces (conceptually, they're somewhat like
>
> global
>
> content settings, hence the name).
>
>
> I'm not convinced that having a different interface warrants them being
> in a different namespace. We have different interfaces for all functions
> in every other module. What makes this module different?

Sure, we can have them top-level. I just wanted to stick to what we
discussed in the extension API review for the initial version.

> http://codereview.chromium.org/7029031/
>

[Matt Perry, 2011-06-13 17:49:22]

On 2011/06/11 16:16:19, Bernhard Bauer wrote:
> On Thursday, June 9, 2011,  <mailto:mpcomplete@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/7029031/diff/40038/chrome/common/extensions/ap...
> > File chrome/common/extensions/api/extension_api.json (right):
> >
> >
>
http://codereview.chromium.org/7029031/diff/40038/chrome/common/extensions/ap...
> > chrome/common/extensions/api/extension_api.json:5844: "global": {
> > On 2011/06/09 13:57:49, Bernhard Bauer wrote:
> >
> > On 2011/06/08 19:30:18, Matt Perry wrote:
> >> Grouping these settings this way feels weird to me. Any reason not
> >
> > to just
> >
> > have
> >> the 3 properties below at the top level?
> >
> >
> >
> > I think that having them at the top level would be more confusing, as
> >
> > they have
> >
> > slightly different interfaces (conceptually, they're somewhat like
> >
> > global
> >
> > content settings, hence the name).
> >
> >
> > I'm not convinced that having a different interface warrants them being
> > in a different namespace. We have different interfaces for all functions
> > in every other module. What makes this module different?
> 
> Sure, we can have them top-level. I just wanted to stick to what we
> discussed in the extension API review for the initial version.

Oh OK, I don't remember covering that in the API review. If we agreed on it in
the meeting, then let's leave it as is.