Extensions Docs Server: Enum values do not show up if enum is a type

Extensions Docs Server: Enum values do not show up if enum is a type

If an enumerated string is a type, the possible values were not shown.

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

[not at google - send to devlin, 2012-09-11 01:14:10]

http://codereview.chromium.org/10907151/diff/1/tools/json_schema_compiler/cpp...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/10907151/diff/1/tools/json_schema_compiler/cpp...
tools/json_schema_compiler/cpp_type_generator.py:212: key=lambda ns: ns.name):
incremental cleanup: this can be operator.attrgetter I believe.

http://codereview.chromium.org/10907151/diff/1/tools/json_schema_compiler/tes...
File tools/json_schema_compiler/test/font_settings.json (left):

http://codereview.chromium.org/10907151/diff/1/tools/json_schema_compiler/tes...
tools/json_schema_compiler/test/font_settings.json:31: ],
this doesn't seem right?

[cduvall, 2012-09-11 19:23:46]

https://chromiumcodereview.appspot.com/10907151/diff/1/tools/json_schema_comp...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://chromiumcodereview.appspot.com/10907151/diff/1/tools/json_schema_comp...
tools/json_schema_compiler/cpp_type_generator.py:212: key=lambda ns: ns.name):
On 2012/09/11 01:14:10, kalman wrote:
> incremental cleanup: this can be operator.attrgetter I believe.

Done.

https://chromiumcodereview.appspot.com/10907151/diff/1/tools/json_schema_comp...
File tools/json_schema_compiler/test/font_settings.json (left):

https://chromiumcodereview.appspot.com/10907151/diff/1/tools/json_schema_comp...
tools/json_schema_compiler/test/font_settings.json:31: ],
On 2012/09/11 01:14:10, kalman wrote:
> this doesn't seem right?

I just made this a string type (not enum) so we can use it to test string types.
I renamed it to FakeStringType so it will be more clear.

[not at google - send to devlin, 2012-09-11 21:55:12]

lgtm

https://chromiumcodereview.appspot.com/10907151/diff/1/tools/json_schema_comp...
File tools/json_schema_compiler/test/font_settings.json (left):

https://chromiumcodereview.appspot.com/10907151/diff/1/tools/json_schema_comp...
tools/json_schema_compiler/test/font_settings.json:31: ],
On 2012/09/11 19:23:46, cduvall wrote:
> On 2012/09/11 01:14:10, kalman wrote:
> > this doesn't seem right?
> 
> I just made this a string type (not enum) so we can use it to test string
types.
> I renamed it to FakeStringType so it will be more clear.

Oh it's *test* data. That totally confused me. Ok.

[commit-bot: I haz the power, 2012-09-11 21:59:41]

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

[commit-bot: I haz the power, 2012-09-11 22:16:27]

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

[not at google - send to devlin, 2012-09-11 23:07:43]

https://chromiumcodereview.appspot.com/10907151/diff/9003/tools/json_schema_c...
File tools/json_schema_compiler/cc_generator.py (right):

https://chromiumcodereview.appspot.com/10907151/diff/9003/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:753: .Append('  NOTREACHED();')
is this necessary?

[cduvall, 2012-09-11 23:10:38]

I'm going to have to work on this a little more, it's having compile problems
with the enum changes.

https://chromiumcodereview.appspot.com/10907151/diff/9003/tools/json_schema_c...
File tools/json_schema_compiler/cc_generator.py (right):

https://chromiumcodereview.appspot.com/10907151/diff/9003/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:753: .Append('  NOTREACHED();')
On 2012/09/11 23:07:43, kalman wrote:
> is this necessary?

I just had it because that's what used to be there. It looks like there's more
problems around though, its still not compiling.

[not at google - send to devlin, 2012-09-11 23:14:09]

https://chromiumcodereview.appspot.com/10907151/diff/9003/tools/json_schema_c...
File tools/json_schema_compiler/cc_generator.py (right):

https://chromiumcodereview.appspot.com/10907151/diff/9003/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:753: .Append('  NOTREACHED();')
On 2012/09/11 23:10:38, cduvall wrote:
> On 2012/09/11 23:07:43, kalman wrote:
> > is this necessary?
> 
> I just had it because that's what used to be there. It looks like there's more
> problems around though, its still not compiling.

Used to be there when?

[cduvall, 2012-09-11 23:18:28]

https://chromiumcodereview.appspot.com/10907151/diff/9003/tools/json_schema_c...
File tools/json_schema_compiler/cc_generator.py (right):

https://chromiumcodereview.appspot.com/10907151/diff/9003/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:753: .Append('  NOTREACHED();')
On 2012/09/11 23:14:09, kalman wrote:
> On 2012/09/11 23:10:38, cduvall wrote:
> > On 2012/09/11 23:07:43, kalman wrote:
> > > is this necessary?
> > 
> > I just had it because that's what used to be there. It looks like there's
more
> > problems around though, its still not compiling.
> 
> Used to be there when?

Were you talking about the NOTREACHED or the default case? The default case was
needed to handle the *_NONE enum value, because I got a compile error that the
switch case didn't handle it.

The problem is that normally an enum will only have a *_NONE value if it is
optional, but when an enum is a type, you can't know if its going to be optional
or not.

[not at google - send to devlin, 2012-09-11 23:19:52]

https://chromiumcodereview.appspot.com/10907151/diff/9003/tools/json_schema_c...
File tools/json_schema_compiler/cc_generator.py (right):

https://chromiumcodereview.appspot.com/10907151/diff/9003/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:753: .Append('  NOTREACHED();')
On 2012/09/11 23:18:28, cduvall wrote:
> On 2012/09/11 23:14:09, kalman wrote:
> > On 2012/09/11 23:10:38, cduvall wrote:
> > > On 2012/09/11 23:07:43, kalman wrote:
> > > > is this necessary?
> > > 
> > > I just had it because that's what used to be there. It looks like there's
> more
> > > problems around though, its still not compiling.
> > 
> > Used to be there when?
> 
> Were you talking about the NOTREACHED or the default case? The default case
was
> needed to handle the *_NONE enum value, because I got a compile error that the
> switch case didn't handle it.
> 
> The problem is that normally an enum will only have a *_NONE value if it is
> optional, but when an enum is a type, you can't know if its going to be
optional
> or not.

Yeah, the thing abou the default case vs the fall-through. That makes sense
though. Could you add a comment here explaining that?

[cduvall, 2012-09-12 00:56:40]

Now it should compile, running the trybots to make sure it actually works.

[cduvall, 2012-09-13 23:10:14]

benjhayden: downloads API
miket: socket API
kalman: everything else

I deleted some tests because they expected invalid enums. Enums were previously
stored as std::strings, so this could be tested. Now, since they are stored as
enums in the C++, they cannot hold arbitrary values. I think enums should be
checked for correctness before they get to the C++ code, but correct me if I'm
wrong.

Patch Set 6 has some green try-bot runs, fyi. There was a compile warning, but
I'm not sure what its from. I'm still looking into that, but I think it will be
a very small change, so the patch is ready for review.

[miket_OOO, 2012-09-13 23:19:09]

Socket LGTM, and thanks.

[not at google - send to devlin, 2012-09-14 01:44:51]

this is turning into a bit of an epic. but looking forward to it.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right):

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/context_menu/context_menu_api.cc:79: NOTREACHED();
the problem is the presence of a NONE case right? can you just deal with that
explicitly?

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/context_menu/context_menu_api.cc:249:
NOTREACHED();
ditto

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/context_menu/context_menu_api.cc:353:
NOTREACHED();
ditto

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:434:
DownloadItem::DownloadState state = StateEnumFromString(state_string);
if you generate a ToString as I suggested, this would be much simpler.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:447: danger_string);
etc

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:587: }
etc

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:67:
script_value->GetAsString(&script);
etc

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:74:
MediaGalleries::GETMEDIAFILESYSTEMSINTERACTIVITY_NO;
wow... what happened here? something funky with the string enum conversion.

this should be GET_MEDIA_FILESYSTEMS_INTERACTIVITY_NO (see media_galleries.idl)

and so on

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:78: }
also this can use switch now that it's an enum.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:129: if (type == kTCPOption)
{
etc

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:134: c.Append()
nit: style like

(c.Concat(...)
  .Append()
  .Concat(...)
  .Append())

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:230: (c.Sblock('else {')
would be nice to generate style-guide compatible code here, like
} else {
rather than
}
else {
you might need to extend code.py to be able to do it, though. Maybe, err, Mblock
(M for Middle) or something.

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:729: (c.Append('default:')
so... is this a place where we legitimately don't know whether there's a NONE
value being generated? so we need to catch it in default too?

I mean, surely, we know. because we end up generating the code. question is how
much pain it is.

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:759: # is not handled in the other
cases.
confused, can't you handle the NONE case explicitly then?

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:773: .Sblock('bool
GetEnumFromString(const std::string& enum_string,'
does it make sense to return the enum directly? We have a _NONE value generated
for enums to use as the default case. out-param style methods are so awkward.

Also, since it's on the enum type already, FromString() is probably enough.

Also, seems like this should be using GenerateStringToEnumConverstion in some
way (or GSTEC should be modified to call this).

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:774: ' %(class)s* %(var)s) {'))
how about generating a ToString(enum) too? seems useful. There might be code
around here somewhere for that.

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:780: # on Windows.
haha

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:816: (c.Append('default:')
ditto

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
File tools/json_schema_compiler/util.h (right):

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/util.h:53: *out = static_cast<T>(value);
i don't understand, ListValue enums are stored as strings, how can this
GetInteger? maybe the comment is throwing me off.

[benjhayden, 2012-09-14 13:17:48]

LGTM, one nit, and I'd also like to see the ToString() method that kalman
suggested if possible.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:589:
download_params->set_post_body(*options.body);
Why remove the .get()? I think it's more explicit than using the operator* on
the scoped_ptr directly.

[cduvall, 2012-09-17 22:07:45]

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right):

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/context_menu/context_menu_api.cc:79: NOTREACHED();
On 2012/09/14 01:44:51, kalman wrote:
> the problem is the presence of a NONE case right? can you just deal with that
> explicitly?

Done.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/context_menu/context_menu_api.cc:249:
NOTREACHED();
On 2012/09/14 01:44:51, kalman wrote:
> ditto

Done.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/context_menu/context_menu_api.cc:353:
NOTREACHED();
On 2012/09/14 01:44:51, kalman wrote:
> ditto

Done.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:434:
DownloadItem::DownloadState state = StateEnumFromString(state_string);
On 2012/09/14 01:44:51, kalman wrote:
> if you generate a ToString as I suggested, this would be much simpler.

Done.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:447: danger_string);
On 2012/09/14 01:44:51, kalman wrote:
> etc

Done.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:587: }
On 2012/09/14 01:44:51, kalman wrote:
> etc

Done.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:589:
download_params->set_post_body(*options.body);
On 2012/09/14 13:17:48, benjhayden_chromium wrote:
> Why remove the .get()? I think it's more explicit than using the operator* on
> the scoped_ptr directly.

Done.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:67:
script_value->GetAsString(&script);
On 2012/09/14 01:44:51, kalman wrote:
> etc

Done.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:74:
MediaGalleries::GETMEDIAFILESYSTEMSINTERACTIVITY_NO;
On 2012/09/14 01:44:51, kalman wrote:
> wow... what happened here? something funky with the string enum conversion.
> 
> this should be GET_MEDIA_FILESYSTEMS_INTERACTIVITY_NO (see
media_galleries.idl)
> 
> and so on

Done.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:78: }
On 2012/09/14 01:44:51, kalman wrote:
> also this can use switch now that it's an enum.

Done.

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10907151/diff/11006/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:129: if (type == kTCPOption)
{
On 2012/09/14 01:44:51, kalman wrote:
> etc

Done.

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:134: c.Append()
On 2012/09/14 01:44:51, kalman wrote:
> nit: style like
> 
> (c.Concat(...)
>   .Append()
>   .Concat(...)
>   .Append())

Done.

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:230: (c.Sblock('else {')
On 2012/09/14 01:44:51, kalman wrote:
> would be nice to generate style-guide compatible code here, like
> } else {
> rather than
> }
> else {
> you might need to extend code.py to be able to do it, though. Maybe, err,
Mblock
> (M for Middle) or something.

Done.

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:729: (c.Append('default:')
On 2012/09/14 01:44:51, kalman wrote:
> so... is this a place where we legitimately don't know whether there's a NONE
> value being generated? so we need to catch it in default too?
> 
> I mean, surely, we know. because we end up generating the code. question is
how
> much pain it is.

Done.

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:759: # is not handled in the other
cases.
On 2012/09/14 01:44:51, kalman wrote:
> confused, can't you handle the NONE case explicitly then?

Done.

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:773: .Sblock('bool
GetEnumFromString(const std::string& enum_string,'
On 2012/09/14 01:44:51, kalman wrote:
> does it make sense to return the enum directly? We have a _NONE value
generated
> for enums to use as the default case. out-param style methods are so awkward.
> 
> Also, since it's on the enum type already, FromString() is probably enough.
> 
> Also, seems like this should be using GenerateStringToEnumConverstion in some
> way (or GSTEC should be modified to call this).

Done.

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:774: ' %(class)s* %(var)s) {'))
On 2012/09/14 01:44:51, kalman wrote:
> how about generating a ToString(enum) too? seems useful. There might be code
> around here somewhere for that.

Done.

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:816: (c.Append('default:')
On 2012/09/14 01:44:51, kalman wrote:
> ditto

Done.

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
File tools/json_schema_compiler/util.h (right):

http://codereview.chromium.org/10907151/diff/11006/tools/json_schema_compiler...
tools/json_schema_compiler/util.h:53: *out = static_cast<T>(value);
On 2012/09/14 01:44:51, kalman wrote:
> i don't understand, ListValue enums are stored as strings, how can this
> GetInteger? maybe the comment is throwing me off.

I clarified the comment. This happens in the JSC test code, idl_basics.idl
Function14.

[not at google - send to devlin, 2012-09-18 03:55:20]

just one more....

p.s. note to self: update c/b/e/settings/settings_storage_quota_enforcer.cc to
use ToString.

http://codereview.chromium.org/10907151/diff/74/chrome/browser/extensions/api...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10907151/diff/74/chrome/browser/extensions/api...
chrome/browser/extensions/api/socket/socket_api.cc:125: type = kUnknown;
going to string here is actually unnecessary, you should be able to directly
switch on the params_->type enum. Then delete kTCPOption.

(the same is also true the downloads API but that involves adding a method ...
meh).

http://codereview.chromium.org/10907151/diff/74/tools/json_schema_compiler/cc...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/10907151/diff/74/tools/json_schema_compiler/cc...
tools/json_schema_compiler/cc_generator.py:674:
self._cpp_type_generator.GetCompiledType(prop)))
use %s(tring) replacements so that you don't need to repeat that calculation?

http://codereview.chromium.org/10907151/diff/74/tools/json_schema_compiler/cc...
tools/json_schema_compiler/cc_generator.py:778: .Append('if
(!enum_value->GetAsString(&temp_string))')
Pulling the string out of the value here seems roundabout. Can you make it so
that ToString here does the switching, then CreateEnumValue calls this method?

http://codereview.chromium.org/10907151/diff/74/tools/json_schema_compiler/cc...
tools/json_schema_compiler/cc_generator.py:838:
self._cpp_type_generator.GetEnumValue(prop, enum_value),
can you make this method just call FromString? it will generate less code.

http://codereview.chromium.org/10907151/diff/74/tools/json_schema_compiler/h_...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/10907151/diff/74/tools/json_schema_compiler/h_...
tools/json_schema_compiler/h_generator.py:327: enum_from_string = 'static ' +
enum_from_string
change to use 'static %s' %...

[not at google - send to devlin, 2012-09-18 03:55:20]

just one more....

p.s. note to self: update c/b/e/settings/settings_storage_quota_enforcer.cc to
use ToString.

[cduvall, 2012-09-21 00:39:28]

https://codereview.chromium.org/10907151/diff/74/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

https://codereview.chromium.org/10907151/diff/74/chrome/browser/extensions/ap...
chrome/browser/extensions/api/socket/socket_api.cc:125: type = kUnknown;
On 2012/09/18 03:55:20, kalman wrote:
> going to string here is actually unnecessary, you should be able to directly
> switch on the params_->type enum. Then delete kTCPOption.
> 
> (the same is also true the downloads API but that involves adding a method ...
> meh).

Done.

https://codereview.chromium.org/10907151/diff/74/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cc_generator.py (right):

https://codereview.chromium.org/10907151/diff/74/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:674:
self._cpp_type_generator.GetCompiledType(prop)))
On 2012/09/18 03:55:20, kalman wrote:
> use %s(tring) replacements so that you don't need to repeat that calculation?

Done.

https://codereview.chromium.org/10907151/diff/74/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:778: .Append('if
(!enum_value->GetAsString(&temp_string))')
On 2012/09/18 03:55:20, kalman wrote:
> Pulling the string out of the value here seems roundabout. Can you make it so
> that ToString here does the switching, then CreateEnumValue calls this method?

Done.

https://codereview.chromium.org/10907151/diff/74/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:838:
self._cpp_type_generator.GetEnumValue(prop, enum_value),
On 2012/09/18 03:55:20, kalman wrote:
> can you make this method just call FromString? it will generate less code.

Done, assuming you meant ToString.

https://codereview.chromium.org/10907151/diff/74/tools/json_schema_compiler/h...
File tools/json_schema_compiler/h_generator.py (right):

https://codereview.chromium.org/10907151/diff/74/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:327: enum_from_string = 'static ' +
enum_from_string
On 2012/09/18 03:55:20, kalman wrote:
> change to use 'static %s' %...

Done.

[not at google - send to devlin, 2012-09-21 01:11:52]

lgtm

[commit-bot: I haz the power, 2012-09-21 02:03:54]

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

[commit-bot: I haz the power, 2012-09-21 02:38:15]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-21 02:47:57]

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

[cduvall, 2012-09-21 03:52:12]

+zelidrag: for rtcPrivate changes.

I wasn't quite sure what to do with the RtcPrivateEventRouter::LAUNCH_ACTIVATE
case, so I mapped that to RTC_PRIVATE_ACTION_TYPE_NONE, since it seemed the best
fit.

[commit-bot: I haz the power, 2012-09-24 22:44:17]

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

[not at google - send to devlin, 2012-09-24 22:46:28]

http://codereview.chromium.org/10907151/diff/41001/chrome/browser/extensions/...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.cc (right):

http://codereview.chromium.org/10907151/diff/41001/chrome/browser/extensions/...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:56: action_type =
api::rtc_private::RTC_PRIVATE_ACTION_TYPE_NONE;
why does this map to NONE not ACTIVATE?

http://codereview.chromium.org/10907151/diff/41001/chrome/browser/extensions/...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:69: break;
this method could be more concise by returning the enums rather than breaking,
and getting rid of the default case with the NOTREACHED outside.

[cduvall, 2012-09-24 22:51:47]

https://chromiumcodereview.appspot.com/10907151/diff/41001/chrome/browser/ext...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.cc (right):

https://chromiumcodereview.appspot.com/10907151/diff/41001/chrome/browser/ext...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:56: action_type =
api::rtc_private::RTC_PRIVATE_ACTION_TYPE_NONE;
On 2012/09/24 22:46:29, kalman wrote:
> why does this map to NONE not ACTIVATE?

In the IDL file there is only "chat", "voice", and "video", there is no
"activate". NONE seemed to fit the best.

https://chromiumcodereview.appspot.com/10907151/diff/41001/chrome/browser/ext...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:69: break;
On 2012/09/24 22:46:29, kalman wrote:
> this method could be more concise by returning the enums rather than breaking,
> and getting rid of the default case with the NOTREACHED outside.

Done.

[not at google - send to devlin, 2012-09-24 22:54:14]

https://chromiumcodereview.appspot.com/10907151/diff/41001/chrome/browser/ext...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.cc (right):

https://chromiumcodereview.appspot.com/10907151/diff/41001/chrome/browser/ext...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:56: action_type =
api::rtc_private::RTC_PRIVATE_ACTION_TYPE_NONE;
On 2012/09/24 22:51:47, cduvall wrote:
> On 2012/09/24 22:46:29, kalman wrote:
> > why does this map to NONE not ACTIVATE?
> 
> In the IDL file there is only "chat", "voice", and "video", there is no
> "activate". NONE seemed to fit the best.

Bizarre. I don't know how the code works as is, then. If "action" is missing
from the JSON then that would usually cause the JSON schema validation on the
renderer to kick in and reject anything.

I guess you'll need for the person who wrote this code to comment.

[zel, 2012-09-24 23:00:57]

LGTM

https://chromiumcodereview.appspot.com/10907151/diff/41001/chrome/browser/ext...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.cc (right):

https://chromiumcodereview.appspot.com/10907151/diff/41001/chrome/browser/ext...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:56: action_type =
api::rtc_private::RTC_PRIVATE_ACTION_TYPE_NONE;
On 2012/09/24 22:54:14, kalman wrote:
> On 2012/09/24 22:51:47, cduvall wrote:
> > On 2012/09/24 22:46:29, kalman wrote:
> > > why does this map to NONE not ACTIVATE?
> > 
> > In the IDL file there is only "chat", "voice", and "video", there is no
> > "activate". NONE seemed to fit the best.
> 
> Bizarre. I don't know how the code works as is, then. If "action" is missing
> from the JSON then that would usually cause the JSON schema validation on the
> renderer to kick in and reject anything.
> 
> I guess you'll need for the person who wrote this code to comment.

That's fine as it is right now. We will modify C++ part of the API to match this
as well.

[commit-bot: I haz the power, 2012-09-24 23:03:01]

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

[commit-bot: I haz the power, 2012-09-25 01:00:43]

Change committed as 158475