Reimplement automation API on top of C++-backed AXTree.

Reimplement automation API on top of C++-backed AXTree.

Design doc: https://docs.google.com/a/google.com/document/d/1nKMfTbS5W6uGlqRHf0XRZLVzAcKD_vqu8lw7l9zHoBw/edit?usp=sharing

BUG=495323
Committed: https://crrev.com/0e90449a7c9bd8f051fce4988a8cccd6c1c64661
Cr-Commit-Position: refs/heads/master@{#335183}

[dmazzoni, 2015-06-04 23:01:29]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org, dtseng@chromium.org

[dmazzoni, 2015-06-04 23:01:29]

Ready for a first look!

[dmazzoni, 2015-06-08 17:55:28]

dmazzoni@chromium.org changed reviewers:
+ dcheng@chromium.org

[David Tseng, 2015-06-10 15:56:31]

https://codereview.chromium.org/1155183006/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/automation/automation_apitest.cc (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/automation/automation_apitest.cc:283: int tree_id
= AXTreeIDRegistry::GetInstance()->GetOrCreateAXTreeID(
Is this code (and the generated tree test) obsolete with this change?

https://codereview.chromium.org/1155183006/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/automation_internal/automation_web_contents_observer.cc
(right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/automation_internal/automation_web_contents_observer.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
Is this class being used anywhere else? If not, then others have preferred it
stay in automation_internal_api.cc. It's also hard to see what changed in the
diff.

https://codereview.chromium.org/1155183006/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:496: // Adds a listener for the
given event type and event phase.
Rebase?

[David Tseng, 2015-06-10 17:48:17]

https://codereview.chromium.org/1155183006/diff/20001/chrome/common/extension...
File chrome/common/extensions/chrome_extension_messages.h (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/common/extension...
chrome/common/extensions/chrome_extension_messages.h:81: //
?

https://codereview.chromium.org/1155183006/diff/20001/chrome/common/extension...
chrome/common/extensions/chrome_extension_messages.h:82:
IPC_MESSAGE_ROUTED1(ExtensionHostMsg_AutomationRequest,
What kind of request?

https://codereview.chromium.org/1155183006/diff/20001/chrome/common/extension...
chrome/common/extensions/chrome_extension_messages.h:83: int /* myint */)
?

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

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:189: if
(args.Length() != 1)
Seems like these cases throughout should cause an error in js at least.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:267: void
AutomationInternalCustomBindings::GetChildIDAtIndex(
These functions should probably get docs; maybe jsdoc annotations.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:365: if
(!*out_node)
GetNodeHelper?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:397: return;
Is return value here undefined in js?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:500: }
TODO?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:504: 
TODO?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:509: }
TODO?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:511: void
AutomationInternalCustomBindings::OnAtomicUpdateFinished(
What calls this function?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:529:
SendTreeChangeEvent("nodeChanged", tree, node);
Define/document these literals.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.h (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.h:54: //
?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:23:
requireNative('automationInternal').GetIntListAttribute;
jsdoc?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:392:
defineStringAttribute('accessKey');
Why are we doing this here? Can we pull this info from the idl?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:626:
//this.invalidate_(this.wrapper);
?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation_custom_bindings.js:168: // TODO:
delete AutomationNode if 'removeNode' event
?

[aboxhall, 2015-06-11 15:05:39]

Initial thoughts; going to look at the rebased version now so some of these may
be obsolete.

https://codereview.chromium.org/1155183006/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/automation_internal/automation_web_contents_observer.cc
(right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/automation_internal/automation_web_contents_observer.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
Also, wrong year :)

https://codereview.chromium.org/1155183006/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:485: static void doDefault();
Once again I wish Rietveld did move detection. Any substantive changes in here?

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

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:273: if
(args.Length() < 3 || !args[2]->IsNumber())
Do args validation before getting the node?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:453: void
AutomationInternalCustomBindings::OnAccessibilityEvent(
I can't figure out how this event gets dispatched. I see
DispatchAccessibilityEvent() in AutomationEventRouter, but can't see where it
gets called.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:458: if (iter
== tree_id_to_tree_cache_map_.end()) {
I was trying to figure out where this got called so I could work out why it
makes sense to always create a new TreeCache here. Presumably an event is only
dispatched for trees at least one extension has requested?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.h (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.h:54: //
Contentless comment.

Were you going to add more comments for these? Would be nice to specify what
args are expected.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.h:61: bool
GetNodeHelper(
Maybe group these by routed vs local?

[aboxhall, 2015-06-11 17:42:46]

https://codereview.chromium.org/1155183006/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:401: long scrollX;
One thing we were trying to avoid with mixins was the API explosion of having
every possible attribute exposed on every automation node, and having a long
list of attributes to parse at once in the documentation.

Is there any way we can keep this "mixin" style interface, but have it
implemented more efficiently?

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:39:
this.attributes = {};
Can this be removed too?

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:273:
console.error('Error in event handler for ' + event.type +
Maybe logging.WARN here as well (I know you didn't add this one - I probably
did).

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:396:
defineStringAttribute('accessKey');
Maybe create arrays of each type of attribute and loop over them, rather than
this long list of method calls? (Use dicts for node/nodelist attributes,
obviously.)

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation_custom_bindings.js:144: var
childTreeID = GetIntAttribute(treeID, nodeID, 'childTreeId');
Can you add a comment explaining what's going on in this block?

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation_custom_bindings.js:165:
console.error('Error in tree change observer for ' +
Maybe use logging.WARNING - logs to stderr.
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...

Extension developers will be able to see console.logs (though that may be what
we want here?)

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/automation/tests/tabs/attributes.js
(left):

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/attributes.js:58:
assertTrue('owns' in combobox, 'combobox should have an owns attribute');
Are these things no longer true?

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/automation/tests/tabs/attributes.js
(right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/attributes.js:57:
console.error('combobox: ' + combobox);
Stray console.errors

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/attributes.js:80:
assertEq(undefined, ariaLink.url);
Whoa, so it's present but undefined?

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/attributes.js:90: var
name = textField.name;
I'm confused as to why this has changed from id to name here, and id to
description elsewhere.

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/automation/tests/tabs/sanity_check.js
(right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/sanity_check.js:8:
var result = JSON.parse(JSON.stringify(state));
So annoying there's not a better (simple) way to do this.

[dmazzoni, 2015-06-11 19:09:12]

OK, I've addressed most feedback. Worth taking another
look, but I'll hopefully finish addressing everything
shortly.

https://codereview.chromium.org/1155183006/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/automation/automation_apitest.cc (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/automation/automation_apitest.cc:283: int tree_id
= AXTreeIDRegistry::GetInstance()->GetOrCreateAXTreeID(
On 2015/06/10 15:56:31, David Tseng wrote:
> Is this code (and the generated tree test) obsolete with this change?

You're right, deleted.

https://codereview.chromium.org/1155183006/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/automation_internal/automation_web_contents_observer.cc
(right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/automation_internal/automation_web_contents_observer.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/06/10 15:56:31, David Tseng wrote:
> Is this class being used anywhere else? If not, then others have preferred it
> stay in automation_internal_api.cc. It's also hard to see what changed in the
> diff.

OK, moved back to automation_internal_api.cc

https://codereview.chromium.org/1155183006/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:485: static void doDefault();
On 2015/06/11 15:05:38, aboxhall wrote:
> Once again I wish Rietveld did move detection. Any substantive changes in
here?

The change was just to take all of the mixins and put them directly in
AutomationNode.

https://codereview.chromium.org/1155183006/diff/20001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:496: // Adds a listener for the
given event type and event phase.
On 2015/06/10 15:56:31, David Tseng wrote:
> Rebase?

What's wrong with this line? I think it's still correct.

https://codereview.chromium.org/1155183006/diff/20001/chrome/common/extension...
File chrome/common/extensions/chrome_extension_messages.h (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/common/extension...
chrome/common/extensions/chrome_extension_messages.h:82:
IPC_MESSAGE_ROUTED1(ExtensionHostMsg_AutomationRequest,
On 2015/06/10 17:48:15, David Tseng wrote:
> What kind of request?

Oops, this was included by mistake. All changes to this file reverted.

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

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:189: if
(args.Length() != 1)
On 2015/06/10 17:48:16, David Tseng wrote:
> Seems like these cases throughout should cause an error in js at least.

Done. I have it throw an exception for invalid arguments now. I don't see a need
for custom exception messages for each function since these are only called from
internal bindings.

For valid arguments but where the tree, node or attribute isn't found I think
returning undefined makes sense.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:267: void
AutomationInternalCustomBindings::GetChildIDAtIndex(
On 2015/06/10 17:48:16, David Tseng wrote:
> These functions should probably get docs; maybe jsdoc annotations.

Added concise docs to the header file now. Does that look good or would explicit
jsdoc be better?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:273: if
(args.Length() < 3 || !args[2]->IsNumber())
On 2015/06/11 15:05:38, aboxhall wrote:
> Do args validation before getting the node?

Done.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:365: if
(!*out_node)
On 2015/06/10 17:48:16, David Tseng wrote:
> GetNodeHelper?

Done.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:397: return;
On 2015/06/10 17:48:16, David Tseng wrote:
> Is return value here undefined in js?

Yes, the default will be undefined.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:453: void
AutomationInternalCustomBindings::OnAccessibilityEvent(
On 2015/06/11 15:05:38, aboxhall wrote:
> I can't figure out how this event gets dispatched. I see
> DispatchAccessibilityEvent() in AutomationEventRouter, but can't see where it
> gets called.

It gets called from AutomationWebContentsObserver::AccessibilityEventReceived
and from AutomationManagerAura::SendEvent, exactly the same as before. The only
difference is that now it's sending a low-level IPC rather than an extension
message.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:458: if (iter
== tree_id_to_tree_cache_map_.end()) {
On 2015/06/11 15:05:38, aboxhall wrote:
> I was trying to figure out where this got called so I could work out why it
> makes sense to always create a new TreeCache here. Presumably an event is only
> dispatched for trees at least one extension has requested?

Yeah, AutomationEventRouter keeps track of exactly what ax_tree_ids each
extension wants to listen to (and has permission to listen to).

Most of that landed in the previous change:
https://codereview.chromium.org/1151523009/

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:500: }
On 2015/06/10 17:48:15, David Tseng wrote:
> TODO?

Not necessarily needed? This was needed to implement the right semantics for the
HIDE event on Windows, but maybe we only really need nodeRemoved?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:504: 
On 2015/06/10 17:48:16, David Tseng wrote:
> TODO?

Not needed, added a comment.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:509: }
On 2015/06/10 17:48:15, David Tseng wrote:
> TODO?

Done.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:511: void
AutomationInternalCustomBindings::OnAtomicUpdateFinished(
On 2015/06/10 17:48:16, David Tseng wrote:
> What calls this function?

AXTree calls this because we implement AXTreeDelegate and call
cache->tree.SetDelegate(this).

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:529:
SendTreeChangeEvent("nodeChanged", tree, node);
On 2015/06/10 17:48:16, David Tseng wrote:
> Define/document these literals.

Switched to use the enum and ToString function auto-generated from the IDL. Yay
for type safety!

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.h (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.h:54: //
On 2015/06/11 15:05:38, aboxhall wrote:
> Contentless comment.
> 
> Were you going to add more comments for these? Would be nice to specify what
> args are expected.

Done.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.h:61: bool
GetNodeHelper(
On 2015/06/11 15:05:38, aboxhall wrote:
> Maybe group these by routed vs local?

Done.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:23:
requireNative('automationInternal').GetIntListAttribute;
On 2015/06/10 17:48:16, David Tseng wrote:
> jsdoc?

Done.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:392:
defineStringAttribute('accessKey');
On 2015/06/10 17:48:16, David Tseng wrote:
> Why are we doing this here? Can we pull this info from the idl?

This is the actual implementation. See the code for defineStringAttribute,
above.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:626:
//this.invalidate_(this.wrapper);
On 2015/06/10 17:48:16, David Tseng wrote:
> ?

Done.

https://codereview.chromium.org/1155183006/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:401: long scrollX;
On 2015/06/11 17:42:45, aboxhall wrote:
> One thing we were trying to avoid with mixins was the API explosion of having
> every possible attribute exposed on every automation node, and having a long
> list of attributes to parse at once in the documentation.
> 
> Is there any way we can keep this "mixin" style interface, but have it
> implemented more efficiently?

Rather than mixins, I think we could bundle attributes together in sub-objects.

For example, instead of node.scrollXMin, etc. you'd have node.scroll.xMin.

If you access node.scroll, it'd return an object if it has scrollable
interfaces, or undefined if it doesn't.

So you could write code like:

if (node.scroll) {
  outputScrollPosition(node.scroll.x, node.scroll.y);
}

Something like that sound good as a follow-up?

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:39:
this.attributes = {};
On 2015/06/11 17:42:46, aboxhall wrote:
> Can this be removed too?

Yes! done.

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:273:
console.error('Error in event handler for ' + event.type +
On 2015/06/11 17:42:46, aboxhall wrote:
> Maybe logging.WARN here as well (I know you didn't add this one - I probably
> did).

Done.

[aboxhall, 2015-06-11 20:32:42]

Generally looking good; will wait for the final version to stamp.

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

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:189: if
(args.Length() != 1)
On 2015/06/11 19:09:12, dmazzoni wrote:
> On 2015/06/10 17:48:16, David Tseng wrote:
> > Seems like these cases throughout should cause an error in js at least.
> 
> Done. I have it throw an exception for invalid arguments now. I don't see a
need
> for custom exception messages for each function since these are only called
from
> internal bindings.
> 
> For valid arguments but where the tree, node or attribute isn't found I think
> returning undefined makes sense.

Looks like other custom bindings code uses CHECKs for this, e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex....
I think that makes sense, because it's a "should never happen" case because the
caller is our code.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:458: if (iter
== tree_id_to_tree_cache_map_.end()) {
On 2015/06/11 19:09:11, dmazzoni wrote:
> On 2015/06/11 15:05:38, aboxhall wrote:
> > I was trying to figure out where this got called so I could work out why it
> > makes sense to always create a new TreeCache here. Presumably an event is
only
> > dispatched for trees at least one extension has requested?
> 
> Yeah, AutomationEventRouter keeps track of exactly what ax_tree_ids each
> extension wants to listen to (and has permission to listen to).

Ah, I see it now. Technically the permissions check happens before
AutomationEventRouter::RegisterListener*() gets called, so we're kind of
dependent on things happening in the right order to make sure permissions get
enforced. Perhaps a follow-up might be to move that permissions check into
AutomationEventRouter?

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:500: }
On 2015/06/11 19:09:12, dmazzoni wrote:
> On 2015/06/10 17:48:15, David Tseng wrote:
> > TODO?
> 
> Not necessarily needed? This was needed to implement the right semantics for
the
> HIDE event on Windows, but maybe we only really need nodeRemoved?
> 

Could these null implementations be moved up into ax_tree.cc? Or would that be
too confusing?

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:396:
defineStringAttribute('accessKey');
On 2015/06/11 17:42:45, aboxhall wrote:
> Maybe create arrays of each type of attribute and loop over them, rather than
> this long list of method calls? (Use dicts for node/nodelist attributes,
> obviously.)

Ping on this.

https://codereview.chromium.org/1155183006/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/api/automation_internal/automation_event_router.cc
(right):

https://codereview.chromium.org/1155183006/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/automation_internal/automation_event_router.cc:65:
if (rph) {
Could you add a comment as to why rph may not exist?

[aboxhall, 2015-06-11 22:22:34]

https://codereview.chromium.org/1155183006/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:401: long scrollX;
On 2015/06/11 19:09:12, dmazzoni wrote:
> On 2015/06/11 17:42:45, aboxhall wrote:
> > One thing we were trying to avoid with mixins was the API explosion of
having
> > every possible attribute exposed on every automation node, and having a long
> > list of attributes to parse at once in the documentation.
> > 
> > Is there any way we can keep this "mixin" style interface, but have it
> > implemented more efficiently?
> 
> Rather than mixins, I think we could bundle attributes together in
sub-objects.
> 
> For example, instead of node.scrollXMin, etc. you'd have node.scroll.xMin.
> 
> If you access node.scroll, it'd return an object if it has scrollable
> interfaces, or undefined if it doesn't.
> 
> So you could write code like:
> 
> if (node.scroll) {
>   outputScrollPosition(node.scroll.x, node.scroll.y);
> }
> 
> Something like that sound good as a follow-up?

Sorry, missed this in my responses earlier.

Sure, something like this should work. As long as we have a solution to the two
problems I mentioned, I don't mind that much what it looks like. When do you
think you might get to that follow-up?

[dmazzoni, 2015-06-12 17:51:36]

I think all feedback is addressed now!

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

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:189: if
(args.Length() != 1)
On 2015/06/11 20:32:42, aboxhall wrote:
> On 2015/06/11 19:09:12, dmazzoni wrote:
> > On 2015/06/10 17:48:16, David Tseng wrote:
> > > Seems like these cases throughout should cause an error in js at least.
> > 
> > Done. I have it throw an exception for invalid arguments now. I don't see a
> need
> > for custom exception messages for each function since these are only called
> from
> > internal bindings.
> > 
> > For valid arguments but where the tree, node or attribute isn't found I
think
> > returning undefined makes sense.
> 
> Looks like other custom bindings code uses CHECKs for this, e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex....
> I think that makes sense, because it's a "should never happen" case because
the
> caller is our code.

That's pretty hard to debug, though - there's no way to trace who called it from
the C++ stack trace. Given how complicated these interfaces are, I think a JS
exception makes more sense.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:458: if (iter
== tree_id_to_tree_cache_map_.end()) {
On 2015/06/11 20:32:42, aboxhall wrote:
> On 2015/06/11 19:09:11, dmazzoni wrote:
> > On 2015/06/11 15:05:38, aboxhall wrote:
> > > I was trying to figure out where this got called so I could work out why
it
> > > makes sense to always create a new TreeCache here. Presumably an event is
> only
> > > dispatched for trees at least one extension has requested?
> > 
> > Yeah, AutomationEventRouter keeps track of exactly what ax_tree_ids each
> > extension wants to listen to (and has permission to listen to).
> 
> Ah, I see it now. Technically the permissions check happens before
> AutomationEventRouter::RegisterListener*() gets called, so we're kind of
> dependent on things happening in the right order to make sure permissions get
> enforced. Perhaps a follow-up might be to move that permissions check into
> AutomationEventRouter?

I'm not sure how to do that because AutomationEventRouter is working at the
level of process IDs and routing IDs, or RenderProcesses and RenderViewHosts - I
couldn't find a way to get from the RenderViewHost of an extension background
page to its Extension object and therefore its permissions. It's usually only
done in one direction.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:500: }
On 2015/06/11 20:32:42, aboxhall wrote:
> On 2015/06/11 19:09:12, dmazzoni wrote:
> > On 2015/06/10 17:48:15, David Tseng wrote:
> > > TODO?
> > 
> > Not necessarily needed? This was needed to implement the right semantics for
> the
> > HIDE event on Windows, but maybe we only really need nodeRemoved?
> > 
> 
> Could these null implementations be moved up into ax_tree.cc? Or would that be
> too confusing?

To be clear, these are pure virtual functions in AXTreeDelegate, so they're
required to be implemented. There are only four of them so I don't think it's a
big deal to occasionally leave one blank.

The other alternative would be to make them not pure virtual.

There's no clear rule for this, but in general I favor pure virtual when *most*
implementers of an interface probably want to implement *most* of the
interfaces, which is clearly true here. It means that if you add a new function
to the interface, you're forced to consider all of the implementers of that
interface and see whether it applies to them.

The other extreme is interfaces where *most* implementers only need a handful of
the interfaces. A good example of that is WebContentsDelegate.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation_custom_bindings.js:168: // TODO:
delete AutomationNode if 'removeNode' event
On 2015/06/10 17:48:16, David Tseng wrote:
> ?

Done. I needed to remove the mapping from the id to the AutomationNodeImpl
inside AutomationRootNodeImpl.

https://codereview.chromium.org/1155183006/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:401: long scrollX;
On 2015/06/11 22:22:34, aboxhall wrote:
> On 2015/06/11 19:09:12, dmazzoni wrote:
> > On 2015/06/11 17:42:45, aboxhall wrote:
> > > One thing we were trying to avoid with mixins was the API explosion of
> having
> > > every possible attribute exposed on every automation node, and having a
long
> > > list of attributes to parse at once in the documentation.
> > > 
> > > Is there any way we can keep this "mixin" style interface, but have it
> > > implemented more efficiently?
> > 
> > Rather than mixins, I think we could bundle attributes together in
> sub-objects.
> > 
> > For example, instead of node.scrollXMin, etc. you'd have node.scroll.xMin.
> > 
> > If you access node.scroll, it'd return an object if it has scrollable
> > interfaces, or undefined if it doesn't.
> > 
> > So you could write code like:
> > 
> > if (node.scroll) {
> >   outputScrollPosition(node.scroll.x, node.scroll.y);
> > }
> > 
> > Something like that sound good as a follow-up?
> 
> Sorry, missed this in my responses earlier.
> 
> Sure, something like this should work. As long as we have a solution to the
two
> problems I mentioned, I don't mind that much what it looks like. When do you
> think you might get to that follow-up?

I'll send out a proposal for just one set of attributes (former mixin) right
away, to follow this change.

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation_custom_bindings.js:144: var
childTreeID = GetIntAttribute(treeID, nodeID, 'childTreeId');
On 2015/06/11 17:42:46, aboxhall wrote:
> Can you add a comment explaining what's going on in this block?

Done. This code used to be in automation_node.js.

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation_custom_bindings.js:165:
console.error('Error in tree change observer for ' +
On 2015/06/11 17:42:46, aboxhall wrote:
> Maybe use logging.WARNING - logs to stderr.
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...
> 
> Extension developers will be able to see console.logs (though that may be what
> we want here?)

This is intended for extension developers. Which is better?

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/automation/tests/tabs/attributes.js
(left):

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/attributes.js:58:
assertTrue('owns' in combobox, 'combobox should have an owns attribute');
On 2015/06/11 17:42:46, aboxhall wrote:
> Are these things no longer true?

Remember, aria-owns isn't exposed as an attribute anymore, on purpose. This test
wasn't updated earlier because the whole test was disabled. Hopefully it isn't
flaky anymore.

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/automation/tests/tabs/attributes.js
(right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/attributes.js:57:
console.error('combobox: ' + combobox);
On 2015/06/11 17:42:46, aboxhall wrote:
> Stray console.errors

Done.

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/attributes.js:80:
assertEq(undefined, ariaLink.url);
On 2015/06/11 17:42:46, aboxhall wrote:
> Whoa, so it's present but undefined?

Yes. There's no way to make it "not present" without making it a first-class
JavaScript object, which is exactly what we don't want.

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/attributes.js:90: var
name = textField.name;
On 2015/06/11 17:42:46, aboxhall wrote:
> I'm confused as to why this has changed from id to name here, and id to
> description elsewhere.

Good catch, this should be description too. The test still passed because this
was just the error string if it failed.

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/automation/tests/tabs/sanity_check.js
(right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/sanity_check.js:8:
var result = JSON.parse(JSON.stringify(state));
On 2015/06/11 17:42:46, aboxhall wrote:
> So annoying there's not a better (simple) way to do this.

Acknowledged.

https://codereview.chromium.org/1155183006/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/api/automation_internal/automation_event_router.cc
(right):

https://codereview.chromium.org/1155183006/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/automation_internal/automation_event_router.cc:65:
if (rph) {
On 2015/06/11 20:32:42, aboxhall wrote:
> Could you add a comment as to why rph may not exist?

I was wrong. It should always exist because we explicitly catch
NOTIFICATION_RENDERER_PROCESS_TERMINATED below.

I changed this to a CHECK.

[dmazzoni, 2015-06-12 17:55:37]

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:396:
defineStringAttribute('accessKey');
On 2015/06/11 20:32:42, aboxhall wrote:
> On 2015/06/11 17:42:45, aboxhall wrote:
> > Maybe create arrays of each type of attribute and loop over them, rather
than
> > this long list of method calls? (Use dicts for node/nodelist attributes,
> > obviously.)
> 
> Ping on this.

Oops, this one's not included in the latest patch set. Coming shortly...

[dmazzoni, 2015-06-12 18:08:00]

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:396:
defineStringAttribute('accessKey');
On 2015/06/12 17:55:37, dmazzoni wrote:
> On 2015/06/11 20:32:42, aboxhall wrote:
> > On 2015/06/11 17:42:45, aboxhall wrote:
> > > Maybe create arrays of each type of attribute and loop over them, rather
> than
> > > this long list of method calls? (Use dicts for node/nodelist attributes,
> > > obviously.)
> > 
> > Ping on this.
> 
> Oops, this one's not included in the latest patch set. Coming shortly...

Done now, is this better?

[aboxhall, 2015-06-12 20:43:10]

LGTM with one open question (CHECK vs exception). Happy to go with whatever you
decide on that, though.

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

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:189: if
(args.Length() != 1)
On 2015/06/12 17:51:35, dmazzoni wrote:
> On 2015/06/11 20:32:42, aboxhall wrote:
> > On 2015/06/11 19:09:12, dmazzoni wrote:
> > > On 2015/06/10 17:48:16, David Tseng wrote:
> > > > Seems like these cases throughout should cause an error in js at least.
> > > 
> > > Done. I have it throw an exception for invalid arguments now. I don't see
a
> > need
> > > for custom exception messages for each function since these are only
called
> > from
> > > internal bindings.
> > > 
> > > For valid arguments but where the tree, node or attribute isn't found I
> think
> > > returning undefined makes sense.
> > 
> > Looks like other custom bindings code uses CHECKs for this, e.g.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex....
> > I think that makes sense, because it's a "should never happen" case because
> the
> > caller is our code.
> 
> That's pretty hard to debug, though - there's no way to trace who called it
from
> the C++ stack trace. Given how complicated these interfaces are, I think a JS
> exception makes more sense.

With a CHECK, it'll be super obvious that something terribly wrong has happened
and it won't just silently get into an inconsistent state. Can we make that
guarantee with an exception? If so I'm happy to go with that. With a CHECK, we
can always make a local modification to debug if we start getting crash reports.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:458: if (iter
== tree_id_to_tree_cache_map_.end()) {
On 2015/06/12 17:51:35, dmazzoni wrote:
> On 2015/06/11 20:32:42, aboxhall wrote:
> > On 2015/06/11 19:09:11, dmazzoni wrote:
> > > On 2015/06/11 15:05:38, aboxhall wrote:
> > > > I was trying to figure out where this got called so I could work out why
> it
> > > > makes sense to always create a new TreeCache here. Presumably an event
is
> > only
> > > > dispatched for trees at least one extension has requested?
> > > 
> > > Yeah, AutomationEventRouter keeps track of exactly what ax_tree_ids each
> > > extension wants to listen to (and has permission to listen to).
> > 
> > Ah, I see it now. Technically the permissions check happens before
> > AutomationEventRouter::RegisterListener*() gets called, so we're kind of
> > dependent on things happening in the right order to make sure permissions
get
> > enforced. Perhaps a follow-up might be to move that permissions check into
> > AutomationEventRouter?
> 
> I'm not sure how to do that because AutomationEventRouter is working at the
> level of process IDs and routing IDs, or RenderProcesses and RenderViewHosts -
I
> couldn't find a way to get from the RenderViewHost of an extension background
> page to its Extension object and therefore its permissions. It's usually only
> done in one direction.

Right, I see. I misunderstood the 'has permission to' parenthetical earlier -
the AER doesn't keep track of what permissions the process has per se, but
trusts that it has them when RegisterListener is called.

Incidentally, this isn't new in this change, but we should probably handle the
case where an extension revokes permissions to a particular host - as it stands,
I think we'll keep sending tree updates even after permissions are revoked. We
should probably unregister the listener at that point.

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:500: }
On 2015/06/12 17:51:35, dmazzoni wrote:
> On 2015/06/11 20:32:42, aboxhall wrote:
> > On 2015/06/11 19:09:12, dmazzoni wrote:
> > > On 2015/06/10 17:48:15, David Tseng wrote:
> > > > TODO?
> > > 
> > > Not necessarily needed? This was needed to implement the right semantics
for
> > the
> > > HIDE event on Windows, but maybe we only really need nodeRemoved?
> > > 
> > 
> > Could these null implementations be moved up into ax_tree.cc? Or would that
be
> > too confusing?
> 
> To be clear, these are pure virtual functions in AXTreeDelegate, so they're
> required to be implemented. There are only four of them so I don't think it's
a
> big deal to occasionally leave one blank.
> 
> The other alternative would be to make them not pure virtual.
> 
> There's no clear rule for this, but in general I favor pure virtual when
*most*
> implementers of an interface probably want to implement *most* of the
> interfaces, which is clearly true here. It means that if you add a new
function
> to the interface, you're forced to consider all of the implementers of that
> interface and see whether it applies to them.
> 
> The other extreme is interfaces where *most* implementers only need a handful
of
> the interfaces. A good example of that is WebContentsDelegate.

Makes sense - I wasn't sure to what extent we generally expected these to be
non-null implementations.

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1155183006/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_node.js:396:
defineStringAttribute('accessKey');
On 2015/06/12 18:08:00, dmazzoni wrote:
> On 2015/06/12 17:55:37, dmazzoni wrote:
> > On 2015/06/11 20:32:42, aboxhall wrote:
> > > On 2015/06/11 17:42:45, aboxhall wrote:
> > > > Maybe create arrays of each type of attribute and loop over them, rather
> > than
> > > > this long list of method calls? (Use dicts for node/nodelist attributes,
> > > > obviously.)
> > > 
> > > Ping on this.
> > 
> > Oops, this one's not included in the latest patch set. Coming shortly...
> 
> Done now, is this better?

This looks great, thanks!

[David Tseng, 2015-06-15 21:02:11]

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:189: void
AutomationInternalCustomBindings::DestroyAccessibilityTree(
nit: can combine with above

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation/automation_node.js:691: return
result;
nit: return result || undefined;

[David Tseng, 2015-06-15 21:02:32]

lgtm

[dcheng, 2015-06-15 21:36:56]

Mostly a bunch of minor nits.

https://codereview.chromium.org/1155183006/diff/160001/chrome/browser/extensi...
File
chrome/browser/extensions/api/automation_internal/automation_event_router.cc
(right):

https://codereview.chromium.org/1155183006/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/automation_internal/automation_event_router.cc:65:
CHECK(rph);
What's the point of this CHECK?

https://codereview.chromium.org/1155183006/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/automation_internal/automation_event_router.cc:136:
[process_id](AutomationListener& item) {
const AutomationListener& to prevent typos like the one on line 137?

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:126: }
#undef ROUTE_FUNCTION

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:131: }
Do we need to delete TreeCache pointers when this is destroyed?

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:286:
scoped_ptr<base::DictionaryValue> state_dict(new base::DictionaryValue());
Just create this directly on the stack.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:405:
scoped_ptr<base::ListValue> list(new base::ListValue());
Create this directly on the stack as well.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:471:
base::Value* value) {
Pass this by const reference, since I don't think you actually need to mutate it
here.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:472:
v8::Isolate* isolate = context()->isolate();
Why do we use context()->isolate(), args.GetIsolate(), and GetIsolate()
throughout this file?

It seems like we should have one, but probably not all 3.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:475:
scoped_ptr<content::V8ValueConverter> converter(
Ditto: just create this on the stack.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:512:
scoped_ptr<base::ListValue> args(new base::ListValue());
Create this on the stack.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:523:
CHECK(v8_args->IsArray());
DCHECK? This should basically never be true.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:597:
scoped_ptr<base::ListValue> args(new base::ListValue());
Create this on the stack.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:610:
CHECK(v8_args->IsArray());
Ditto: this should be DCHECK instead of CHECK>

[dmazzoni, 2015-06-16 18:19:49]

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

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:189: if
(args.Length() != 1)
On 2015/06/12 20:43:10, aboxhall wrote:
> On 2015/06/12 17:51:35, dmazzoni wrote:
> > On 2015/06/11 20:32:42, aboxhall wrote:
> > > On 2015/06/11 19:09:12, dmazzoni wrote:
> > > > On 2015/06/10 17:48:16, David Tseng wrote:
> > > > > Seems like these cases throughout should cause an error in js at
least.
> > > > 
> > > > Done. I have it throw an exception for invalid arguments now. I don't
see
> a
> > > need
> > > > for custom exception messages for each function since these are only
> called
> > > from
> > > > internal bindings.
> > > > 
> > > > For valid arguments but where the tree, node or attribute isn't found I
> > think
> > > > returning undefined makes sense.
> > > 
> > > Looks like other custom bindings code uses CHECKs for this, e.g.
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex....
> > > I think that makes sense, because it's a "should never happen" case
because
> > the
> > > caller is our code.
> > 
> > That's pretty hard to debug, though - there's no way to trace who called it
> from
> > the C++ stack trace. Given how complicated these interfaces are, I think a
JS
> > exception makes more sense.
> 
> With a CHECK, it'll be super obvious that something terribly wrong has
happened
> and it won't just silently get into an inconsistent state. Can we make that
> guarantee with an exception? If so I'm happy to go with that. With a CHECK, we
> can always make a local modification to debug if we start getting crash
reports.

OK, solved both problems. Now it grabs a JS stack trace and then does a
LOG(FATAL). Best of both worlds.

https://codereview.chromium.org/1155183006/diff/160001/chrome/browser/extensi...
File
chrome/browser/extensions/api/automation_internal/automation_event_router.cc
(right):

https://codereview.chromium.org/1155183006/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/automation_internal/automation_event_router.cc:65:
CHECK(rph);
On 2015/06/15 21:36:54, dcheng wrote:
> What's the point of this CHECK?

Done.

https://codereview.chromium.org/1155183006/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/automation_internal/automation_event_router.cc:136:
[process_id](AutomationListener& item) {
On 2015/06/15 21:36:55, dcheng wrote:
> const AutomationListener& to prevent typos like the one on line 137?

Done.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:126: }
On 2015/06/15 21:36:55, dcheng wrote:
> #undef ROUTE_FUNCTION

Done.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:131: }
On 2015/06/15 21:36:55, dcheng wrote:
> Do we need to delete TreeCache pointers when this is destroyed?

Done.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:286:
scoped_ptr<base::DictionaryValue> state_dict(new base::DictionaryValue());
On 2015/06/15 21:36:55, dcheng wrote:
> Just create this directly on the stack.

Done.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:405:
scoped_ptr<base::ListValue> list(new base::ListValue());
On 2015/06/15 21:36:55, dcheng wrote:
> Create this directly on the stack as well.

Done.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:471:
base::Value* value) {
On 2015/06/15 21:36:55, dcheng wrote:
> Pass this by const reference, since I don't think you actually need to mutate
it
> here.

Done.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:472:
v8::Isolate* isolate = context()->isolate();
On 2015/06/15 21:36:55, dcheng wrote:
> Why do we use context()->isolate(), args.GetIsolate(), and GetIsolate()
> throughout this file?
> 
> It seems like we should have one, but probably not all 3.

Good idea. Switched to GetIsolate() throughout.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:475:
scoped_ptr<content::V8ValueConverter> converter(
On 2015/06/15 21:36:55, dcheng wrote:
> Ditto: just create this on the stack.

Can't, there's no public constructor.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:512:
scoped_ptr<base::ListValue> args(new base::ListValue());
On 2015/06/15 21:36:55, dcheng wrote:
> Create this on the stack.

Done.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:523:
CHECK(v8_args->IsArray());
On 2015/06/15 21:36:55, dcheng wrote:
> DCHECK? This should basically never be true.

Done.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:597:
scoped_ptr<base::ListValue> args(new base::ListValue());
On 2015/06/15 21:36:55, dcheng wrote:
> Create this on the stack.

Done.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:610:
CHECK(v8_args->IsArray());
On 2015/06/15 21:36:55, dcheng wrote:
> Ditto: this should be DCHECK instead of CHECK>

Done.

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1155183006/diff/160001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation/automation_node.js:691: return
result;
On 2015/06/15 21:02:11, David Tseng wrote:
> nit: return result || undefined;

Done.

[aboxhall, 2015-06-16 18:22:44]

lgtm

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

https://codereview.chromium.org/1155183006/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:189: if
(args.Length() != 1)
On 2015/06/16 18:19:48, dmazzoni wrote:
> On 2015/06/12 20:43:10, aboxhall wrote:
> > On 2015/06/12 17:51:35, dmazzoni wrote:
> > > On 2015/06/11 20:32:42, aboxhall wrote:
> > > > On 2015/06/11 19:09:12, dmazzoni wrote:
> > > > > On 2015/06/10 17:48:16, David Tseng wrote:
> > > > > > Seems like these cases throughout should cause an error in js at
> least.
> > > > > 
> > > > > Done. I have it throw an exception for invalid arguments now. I don't
> see
> > a
> > > > need
> > > > > for custom exception messages for each function since these are only
> > called
> > > > from
> > > > > internal bindings.
> > > > > 
> > > > > For valid arguments but where the tree, node or attribute isn't found
I
> > > think
> > > > > returning undefined makes sense.
> > > > 
> > > > Looks like other custom bindings code uses CHECKs for this, e.g.
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex....
> > > > I think that makes sense, because it's a "should never happen" case
> because
> > > the
> > > > caller is our code.
> > > 
> > > That's pretty hard to debug, though - there's no way to trace who called
it
> > from
> > > the C++ stack trace. Given how complicated these interfaces are, I think a
> JS
> > > exception makes more sense.
> > 
> > With a CHECK, it'll be super obvious that something terribly wrong has
> happened
> > and it won't just silently get into an inconsistent state. Can we make that
> > guarantee with an exception? If so I'm happy to go with that. With a CHECK,
we
> > can always make a local modification to debug if we start getting crash
> reports.
> 
> OK, solved both problems. Now it grabs a JS stack trace and then does a
> LOG(FATAL). Best of both worlds.
> 

Nice one :)

[dmazzoni, 2015-06-16 21:59:17]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-06-16 21:59:18]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/1155183006/#ps200001
(title: "Add support for input type html attribute")

[commit-bot: I haz the power, 2015-06-16 22:01:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1155183006/200001

[commit-bot: I haz the power, 2015-06-16 22:13:04]

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

[commit-bot: I haz the power, 2015-06-16 22:13:06]

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

[dmazzoni, 2015-06-16 22:17:24]

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

[dmazzoni, 2015-06-16 22:17:26]

+kalman for:

    chrome/renderer/extensions/automation_internal_custom_bindings.cc
chrome/renderer/extensions/automation_internal_custom_bindings.h
chrome/renderer/resources/extensions/automation_custom_bindings.js

[not at google - send to devlin, 2015-06-17 21:14:48]

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:139: const
IPC::Message& message) {
1. There doesn't seem much point this method imitating the OnMessageReceived
interface anymore, specifically, the way it returns a bool. The comment should
be moved up to the "return false" on AutomationMessageFilter::OnMessageReceived.

2. Given the above point, isn't there some way of saying that some message must
be handled on some thread, and then returning a bool makes sense again?

3. There should probably be a guard against is_valid() in here. There should
always have been, in fact. I wonder where else we have this problem.

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:224: const
auto& iter = tree_id_to_tree_cache_map_.find(tree_id);
I think this shouldn't be a const reference, just a copy of the iterator. Same
elsewhere.

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:235:
ui::AXNode* node;
Always nice to initialise pointers to null.

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:443:
std::string trace;
This stringify-stack-trace utility looks handy. In fact we do something similar
in other place(s). Could you move it to ScriptContext? (not the
exception-throwing part, which I'm a little uneasy about but I'll leave it alone
- just the stringify part).

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:446: for
(size_t i = 0; i < (size_t)stack_trace->GetFrameCount(); ++i) {
static_cast<size_t> not (size_t)?

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:640:
content::V8ValueConverter::create());
You should avoid dealing with base::Values as much as possible, if you can avoid
it, which I think you can. It adds a layer of inefficiency and churn, as does
dealing with V8ValueConverter.

I.e. if ultimately what you want is as v8::Value then construct it directly.

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation_custom_bindings.js:152:
privates(root).impl.hostNode = node;
Glancing at this code (and not understanding it), the frequency of
"privates(root).impl" makes me think whatever this is doing should be pulled
into the class itself. Like,
privates(root).impl.setHostNodeForSomethingSomething(node).

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation_custom_bindings.js:165: var
observers = automationUtil.treeChangeObservers;
I don't know where this code came from, but note that extension events have had
problems with listeners adding or removing other listeners from within their
callbacks, which this code isn't safe against.

[dcheng, 2015-06-17 22:55:09]

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:446: for
(size_t i = 0; i < (size_t)stack_trace->GetFrameCount(); ++i) {
On 2015/06/17 at 21:14:48, kalman wrote:
> static_cast<size_t> not (size_t)?

How about just using int instead of casting?

[dmazzoni, 2015-06-18 07:23:54]

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:139: const
IPC::Message& message) {
On 2015/06/17 21:14:48, kalman wrote:
> 1. There doesn't seem much point this method imitating the OnMessageReceived
> interface anymore, specifically, the way it returns a bool. The comment should
> be moved up to the "return false" on
AutomationMessageFilter::OnMessageReceived.

Good call, done.

> 2. Given the above point, isn't there some way of saying that some message
must
> be handled on some thread, and then returning a bool makes sense again?

I don't think it makes sense for it to be an error if nobody handles the
message. It's not hard to imagine a scenario where this
AutomationInternalCustomBindings object gets deleted but there are still IPCs in
flight destined for this process.

> 3. There should probably be a guard against is_valid() in here. There should
> always have been, in fact. I wonder where else we have this problem.

Sorry, I don't understand - I don't see is_valid() checks in most message
handlers, can you point me to an example?

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:224: const
auto& iter = tree_id_to_tree_cache_map_.find(tree_id);
On 2015/06/17 21:14:47, kalman wrote:
> I think this shouldn't be a const reference, just a copy of the iterator. Same
> elsewhere.

Done.

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:235:
ui::AXNode* node;
On 2015/06/17 21:14:48, kalman wrote:
> Always nice to initialise pointers to null.

Done.

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:443:
std::string trace;
On 2015/06/17 21:14:48, kalman wrote:
> This stringify-stack-trace utility looks handy. In fact we do something
similar
> in other place(s). Could you move it to ScriptContext? (not the
> exception-throwing part, which I'm a little uneasy about but I'll leave it
alone
> - just the stringify part).

Done.

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:446: for
(size_t i = 0; i < (size_t)stack_trace->GetFrameCount(); ++i) {
On 2015/06/17 22:55:09, dcheng wrote:
> On 2015/06/17 at 21:14:48, kalman wrote:
> > static_cast<size_t> not (size_t)?
> 
> How about just using int instead of casting?

Done.

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:640:
content::V8ValueConverter::create());
On 2015/06/17 21:14:47, kalman wrote:
> You should avoid dealing with base::Values as much as possible, if you can
avoid
> it, which I think you can. It adds a layer of inefficiency and churn, as does
> dealing with V8ValueConverter.
> 
> I.e. if ultimately what you want is as v8::Value then construct it directly.

OK, done. I got rid of all uses of V8ValueConverter except one, because we
already get base::Values as input to OnAccessibilityEvent.

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation_custom_bindings.js:152:
privates(root).impl.hostNode = node;
On 2015/06/17 21:14:48, kalman wrote:
> Glancing at this code (and not understanding it), the frequency of
> "privates(root).impl" makes me think whatever this is doing should be pulled
> into the class itself. Like,
> privates(root).impl.setHostNodeForSomethingSomething(node).

Done.

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation_custom_bindings.js:165: var
observers = automationUtil.treeChangeObservers;
On 2015/06/17 21:14:48, kalman wrote:
> I don't know where this code came from, but note that extension events have
had
> problems with listeners adding or removing other listeners from within their
> callbacks, which this code isn't safe against.

Good idea. I changed it to make a copy of the array first.

[not at google - send to devlin, 2015-06-18 17:34:38]

Sorry about the second round of non-trivial reviews.

The V8ValueBuilder stuff is more a wish of course, and I don't expect you to do
it. I might though, it looks like fun.

If you're comfortable to submit this without me having another look, lgtm.
However also happy to glance again later.

You might want a second opinion about the JS there as well regarding the JSDoc.

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation_custom_bindings.js:152:
privates(root).impl.hostNode = node;
On 2015/06/18 07:23:53, dmazzoni wrote:
> On 2015/06/17 21:14:48, kalman wrote:
> > Glancing at this code (and not understanding it), the frequency of
> > "privates(root).impl" makes me think whatever this is doing should be pulled
> > into the class itself. Like,
> > privates(root).impl.setHostNodeForSomethingSomething(node).
> 
> Done.

Is there a semantically reasonable function that could fire those
privates(root).impl.dispatchEvent() functions as well?

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/extens...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:333:
v8::Integer::New(GetIsolate(), location.height()));
(musing) I wish we had a class like
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
for v8 values. It would cut out so much boilerplate... everywhere.

args.GetReturnValue().Set(V8ObjectBuilder(GetIsolate())
  .Set("left", location.x())
  .Set("top", location.y())
  .Set("width", location.width())
  .Set("height", location.height())
  .Build());

imagine that...

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:497:
v8::Local<v8::Value> AutomationInternalCustomBindings::CreateV8String(
Pity that https://codereview.chromium.org/1185443004/ hasn't landed yet,
v8_helpers.h provides a safer version of this.

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:498: const
std::string& str) {
Also, it seems silly that fairly often you're converting from a const char* to a
std::string to pass in here, then going around and converting back to a const
char* again. Perhaps provide a const char* version of this method?

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:535:
base::ListValue args;
Honestly, type safety be damned, I'd just put a "nocompile" attribute on
AXEventParams in automation.idl and construct this by hand. It's so trivial.

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation/automation_node.js:707:
this.hostNode = null;
Initialisation of these properties should go in the prototype I think, and be
given JSDoc annotations. I arrived here in exploring how hostNode is used and
whether it's supposed to be private. Is it?

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation_custom_bindings.js:170:
observers.push(automationUtil.treeChangeObservers[i]);
You can also do just "var observers =
automationUtil.treeChangeObservers.slice();"

https://codereview.chromium.org/1155183006/diff/220001/extensions/renderer/lo...
File extensions/renderer/logging_native_handler.cc (right):

https://codereview.chromium.org/1155183006/diff/220001/extensions/renderer/lo...
extensions/renderer/logging_native_handler.cc:75: *error_message += "\n" +
context()->GetStackTraceAsString();
Thank you :-)

[dmazzoni, 2015-06-18 21:31:53]

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/1155183006/diff/200001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation_custom_bindings.js:152:
privates(root).impl.hostNode = node;
On 2015/06/18 17:34:38, kalman wrote:
> Is there a semantically reasonable function that could fire those
> privates(root).impl.dispatchEvent() functions as well?

I'm not sure why - I consider automation_custom_bindings.js and
automation_node.js to be almost friend classes - they're both implementing
private impl classes that the client doesn't see, I don't see any reason to add
layers of abstraction around calling a function on AutomationNodeImpl vs one on
AutomationNode. Or am I misunderstanding your concern?

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/extens...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:333:
v8::Integer::New(GetIsolate(), location.height()));
On 2015/06/18 17:34:38, kalman wrote:
> (musing) I wish we had a class like
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
> for v8 values. It would cut out so much boilerplate... everywhere.
> 
> args.GetReturnValue().Set(V8ObjectBuilder(GetIsolate())
>   .Set("left", location.x())
>   .Set("top", location.y())
>   .Set("width", location.width())
>   .Set("height", location.height())
>   .Build());
> 
> imagine that...

Sounds nice!

In this particular case I don't anticipate needing very many more interfaces -
this is essentially complete now, so I don't want to overengineer it either.

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:498: const
std::string& str) {
On 2015/06/18 17:34:38, kalman wrote:
> Also, it seems silly that fairly often you're converting from a const char* to
a
> std::string to pass in here, then going around and converting back to a const
> char* again. Perhaps provide a const char* version of this method?

Done.

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:535:
base::ListValue args;
On 2015/06/18 17:34:38, kalman wrote:
> Honestly, type safety be damned, I'd just put a "nocompile" attribute on
> AXEventParams in automation.idl and construct this by hand. It's so trivial.

You're right, it even ends up being less code. Done.

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation/automation_node.js:707:
this.hostNode = null;
On 2015/06/18 17:34:38, kalman wrote:
> Initialisation of these properties should go in the prototype I think, and be
> given JSDoc annotations. I arrived here in exploring how hostNode is used and
> whether it's supposed to be private. Is it?

Done - moved to the prototype, made private, and added jsdoc.

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation_custom_bindings.js:170:
observers.push(automationUtil.treeChangeObservers[i]);
On 2015/06/18 17:34:38, kalman wrote:
> You can also do just "var observers =
> automationUtil.treeChangeObservers.slice();"

Done.

https://codereview.chromium.org/1155183006/diff/220001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation_custom_bindings.js:170:
observers.push(automationUtil.treeChangeObservers[i]);
On 2015/06/18 17:34:38, kalman wrote:
> You can also do just "var observers =
> automationUtil.treeChangeObservers.slice();"

Done.

[not at google - send to devlin, 2015-06-18 22:52:16]

lgtm

[dmazzoni, 2015-06-18 23:36:50]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-06-18 23:36:50]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/1155183006/#ps280001
(title: "Copy observers")

[commit-bot: I haz the power, 2015-06-18 23:39:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1155183006/280001

[commit-bot: I haz the power, 2015-06-19 00:55:03]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-06-19 00:56:02]

Patchset 15 (id:??) landed as
https://crrev.com/0e90449a7c9bd8f051fce4988a8cccd6c1c64661
Cr-Commit-Position: refs/heads/master@{#335183}

[dmazzoni, 2015-06-19 20:59:25]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/1198613002/ by dmazzoni@chromium.org.

The reason for reverting is: Reverting as likely cause of bug 502311
.