Allow updateArgumentsPostValidate to support callbacks and added / removed arguments.

Allow updateArgumentsPostValidate to support callbacks and added / removed arguments.

This also pulls the normalization code out of schema_generated_bindings.js.

BUG=None
TEST=Existing tests


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

[benwells, 2012-06-06 21:17:09]

green bots now

[not at google - send to devlin, 2012-06-07 00:17:11]

Thanks for this cleanup. Sorry I have so many comments :\

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.cc:546:
source_map_.RegisterSource("validateUtils", IDR_VALIDATE_UTILS_JS);
Bikeshed moment:

I'd have called it schemaUtil cos it does more than just validation, and it's
shorter, plus validateUtils sounds grammatically odd to me (like seems like it
should be "validationUtils").

Change if you agree, don't change if you don't :)

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:19: var
validateUtils = require('validateUtils');
nit: maintain blank line after this.

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:149:
self.funDefs = {};
Sadly I think that funDefs is a silly name. functionSchemas?

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:155:
self.funDefs[f.name] = apiFunction;
nit: inline apiFunction like

self.functionSchemas[f.name] = {
  name: f.name,
  definition: f
};

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:159:
CustomBindingsObject.prototype.setApiDef = function(apiDef) {
is this used? Grep cannot find.

assuming this is here for a future patch of yours:

I think that the functionSchemas object will need to be initialised in the
constructor for this to work, otherwise the functions here will overwrite those
in setSchema. same for the parameters object (for consistency).

Unless that's desired. Ngh, this is too complicated. Could you comment on this
method why it's needed?

Also add a comment to setSchema like "Sets the schema for the type that this is
customising" or something.

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/storage_custom_bindings.js (right):

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/storage_custom_bindings.js:31: arguments =
normalizeArgumentsAndValidate(arguments, funDef);
I don't think that overwriting "arguments" is a polite thing to do. Can you call
it "args".

I'm actually a bit surprised that passing an arguments object into
normalizeArgumentsAndValidate works. It expects an array, so you'll need to
Array.prototype.slice.call it when passing through to
normalizeArgumentsAndValidate. And... add a comment to
normalizeArgumentsAndValidate to say that |args| should be an array :p

[benwells, 2012-06-07 01:45:40]

PTAL...

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.cc:546:
source_map_.RegisterSource("validateUtils", IDR_VALIDATE_UTILS_JS);
On 2012/06/07 00:17:11, kalman wrote:
> Bikeshed moment:
> 
> I'd have called it schemaUtil cos it does more than just validation, and it's
> shorter, plus validateUtils sounds grammatically odd to me (like seems like it
> should be "validationUtils").
> 
> Change if you agree, don't change if you don't :)

Done (as schemaUtils).

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:19: var
validateUtils = require('validateUtils');
On 2012/06/07 00:17:11, kalman wrote:
> nit: maintain blank line after this.

Done.

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:149:
self.funDefs = {};
On 2012/06/07 00:17:11, kalman wrote:
> Sadly I think that funDefs is a silly name. functionSchemas?

Ok mr grown up. How about funSchemas?

... oh all right ...

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:155:
self.funDefs[f.name] = apiFunction;
On 2012/06/07 00:17:11, kalman wrote:
> nit: inline apiFunction like
> 
> self.functionSchemas[f.name] = {
>   name: f.name,
>   definition: f
> };

Done.

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:159:
CustomBindingsObject.prototype.setApiDef = function(apiDef) {
On 2012/06/07 00:17:11, kalman wrote:
> is this used? Grep cannot find.
> 
> assuming this is here for a future patch of yours:
> 
> I think that the functionSchemas object will need to be initialised in the
> constructor for this to work, otherwise the functions here will overwrite
those
> in setSchema. same for the parameters object (for consistency).
> 
> Unless that's desired. Ngh, this is too complicated. Could you comment on this
> method why it's needed?
> 
> Also add a comment to setSchema like "Sets the schema for the type that this
is
> customising" or something.

Yeah sorry this is an experiment which lived past its time. Not needed.

Gone now.

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/storage_custom_bindings.js (right):

http://codereview.chromium.org/10535030/diff/3005/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/storage_custom_bindings.js:31: arguments =
normalizeArgumentsAndValidate(arguments, funDef);
On 2012/06/07 00:17:11, kalman wrote:
> I don't think that overwriting "arguments" is a polite thing to do. Can you
call
> it "args".
> 
> I'm actually a bit surprised that passing an arguments object into
> normalizeArgumentsAndValidate works. It expects an array, so you'll need to
> Array.prototype.slice.call it when passing through to
> normalizeArgumentsAndValidate. And... add a comment to
> normalizeArgumentsAndValidate to say that |args| should be an array :p

Done.

[not at google - send to devlin, 2012-06-07 03:15:04]

lgtm

http://codereview.chromium.org/10535030/diff/14002/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/schema_utils.js (right):

http://codereview.chromium.org/10535030/diff/14002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/schema_utils.js:11: if
(chromeHidden.JSONSchemaValidator) {
Just throwing this out there, use it as you will.

I notice in event.js there is code like

var validate = require('schemaUtils').validate;
...
if (validate) {
}

which is totally bizarre unless you understand that validate only gets set if
there's a JSONSchemaValidator, which isn't true in unit tests. What!?

I'm curious, what unit tests were failing?  Is it possible to load
JSONSchemaValidator in those tests?  Or, what do you think about having a

} else {
  function validate() {
  }
}

as a hack until JSONSchemaValidator properly uses the module system, then remove
that weird line from event.js?

http://codereview.chromium.org/10535030/diff/14002/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/storage_custom_bindings.js (right):

http://codereview.chromium.org/10535030/diff/14002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/storage_custom_bindings.js:16: return
extendedSchema;
just curious, could you have used something like this black magic to solve your
problem in a different way?

(can't remember the specifics of your problem)

http://codereview.chromium.org/10535030/diff/14002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/storage_custom_bindings.js:30: var
funSchema = this.functionSchemas[functionName];
Woohoo!  Par-tay!

Ok I'll stop now.

http://codereview.chromium.org/10535030/diff/14002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/storage_custom_bindings.js:40:
apiFunctions.forEach(bindApiFunction);
thanks!

[benwells, 2012-06-07 04:11:48]

ok, gonna land now.

http://codereview.chromium.org/10535030/diff/14002/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/schema_utils.js (right):

http://codereview.chromium.org/10535030/diff/14002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/schema_utils.js:11: if
(chromeHidden.JSONSchemaValidator) {
On 2012/06/07 03:15:04, kalman wrote:
> Just throwing this out there, use it as you will.
> 
> I notice in event.js there is code like
> 
> var validate = require('schemaUtils').validate;
> ...
> if (validate) {
> }
> 
> which is totally bizarre unless you understand that validate only gets set if
> there's a JSONSchemaValidator, which isn't true in unit tests. What!?
> 
> I'm curious, what unit tests were failing?  Is it possible to load
> JSONSchemaValidator in those tests?  Or, what do you think about having a
> 
> } else {
>   function validate() {
>   }
> }
> 
> as a hack until JSONSchemaValidator properly uses the module system, then
remove
> that weird line from event.js?

I like the latter. The unit test was testing event_unittest. Pulling in
JSONSchemaValidator, and doing the unit testing, doesn't feel right. Ideally
we'd have mock versions of all these I guess.

http://codereview.chromium.org/10535030/diff/14002/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/storage_custom_bindings.js (right):

http://codereview.chromium.org/10535030/diff/14002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/storage_custom_bindings.js:16: return
extendedSchema;
On 2012/06/07 03:15:04, kalman wrote:
> just curious, could you have used something like this black magic to solve
your
> problem in a different way?
> 
> (can't remember the specifics of your problem)

No I don't think this would help, not that I can see anyway.

[commit-bot: I haz the power, 2012-06-07 04:12:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10535030/14004

[commit-bot: I haz the power, 2012-06-07 04:51:39]

Try job failure for 10535030-14004 (retry) on mac_rel for step "browser_tests"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-07 06:48:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10535030/9004

[commit-bot: I haz the power, 2012-06-07 08:49:00]

Change committed as 140980