[Extensions Bindings] Add native declarativeContent verification

[Extensions Bindings] Add native declarativeContent verification

The declarativeContent API exposes constructors that initialize certain
types. These constructors (sometimes) take arguments that need to be
validated against the schema. Add native support for this validation by
allowing native hooks to modify the API binding's object template, and
using this to expose the constructor methods rather than relying on the
JS custom bindings.

Add additional declarativeContent-related tests and unittests for hooks
modifying the API binding template.

BUG=653596
Review-Url: https://codereview.chromium.org/2853023002
Cr-Commit-Position: refs/heads/master@{#469182}
Committed: https://chromium.googlesource.com/chromium/src/+/92d03c12fddf6214bcb74ec328a284f39eb2f452

[Devlin, 2017-05-01 17:15:26]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 17:16:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-05-01 19:27:10]

Patchset #3 (id:40001) has been deleted

[Devlin, 2017-05-01 19:27:21]

Description was changed from

==========
declcontent
BUG=
==========

to

==========
[Extensions Bindings] Add native declarativeContent verification

The declarativeContent API exposes constructors that initialize certain
types. These constructors (sometimes) take arguments that need to be
validated against the schema. Add native support for this validation by
allowing native hooks to modify the API binding's object template, and
using this to expose the constructor methods rather than relying on the
JS custom bindings.

Add additional declarativeContent-related tests and unittests for hooks
modifying the API binding template.

BUG=653596
==========

[Devlin, 2017-05-01 19:27:29]

Patchset #1 (id:1) has been deleted

[Devlin, 2017-05-01 19:28:01]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 19:28:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-05-01 19:29:44]

rdevlin.cronin@chromium.org changed reviewers:
+ jbroman@chromium.org, lazyboy@chromium.org

[Devlin, 2017-05-01 19:29:45]

Hey folks, mind taking a look?

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
File extensions/renderer/declarative_content_hooks_delegate.cc (right):

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:214:
arguments.ThrowTypeError("Invalid invocation.");
Note: will rebase and use the new API invocation errors here.

[jbroman, 2017-05-01 20:16:40]

Some initial thoughts (mostly a couple of safety questions).

https://codereview.chromium.org/2853023002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/native_bindings/declarative_content/background.js
(right):

https://codereview.chromium.org/2853023002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/native_bindings/declarative_content/background.js:13:
var imageData = ctx.createImageData(19, 19);
nit: Why not just "new ImageData(19, 19)"?

https://codereview.chromium.org/2853023002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/native_bindings/declarative_content/background.js:26:
console.warn(JSON.stringify(rule));
Was this left here from debugging?

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
File extensions/renderer/declarative_content_hooks_delegate.cc (right):

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:29: // Copies the
properties from src -> dst, similar to Object.assign().
Don't know how much you care, but at a glance I think there's at least three
behaviour differences (which may or may not be desirable):
- Object.assign copies symbol keys
- Object.assign uses Set rather than CreateDataProperty
- Object.assign checks that it is an own property, if that changes during
iteration

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:42:
DCHECK(key->IsString() || key->IsNumber());
Note that this is true (without a property filter, GetOwnPropertyNames does skip
symbols), but Object.assign does not. But you only really need to downcast to
v8::Name, so I'm not sure this DCHECK is that helpful.

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:48: bool success =
false;
super-nit: might as well factor out the result checking, too?

v8::Maybe<bool> set_result =
    key->IsString()
        ? dst->CreateDataProperty(context,
                                  key.As<v8::String>(),
                                  prop_value)
        : dst->CreateDataProperty(context,
                                  key.As<v8::Uint32>()->Value(),
                                  prop_value);
if (!set_result.IsJust() || !set_result.FromJust())
  return false;

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:96: const char*
selector = *v8::String::Utf8Value(val.As<v8::String>());
This is unsafe. The buffer returned by Utf8Value::operator* is only valid until
the Utf8Value is destroyed (the V8 internal representation is different). If you
want to use Utf8Value, you'll have to have it on the stack and not just as a
temporary. (Aside: if you're using Utf8Value, it does provide the length for
you, so you don't have to use the implicit strlen that appears here on both
conversion to WebString and std::string.)

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:174:
&DeclarativeContentHooksDelegate::HandleCall,
This seems slightly paradoxical. If the DeclarativeContentHooksDelegate always
outlives the function, then the weak ptr is unnecessary. If not, then the vector
will have been destroyed and it will be illegal to access the callback in the
first place, before we even get to the weak ptr.

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
File extensions/renderer/declarative_content_hooks_delegate.h (right):

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.h:41:
std::vector<std::unique_ptr<HandlerCallback>> callbacks_;
Maybe comment why this is a vector<unique_ptr<Callback>> rather than just a
vector<Callback>? It's to make the pointers held by v8::External stable.

[Devlin, 2017-05-01 20:46:48]

https://codereview.chromium.org/2853023002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/native_bindings/declarative_content/background.js
(right):

https://codereview.chromium.org/2853023002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/native_bindings/declarative_content/background.js:13:
var imageData = ctx.createImageData(19, 19);
On 2017/05/01 20:16:40, jbroman wrote:
> nit: Why not just "new ImageData(19, 19)"?

copy-paste code.  Simplified.

https://codereview.chromium.org/2853023002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/native_bindings/declarative_content/background.js:26:
console.warn(JSON.stringify(rule));
On 2017/05/01 20:16:40, jbroman wrote:
> Was this left here from debugging?

Whoops, thought I caught them all.  Removed.

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
File extensions/renderer/declarative_content_hooks_delegate.cc (right):

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:29: // Copies the
properties from src -> dst, similar to Object.assign().
On 2017/05/01 20:16:40, jbroman wrote:
> Don't know how much you care, but at a glance I think there's at least three
> behaviour differences (which may or may not be desirable):
> - Object.assign copies symbol keys
> - Object.assign uses Set rather than CreateDataProperty
> - Object.assign checks that it is an own property, if that changes during
> iteration

I'm not too worried about those.  The goal wasn't so much to implement
Object.assign() as just to copy the parameters over.  I only mentioned
Object.assign() as a "it's basically like this"-type comment, but I realize that
it might be confusing.  I've removed the reference to avoid making it seem like
it's supposed to be a spec-appropriate implementation.

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:42:
DCHECK(key->IsString() || key->IsNumber());
On 2017/05/01 20:16:40, jbroman wrote:
> Note that this is true (without a property filter, GetOwnPropertyNames does
skip
> symbols), but Object.assign does not. But you only really need to downcast to
> v8::Name, so I'm not sure this DCHECK is that helpful.

I think the DCHECK is useful only because below we assume the value is either a
string or uint; it just makes that assumption more clear.

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:48: bool success =
false;
On 2017/05/01 20:16:40, jbroman wrote:
> super-nit: might as well factor out the result checking, too?
> 
> v8::Maybe<bool> set_result =
>     key->IsString()
>         ? dst->CreateDataProperty(context,
>                                   key.As<v8::String>(),
>                                   prop_value)
>         : dst->CreateDataProperty(context,
>                                   key.As<v8::Uint32>()->Value(),
>                                   prop_value);
> if (!set_result.IsJust() || !set_result.FromJust())
>   return false;

Sure.  I thought it would be too verbose, but if you're fine with it, works for
me. :)

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:96: const char*
selector = *v8::String::Utf8Value(val.As<v8::String>());
On 2017/05/01 20:16:40, jbroman wrote:
> This is unsafe. The buffer returned by Utf8Value::operator* is only valid
until
> the Utf8Value is destroyed (the V8 internal representation is different). If
you
> want to use Utf8Value, you'll have to have it on the stack and not just as a
> temporary. (Aside: if you're using Utf8Value, it does provide the length for
> you, so you don't have to use the implicit strlen that appears here on both
> conversion to WebString and std::string.)

Oh, I had no idea.  Fixed.  Thanks for catching it!

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:174:
&DeclarativeContentHooksDelegate::HandleCall,
On 2017/05/01 20:16:40, jbroman wrote:
> This seems slightly paradoxical. If the DeclarativeContentHooksDelegate always
> outlives the function, then the weak ptr is unnecessary. If not, then the
vector
> will have been destroyed and it will be illegal to access the callback in the
> first place, before we even get to the weak ptr.

Fair enough - habit.  Removed.

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
File extensions/renderer/declarative_content_hooks_delegate.h (right):

https://codereview.chromium.org/2853023002/diff/60001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.h:41:
std::vector<std::unique_ptr<HandlerCallback>> callbacks_;
On 2017/05/01 20:16:40, jbroman wrote:
> Maybe comment why this is a vector<unique_ptr<Callback>> rather than just a
> vector<Callback>? It's to make the pointers held by v8::External stable.

Done.

[jbroman, 2017-05-02 18:36:34]

https://codereview.chromium.org/2853023002/diff/80001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/declarative_content_custom_bindings.js
(right):

https://codereview.chromium.org/2853023002/diff/80001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:39:
// Fake calling the original function as a constructor.
Yeah, eww. Can't think of something better, short of the longer-term solution
you talk about here.

Naturally this isn't quite perfect emulation (e.g. new.target or IsConstructCall
will give it away, as with the fact that we're ignoring the return value), but
it's probably good enough in the meantime.

https://codereview.chromium.org/2853023002/diff/80001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:40:
this.__proto__ = nativeSetIcon.prototype;
This can run author script, like this:

Object.defineProperty(chrome.declarativeContent.SetIcon.prototype, '__proto__',
                      { set() { console.error('ran author script!'); } });

I think Object.setPrototypeOf (which is in safe builtins already as
$Object.setPrototypeOf) is the proper workaround. Not sure if this issue occurs
elsewhere in the extension JS bindings.

https://codereview.chromium.org/2853023002/diff/80001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:100:
setIcon(parameters, $Function.bind(function(data) {
Good catch. :)

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
File extensions/renderer/declarative_content_hooks_delegate.cc (right):

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:42:
DCHECK(key->IsString() || key->IsNumber());
nit: might as well narrow IsNumber to IsUint32, since we expect this to be an
array index.

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:98: *error = "Invalid
css: " + std::string(*selector, selector.length());
nit: "Invalid CSS selector", maybe?

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:111: // to |this|.
Returns true on success.
nit: |this_object|, since that's what it's called here

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:137:
!CanonicalizeCssSelectors(context, this_object, error)) {
Is there test coverage for this? I guess it will be covered by general tests of
this feature, once native bindings are turned on?

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:164: for (const auto
type : kTypes) {
nit: const auto&

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:168: //
base::Unretained is safe.
And spec, type_refs, and type, and the callback itself.

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:194: // an unmodified
instance. But we don't know how many extensions enforcing
Script can still break that, e.g. by using Reflect.construct to do a construct
call where the callee and newTarget are different. Something similar can happen
with ES6 classes.

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:208:
std::vector<v8::Local<v8::Value>> argument_list = arguments.GetAll();
nit: slightly shorter (don't need the vector for anything else):

if (arguments.Length() > 1) {
  arguments.ThrowTypeError("Too many arguments.");
  return;
}

v8::Local<v8::Object> properties;
if (arguments.Length() >= 1 && !arguments.GetNext(&properties)) {
  arguments.ThrowTypeError("Expected object.");
  return;
}

Or is this going to need to handle more stuff in the future?

[Devlin, 2017-05-02 20:51:44]

https://codereview.chromium.org/2853023002/diff/80001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/declarative_content_custom_bindings.js
(right):

https://codereview.chromium.org/2853023002/diff/80001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:39:
// Fake calling the original function as a constructor.
On 2017/05/02 18:36:33, jbroman wrote:
> Yeah, eww. Can't think of something better, short of the longer-term solution
> you talk about here.
> 
> Naturally this isn't quite perfect emulation (e.g. new.target or
IsConstructCall
> will give it away, as with the fact that we're ignoring the return value), but
> it's probably good enough in the meantime.

Yep. :/  See also the note in declarative_content_hooks_delegate about wishing
we could use IsConstructCall.

https://codereview.chromium.org/2853023002/diff/80001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/declarative_content_custom_bindings.js:40:
this.__proto__ = nativeSetIcon.prototype;
On 2017/05/02 18:36:33, jbroman wrote:
> This can run author script, like this:
> 
> Object.defineProperty(chrome.declarativeContent.SetIcon.prototype,
'__proto__',
>                       { set() { console.error('ran author script!'); } });
> 
> I think Object.setPrototypeOf (which is in safe builtins already as
> $Object.setPrototypeOf) is the proper workaround. Not sure if this issue
occurs
> elsewhere in the extension JS bindings.

Wild.  Wouldn't have expected that.  Fixed; thanks for catching it.

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
File extensions/renderer/declarative_content_hooks_delegate.cc (right):

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:42:
DCHECK(key->IsString() || key->IsNumber());
On 2017/05/02 18:36:34, jbroman wrote:
> nit: might as well narrow IsNumber to IsUint32, since we expect this to be an
> array index.

Done.

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:98: *error = "Invalid
css: " + std::string(*selector, selector.length());
On 2017/05/02 18:36:34, jbroman wrote:
> nit: "Invalid CSS selector", maybe?

Done.

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:111: // to |this|.
Returns true on success.
On 2017/05/02 18:36:34, jbroman wrote:
> nit: |this_object|, since that's what it's called here

Whoops, done.

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:137:
!CanonicalizeCssSelectors(context, this_object, error)) {
On 2017/05/02 18:36:34, jbroman wrote:
> Is there test coverage for this? I guess it will be covered by general tests
of
> this feature, once native bindings are turned on?

Yep, that was my plan.  Since this is more just an API feature than a bindings
feature, I'm banking on the tests for that exercising it.  (In reality, there's
probably some cases that that's not the case, but I think it's out of scope of
the bindings rewrite to ensure each API has tests - though that's certainly
worth doing too.)

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:164: for (const auto
type : kTypes) {
On 2017/05/02 18:36:33, jbroman wrote:
> nit: const auto&

Whoops, done.

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:168: //
base::Unretained is safe.
On 2017/05/02 18:36:33, jbroman wrote:
> And spec, type_refs, and type, and the callback itself.

Done.

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:194: // an unmodified
instance. But we don't know how many extensions enforcing
On 2017/05/02 18:36:34, jbroman wrote:
> Script can still break that, e.g. by using Reflect.construct to do a construct
> call where the callee and newTarget are different. Something similar can
happen
> with ES6 classes.

Hmm... interesting.  Could we just check info.NewTarget() == (this_function)**?

(**It looks like we'd normally want info.Callee(), but that's deprecated with
the advice to pass the callee via Data, but I don't know how we'd do that with a
function template, since what we want to pass is the function stamped out of the
template... but maybe there's a way?)

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:208:
std::vector<v8::Local<v8::Value>> argument_list = arguments.GetAll();
On 2017/05/02 18:36:34, jbroman wrote:
> nit: slightly shorter (don't need the vector for anything else):
> 
> if (arguments.Length() > 1) {
>   arguments.ThrowTypeError("Too many arguments.");
>   return;
> }
> 
> v8::Local<v8::Object> properties;
> if (arguments.Length() >= 1 && !arguments.GetNext(&properties)) {
>   arguments.ThrowTypeError("Expected object.");
>   return;
> }
> 
> Or is this going to need to handle more stuff in the future?

Nah, that should work.  Done.

[jbroman, 2017-05-02 23:09:08]

lgtm

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
File extensions/renderer/declarative_content_hooks_delegate.cc (right):

https://codereview.chromium.org/2853023002/diff/80001/extensions/renderer/dec...
extensions/renderer/declarative_content_hooks_delegate.cc:194: // an unmodified
instance. But we don't know how many extensions enforcing
On 2017/05/02 at 20:51:43, Devlin wrote:
> On 2017/05/02 18:36:34, jbroman wrote:
> > Script can still break that, e.g. by using Reflect.construct to do a
construct
> > call where the callee and newTarget are different. Something similar can
happen
> > with ES6 classes.
> 
> Hmm... interesting.  Could we just check info.NewTarget() ==
(this_function)**?
> 
> (**It looks like we'd normally want info.Callee(), but that's deprecated with
the advice to pass the callee via Data, but I don't know how we'd do that with a
function template, since what we want to pass is the function stamped out of the
template... but maybe there's a way?)

I think you can use v8::Signature to ensure that info.Holder() is associated
with the FunctionTemplate you expect, but all it lets the author do is mess with
the prototype chain, which they can do slightly later anyhow.

Aside: I thought we checked this in Blink, but apparently we don't, because I
can get a rather strange object by doing something like
"Reflect.construct(ImageData, [1, 2], Image)", which is associated with
HTMLImageElement.prototype but you is actually an ImageData (not a big deal --
just does the moral equivalent of swapping the prototype after construction,
except you get to do it earlier).

[lazyboy, 2017-05-03 18:47:01]

lgtm

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
File extensions/renderer/declarative_content_hooks_delegate.cc (right):

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
extensions/renderer/declarative_content_hooks_delegate.cc:66:
v8::Local<v8::String> key = gin::StringToSymbol(isolate, "css");
declarative_content_constants::kCss

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
extensions/renderer/declarative_content_hooks_delegate.cc:93:
blink::CanonicalizeSelector(
note: The blink::CanonicalizeSelector call from css_natives_handler has a vague
TODO saying "this API shouldn't exist", although I don't know what "this" refers
to. So pointing it out.

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
extensions/renderer/declarative_content_hooks_delegate.cc:99: "Invalid css
selector: " + std::string(*selector, selector.length());
nit: s/css/CSS since we're not talking about the "css" key.

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
extensions/renderer/declarative_content_hooks_delegate.cc:127: context,
gin::StringToSymbol(isolate, "instanceType"),
declarative_content_constants::kInstanceType

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
extensions/renderer/declarative_content_hooks_delegate.cc:137: if (type_name ==
"declarativeContent.PageStateMatcher" &&
The full names match declarative_contents_constants::kPageStateMatcherType,
should we be using that instead? Same for line 160 below.

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
extensions/renderer/declarative_content_hooks_delegate.cc:172:
callbacks_.push_back(base::MakeUnique<HandlerCallback>(
At size 4, the vector is going to most likely realloc. So reserve?

[Devlin, 2017-05-03 21:42:22]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 21:43:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 22:52:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 22:52:22]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-05-03 22:55:02]

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
File extensions/renderer/declarative_content_hooks_delegate.cc (right):

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
extensions/renderer/declarative_content_hooks_delegate.cc:66:
v8::Local<v8::String> key = gin::StringToSymbol(isolate, "css");
On 2017/05/03 18:47:00, lazyboy wrote:
> declarative_content_constants::kCss

Done.  (Note: this meant moving the file, which was previously in
chrome/browser, but that seems like the right thing to do.)

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
extensions/renderer/declarative_content_hooks_delegate.cc:93:
blink::CanonicalizeSelector(
On 2017/05/03 18:47:00, lazyboy wrote:
> note: The blink::CanonicalizeSelector call from css_natives_handler has a
vague
> TODO saying "this API shouldn't exist", although I don't know what "this"
refers
> to. So pointing it out.

Added a note.

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
extensions/renderer/declarative_content_hooks_delegate.cc:99: "Invalid css
selector: " + std::string(*selector, selector.length());
On 2017/05/03 18:47:00, lazyboy wrote:
> nit: s/css/CSS since we're not talking about the "css" key.

Done.

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
extensions/renderer/declarative_content_hooks_delegate.cc:127: context,
gin::StringToSymbol(isolate, "instanceType"),
On 2017/05/03 18:47:01, lazyboy wrote:
> declarative_content_constants::kInstanceType

Done.

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
extensions/renderer/declarative_content_hooks_delegate.cc:137: if (type_name ==
"declarativeContent.PageStateMatcher" &&
On 2017/05/03 18:47:00, lazyboy wrote:
> The full names match declarative_contents_constants::kPageStateMatcherType,
> should we be using that instead? Same for line 160 below.

Done.

https://codereview.chromium.org/2853023002/diff/100001/extensions/renderer/de...
extensions/renderer/declarative_content_hooks_delegate.cc:172:
callbacks_.push_back(base::MakeUnique<HandlerCallback>(
On 2017/05/03 18:47:01, lazyboy wrote:
> At size 4, the vector is going to most likely realloc. So reserve?

Done.

[Devlin, 2017-05-03 22:55:07]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2017-05-03 22:55:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
lazyboy@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2853023002/#ps120001
(title: "lazyboy's")

[commit-bot: I haz the power, 2017-05-03 22:55:50]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 23:05:00]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493852107334740,
"parent_rev": "44acfaa84e1b59c9cda18fc9c9b8a2cd5dc35564", "commit_rev":
"92d03c12fddf6214bcb74ec328a284f39eb2f452"}

[commit-bot: I haz the power, 2017-05-03 23:05:12]

Description was changed from

==========
[Extensions Bindings] Add native declarativeContent verification

The declarativeContent API exposes constructors that initialize certain
types. These constructors (sometimes) take arguments that need to be
validated against the schema. Add native support for this validation by
allowing native hooks to modify the API binding's object template, and
using this to expose the constructor methods rather than relying on the
JS custom bindings.

Add additional declarativeContent-related tests and unittests for hooks
modifying the API binding template.

BUG=653596
==========

to

==========
[Extensions Bindings] Add native declarativeContent verification

The declarativeContent API exposes constructors that initialize certain
types. These constructors (sometimes) take arguments that need to be
validated against the schema. Add native support for this validation by
allowing native hooks to modify the API binding's object template, and
using this to expose the constructor methods rather than relying on the
JS custom bindings.

Add additional declarativeContent-related tests and unittests for hooks
modifying the API binding template.

BUG=653596

Review-Url: https://codereview.chromium.org/2853023002
Cr-Commit-Position: refs/heads/master@{#469182}
Committed:
https://chromium.googlesource.com/chromium/src/+/92d03c12fddf6214bcb74ec328a2...
==========

[commit-bot: I haz the power, 2017-05-03 23:05:13]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/92d03c12fddf6214bcb74ec328a2...