Add human readable programmatic enum name/values to chrome.automation.

Add human readable programmatic enum name/values to chrome.automation.

This adds three new bindings to the automation API:
chrome.automation.Event
chrome.automation.Role
chrome.automation.State

Each is an object mapping an enum name to its value. Currently, this is an identity mapping. However, with the way the cl constructs the bindings, we could in the future substitute with integer values (and eliminate the stringification on either end of our extension/browser IPC).

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

[David Tseng, 2014-05-29 18:16:30]

Ready for an initial look mostly to validate the approach. I haven't gone
through and substituted all string instances with their programmatic identifiers
yet.

[not at google - send to devlin, 2014-05-29 18:26:20]

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:50:
Local<Object> ret = Object::New(GetIsolate());
s/ret/additions/ ?

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:55: ++i) {
should fit on 1 line? (might as well just "git cl format")

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:82: }
you should be able to simplify this a lot with templates over the enum type.
something like

template <typename EnumType>
v8::Local<v8::Object> ToEnumObject(
    v8::Isolate* isolate, EnumType from, EnumType to) {
  v8::Local<v8::Object> object = Object::New(isolate);
  for (int i = from; i <= to; ++i) {
    v8::Local<v8::String> value = v8::String::NewFromUtf8(
        isolate, ui::ToString(static_cast<EnumType>(i).c_str()));
   object->Set(value, value);
  }
  return object;
}

apologies for writing out the code, I'm sure you can do it... but templates are
fun. I can't help myself.

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
File chrome/renderer/extensions/automation_internal_custom_bindings.h (right):

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
chrome/renderer/extensions/automation_internal_custom_bindings.h:24: void
GetSchemaAdditions(const v8::FunctionCallbackInfo<v8::Value>& args);
could you add comment to (both of these) methods(s)?

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
chrome/renderer/resources/extensions/automation_custom_bindings.js:121:
exports.binding.State = schema.State;
you could go:

forEach(schema, function(k, v) {
  exports.binding[k] = v;
});

[aboxhall, 2014-05-29 18:35:48]

No substantial code-level comments.

- Any chance of a trivial test to show how to use the enums?
- Will these enums be available in documentation?

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:56:
Local<String> value =
The formatting here is also a little weird, I think. GetIsolate() should be
lined up with ui::ToString(... (did clang format it this way?)

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
chrome/renderer/resources/extensions/automation_custom_bindings.js:121:
exports.binding.State = schema.State;
On 2014/05/29 18:26:21, kalman wrote:
> you could go:
> 
> forEach(schema, function(k, v) {
>   exports.binding[k] = v;
> });

-1 current code has same number of lines and is more readable.

[not at google - send to devlin, 2014-05-29 18:45:49]

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
chrome/renderer/resources/extensions/automation_custom_bindings.js:121:
exports.binding.State = schema.State;
On 2014/05/29 18:35:49, aboxhall wrote:
> On 2014/05/29 18:26:21, kalman wrote:
> > you could go:
> > 
> > forEach(schema, function(k, v) {
> >   exports.binding[k] = v;
> > });
> 
> -1 current code has same number of lines and is more readable. 

I don't think it's the job of this code to know what "schema additions" are.
That's the job of the place that gives us the schema additions.

[David Tseng, 2014-05-29 19:04:40]

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:50:
Local<Object> ret = Object::New(GetIsolate());
On 2014/05/29 18:26:21, kalman wrote:
> s/ret/additions/ ?

Done.

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:55: ++i) {
On 2014/05/29 18:26:21, kalman wrote:
> should fit on 1 line? (might as well just "git cl format")

Done.

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:56:
Local<String> value =
On 2014/05/29 18:35:49, aboxhall wrote:
> The formatting here is also a little weird, I think. GetIsolate() should be
> lined up with ui::ToString(... (did clang format it this way?)

Done; (git cl formatted)

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:82: }
On 2014/05/29 18:26:21, kalman wrote:
> you should be able to simplify this a lot with templates over the enum type.
> something like
> 
> template <typename EnumType>
> v8::Local<v8::Object> ToEnumObject(
>     v8::Isolate* isolate, EnumType from, EnumType to) {
>   v8::Local<v8::Object> object = Object::New(isolate);
>   for (int i = from; i <= to; ++i) {
>     v8::Local<v8::String> value = v8::String::NewFromUtf8(
>         isolate, ui::ToString(static_cast<EnumType>(i).c_str()));
>    object->Set(value, value);
>   }
>   return object;
> }
> 
> apologies for writing out the code, I'm sure you can do it... but templates
are
> fun. I can't help myself.

No worries :); I like templates too (less code is always a win).

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
chrome/renderer/resources/extensions/automation_custom_bindings.js:121:
exports.binding.State = schema.State;
On 2014/05/29 18:45:50, kalman wrote:
> On 2014/05/29 18:35:49, aboxhall wrote:
> > On 2014/05/29 18:26:21, kalman wrote:
> > > you could go:
> > > 
> > > forEach(schema, function(k, v) {
> > >   exports.binding[k] = v;
> > > });
> > 
> > -1 current code has same number of lines and is more readable. 
> 
> I don't think it's the job of this code to know what "schema additions" are.
> That's the job of the place that gives us the schema additions.

Do you feel strongly? I could go either way on this.

[David Tseng, 2014-05-29 19:13:21]

On 2014/05/29 18:35:48, aboxhall wrote:
> No substantial code-level comments.
> 
> - Any chance of a trivial test to show how to use the enums?

I will go about substituting all strings in tests with the identifiers (e.g.
chrome.automation.Event.loadComplete).
> - Will these enums be available in documentation?

Not with this cl; I'm not quite sure how to handle this one without further
investigation; perhaps kalman has some thoughtson this as well.
> 
>
https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
> File chrome/renderer/extensions/automation_internal_custom_bindings.cc
(right):
> 
>
https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/extensio...
> chrome/renderer/extensions/automation_internal_custom_bindings.cc:56:
> Local<String> value =
> The formatting here is also a little weird, I think. GetIsolate() should be
> lined up with ui::ToString(... (did clang format it this way?)
> 
>
https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
> File chrome/renderer/resources/extensions/automation_custom_bindings.js
(right):
> 
>
https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
> chrome/renderer/resources/extensions/automation_custom_bindings.js:121:
> exports.binding.State = schema.State;
> On 2014/05/29 18:26:21, kalman wrote:
> > you could go:
> > 
> > forEach(schema, function(k, v) {
> >   exports.binding[k] = v;
> > });
> 
> -1 current code has same number of lines and is more readable.

[not at google - send to devlin, 2014-05-29 19:48:13]

lgtm

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
chrome/renderer/resources/extensions/automation_custom_bindings.js:121:
exports.binding.State = schema.State;
On 2014/05/29 19:04:40, David Tseng wrote:
> On 2014/05/29 18:45:50, kalman wrote:
> > On 2014/05/29 18:35:49, aboxhall wrote:
> > > On 2014/05/29 18:26:21, kalman wrote:
> > > > you could go:
> > > > 
> > > > forEach(schema, function(k, v) {
> > > >   exports.binding[k] = v;
> > > > });
> > > 
> > > -1 current code has same number of lines and is more readable. 
> > 
> > I don't think it's the job of this code to know what "schema additions" are.
> > That's the job of the place that gives us the schema additions.
> 
> Do you feel strongly? I could go either way on this.

I rarely feel strongly to push back on a push back. I think it's better though,
in the same way that we automatically populate the bindings from
automation.generate(), we could automatically populate the rest of it from
whatever the "additions" are.

https://codereview.chromium.org/304293002/diff/150001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/304293002/diff/150001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:19: using
v8::String;
I would loosely prefer not doing "using" here, since we never do anywhere else
in chrome/renderer/extensions, but I have noticed you're fond of that. The names
are just...  quite generic.

https://codereview.chromium.org/304293002/diff/150001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:71:
ToEnumObject<ui::AXEvent>(
hm, can C++ not guess that this is ui::AXEvent (etc) from the enum parameters
you're passing it?

[aboxhall, 2014-05-29 21:12:30]

On 2014/05/29 19:48:13, kalman wrote:
> lgtm
> 
>
https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
> File chrome/renderer/resources/extensions/automation_custom_bindings.js
(right):
> 
>
https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
> chrome/renderer/resources/extensions/automation_custom_bindings.js:121:
> exports.binding.State = schema.State;
> On 2014/05/29 19:04:40, David Tseng wrote:
> > On 2014/05/29 18:45:50, kalman wrote:
> > > On 2014/05/29 18:35:49, aboxhall wrote:
> > > > On 2014/05/29 18:26:21, kalman wrote:
> > > > > you could go:
> > > > > 
> > > > > forEach(schema, function(k, v) {
> > > > >   exports.binding[k] = v;
> > > > > });
> > > > 
> > > > -1 current code has same number of lines and is more readable. 
> > > 
> > > I don't think it's the job of this code to know what "schema additions"
are.
> > > That's the job of the place that gives us the schema additions.
> > 
> > Do you feel strongly? I could go either way on this.
> 
> I rarely feel strongly to push back on a push back. I think it's better
though,
> in the same way that we automatically populate the bindings from
> automation.generate(), we could automatically populate the rest of it from
> whatever the "additions" are.

I don't feel that strongly about it either; this reasoning seems good enough to
me.

[aboxhall, 2014-05-29 21:14:22]

lgtm

[David Tseng, 2014-05-29 22:12:25]

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/304293002/diff/20002/chrome/renderer/resource...
chrome/renderer/resources/extensions/automation_custom_bindings.js:121:
exports.binding.State = schema.State;
On 2014/05/29 19:48:14, kalman wrote:
> On 2014/05/29 19:04:40, David Tseng wrote:
> > On 2014/05/29 18:45:50, kalman wrote:
> > > On 2014/05/29 18:35:49, aboxhall wrote:
> > > > On 2014/05/29 18:26:21, kalman wrote:
> > > > > you could go:
> > > > > 
> > > > > forEach(schema, function(k, v) {
> > > > >   exports.binding[k] = v;
> > > > > });
> > > > 
> > > > -1 current code has same number of lines and is more readable. 
> > > 
> > > I don't think it's the job of this code to know what "schema additions"
are.
> > > That's the job of the place that gives us the schema additions.
> > 
> > Do you feel strongly? I could go either way on this.
> 
> I rarely feel strongly to push back on a push back. I think it's better
though,
> in the same way that we automatically populate the bindings from
> automation.generate(), we could automatically populate the rest of it from
> whatever the "additions" are.

Ok; I've left a better comment. Also, I think we might have additional top level
bindings here at some point (e.g. for text direction), so I've gone with
kalman's suggestion here.

https://codereview.chromium.org/304293002/diff/150001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/304293002/diff/150001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:19: using
v8::String;
On 2014/05/29 19:48:14, kalman wrote:
> I would loosely prefer not doing "using" here, since we never do anywhere else
> in chrome/renderer/extensions, but I have noticed you're fond of that. The
names
> are just...  quite generic.

Reverted back to v8 prefixes.

https://codereview.chromium.org/304293002/diff/150001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:71:
ToEnumObject<ui::AXEvent>(
On 2014/05/29 19:48:14, kalman wrote:
> hm, can C++ not guess that this is ui::AXEvent (etc) from the enum parameters
> you're passing it?

Looks like clang can infer the type from the arguments; done.

[David Tseng, 2014-05-29 22:12:37]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-05-29 22:16:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtseng@chromium.org/304293002/170001

[commit-bot: I haz the power, 2014-05-30 02:01:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-30 07:10:52]

Change committed as 273747