Lazily require types when validating Extensions API calls

Lazily require types when validating Extensions API calls

Custom types now have their own JS files, which is referenced
by the 'js_module' key in the schema. This makes it so $ref
types can be referenced by other APIs without having to load
the whole API of the type referenced.

BUG=222156
TBR=ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189839

[cduvall, 2013-03-20 20:17:29]

I ran some browser_tests on this that I thought might be affected, and they all
pass. Running a full try run to make sure though.

[not at google - send to devlin, 2013-03-21 00:01:13]

nice! mostly nits.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:121: var customType =
require(t['js_module'])[t['js_module']];
* t -> type
* save reference to t['js_module;] (also: t.js_module)
* CHECK things (assume that people will leave out js_module and you need to help
them)

btw we should add a CHECK to logging.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:209: // Add types to global
schemaValidator
"the ones for the types we depend on will be added as needed"

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:216: if (t.type == 'object' &&
'js_module' in t) {
i think this type == 'object' thing isn't necessary

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:218: mod[parts[parts.length -
1]] = createCustomType(t);
looks like this splitting shenanigans also unnecessary

wait... what does this even do? it seems like it adds e.g. a
chrome.storage.StorageArea property, but I don't see it there. nor it is
necessary. what happens if you leave it all out (except for the addTypes part of
course).

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:369: var constructor;
not initializing things makes me slightly uncomfortable, C++ thing I suppose,
but = null?

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:372: var constructor =
createCustomType(type);
var here doesn't make sense

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/json_schema.js (right):

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/json_schema.js:179: if (schema['$ref']) {
save a reference to schema['$ref'], it'd make this a lot easier to parse.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/json_schema.js:181:
this.types[schema['$ref']] = loadRefType(schema['$ref']);
CHECK that it found it.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/json_schema.js:250:
this.types[schema["$ref"]] = loadRefType(schema["$ref"]);
save schema[$ref], CHECK

actually this logic is repeated, add a getOrLoadType method?

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/schema_utils.js (right):

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/schema_utils.js:60: }
is this necessary, the schema validator knows how to load $refs already?

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/utils.js (right):

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/utils.js:41: function loadRefType(ref,
defaultSchema) {
loadTypeSchema?

ref -> typeName?

(the fact that these are used for looking up $ref isn't interesting to this
method)

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/utils.js:45: return undefined;
CHECK

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/utils.js:52: if (types[i]['id'] == ref)
.id

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/utils.js:54: }
return null (nice to be explicit)

[cduvall, 2013-03-21 00:59:51]

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:121: var customType =
require(t['js_module'])[t['js_module']];
On 2013/03/21 00:01:13, kalman wrote:
> * t -> type
> * save reference to t['js_module;] (also: t.js_module)
> * CHECK things (assume that people will leave out js_module and you need to
help
> them)
> 
> btw we should add a CHECK to logging.

Done.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:209: // Add types to global
schemaValidator
On 2013/03/21 00:01:13, kalman wrote:
> "the ones for the types we depend on will be added as needed"

Done.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:216: if (t.type == 'object' &&
'js_module' in t) {
On 2013/03/21 00:01:13, kalman wrote:
> i think this type == 'object' thing isn't necessary

Done, took it all out.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:218: mod[parts[parts.length -
1]] = createCustomType(t);
On 2013/03/21 00:01:13, kalman wrote:
> looks like this splitting shenanigans also unnecessary
> 
> wait... what does this even do? it seems like it adds e.g. a
> chrome.storage.StorageArea property, but I don't see it there. nor it is
> necessary. what happens if you leave it all out (except for the addTypes part
of
> course).

Done.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:369: var constructor;
On 2013/03/21 00:01:13, kalman wrote:
> not initializing things makes me slightly uncomfortable, C++ thing I suppose,
> but = null?

Done.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:372: var constructor =
createCustomType(type);
On 2013/03/21 00:01:13, kalman wrote:
> var here doesn't make sense

Done.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/json_schema.js (right):

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/json_schema.js:179: if (schema['$ref']) {
On 2013/03/21 00:01:13, kalman wrote:
> save a reference to schema['$ref'], it'd make this a lot easier to parse.

Done.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/json_schema.js:181:
this.types[schema['$ref']] = loadRefType(schema['$ref']);
On 2013/03/21 00:01:13, kalman wrote:
> CHECK that it found it.

Done.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/json_schema.js:250:
this.types[schema["$ref"]] = loadRefType(schema["$ref"]);
On 2013/03/21 00:01:13, kalman wrote:
> save schema[$ref], CHECK
> 
> actually this logic is repeated, add a getOrLoadType method?

I don't think this should CHECK, because it throws the "unknownSchemaReference"
error if the schema wasn't found.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/schema_utils.js (right):

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/schema_utils.js:60: }
On 2013/03/21 00:01:13, kalman wrote:
> is this necessary, the schema validator knows how to load $refs already?

Hmm, it seemed necessary when I did it, but now I can't find the test that
failed. I'll take it out.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/utils.js (right):

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/utils.js:41: function loadRefType(ref,
defaultSchema) {
On 2013/03/21 00:01:13, kalman wrote:
> loadTypeSchema?
> 
> ref -> typeName?
> 
> (the fact that these are used for looking up $ref isn't interesting to this
> method)

Done.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/utils.js:45: return undefined;
On 2013/03/21 00:01:13, kalman wrote:
> CHECK

Done.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/utils.js:52: if (types[i]['id'] == ref)
On 2013/03/21 00:01:13, kalman wrote:
> .id

Done.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/utils.js:54: }
On 2013/03/21 00:01:13, kalman wrote:
> return null (nice to be explicit)

returned undefined to be consistent. Is null better?

[not at google - send to devlin, 2013-03-21 01:36:55]

minor comments left, lgtm.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:209: // Add types to global
schemaValidator
On 2013/03/21 00:59:51, cduvall wrote:
> On 2013/03/21 00:01:13, kalman wrote:
> > "the ones for the types we depend on will be added as needed"
> 
> Done.

my bad grammar sorry. 'the types we depend on from other namespaces...'

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/json_schema.js (right):

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/json_schema.js:250:
this.types[schema["$ref"]] = loadRefType(schema["$ref"]);
On 2013/03/21 00:59:51, cduvall wrote:
> On 2013/03/21 00:01:13, kalman wrote:
> > save schema[$ref], CHECK
> > 
> > actually this logic is repeated, add a getOrLoadType method?
> 
> I don't think this should CHECK, because it throws the
"unknownSchemaReference"
> error if the schema wasn't found.

ah ok.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/utils.js (right):

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/utils.js:54: }
On 2013/03/21 00:59:51, cduvall wrote:
> On 2013/03/21 00:01:13, kalman wrote:
> > return null (nice to be explicit)
> 
> returned undefined to be consistent. Is null better?

that is always a question with JS.

my rule at least: if it's defined (i.e. if we know what it is), then don't
return undefined. Here we know definitely that it's not found, so it's defined,
therefore it should be null.

OTOH void return functions have an undefined result, so should be undefined.

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:123: CHECK(jsModule, 'Custom
type has no "js_module" property.');
'custom type ' + type.id + ' must have a...'

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:124: var customType =
require(jsModule)[jsModule];
i don't really know how paranoid we should be here, but let's be really
paranoid, because it seems like it could be a common problem and 'could not
access blah of undefined' is always a pain to see and debug.

jsModuleName = type.js_module...
CHECK(jsModuleName, ...)
jsModule = require(jsModuleName)
CHEKC(jsModule, 'no module ' + jsModuleName + ' found for ' + type.id);
customType = jsModule[jsModuleName];
CHECK(customType, jsModuleName + ' must export itself);

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:372: constructor =
createCustomType(type);
looks like it's actually impossible for createCustomType to return anything that
would fail here, so this could be a bit simpler with

var ref = propertyDef['$ref'];
var type = ...
CHECK(type, "schema for $ref type " + ref + ' not found');
var constructor = createCustomType(type);

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/json_schema.js (right):

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
chrome/renderer/resources/extensions/json_schema.js:260: this.validate(instance,
this.types[ref], path)
accessing this.types[ref] here kind of defeats the point of adding that
getOrAddType method.

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/utils.js (right):

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
chrome/renderer/resources/extensions/utils.js:46: '" with no default schema.');
with neither namespace nor default schema

[cduvall, 2013-03-21 17:16:07]

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:209: // Add types to global
schemaValidator
On 2013/03/21 01:36:55, kalman wrote:
> On 2013/03/21 00:59:51, cduvall wrote:
> > On 2013/03/21 00:01:13, kalman wrote:
> > > "the ones for the types we depend on will be added as needed"
> > 
> > Done.
> 
> my bad grammar sorry. 'the types we depend on from other namespaces...'

Done.

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/utils.js (right):

https://codereview.chromium.org/12647017/diff/9001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/utils.js:54: }
On 2013/03/21 01:36:55, kalman wrote:
> On 2013/03/21 00:59:51, cduvall wrote:
> > On 2013/03/21 00:01:13, kalman wrote:
> > > return null (nice to be explicit)
> > 
> > returned undefined to be consistent. Is null better?
> 
> that is always a question with JS.
> 
> my rule at least: if it's defined (i.e. if we know what it is), then don't
> return undefined. Here we know definitely that it's not found, so it's
defined,
> therefore it should be null.
> 
> OTOH void return functions have an undefined result, so should be undefined.

Done.

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:123: CHECK(jsModule, 'Custom
type has no "js_module" property.');
On 2013/03/21 01:36:55, kalman wrote:
> 'custom type ' + type.id + ' must have a...'

Done.

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:124: var customType =
require(jsModule)[jsModule];
On 2013/03/21 01:36:55, kalman wrote:
> i don't really know how paranoid we should be here, but let's be really
> paranoid, because it seems like it could be a common problem and 'could not
> access blah of undefined' is always a pain to see and debug.
> 
> jsModuleName = type.js_module...
> CHECK(jsModuleName, ...)
> jsModule = require(jsModuleName)
> CHEKC(jsModule, 'no module ' + jsModuleName + ' found for ' + type.id);
> customType = jsModule[jsModuleName];
> CHECK(customType, jsModuleName + ' must export itself);

Done.

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:372: constructor =
createCustomType(type);
On 2013/03/21 01:36:55, kalman wrote:
> looks like it's actually impossible for createCustomType to return anything
that
> would fail here, so this could be a bit simpler with
> 
> var ref = propertyDef['$ref'];
> var type = ...
> CHECK(type, "schema for $ref type " + ref + ' not found');
> var constructor = createCustomType(type);

Done.

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/json_schema.js (right):

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
chrome/renderer/resources/extensions/json_schema.js:260: this.validate(instance,
this.types[ref], path)
On 2013/03/21 01:36:55, kalman wrote:
> accessing this.types[ref] here kind of defeats the point of adding that
> getOrAddType method.

Done.

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/utils.js (right):

https://codereview.chromium.org/12647017/diff/17001/chrome/renderer/resources...
chrome/renderer/resources/extensions/utils.js:46: '" with no default schema.');
On 2013/03/21 01:36:55, kalman wrote:
> with neither namespace nor default schema

Done.

[commit-bot: I haz the power, 2013-03-21 17:17:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/12647017/32001

[commit-bot: I haz the power, 2013-03-21 17:17:25]

Presubmit check for 12647017-32001 failed and returned exit status 1.

INFO:root:Found 13 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/renderer/resources/renderer_resources.grd
    chrome/chrome_renderer.gypi

Presubmit checks took 7.2s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2013-03-21 17:23:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/12647017/32001

[commit-bot: I haz the power, 2013-03-22 05:42:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/12647017/32001

[not at google - send to devlin, 2013-03-22 05:43:04]

Clark, the CQ looks stuck but it also looks happy. Could you dcommit it asap
tomorrow? Would be good to get this bug fix in before the dev channel is cut on
Monday.

[cduvall, 2013-03-22 06:57:49]

On 2013/03/22 05:43:04, kalman wrote:
> Clark, the CQ looks stuck but it also looks happy. Could you dcommit it asap
> tomorrow? Would be good to get this bug fix in before the dev channel is cut
on
> Monday.

Ok, I'll dcommit first thing tomorrow.