Make all extension api types fully qualified.

Make all extension api types fully qualified.

BUG=123073
TEST= (unit_tests --gtest_filter=ExtensionAPI) && (browser_tests --gtest_filter=ExtensionApiTest)

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

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

[asargent_no_longer_on_chrome, 2012-05-03 22:19:58]

Would it simplify things if we just insisted that all $ref values in the API
json files had a namespace prefix? (Or at least all references to non-local
types?)

[bryeung, 2012-05-04 00:23:00]

As a little more context for others on the thread, Antony is helping
me figure out the right fix for
http://code.google.com/p/chromium/issues/detail?id=123073

So far this CL modifies the schemas when they are loaded to fully
qualify all of the types and $refs with the namespace of the schema.
Clearly this doesn't work, as the schemas can refer to types from
other schemas.  So...

On Thu, May 3, 2012 at 6:19 PM,  <asargent@chromium.org> wrote:
> Would it simplify things if we just insisted that all $ref values in the API
> json files had a namespace prefix? (Or at least all references to non-local
> types?)

I started down that road, but turned back when I realized that json
schema compiler seems to explicitly support inference of namespace
from the list of dependencies (the code that lead me to believe that
is _ResolveTypeNamespace in
tools/json_schema_compiler/cpp_type_generator.py).

So, it seems there are two solutions possible now:
1) Force all $ref values to specify fully qualified types.
2) Add code to chrome/common/extensions/api/extension_api.cc that does
the same kind of namespace inference as is being done by json schema
compiler.

I would actually be fine to implement either.  I like 1) for it's
simplicity, but it's probably a larger (although LOC negative, I
suspect) change.

Bryan

[asargent_no_longer_on_chrome, 2012-05-09 18:37:22]

LGTM w/ a couple of nits

(BTW, I fixed the subject of the review for you to remove the "not for review"
bit)

http://codereview.chromium.org/10367002/diff/7002/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.cc (right):

http://codereview.chromium.org/10367002/diff/7002/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.cc:144: // |schema_namespace| if they
do not already specify a namespace.
nit: looks like this comment should go with PrefixTypesWithNamespace , and the
comment there should go here (this function is about refs, that one about
types).

http://codereview.chromium.org/10367002/diff/7002/tools/json_schema_compiler/...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/10367002/diff/7002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:212: raise KeyError('Cannot
resolve type: %s.  Maybe it needs qualifying?' %
optional suggestion: it might be more clear to say something like "Maybe it
needs a namespace prefix if it comes from another namespace?" ('qualifying'
seems kind of vague to me when thinking about it from the perspective of someone
who's just making some API additions/changes)

http://codereview.chromium.org/10367002/diff/7002/tools/json_schema_compiler/...
File tools/json_schema_compiler/idl_schema.py (right):

http://codereview.chromium.org/10367002/diff/7002/tools/json_schema_compiler/...
tools/json_schema_compiler/idl_schema.py:9: import schema_util
nit: I've been trying to move all the code in this directory to separate the
list of system imports from the local package imports, to comply with standard
python style which our python style guide recommends following:

http://www.python.org/dev/peps/pep-0008/#imports

So, put the import of schema_util in a separate group at the bottom here.

[bryeung, 2012-05-09 19:12:00]

Thanks Antony!  All fixed and off to the try bots.

http://codereview.chromium.org/10367002/diff/7002/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.cc (right):

http://codereview.chromium.org/10367002/diff/7002/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.cc:144: // |schema_namespace| if they
do not already specify a namespace.
On 2012/05/09 18:37:23, Antony Sargent wrote:
> nit: looks like this comment should go with PrefixTypesWithNamespace , and the
> comment there should go here (this function is about refs, that one about
> types).

Oops.  Good catch!  Done.

http://codereview.chromium.org/10367002/diff/7002/tools/json_schema_compiler/...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/10367002/diff/7002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:212: raise KeyError('Cannot
resolve type: %s.  Maybe it needs qualifying?' %
On 2012/05/09 18:37:23, Antony Sargent wrote:
> optional suggestion: it might be more clear to say something like "Maybe it
> needs a namespace prefix if it comes from another namespace?" ('qualifying'
> seems kind of vague to me when thinking about it from the perspective of
someone
> who's just making some API additions/changes) 

Done.

http://codereview.chromium.org/10367002/diff/7002/tools/json_schema_compiler/...
File tools/json_schema_compiler/idl_schema.py (right):

http://codereview.chromium.org/10367002/diff/7002/tools/json_schema_compiler/...
tools/json_schema_compiler/idl_schema.py:9: import schema_util
On 2012/05/09 18:37:23, Antony Sargent wrote:
> nit: I've been trying to move all the code in this directory to separate the
> list of system imports from the local package imports, to comply with standard
> python style which our python style guide recommends following:
> 
> http://www.python.org/dev/peps/pep-0008/#imports
> 
> So, put the import of schema_util in a separate group at the bottom here.

Done.

[bryeung, 2012-05-09 21:35:08]

Antony: could you sanity check the latest patch set?  It's just a couple of
lines.

[asargent_no_longer_on_chrome, 2012-05-09 21:43:02]

looks sane to me!

[commit-bot: I haz the power, 2012-05-10 13:04:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10367002/15047

[commit-bot: I haz the power, 2012-05-10 14:38:16]

Change committed as 136296

[commit-bot: I haz the power, 2012-05-10 18:05:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10367002/15047

[commit-bot: I haz the power, 2012-05-10 18:38:37]

Try job failure for 10367002-15047 (retry) on mac_rel for step "compile"
(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-05-10 18:43:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10367002/15047

[commit-bot: I haz the power, 2012-05-10 19:09:46]

Try job failure for 10367002-15047 (retry) (retry) (retry) on android for steps
"Compile, build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-05-10 20:40:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10367002/15047

[commit-bot: I haz the power, 2012-05-10 20:49:53]

Try job failure for 10367002-15047 (retry) on android for steps "Compile,
build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-05-11 14:44:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10367002/15047

[commit-bot: I haz the power, 2012-05-11 17:00:23]

Change committed as 136588