[Mojo] Make javascript enums extensible.

[Mojo] Make javascript enums extensible.

BUG=581390
TEST=mojo_js_unittests

[mbjorge, 2016-09-30 19:10:47]

mbjorge@chromium.org changed reviewers:
+ rockot@chromium.org,, yzshen@chromium.org

[mbjorge, 2016-09-30 19:10:48]

https://codereview.chromium.org/2380303003/diff/1/mojo/public/tools/bindings/...
File mojo/public/tools/bindings/generators/mojom_js_generator.py (right):

https://codereview.chromium.org/2380303003/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/mojom_js_generator.py:141: def
ElementCodecType(kind):
I wasn't totally sure if this was the best place for this since I don't think
the enum kind.name is really a codec type? But it kept the type lookup in
JavaScriptValidateArrayParams and JavascriptValidateMapParams cleaner.

[Ken Rockot(use gerrit already), 2016-09-30 21:14:37]

rockot@chromium.org changed reviewers:
+ rockot@chromium.org
- rockot@chromium.org,

[Ken Rockot(use gerrit already), 2016-09-30 21:14:38]

Looks pretty good, just one suggestion inline

https://codereview.chromium.org/2380303003/diff/1/mojo/public/tools/bindings/...
File mojo/public/tools/bindings/generators/mojom_js_generator.py (right):

https://codereview.chromium.org/2380303003/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/mojom_js_generator.py:141: def
ElementCodecType(kind):
On 2016/09/30 at 19:10:48, mbjorge wrote:
> I wasn't totally sure if this was the best place for this since I don't think
the enum kind.name is really a codec type? But it kept the type lookup in
JavaScriptValidateArrayParams and JavascriptValidateMapParams cleaner.

I think it would make more sense to just add a codec type for enums in see
mojo/public/js/codec.js. It could be constructed over the generated enum type
similar to how the PointerTo codec is constructed for struct types.

[mbjorge, 2016-09-30 21:36:49]

On 2016/09/30 at 21:14:38, rockot wrote:
> Looks pretty good, just one suggestion inline
> 
>
https://codereview.chromium.org/2380303003/diff/1/mojo/public/tools/bindings/...
> File mojo/public/tools/bindings/generators/mojom_js_generator.py (right):
> 
>
https://codereview.chromium.org/2380303003/diff/1/mojo/public/tools/bindings/...
> mojo/public/tools/bindings/generators/mojom_js_generator.py:141: def
ElementCodecType(kind):
> On 2016/09/30 at 19:10:48, mbjorge wrote:
> > I wasn't totally sure if this was the best place for this since I don't
think the enum kind.name is really a codec type? But it kept the type lookup in
JavaScriptValidateArrayParams and JavascriptValidateMapParams cleaner.
> 
> I think it would make more sense to just add a codec type for enums in see
mojo/public/js/codec.js. It could be constructed over the generated enum type
similar to how the PointerTo codec is constructed for struct types.

Done. Thanks for the suggestion :)

[Ken Rockot(use gerrit already), 2016-09-30 21:51:54]

LGTM! Thanks

https://codereview.chromium.org/2380303003/diff/20001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/js_templates/enum_definition.tmpl
(right):

https://codereview.chromium.org/2380303003/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/js_templates/enum_definition.tmpl:16:
switch (value) {
nit: 2 space indentation, not 4. Note, the jinja indentation and the templated
code indentation are independent. the jinja "if" indent is correct on the line
above, but this line should be indented  with respect to the function { block
containing it. Same indentation change applies to the rest of this generated
function and the one below.


foo = function(...) {
  // two spaces
};

https://codereview.chromium.org/2380303003/diff/20001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_js_generator.py (right):

https://codereview.chromium.org/2380303003/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_js_generator.py:134: return "new
codec.%s(%s)" % (enum_type, element_type)
nit: No need for enum_type, you can just use "new codec.Enum(%s)"

[mbjorge, 2016-09-30 22:14:51]

https://codereview.chromium.org/2380303003/diff/20001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_js_generator.py (right):

https://codereview.chromium.org/2380303003/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_js_generator.py:134: return "new
codec.%s(%s)" % (enum_type, element_type)
On 2016/09/30 at 21:51:54, Ken Rockot wrote:
> nit: No need for enum_type, you can just use "new codec.Enum(%s)"

Done.

[mbjorge, 2016-09-30 22:16:14]

>
https://codereview.chromium.org/2380303003/diff/20001/mojo/public/tools/bindi...
> mojo/public/tools/bindings/generators/js_templates/enum_definition.tmpl:16:
switch (value) {
> nit: 2 space indentation, not 4. Note, the jinja indentation and the templated
code indentation are independent. the jinja "if" indent is correct on the line
above, but this line should be indented  with respect to the function { block
containing it. Same indentation change applies to the rest of this generated
function and the one below.
> 
> 
> foo = function(...) {
>   // two spaces
> };
> 

Ahh, got it. Done.

[mbjorge, 2016-10-03 16:19:06]

The CQ bit was checked by mbjorge@chromium.org

[mbjorge, 2016-10-03 16:19:06]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2380303003/#ps40001
(title: "Fix whitespace issues")

[commit-bot: I haz the power, 2016-10-03 16:19:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 16:21:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 16:21:21]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mbjorge, 2016-10-03 16:55:17]

The CQ bit was checked by mbjorge@chromium.org

[mbjorge, 2016-10-03 16:55:18]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2380303003/#ps60001
(title: "Rebased")

[commit-bot: I haz the power, 2016-10-03 16:55:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 18:06:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 18:06:04]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mbjorge, 2016-10-05 17:50:30]

The CQ bit was checked by mbjorge@chromium.org

[mbjorge, 2016-10-05 17:50:30]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2380303003/#ps80001
(title: "Fix enums nested in structs")

[commit-bot: I haz the power, 2016-10-05 17:51:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 17:54:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 17:54:07]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mbjorge, 2016-10-05 17:58:40]

The CQ bit was checked by mbjorge@chromium.org

[mbjorge, 2016-10-05 17:58:40]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2380303003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-05 17:58:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 18:03:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 18:03:09]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mbjorge, 2016-10-05 18:55:41]

Not sure what I've done, but I seem to not be able to push a patchset CQ likes.
Going to abandon this CL and move it to a fresh place:
https://codereview.chromium.org/2394873003