Fixing json compiler preview.

Fixing json compiler preview.

BUG=

Committed: https://crrev.com/84c28d28e4870fbff9c6988da0721bcf98825ffb
Cr-Commit-Position: refs/heads/master@{#292632}

[lfg, 2014-08-28 19:10:43]

lfg@chromium.org changed reviewers:
+ kalman@chromium.org

[lfg, 2014-08-28 19:10:43]

Here's a patch fixing the json compiler preview. Depends on
https://codereview.chromium.org/487533005/ .

[not at google - send to devlin, 2014-08-28 19:22:09]

Thanks for fixing this.

I actually, confusingly, meant the preview.py in
chrome/common/extensions/docs/server2 - but you do need to fix this file as
well, so I'm glad it was noticed.

https://codereview.chromium.org/515983003/diff/1/tools/json_schema_compiler/m...
File tools/json_schema_compiler/model.py (right):

https://codereview.chromium.org/515983003/diff/1/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:133: self.dependencies =
json.get('dependencies', [])
When is this populated?

https://codereview.chromium.org/515983003/diff/1/tools/json_schema_compiler/p...
File tools/json_schema_compiler/preview.py (right):

https://codereview.chromium.org/515983003/diff/1/tools/json_schema_compiler/p...
tools/json_schema_compiler/preview.py:237: return
No point returning after a raise - but I prefer the old behaviour anyway. I
think it was because showing a stack trace is quite user unfriendly.

[lfg, 2014-08-28 19:33:35]

https://codereview.chromium.org/515983003/diff/1/tools/json_schema_compiler/m...
File tools/json_schema_compiler/model.py (right):

https://codereview.chromium.org/515983003/diff/1/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:133: self.dependencies =
json.get('dependencies', [])
On 2014/08/28 19:22:09, kalman wrote:
> When is this populated?

This came directly from the json file (currently, only web_view_internal.json
was using it).

On a second look, it doesn't look that this is needed, I've removed it.

https://codereview.chromium.org/515983003/diff/1/tools/json_schema_compiler/p...
File tools/json_schema_compiler/preview.py (right):

https://codereview.chromium.org/515983003/diff/1/tools/json_schema_compiler/p...
tools/json_schema_compiler/preview.py:237: return
On 2014/08/28 19:22:09, kalman wrote:
> No point returning after a raise - but I prefer the old behaviour anyway. I
> think it was because showing a stack trace is quite user unfriendly.

Oops, that was left over after I debugged this code. Removed.

[not at google - send to devlin, 2014-08-28 19:41:35]

lgtm

https://codereview.chromium.org/515983003/diff/1/tools/json_schema_compiler/m...
File tools/json_schema_compiler/model.py (right):

https://codereview.chromium.org/515983003/diff/1/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:133: self.dependencies =
json.get('dependencies', [])
On 2014/08/28 19:33:35, lfg wrote:
> On 2014/08/28 19:22:09, kalman wrote:
> > When is this populated?
> 
> This came directly from the json file (currently, only web_view_internal.json
> was using it).
> 
> On a second look, it doesn't look that this is needed, I've removed it.

Ah ok. Yeah we used to use a "dependencies" property of schema files but we
stopped a while ago. I guess there are still some hanging around.

https://codereview.chromium.org/515983003/diff/20001/tools/json_schema_compil...
File tools/json_schema_compiler/preview.py (right):

https://codereview.chromium.org/515983003/diff/20001/tools/json_schema_compil...
tools/json_schema_compiler/preview.py:194: '')
Maybe make this like "fake_namespace" or something? '' loses context.

https://codereview.chromium.org/515983003/diff/20001/tools/json_schema_compil...
File tools/json_schema_compiler/schema_loader.py (right):

https://codereview.chromium.org/515983003/diff/20001/tools/json_schema_compil...
tools/json_schema_compiler/schema_loader.py:63: return namespace
(I know the code is already like this but...) |namespace| is assigned two
different concepts here, the namespace name, and the namespace model. The former
should probably be "cpp_namespace".

Also you could just return |namespace| immediately and not need to assign it to
a variable at all.

[lfg, 2014-08-28 23:14:59]

https://codereview.chromium.org/515983003/diff/1/tools/json_schema_compiler/m...
File tools/json_schema_compiler/model.py (right):

https://codereview.chromium.org/515983003/diff/1/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:133: self.dependencies =
json.get('dependencies', [])
On 2014/08/28 19:41:35, kalman wrote:
> On 2014/08/28 19:33:35, lfg wrote:
> > On 2014/08/28 19:22:09, kalman wrote:
> > > When is this populated?
> > 
> > This came directly from the json file (currently, only
web_view_internal.json
> > was using it).
> > 
> > On a second look, it doesn't look that this is needed, I've removed it.
> 
> Ah ok. Yeah we used to use a "dependencies" property of schema files but we
> stopped a while ago. I guess there are still some hanging around.

Acknowledged.

https://codereview.chromium.org/515983003/diff/20001/tools/json_schema_compil...
File tools/json_schema_compiler/preview.py (right):

https://codereview.chromium.org/515983003/diff/20001/tools/json_schema_compil...
tools/json_schema_compiler/preview.py:194: '')
On 2014/08/28 19:41:35, kalman wrote:
> Maybe make this like "fake_namespace" or something? '' loses context.

I've added the option to specify the namespace through command-line arguments.
The default is "generated_api", same as the compiler.py.

https://codereview.chromium.org/515983003/diff/20001/tools/json_schema_compil...
File tools/json_schema_compiler/schema_loader.py (right):

https://codereview.chromium.org/515983003/diff/20001/tools/json_schema_compil...
tools/json_schema_compiler/schema_loader.py:63: return namespace
On 2014/08/28 19:41:35, kalman wrote:
> (I know the code is already like this but...) |namespace| is assigned two
> different concepts here, the namespace name, and the namespace model. The
former
> should probably be "cpp_namespace".
> 
> Also you could just return |namespace| immediately and not need to assign it
to
> a variable at all.

Done.

[lfg, 2014-08-29 14:39:24]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2014-08-29 14:40:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lfg@chromium.org/515983003/140001

[commit-bot: I haz the power, 2014-08-29 15:03:14]

Committed patchset #8 (id:140001) as ae8a2631f3163b27f3a8b06e1f9e3c4bf21aa42c

[commit-bot: I haz the power, 2014-09-10 03:08:36]

Patchset 8 (id:??) landed as
https://crrev.com/84c28d28e4870fbff9c6988da0721bcf98825ffb
Cr-Commit-Position: refs/heads/master@{#292632}