Implement experimental.contentSettings.misc.blockThirdPartyCookies.

Implement experimental.contentSettings.misc.blockThirdPartyCookies.

This adds a Preference class to the extension APIs that can be used for access to other preferences in the future.

BUG=71067
TEST=ExtensionApiTest.ContentSettings:ExtensionApiTest.IncognitoContentSettings

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

[Bernhard Bauer, 2011-02-10 18:28:48]

Please review.

This is what we discussed a couple of days ago via email.

[battre, 2011-02-11 13:49:02]

+mpcomplete due to our discussions on the incognito parameter.

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function_dispatcher.cc:19: #include
"chrome/browser/extensions/extensions_quota_service.h"
Lint disagrees with your include order.

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:44:
EXTENSION_FUNCTION_VALIDATE(incognito_value->GetAsBoolean(&incognito));
maybe
if (details->HasKey("incognito"))
  EXTENSION_FUNCTION_VALIDATE(details->GetBoolean("incognito", &incognito));
?

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:71:
EXTENSION_FUNCTION_VALIDATE(details->Get("value", &value));
make value a full parameter of the function? That way the details dictionary can
be the same for GetPreferenceFunction and SetPreferenceFunction and the code
might look cleaner:
SetPreference("key", "value", {});

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:76:
EXTENSION_FUNCTION_VALIDATE(incognito_value->GetAsBoolean(&incognito));
see above

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:80: if (!browser_pref)
insert NOTREACHED(); as above?

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.h (right):

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.h:13:
~GetPreferenceFunction();
virtual?

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.h:20:
~SetPreferenceFunction();
virtual?

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1262: << "Extension controlled
preference " << pref_key << " has wrong type";
Nit: . before last "?

http://codereview.chromium.org/6480033/diff/4001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6480033/diff/4001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:4604: "parameters": []
"description": "Callback to confirm that setting the preference has been
executed."
?

[Bernhard Bauer, 2011-02-11 15:27:14]

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function_dispatcher.cc:19: #include
"chrome/browser/extensions/extensions_quota_service.h"
On 2011/02/11 13:49:02, battre wrote:
> Lint disagrees with your include order.

Then I guess lint has a different notion of lexicographical ordering than I (or
`sort') have.

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:71:
EXTENSION_FUNCTION_VALIDATE(details->Get("value", &value));
On 2011/02/11 13:49:02, battre wrote:
> make value a full parameter of the function? That way the details dictionary
can
> be the same for GetPreferenceFunction and SetPreferenceFunction and the code
> might look cleaner:
> SetPreference("key", "value", {});

Hm. I tried to keep as close to the original spec as possible so far (I just
changed |{get,set}BlockThirdPartyCookies| to |thirdPartyCookies.{get,set}|).
Changing the interface is likely going to be a separate discussion.

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:80: if (!browser_pref)
On 2011/02/11 13:49:02, battre wrote:
> insert NOTREACHED(); as above?

I've rewritten the checking code quite a bit. It should now correctly
distinguish between malice and stupidity on part of an extension (trying to
access a non-whitelisted preference vs. trying to access a preference without
the right permission), and an internal error.

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.h (right):

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.h:13:
~GetPreferenceFunction();
On 2011/02/11 13:49:02, battre wrote:
> virtual?

Done. I had already made that change, but hadn't uploaded it.

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1262: << "Extension controlled
preference " << pref_key << " has wrong type";
On 2011/02/11 13:49:02, battre wrote:
> Nit: . before last "?

Done.

http://codereview.chromium.org/6480033/diff/4001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6480033/diff/4001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:4604: "parameters": []
On 2011/02/11 13:49:02, battre wrote:
> "description": "Callback to confirm that setting the preference has been
> executed."
> ?

Done, I tried to keep the text similar to most of the other callback
descriptions.

[Bernhard Bauer, 2011-02-11 15:35:13]

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/6480033/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function_dispatcher.cc:19: #include
"chrome/browser/extensions/extensions_quota_service.h"
On 2011/02/11 15:27:15, Bernhard Bauer wrote:
> On 2011/02/11 13:49:02, battre wrote:
> > Lint disagrees with your include order.
> 
> Then I guess lint has a different notion of lexicographical ordering than I
(or
> `sort') have.

It seems my sort was messed up. Done.

[Aaron Boodman, 2011-02-11 18:33:18]

Cool, I really like how clean the browser process side of this turns out. We
just need to clean up the renderer side to be more general, so that other APIs
can make use of this in the future.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_dispatcher.cc:10: #include
"base/ref_counted.h"
thanks for the cleanup here.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:25:
Extension::kContentSettingsPermission
Can you give an example of a pref that would have a different permission value?
This might be early generalization.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:32: for (size_t i = 0; i <
arraysize(pref_mapping); ++i) {
Suggest starting out with this in a map. Even though it is (microscopically)
slower for small numbers of items, it better represents what's actually going
on.

You can find examples of how to have static maps in various places in the code
base. Off the top of my head I believe there is one in extension.cc. Basically
it involves having a struct like StaticData that you use base::Singleton with.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:42: const char
kInvalidPreferenceErrorMessage[] = "Invalid preference '%s'.";
Not used?

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:54: DictionaryValue*
details;
Nit: initialize primitives. Multiple places in this file.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:59:
EXTENSION_FUNCTION_VALIDATE(incognito_value->GetAsBoolean(&incognito));
Nit: I think this is more clear with:

if (details->HasKey("incognito"))
  EXTENSION_FUNCTION_VALIDATE(details->GetBoolean(...))

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:62: :
profile_->GetPrefs();
This seems a bit weird. How do these prefs work in incognito? Don't they fall
through to the default profile normally?

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:73: DCHECK(pref);
Don't be afraid. If this really can't happen, use CHECK.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:74:
result_.reset(pref->GetValue()->DeepCopy());
Is GetValue() always a fundamental Value? Maybe you should CHECK that.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:106:
EXTENSION_FUNCTION_VALIDATE(value->GetType == pref->GetType());
Syntax error? GetType() ?

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1256: #ifndef NDEBUG
I think you should do this in all situations, not just debug.

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

http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4573: "type": "any"
Can it really be any? I think maybe it can only be string, number, or boolean?
If you make this more strict, the extension system will add extra checking in
debug mode to help us catch situations where the C++ returns the wrong type.

It also helps a bit with the documentation.

http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4618: "description": "Whether
third party cookies should be blocked."
If you go with this system of reusing the Preference object, you should specify
in each usage what type the preference wants (eg boolean).

http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:226: { kContentSettingsPermission,
IDS_EXTENSION_PROMPT_WARNING_CONTENT_SETTINGS },
I am not sure if content settings should show up in the permission dialog. It
doesn't seem like a scary enough permission to warrant that.

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

http://codereview.chromium.org/6480033/diff/10006/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:378: function
setupPreferences() {
You are now the second person (along with mpcomplete's webRequest API) to want
stateful objects on the client side of extension bindings.

We should come up with some generalized mechanism for implementing this; ideally
with the object's implementation in some other file than
extension_process_bindings.js.

[Bernhard Bauer, 2011-02-13 18:19:12]

Thanks for the review!

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:25:
Extension::kContentSettingsPermission
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> Can you give an example of a pref that would have a different permission
value?
> This might be early generalization.

Proxy settings, for example. We have to do the permission check for content
settings here, as any extension has access to the experimental.contentSettings
object, and I want to keep this part as general as possible, to make it easier
for additional preferences.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:32: for (size_t i = 0; i <
arraysize(pref_mapping); ++i) {
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> Suggest starting out with this in a map. Even though it is (microscopically)
> slower for small numbers of items, it better represents what's actually going
> on.
> 
> You can find examples of how to have static maps in various places in the code
> base. Off the top of my head I believe there is one in extension.cc. Basically
> it involves having a struct like StaticData that you use base::Singleton with.

Okay, done.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:42: const char
kInvalidPreferenceErrorMessage[] = "Invalid preference '%s'.";
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> Not used?

Right, leftover from a previous version.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:54: DictionaryValue*
details;
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> Nit: initialize primitives. Multiple places in this file.

Done.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:59:
EXTENSION_FUNCTION_VALIDATE(incognito_value->GetAsBoolean(&incognito));
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> Nit: I think this is more clear with:
> 
> if (details->HasKey("incognito"))
>   EXTENSION_FUNCTION_VALIDATE(details->GetBoolean(...))

Done.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:62: :
profile_->GetPrefs();
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> This seems a bit weird. How do these prefs work in incognito? Don't they fall
> through to the default profile normally?

We already have a separate thread going with mpcomplete about that :-)

In general, we do allow separate preferences for incognito mode now, only
falling back to the normal preferences if there is no value for the incognito
preferences (copy-on-write semantics, if you will).

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:73: DCHECK(pref);
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> Don't be afraid. If this really can't happen, use CHECK.

I thought if it really can't happen, we should use DCHECK? :D

The only way this can happen is if there is an invalid preference defined in the
pref mapping, which would be a) a programmer error and b) something we would
find very quickly in tests, so it shouldn't come up in a release build, and we
can leave out the check.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:74:
result_.reset(pref->GetValue()->DeepCopy());
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> Is GetValue() always a fundamental Value? Maybe you should CHECK that.

Does it have to be? In theory, we could return an arbitrary structured value,
no?

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:106:
EXTENSION_FUNCTION_VALIDATE(value->GetType == pref->GetType());
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> Syntax error? GetType() ?

Woah. Fixed.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1256: #ifndef NDEBUG
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> I think you should do this in all situations, not just debug.

So, also make them into |CHECK|s? Ideally, we'd already have checks before that
(like in extension_preference_api.cc), so this should only be necessary as an
additional precaution.

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

http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4573: "type": "any"
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> Can it really be any? I think maybe it can only be string, number, or boolean?
> If you make this more strict, the extension system will add extra checking in
> debug mode to help us catch situations where the C++ returns the wrong type.
> 
> It also helps a bit with the documentation.

The preference system itself allows any sort of |Value|. For a specific
preference, we'd have to list the type in its description.

http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4618: "description": "Whether
third party cookies should be blocked."
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> If you go with this system of reusing the Preference object, you should
specify
> in each usage what type the preference wants (eg boolean).

Done.

http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:226: { kContentSettingsPermission,
IDS_EXTENSION_PROMPT_WARNING_CONTENT_SETTINGS },
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> I am not sure if content settings should show up in the permission dialog. It
> doesn't seem like a scary enough permission to warrant that.

What would that mean for the permissions dialog if an extension wants content
setting permissions? Would you get no message at all?

[Aaron Boodman, 2011-02-14 02:54:01]

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:25:
Extension::kContentSettingsPermission
On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > Can you give an example of a pref that would have a different permission
> value?
> > This might be early generalization.
> 
> Proxy settings, for example. We have to do the permission check for content
> settings here, as any extension has access to the experimental.contentSettings
> object, and I want to keep this part as general as possible, to make it easier
> for additional preferences.

I'm sorry I don't understand. Permission checking is not usually done in the
modules -- it is generalized in extension.cc. Why is this check necessary here?

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:62: :
profile_->GetPrefs();
On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > This seems a bit weird. How do these prefs work in incognito? Don't they
fall
> > through to the default profile normally?
> 
> We already have a separate thread going with mpcomplete about that :-)
> 
> In general, we do allow separate preferences for incognito mode now, only
> falling back to the normal preferences if there is no value for the incognito
> preferences (copy-on-write semantics, if you will).

Should we only allow this if the extension is enabled for incognito mode?

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:73: DCHECK(pref);
On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > Don't be afraid. If this really can't happen, use CHECK.
> 
> I thought if it really can't happen, we should use DCHECK? :D

This is my personal preference, not Chrome policy. In my opinion code should use
CHECK except in cases where there is a significant performance penalty.

Reasons:

1. Many developers don't use debug builds for local development, so they won't
see this issue.

2. There are lots of invalid DCHECKs sitting in the codebase unfortunately, so
invalid DCHECKs are less likely to get attention.

3. Users don't run debug builds, so the assertions don't help us in the field.
Instead of getting an assertion sent to us, we get reports of weird behavior, or
crashes at some later point in the code.

> The only way this can happen is if there is an invalid preference defined in
the
> pref mapping, which would be a) a programmer error and b) something we would
> find very quickly in tests, so it shouldn't come up in a release build, and we
> can leave out the check.

I would make it a check then.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:74:
result_.reset(pref->GetValue()->DeepCopy());
On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > Is GetValue() always a fundamental Value? Maybe you should CHECK that.
> 
> Does it have to be? In theory, we could return an arbitrary structured value,
> no?

I suppose. Ok, n/m.

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1256: #ifndef NDEBUG
On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > I think you should do this in all situations, not just debug.
> 
> So, also make them into |CHECK|s? Ideally, we'd already have checks before
that
> (like in extension_preference_api.cc), so this should only be necessary as an
> additional precaution.

I see. Ok, I don't feel so strongly about this case.

http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:226: { kContentSettingsPermission,
IDS_EXTENSION_PROMPT_WARNING_CONTENT_SETTINGS },
On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > I am not sure if content settings should show up in the permission dialog.
It
> > doesn't seem like a scary enough permission to warrant that.
> 
> What would that mean for the permissions dialog if an extension wants content
> setting permissions? Would you get no message at all?

Yes, it would mean no message at all. But I don't think that is that big of a
deal here. Access to set these settings doesn't seem like that scary of a
privilege.

There are other permissions that fall into this category already, such as
unlimited storage, idle, etc.

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

http://codereview.chromium.org/6480033/diff/10006/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:378: function
setupPreferences() {
On 2011/02/11 18:33:19, Aaron Boodman wrote:
> You are now the second person (along with mpcomplete's webRequest API) to want
> stateful objects on the client side of extension bindings.
> 
> We should come up with some generalized mechanism for implementing this;
ideally
> with the object's implementation in some other file than
> extension_process_bindings.js.

Did you see this comment?

[Bernhard Bauer, 2011-02-14 10:52:07]

On Mon, Feb 14, 2011 at 03:54,  <aa@chromium.org> wrote:
>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_preference_api.cc (right):
>
>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_preference_api.cc:25:
> Extension::kContentSettingsPermission
> On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> > On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > > Can you give an example of a pref that would have a different permission
value?
> > > This might be early generalization.
>
> > Proxy settings, for example. We have to do the permission check for content
> > settings here, as any extension has access to the
experimental.contentSettings
> > object, and I want to keep this part as general as possible, to make it
easier
> > for additional preferences.
>
> I'm sorry I don't understand. Permission checking is not usually done in
> the modules -- it is generalized in extension.cc. Why is this check
> necessary here?

extension_process_bindings.cc checks the permissions, but only for the
function that is called (in this case
"experimental.preferences.{get,set}"), so without the additional
check, we couldn't allow finer-grained access to preferences (like
content settings access vs. proxy settings).

>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_preference_api.cc:62: :
> profile_->GetPrefs();
> On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> > On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > > This seems a bit weird. How do these prefs work in incognito? Don't they
fall
> > > through to the default profile normally?
>
> > We already have a separate thread going with mpcomplete about that :-)
>
> > In general, we do allow separate preferences for incognito mode now, only
> > falling back to the normal preferences if there is no value for the
incognito
> > preferences (copy-on-write semantics, if you will).
>
> Should we only allow this if the extension is enabled for incognito mode?

As I said, the jury is still out on this one ;-) Personally, I agree,
but I'd like to defer updating this code until we have reached a
consensus. I set up a meeting tomorrow to discuss this.

>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_preference_api.cc:73: DCHECK(pref);
> On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> > On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > > Don't be afraid. If this really can't happen, use CHECK.
>
> > I thought if it really can't happen, we should use DCHECK? :D
>
> This is my personal preference, not Chrome policy. In my opinion code should
use CHECK except in cases where there is a significant performance penalty.
>
> Reasons:
>
> 1. Many developers don't use debug builds for local development, so they won't
see this issue.
>
> 2. There are lots of invalid DCHECKs sitting in the codebase unfortunately, so
invalid DCHECKs are less likely to get attention.
>
> 3. Users don't run debug builds, so the assertions don't help us in the field.
Instead of getting an assertion sent to us, we get reports of weird behavior, or
crashes at some later point in the code.
>
> > The only way this can happen is if there is an invalid preference defined in
the
> > pref mapping, which would be a) a programmer error and b) something we would
> > find very quickly in tests, so it shouldn't come up in a release build, and
we
> > can leave out the check.
>
> I would make it a check then.

OK, done.

>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_preference_api.cc:74:
> result_.reset(pref->GetValue()->DeepCopy());
> On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> > On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > > Is GetValue() always a fundamental Value? Maybe you should CHECK that.
>
> > Does it have to be? In theory, we could return an arbitrary structured
value,
> > no?
>
> I suppose. Ok, n/m.
>
>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_prefs.cc (right):
>
>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_prefs.cc:1256: #ifndef NDEBUG
> On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> > On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > > I think you should do this in all situations, not just debug.
>
> > So, also make them into |CHECK|s? Ideally, we'd already have checks before
that
> > (like in extension_preference_api.cc), so this should only be necessary as
an
> > additional precaution.
>
> I see. Ok, I don't feel so strongly about this case.
>
>
http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ex...
> File chrome/common/extensions/extension.cc (right):
>
>
http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ex...
> chrome/common/extensions/extension.cc:226: { kContentSettingsPermission,
> IDS_EXTENSION_PROMPT_WARNING_CONTENT_SETTINGS },
> On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> > On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > > I am not sure if content settings should show up in the permission dialog.
It
> > > doesn't seem like a scary enough permission to warrant that.
>
> > What would that mean for the permissions dialog if an extension wants
content
> > setting permissions? Would you get no message at all?
>
> Yes, it would mean no message at all. But I don't think that is that big of a
deal here. Access to set these settings doesn't seem like that scary of a
privilege.
>
> There are other permissions that fall into this category already, such as
unlimited storage, idle, etc.

OK, done.

>
http://codereview.chromium.org/6480033/diff/10006/chrome/renderer/resources/e...
> File chrome/renderer/resources/extension_process_bindings.js (right):
>
>
http://codereview.chromium.org/6480033/diff/10006/chrome/renderer/resources/e...
> chrome/renderer/resources/extension_process_bindings.js:378: function
> setupPreferences() {
> On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > You are now the second person (along with mpcomplete's webRequest API) to
want
> > stateful objects on the client side of extension bindings.
>
> > We should come up with some generalized mechanism for implementing this;
ideally
> > with the object's implementation in some other file than
> > extension_process_bindings.js.
>
> Did you see this comment?

I did, but I don't know how to do this in a generalized way ;-) If you
have a more specific idea what to do, I'd be glad to get codin' :-)

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

[Aaron Boodman, 2011-02-14 15:58:31]

On 2011/02/14 10:52:07, Bernhard Bauer wrote:
> On Mon, Feb 14, 2011 at 03:54,  <mailto:aa@chromium.org> wrote:
> >
>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> > File chrome/browser/extensions/extension_preference_api.cc (right):
> >
> >
>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> > chrome/browser/extensions/extension_preference_api.cc:25:
> > Extension::kContentSettingsPermission
> > On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> > > On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > > > Can you give an example of a pref that would have a different permission
> value?
> > > > This might be early generalization.
> >
> > > Proxy settings, for example. We have to do the permission check for
content
> > > settings here, as any extension has access to the
> experimental.contentSettings
> > > object, and I want to keep this part as general as possible, to make it
> easier
> > > for additional preferences.
> >
> > I'm sorry I don't understand. Permission checking is not usually done in
> > the modules -- it is generalized in extension.cc. Why is this check
> > necessary here?
> 
> extension_process_bindings.cc checks the permissions, but only for the
> function that is called (in this case
> "experimental.preferences.{get,set}"), so without the additional
> check, we couldn't allow finer-grained access to preferences (like
> content settings access vs. proxy settings).
> 
> >
>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> > chrome/browser/extensions/extension_preference_api.cc:62: :
> > profile_->GetPrefs();
> > On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> > > On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > > > This seems a bit weird. How do these prefs work in incognito? Don't they
> fall
> > > > through to the default profile normally?
> >
> > > We already have a separate thread going with mpcomplete about that :-)
> >
> > > In general, we do allow separate preferences for incognito mode now, only
> > > falling back to the normal preferences if there is no value for the
> incognito
> > > preferences (copy-on-write semantics, if you will).
> >
> > Should we only allow this if the extension is enabled for incognito mode?
> 
> As I said, the jury is still out on this one ;-) Personally, I agree,
> but I'd like to defer updating this code until we have reached a
> consensus. I set up a meeting tomorrow to discuss this.
> 
> >
>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> > chrome/browser/extensions/extension_preference_api.cc:73: DCHECK(pref);
> > On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> > > On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > > > Don't be afraid. If this really can't happen, use CHECK.
> >
> > > I thought if it really can't happen, we should use DCHECK? :D
> >
> > This is my personal preference, not Chrome policy. In my opinion code should
> use CHECK except in cases where there is a significant performance penalty.
> >
> > Reasons:
> >
> > 1. Many developers don't use debug builds for local development, so they
won't
> see this issue.
> >
> > 2. There are lots of invalid DCHECKs sitting in the codebase unfortunately,
so
> invalid DCHECKs are less likely to get attention.
> >
> > 3. Users don't run debug builds, so the assertions don't help us in the
field.
> Instead of getting an assertion sent to us, we get reports of weird behavior,
or
> crashes at some later point in the code.
> >
> > > The only way this can happen is if there is an invalid preference defined
in
> the
> > > pref mapping, which would be a) a programmer error and b) something we
would
> > > find very quickly in tests, so it shouldn't come up in a release build,
and
> we
> > > can leave out the check.
> >
> > I would make it a check then.
> 
> OK, done.
> 
> >
>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> > chrome/browser/extensions/extension_preference_api.cc:74:
> > result_.reset(pref->GetValue()->DeepCopy());
> > On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> > > On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > > > Is GetValue() always a fundamental Value? Maybe you should CHECK that.
> >
> > > Does it have to be? In theory, we could return an arbitrary structured
> value,
> > > no?
> >
> > I suppose. Ok, n/m.
> >
> >
>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> > File chrome/browser/extensions/extension_prefs.cc (right):
> >
> >
>
http://codereview.chromium.org/6480033/diff/10006/chrome/browser/extensions/e...
> > chrome/browser/extensions/extension_prefs.cc:1256: #ifndef NDEBUG
> > On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> > > On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > > > I think you should do this in all situations, not just debug.
> >
> > > So, also make them into |CHECK|s? Ideally, we'd already have checks before
> that
> > > (like in extension_preference_api.cc), so this should only be necessary as
> an
> > > additional precaution.
> >
> > I see. Ok, I don't feel so strongly about this case.
> >
> >
>
http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ex...
> > File chrome/common/extensions/extension.cc (right):
> >
> >
>
http://codereview.chromium.org/6480033/diff/10006/chrome/common/extensions/ex...
> > chrome/common/extensions/extension.cc:226: { kContentSettingsPermission,
> > IDS_EXTENSION_PROMPT_WARNING_CONTENT_SETTINGS },
> > On 2011/02/13 18:19:12, Bernhard Bauer wrote:
> > > On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > > > I am not sure if content settings should show up in the permission
dialog.
> It
> > > > doesn't seem like a scary enough permission to warrant that.
> >
> > > What would that mean for the permissions dialog if an extension wants
> content
> > > setting permissions? Would you get no message at all?
> >
> > Yes, it would mean no message at all. But I don't think that is that big of
a
> deal here. Access to set these settings doesn't seem like that scary of a
> privilege.
> >
> > There are other permissions that fall into this category already, such as
> unlimited storage, idle, etc.
> 
> OK, done.
> 
> >
>
http://codereview.chromium.org/6480033/diff/10006/chrome/renderer/resources/e...
> > File chrome/renderer/resources/extension_process_bindings.js (right):
> >
> >
>
http://codereview.chromium.org/6480033/diff/10006/chrome/renderer/resources/e...
> > chrome/renderer/resources/extension_process_bindings.js:378: function
> > setupPreferences() {
> > On 2011/02/11 18:33:19, Aaron Boodman wrote:
> > > You are now the second person (along with mpcomplete's webRequest API) to
> want
> > > stateful objects on the client side of extension bindings.
> >
> > > We should come up with some generalized mechanism for implementing this;
> ideally
> > > with the object's implementation in some other file than
> > > extension_process_bindings.js.
> >
> > Did you see this comment?
> 
> I did, but I don't know how to do this in a generalized way ;-) If you
> have a more specific idea what to do, I'd be glad to get codin' :-)
> 
> > http://codereview.chromium.org/6480033/

Here is one idea:

1. Add custom_bindings:{file:"content_settings.js",
constructor:"ContentSettings"} to extension_api.json.
2. Add custom_settings.js to render_resources.grd.
3. When "custom_bindings" is present for a node in extension_api.json,
extension_process_bindings.js instantiates a CustomBindingsProxy object that is
defined in extension_process_bindings.js and passes it the node from
extension_api.json.
4. CustomBindingsProxy creates a method on itself for each of the properties
specified in extension_api.json.
5. On first access to one of those methods, CustomBindingsProxy calls a native
function that loads the JavaScript file from the resources, constructs the
"constructor" object, and returns it. The constructed object becomes a property
of CustomBindingsProxy object, maybe called impl_.
6. Each time a method on CustomBindingsProxy is called, CustomBindingsProxy
validates the signature, then passes the call onto the implementation.

We should take a look at all the custom bindings in
extenesion_process_bindings.js and see if this can replace them. There is
probably a bit richer API that needs to be defined between CustomBindingsProxy
and the implementation objects to meet all the use cases.

[Bernhard Bauer, 2011-02-14 18:15:30]

On Mon, Feb 14, 2011 at 16:58,  <aa@chromium.org> wrote:
> Here is one idea:
>
> 1. Add custom_bindings:{file:"content_settings.js",
> constructor:"ContentSettings"} to extension_api.json.
> 2. Add custom_settings.js to render_resources.grd.
> 3. When "custom_bindings" is present for a node in extension_api.json,
> extension_process_bindings.js instantiates a CustomBindingsProxy object that
> is
> defined in extension_process_bindings.js and passes it the node from
> extension_api.json.
> 4. CustomBindingsProxy creates a method on itself for each of the properties
> specified in extension_api.json.
> 5. On first access to one of those methods, CustomBindingsProxy calls a
> native
> function that loads the JavaScript file from the resources, constructs the
> "constructor" object, and returns it. The constructed object becomes a
> property
> of CustomBindingsProxy object, maybe called impl_.
> 6. Each time a method on CustomBindingsProxy is called, CustomBindingsProxy
> validates the signature, then passes the call onto the implementation.
>
> We should take a look at all the custom bindings in
> extenesion_process_bindings.js and see if this can replace them. There is
> probably a bit richer API that needs to be defined between
> CustomBindingsProxy
> and the implementation objects to meet all the use cases.

I see. That does not sound completely trivial though, especially if we
want to accomodate all current custom bindings. Can we defer that to a
later CL?

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

[Aaron Boodman, 2011-02-14 18:28:23]

On Mon, Feb 14, 2011 at 10:15 AM, Bernhard Bauer <bauerb@chromium.org> wrote:
> On Mon, Feb 14, 2011 at 16:58,  <aa@chromium.org> wrote:
>> Here is one idea:
>>
>> 1. Add custom_bindings:{file:"content_settings.js",
>> constructor:"ContentSettings"} to extension_api.json.
>> 2. Add custom_settings.js to render_resources.grd.
>> 3. When "custom_bindings" is present for a node in extension_api.json,
>> extension_process_bindings.js instantiates a CustomBindingsProxy object that
>> is
>> defined in extension_process_bindings.js and passes it the node from
>> extension_api.json.
>> 4. CustomBindingsProxy creates a method on itself for each of the properties
>> specified in extension_api.json.
>> 5. On first access to one of those methods, CustomBindingsProxy calls a
>> native
>> function that loads the JavaScript file from the resources, constructs the
>> "constructor" object, and returns it. The constructed object becomes a
>> property
>> of CustomBindingsProxy object, maybe called impl_.
>> 6. Each time a method on CustomBindingsProxy is called, CustomBindingsProxy
>> validates the signature, then passes the call onto the implementation.
>>
>> We should take a look at all the custom bindings in
>> extenesion_process_bindings.js and see if this can replace them. There is
>> probably a bit richer API that needs to be defined between
>> CustomBindingsProxy
>> and the implementation objects to meet all the use cases.
>
> I see. That does not sound completely trivial though, especially if we
> want to accomodate all current custom bindings. Can we defer that to a
> later CL?

My suggestion would be to implement the abstraction and use it for
your change in the first CL. There's no point checking in an
implementation that is going to change immediately.

Porting the existing custom bindings doesn't need to be done right
away, I just want to be relatively sure that we go in a direction
where they would be portable.

You might also be able to convince Matt to do the abstraction since he
needs the same feature and is familiar with this part of the code.

- a

[Bernhard Bauer, 2011-02-21 16:55:24]

Okay, please take a look at what I came up with. I didn't put the custom
bindings into a separate file, because they need access to sendRequest from 
extension_process_bindings.js. I also didn't go with proxying to an
implementation object, I'm rather calling an initialization method on the object
itself. That way,  afterwards the object is indistinguishable from a non-lazily
initialized object.

In any case, I'd like to expedite landing this CL, as Dominic already has three
other CLs that depend on it. So if there are still non-trivial issues with this
CL, I'd be very happy if we could land it without the custom bindings, just to
not block the others.

[Aaron Boodman, 2011-02-23 01:17:26]

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

http://codereview.chromium.org/6480033/diff/25002/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:438: var
extendedGetSchema = getSchema.slice();
Can you find a way to generalize this such that everyone who uses this pattern,
doesn't have to repeat these two lines?

http://codereview.chromium.org/6480033/diff/25002/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:492: if (t.type ==
'object' && t.customBindings) {
The customBindings flag seems unnecessary since you can just look in the lookup
of customBindings.

http://codereview.chromium.org/6480033/diff/25002/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:493: function
CustomBindingsObject(args) {
This object doesn't seem to do a lot right now. I realize it was partially based
on my ideas, but without the lazy-loading of the implementation, I don't see the
value.

How about just making there be a lookup of customBindings, eg:

var customBindings = {};

People can come along and register themselves as the implementation for a
certain type:

customBindings['Preference'] = function(schema) {
  this.schema_ = schema;
};

customBindings['Preference'].prototype.get = function() {
  chromeHidden.validate(arguments, this.schema.functions.get);
  sendRequest(...);
};

Then in the property parsing code, you create one of these directly.

http://codereview.chromium.org/6480033/diff/25002/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:581: var proto =
customObjects[property["$ref"]];
naming nit: I think this should really be 'ctor' or 'constructor' if that isn't
a reserved javascript word.

[Bernhard Bauer, 2011-02-23 17:33:27]

PTAL. I think it looks much cleaner now.

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

http://codereview.chromium.org/6480033/diff/25002/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:438: var
extendedGetSchema = getSchema.slice();
On 2011/02/23 01:17:26, Aaron Boodman wrote:
> Can you find a way to generalize this such that everyone who uses this
pattern,
> doesn't have to repeat these two lines?

Done. I pulled out a function |extendSchema|.

http://codereview.chromium.org/6480033/diff/25002/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:492: if (t.type ==
'object' && t.customBindings) {
On 2011/02/23 01:17:26, Aaron Boodman wrote:
> The customBindings flag seems unnecessary since you can just look in the
lookup
> of customBindings.

Done.

http://codereview.chromium.org/6480033/diff/25002/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:493: function
CustomBindingsObject(args) {
On 2011/02/23 01:17:26, Aaron Boodman wrote:
> This object doesn't seem to do a lot right now. I realize it was partially
based
> on my ideas, but without the lazy-loading of the implementation, I don't see
the
> value.
> 
> How about just making there be a lookup of customBindings, eg:
> 
> var customBindings = {};
> 
> People can come along and register themselves as the implementation for a
> certain type:
> 
> customBindings['Preference'] = function(schema) {
>   this.schema_ = schema;
> };
> 
> customBindings['Preference'].prototype.get = function() {
>   chromeHidden.validate(arguments, this.schema.functions.get);
>   sendRequest(...);
> };
> 
> Then in the property parsing code, you create one of these directly.

Done. I'm still using CustomBindingsObject as a base class that mangles the
method schemas into a dictionary. If you prefer, I could do that when parsing
the types below and directly set the schemas on the customBindings entry.

http://codereview.chromium.org/6480033/diff/25002/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:581: var proto =
customObjects[property["$ref"]];
On 2011/02/23 01:17:26, Aaron Boodman wrote:
> naming nit: I think this should really be 'ctor' or 'constructor' if that
isn't
> a reserved javascript word.

Done. I think constructor only has a special meaning as a property name.

[Aaron Boodman, 2011-02-23 18:10:23]

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

http://codereview.chromium.org/6480033/diff/30001/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:314: function
extendSchema(schema) {
What I meant was this: the schema is only passed into sendRequest so that the
callback can be extracted. It stinks to simulate a schema just to make that
work.

In prepareRequest is it safe to assume that if the last param is a function,
then it must be the callback? If so, then we can just not pass the schema into
sendRequest. If not, then perhaps we can refactor sendRequest so that the
callback can be sent explicitly.

Either way, not needing to send the schema into sendRequest would simplify this
code. There would be no need to store the hash of parameters, and no need for
the CustomBindingsObject base class as far as I can see.

http://codereview.chromium.org/6480033/diff/30001/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:505:
customBindings[t.id].prototype.setSchema(t);
I don't understand why this is needed. It seems like you can just create the
custom bindings and pass the schema into the constructor when the property is
parsed. It can call setSchema() in its constructor.

http://codereview.chromium.org/6480033/diff/30001/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:585:
constructor.apply(value, args);
Why not just: value = new constructor(value)?

[Aaron Boodman, 2011-02-23 18:46:14]

lg