Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
203753002:
Implement actions for Automation API. (Closed)
|
Created:
6 years, 9 months ago by David Tseng Modified:
6 years, 8 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, joi+watch-content_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, Peter Lundblad Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement actions for Automation API.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261975
BUG=309681
Patch Set 1 #Patch Set 2 : Initial patch. #
Total comments: 3
Patch Set 3 : Promote action IPC's up to public RWH header; add basic events. #Patch Set 4 : Rebase. #Patch Set 5 : Remove repricated API. #
Total comments: 40
Patch Set 6 : Address comments. #Patch Set 7 : Remove braces #
Total comments: 3
Patch Set 8 : Rebase. #
Total comments: 16
Patch Set 9 : Lots of style cleanup. #
Total comments: 12
Patch Set 10 : Small js cleanup. #Patch Set 11 : Rebase #Patch Set 12 : Rebase again... #Patch Set 13 : Fix bad merge. #Patch Set 14 : Disable test on Mac. #Messages
Total messages: 65 (0 generated)
Still W.I.P. Sending out now with a question for @jam. Thanks. https://codereview.chromium.org/203753002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/203753002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:15: #include "content/browser/renderer_host/render_widget_host_impl.h" This is an illegal include; @jam, what would you suggest in terms of plumming; it's four functions which I suppose we can add to WebContents similarly to how accessibility mode was done. Any other options?
https://codereview.chromium.org/203753002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/203753002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:15: #include "content/browser/renderer_host/render_widget_host_impl.h" On 2014/03/24 22:46:37, David Tseng wrote: > This is an illegal include; @jam, what would you suggest in terms of plumming; > it's four functions which I suppose we can add to WebContents similarly to how > accessibility mode was done. Any other options? since you need methods from RenderWidgetHostImpl, and it derives from a public interface RenderWidgetHost, promote these internal methods to the public interface (RWH)
https://codereview.chromium.org/203753002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/203753002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:15: #include "content/browser/renderer_host/render_widget_host_impl.h" On 2014/03/25 15:01:00, jam wrote: > On 2014/03/24 22:46:37, David Tseng wrote: > > This is an illegal include; @jam, what would you suggest in terms of plumming; > > it's four functions which I suppose we can add to WebContents similarly to how > > accessibility mode was done. Any other options? > > since you need methods from RenderWidgetHostImpl, and it derives from a public > interface RenderWidgetHost, promote these internal methods to the public > interface (RWH) Simple enough :). I wanted to be sure I was allowed to do so. There will likely be more of these in the future.
@aboxhall and @dmazzoni, this is ready for a look/review. I had to add basic per node eventing to add a basic sanity test. This cl also features a fix to our RWH resolution (need process id).
https://codereview.chromium.org/203753002/diff/100001/chrome/common/extension... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/203753002/diff/100001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:47: // content/browser/accessibility/browser_accessibility_manager.h:BrowserAccessibilityDelegate nit: wrap to 80 chars https://codereview.chromium.org/203753002/diff/100001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:69: long[] opt_args); What about args that aren't a long, is there a chance we'll have some in the future? https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:75: if (! this.listeners[eventType]) Nit: no space after ! https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:81: removeEventListener: function(callback) { Should this take eventType and capture as parameters too? https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:86: // Not yet initialized. Nit: indent https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_tree.js (right): https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_tree.js:41: * renderer/renderer host pair. What do you mean by renderer/renderer? https://codereview.chromium.org/203753002/diff/100001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/203753002/diff/100001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:419: virtual void AccessibilityDoDefaultAction(int object_id); Add OVERRIDE too?
https://codereview.chromium.org/203753002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/203753002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:90: int process_id = event.process_id; We may wish to consider combining these into some kind of unique ID on the browser side, analogous to/possibly reusing/extending tab id. Out of scope for this CL though. https://codereview.chromium.org/203753002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:252: int automation_id; node_id maybe? Or automation_node_id? https://codereview.chromium.org/203753002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:254: api::automation_internal::ActionType action_type = Nit: newline above this line https://codereview.chromium.org/203753002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:257: content::RenderWidgetHost* rwh = Nit: newline above this line (i.e. group declaring and getting arguments together, keep this separate). https://codereview.chromium.org/203753002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:261: case api::automation_internal::ACTION_TYPE_DO_DEFAULT: Nit: these case statements are all indented, but default is not. IIRC all of them should be indented. https://codereview.chromium.org/203753002/diff/100001/chrome/common/extension... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/203753002/diff/100001/chrome/common/extension... chrome/common/extensions/api/automation.idl:11: // A listener for events on an <code>AutomationNode</code>. Can you add a TODO to make this take an automation.Event? https://codereview.chromium.org/203753002/diff/100001/chrome/common/extension... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/203753002/diff/100001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:47: // content/browser/accessibility/browser_accessibility_manager.h:BrowserAccessibilityDelegate On 2014/04/01 17:05:25, dmazzoni wrote: > nit: wrap to 80 chars Difficult to do. I think this could stay as-is, or alternatively replace content/browser/accessibility with c/b/a or even just leave out? https://codereview.chromium.org/203753002/diff/100001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:69: long[] opt_args); On 2014/04/01 17:05:25, dmazzoni wrote: > What about args that aren't a long, is there a chance we'll have some in the > future? Suggestion: use an object for opt_args and pass by name (e.g. {"start_index": 0, "end_index": 10}). I think this pattern is used elsewhere in extension APIs. https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:85: performAction: function(actionType, opt_args) { Use performAction_ naming scheme for private methods, remove @private (to be consistent with other users of this pattern). https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:99: updateEventListeners: function(eventType) { notifyEventListeners_ ? https://codereview.chromium.org/203753002/diff/100001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/automation/sanity_check/test.js (right): https://codereview.chromium.org/203753002/diff/100001/chrome/test/data/extens... chrome/test/data/extensions/api_test/automation/sanity_check/test.js:100: var okButton = tree.root.firstChild().firstChild(); This suggests to me that we should spec out getting an automation node from a DOM node. This would also be generally useful anyway, and should be fairly straightforward to implement, I think. https://codereview.chromium.org/203753002/diff/100001/chrome/test/data/extens... chrome/test/data/extensions/api_test/automation/sanity_check/test.js:106: chrome.tabs.create({url: 'test.html'}); Can we use chrome.tabs.update to be consistent with above and avoid creating a new tab unnecessarily?
https://codereview.chromium.org/203753002/diff/100001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/203753002/diff/100001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:422: virtual void AccessibilitySetFocus(int object_id); Wait... where is the implementation for this? Not uploaded?
On 2014/04/01 20:20:45, aboxhall wrote: > https://codereview.chromium.org/203753002/diff/100001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_impl.h (right): > > https://codereview.chromium.org/203753002/diff/100001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_impl.h:422: virtual void > AccessibilitySetFocus(int object_id); > Wait... where is the implementation for this? Not uploaded? Never mind, I see what's happening now.
PTAL; thanks. https://codereview.chromium.org/203753002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/203753002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:90: int process_id = event.process_id; On 2014/04/01 17:49:51, aboxhall wrote: > We may wish to consider combining these into some kind of unique ID on the > browser side, analogous to/possibly reusing/extending tab id. Out of scope for > this CL though. I'm ok either way. Leaving them separate means we can easily address RWH for actions though without parsing/mapping back. https://codereview.chromium.org/203753002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:252: int automation_id; On 2014/04/01 17:49:51, aboxhall wrote: > node_id maybe? Or automation_node_id? Sure, automation_node_id. https://codereview.chromium.org/203753002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:254: api::automation_internal::ActionType action_type = On 2014/04/01 17:49:51, aboxhall wrote: > Nit: newline above this line Done. https://codereview.chromium.org/203753002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:257: content::RenderWidgetHost* rwh = On 2014/04/01 17:49:51, aboxhall wrote: > Nit: newline above this line (i.e. group declaring and getting arguments > together, keep this separate). Done. https://codereview.chromium.org/203753002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:261: case api::automation_internal::ACTION_TYPE_DO_DEFAULT: On 2014/04/01 17:49:51, aboxhall wrote: > Nit: these case statements are all indented, but default is not. IIRC all of > them should be indented. Done. (emacs c-mode tab indent failed me here). https://codereview.chromium.org/203753002/diff/100001/chrome/common/extension... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/203753002/diff/100001/chrome/common/extension... chrome/common/extensions/api/automation.idl:11: // A listener for events on an <code>AutomationNode</code>. On 2014/04/01 17:49:51, aboxhall wrote: > Can you add a TODO to make this take an automation.Event? Done. https://codereview.chromium.org/203753002/diff/100001/chrome/common/extension... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/203753002/diff/100001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:47: // content/browser/accessibility/browser_accessibility_manager.h:BrowserAccessibilityDelegate On 2014/04/01 17:49:51, aboxhall wrote: > On 2014/04/01 17:05:25, dmazzoni wrote: > > nit: wrap to 80 chars > > Difficult to do. I think this could stay as-is, or alternatively replace > content/browser/accessibility with c/b/a or even just leave out? I removed this TODO. I initially thought I could reuse the BrowserAccessibilityDelegate interface for actions, but it was easier to stick with defining everything on RWH. https://codereview.chromium.org/203753002/diff/100001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:69: long[] opt_args); On 2014/04/01 17:49:51, aboxhall wrote: > On 2014/04/01 17:05:25, dmazzoni wrote: > > What about args that aren't a long, is there a chance we'll have some in the > > future? > > Suggestion: use an object for opt_args and pass by name (e.g. {"start_index": 0, > "end_index": 10}). I think this pattern is used elsewhere in extension APIs. Updated with the object approach. https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:75: if (! this.listeners[eventType]) On 2014/04/01 17:05:25, dmazzoni wrote: > Nit: no space after ! Done. https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:81: removeEventListener: function(callback) { On 2014/04/01 17:05:25, dmazzoni wrote: > Should this take eventType and capture as parameters too? Added as part of TODO. I'm not sure if we want to allow someone to add the same event listener twice (but possibly vary capture). I added the event type param though. https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:85: performAction: function(actionType, opt_args) { On 2014/04/01 17:49:51, aboxhall wrote: > Use performAction_ naming scheme for private methods, remove @private (to be > consistent with other users of this pattern). Removed the private annotation and added the underscore. https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:86: // Not yet initialized. On 2014/04/01 17:05:25, dmazzoni wrote: > Nit: indent Done. https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:99: updateEventListeners: function(eventType) { On 2014/04/01 17:49:51, aboxhall wrote: > notifyEventListeners_ ? Notify is fine. Actually not private though since it's called by AutomationTree. Changed as well. https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_tree.js (right): https://codereview.chromium.org/203753002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_tree.js:41: * renderer/renderer host pair. On 2014/04/01 17:05:25, dmazzoni wrote: > What do you mean by renderer/renderer? revised to RWH. https://codereview.chromium.org/203753002/diff/100001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/automation/sanity_check/test.js (right): https://codereview.chromium.org/203753002/diff/100001/chrome/test/data/extens... chrome/test/data/extensions/api_test/automation/sanity_check/test.js:100: var okButton = tree.root.firstChild().firstChild(); On 2014/04/01 17:49:51, aboxhall wrote: > This suggests to me that we should spec out getting an automation node from a > DOM node. This would also be generally useful anyway, and should be fairly > straightforward to implement, I think. I was looking for the equivalent of 'getElementById' here. But, yes, we need some way of getting an automation node based on various criteria; let's spec it. https://codereview.chromium.org/203753002/diff/100001/chrome/test/data/extens... chrome/test/data/extensions/api_test/automation/sanity_check/test.js:106: chrome.tabs.create({url: 'test.html'}); On 2014/04/01 17:49:51, aboxhall wrote: > Can we use chrome.tabs.update to be consistent with above and avoid creating a > new tab unnecessarily? I'd prefer to leave this as is. I didn't use update here because I didn't want to specify an existing tab id. I don't think it matters if we create a new tab right? I'm also not sure about state left over from previous tests here. The other aspect I should note is that the tree (before the load_complete event) is in a "unactionable" state. That is, the subtree under root actually changes a few times (allocating new nodes with new ids). So, trying to focus an item before load_complete would fail since the ids potentially changed by the time we send that action over to the target renderer. https://codereview.chromium.org/203753002/diff/100001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/203753002/diff/100001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:419: virtual void AccessibilityDoDefaultAction(int object_id); On 2014/04/01 17:05:25, dmazzoni wrote: > Add OVERRIDE too? Done. https://codereview.chromium.org/203753002/diff/100001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:422: virtual void AccessibilitySetFocus(int object_id); On 2014/04/01 20:20:45, aboxhall wrote: > Wait... where is the implementation for this? Not uploaded? These actions previously existed.
lgtm
lgtm https://codereview.chromium.org/203753002/diff/100001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/automation/sanity_check/test.js (right): https://codereview.chromium.org/203753002/diff/100001/chrome/test/data/extens... chrome/test/data/extensions/api_test/automation/sanity_check/test.js:100: var okButton = tree.root.firstChild().firstChild(); On 2014/04/01 21:27:15, David Tseng wrote: > On 2014/04/01 17:49:51, aboxhall wrote: > > This suggests to me that we should spec out getting an automation node from a > > DOM node. This would also be generally useful anyway, and should be fairly > > straightforward to implement, I think. > > I was looking for the equivalent of 'getElementById' here. But, yes, we need > some way of getting an automation node based on various criteria; let's spec it. Let's chat about this. I spent some time chatting to adamk@ and it should be possible to pass a querySelector over to Blink and pass an a11y node ID back, which would do most of what we want. We possibly also want some kind of search by text API? https://codereview.chromium.org/203753002/diff/100001/chrome/test/data/extens... chrome/test/data/extensions/api_test/automation/sanity_check/test.js:106: chrome.tabs.create({url: 'test.html'}); On 2014/04/01 21:27:15, David Tseng wrote: > On 2014/04/01 17:49:51, aboxhall wrote: > > Can we use chrome.tabs.update to be consistent with above and avoid creating a > > new tab unnecessarily? > > I'd prefer to leave this as is. > > I didn't use update here because I didn't want to specify an existing tab id. I > don't think it matters if we create a new tab right? I'm also not sure about > state left over from previous tests here. > > The other aspect I should note is that the tree (before the load_complete event) > is in a "unactionable" state. That is, the subtree under root actually changes a > few times (allocating new nodes with new ids). So, trying to focus an item > before load_complete would fail since the ids potentially changed by the time we > send that action over to the target renderer. I see, makes sense. https://codereview.chromium.org/203753002/diff/140001/chrome/common/extension... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/203753002/diff/140001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:71: object opt_args); Tabs :(
https://codereview.chromium.org/203753002/diff/140001/chrome/common/extension... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/203753002/diff/140001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:71: object opt_args); On 2014/04/01 21:39:52, aboxhall wrote: > Tabs :( Should I add a TODO to accept a tab id?
lgtm https://codereview.chromium.org/203753002/diff/140001/chrome/common/extension... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/203753002/diff/140001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:71: object opt_args); On 2014/04/01 21:58:18, David Tseng wrote: > On 2014/04/01 21:39:52, aboxhall wrote: > > Tabs :( > > Should I add a TODO to accept a tab id? Sorry, I meant there were tab characters in here! I should have been more explicit. I don't see them now, either; I don't know what's up with Rietveld.
+ kalman kalman/jam, please take a look for OWNERS approval. @jam: content/* @kalman: I think you should take a look at everything else. Thanks
sorry this took me so long https://codereview.chromium.org/203753002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/203753002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:253: EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(2, &automation_node_id)); the schema compiler will generate all of this for you. you shouldn't need any manual Value unpacking: api::automation_internal::PerformAction::Params params( api::automation_internal::PerformaAction::Params::Create(args_)); then access params.processId etc. it will also unpack to the correct enums so GetActionType should be unnecessary. p.s. people typically will often do "using api::automation_internal::PerformAction" as well, but up to you. https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... chrome/common/extensions/api/automation.idl:11: // TODO(dtseng/aboxhall): Make this take an |automation.Event| param. this TODO is going to show up in the official developer docs. https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... chrome/common/extensions/api/automation.idl:35: long? index_in_parent; as a general comment in here and automation_internal, DOM APIs seem to usually be camel case not underscores. https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:60: callback EnableCallback = void(long process_id, long routing_id); if this gets any longer you should pass a dictionary into here, but 2 arguments is fine. https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:71: object opt_args); this is a very long argument list, strongly consider passing in a dictionary with keys "processId", "routingId", etc. https://codereview.chromium.org/203753002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/203753002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:60: }, indent -= 2 https://codereview.chromium.org/203753002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:82: // TODO(dtseng/aboxhall): Check this impl against spec. maybe this is too hacky, and I wish you could just "new EventTarget()" and do this, but could you create a fake div element and fire events on that? like: function AutomationNodeImpl() { self.eventTarget = document.createElement('div'); } here: addEventListener: function(eventType, callback, capture) { self.eventTarget.addEventListener(eventTYpe, callback, capture); } same for removeEventListner then when you disaptch the event notifyEventListeners: function(eventType) { self.eventTarget.disptachEvent(new CustomEvent(eventType, {/*details*/})); }
https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... chrome/common/extensions/api/automation.idl:11: // TODO(dtseng/aboxhall): Make this take an |automation.Event| param. On 2014/04/02 21:52:14, kalman wrote: > this TODO is going to show up in the official developer docs. Can we hide this from the official developer docs until the API is at least not behind a flag?
On 2014/04/02 21:57:25, aboxhall wrote: > https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... > File chrome/common/extensions/api/automation.idl (right): > > https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... > chrome/common/extensions/api/automation.idl:11: // TODO(dtseng/aboxhall): Make > this take an |automation.Event| param. > On 2014/04/02 21:52:14, kalman wrote: > > this TODO is going to show up in the official developer docs. > > Can we hide this from the official developer docs until the API is at least not > behind a flag? yes you can annotate the whole automation API with [nodoc]
On 2014/04/02 21:59:33, kalman wrote: > On 2014/04/02 21:57:25, aboxhall wrote: > > > https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... > > File chrome/common/extensions/api/automation.idl (right): > > > > > https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... > > chrome/common/extensions/api/automation.idl:11: // TODO(dtseng/aboxhall): Make > > this take an |automation.Event| param. > > On 2014/04/02 21:52:14, kalman wrote: > > > this TODO is going to show up in the official developer docs. > > > > Can we hide this from the official developer docs until the API is at least > not > > behind a flag? > > yes you can annotate the whole automation API with [nodoc] ... actually wait. It shouldn't even be showing up unless you've added a .html file for it. so you're fie.
https://codereview.chromium.org/203753002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/203753002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:82: // TODO(dtseng/aboxhall): Check this impl against spec. On 2014/04/02 21:52:14, kalman wrote: > maybe this is too hacky, and I wish you could just "new EventTarget()" and do > this, but could you create a fake div element and fire events on that? > > like: > > function AutomationNodeImpl() { > self.eventTarget = document.createElement('div'); > } > > here: > > addEventListener: function(eventType, callback, capture) { > self.eventTarget.addEventListener(eventTYpe, callback, capture); > } > > same for removeEventListner > > then when you disaptch the event > > notifyEventListeners: function(eventType) { > self.eventTarget.disptachEvent(new CustomEvent(eventType, {/*details*/})); > } That won't work: note that we plan to implement capture and bubbling phases here as well. We'll be fleshing out the event listener stuff in a later CL.
https://codereview.chromium.org/203753002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/203753002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:82: // TODO(dtseng/aboxhall): Check this impl against spec. > That won't work: note that we plan to implement capture and bubbling phases here > as well. > > We'll be fleshing out the event listener stuff in a later CL. Ok. I hope that eventually you can pull that out into some external Event class which this can delegate too, or if you're really lucky, blink will need to expose something like that for itself (since they're rewriting the editing stuff in JS).
https://codereview.chromium.org/203753002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/203753002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:82: // TODO(dtseng/aboxhall): Check this impl against spec. On 2014/04/02 22:09:40, kalman wrote: > > > That won't work: note that we plan to implement capture and bubbling phases > here > > as well. > > > > We'll be fleshing out the event listener stuff in a later CL. > > Ok. > > I hope that eventually you can pull that out into some external Event class > which this can delegate too, or if you're really lucky, blink will need to > expose something like that for itself (since they're rewriting the editing stuff > in JS). That's sort of what I think we had in mind; this cl just got the bare minimum going to be able to test actions. We'll update the design doc accordingly.
https://codereview.chromium.org/203753002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/203753002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:253: EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(2, &automation_node_id)); On 2014/04/02 21:52:14, kalman wrote: > the schema compiler will generate all of this for you. you shouldn't need any > manual Value unpacking: > > api::automation_internal::PerformAction::Params params( > api::automation_internal::PerformaAction::Params::Create(args_)); > > then access params.processId etc. > > it will also unpack to the correct enums so GetActionType should be unnecessary. > > p.s. people typically will often do "using > api::automation_internal::PerformAction" as well, but up to you. Done. https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... chrome/common/extensions/api/automation.idl:35: long? index_in_parent; On 2014/04/02 21:52:14, kalman wrote: > as a general comment in here and automation_internal, DOM APIs seem to usually > be camel case not underscores. I think I got them all :). This is with the notable exception of enums. https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:60: callback EnableCallback = void(long process_id, long routing_id); On 2014/04/02 21:52:14, kalman wrote: > if this gets any longer you should pass a dictionary into here, but 2 arguments > is fine. Ok. https://codereview.chromium.org/203753002/diff/160001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:71: object opt_args); On 2014/04/02 21:52:14, kalman wrote: > this is a very long argument list, strongly consider passing in a dictionary > with keys "processId", "routingId", etc. Done. https://codereview.chromium.org/203753002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/203753002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:60: }, On 2014/04/02 21:52:14, kalman wrote: > indent -= 2 Done.
lgtm
just trivial comments left, lgtm, though the changes to make the event structured isn't a trivial amount of typing (sorry). https://codereview.chromium.org/203753002/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/203753002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:88: axTreeUpdate->SetString("eventType", ToString(iter->event_type)); you should be able to construct these arguments without involving the Value API, too: AXEventParams args; params.process_id = event.process_id; params.routing_id = event.routing_id; params.event_type = ...; for (...) { linked_ptr<AXNodeData> node(new AXNodeData()); ... args.nodes.push_back(node); } DispatchEvent(browser_context_, OnAccessibilityEvent::kEventName, OnAccessibilityEvent::Create(args)); https://codereview.chromium.org/203753002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:111: void AddNodeData(const ui::AXNodeData& node, you should be able to return, say, a scoped_ptr<AXNodeData> here (where AXNodeData is again generated) https://codereview.chromium.org/203753002/diff/180001/chrome/common/extension... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/203753002/diff/180001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:77: object opt_args); you could use object? here to actually indicate it's optional. https://codereview.chromium.org/203753002/diff/180001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/203753002/diff/180001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:112: opt_args ? opt_args : {}); indent += 1 you could also write "opt_args || {}" if you want. though if you made the args optional in the IDL you wouldn't need it at all.
https://codereview.chromium.org/203753002/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/203753002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:88: axTreeUpdate->SetString("eventType", ToString(iter->event_type)); On 2014/04/03 18:57:13, kalman wrote: > you should be able to construct these arguments without involving the Value API, > too: > > AXEventParams args; > params.process_id = event.process_id; > params.routing_id = event.routing_id; > params.event_type = ...; > for (...) { > linked_ptr<AXNodeData> node(new AXNodeData()); > ... > args.nodes.push_back(node); > } > DispatchEvent(browser_context_, > OnAccessibilityEvent::kEventName, > OnAccessibilityEvent::Create(args)); This seems outside of scope for this change.
https://codereview.chromium.org/203753002/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/203753002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:88: axTreeUpdate->SetString("eventType", ToString(iter->event_type)); > This seems outside of scope for this change. fair enough.
https://codereview.chromium.org/203753002/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/203753002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:88: axTreeUpdate->SetString("eventType", ToString(iter->event_type)); On 2014/04/03 18:57:13, kalman wrote: > you should be able to construct these arguments without involving the Value API, > too: > > AXEventParams args; > params.process_id = event.process_id; > params.routing_id = event.routing_id; > params.event_type = ...; > for (...) { > linked_ptr<AXNodeData> node(new AXNodeData()); > ... > args.nodes.push_back(node); > } > DispatchEvent(browser_context_, > OnAccessibilityEvent::kEventName, > OnAccessibilityEvent::Create(args)); I have this pretty much ready for am putting it off for the next cl so I can unblock other changes. One question though (not affecting this cl), after creating args above, it looks like we'll need to convert back to base Values again to populate the event. Wouldn't it be more efficient to just populate a base Values in the first place like we do now? (obviously, the hand rolled attribute names are bad, but it seems less wasteful). https://codereview.chromium.org/203753002/diff/180001/chrome/common/extension... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/203753002/diff/180001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:77: object opt_args); On 2014/04/03 18:57:13, kalman wrote: > you could use object? here to actually indicate it's optional. The schema compiler doesn't like this: chrome/common/extensions/api/automation_internal.idl could not be parsed: /usr/local/google/home/dtseng/projects/chrome/src/chrome/common/extensions/api/automation_internal.idl(77) : Error : Unexpected "?" after symbol object. I'm guessing it's only valid within dictionaries? https://codereview.chromium.org/203753002/diff/180001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/203753002/diff/180001/chrome/renderer/resourc... chrome/renderer/resources/extensions/automation/automation_node.js:112: opt_args ? opt_args : {}); On 2014/04/03 18:57:13, kalman wrote: > indent += 1 > > you could also write "opt_args || {}" if you want. > > though if you made the args optional in the IDL you wouldn't need it at all. Done (made into opt_args || {}). The indent is fine I think since the above is within an object literal.
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/203753002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
content/browser/renderer_host/render_view_host_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/browser/renderer_host/render_view_host_impl.cc
Hunk #1 FAILED at 1727.
1 out of 1 hunk FAILED -- saving rejects to file
content/browser/renderer_host/render_view_host_impl.cc.rej
Patch: content/browser/renderer_host/render_view_host_impl.cc
Index: content/browser/renderer_host/render_view_host_impl.cc
diff --git a/content/browser/renderer_host/render_view_host_impl.cc
b/content/browser/renderer_host/render_view_host_impl.cc
index
43bfeed9668a963dcc5eb3b35cdcce825d008d50..3e0fab420d27c4be54d7788dba2332dccc0c29ea
100644
--- a/content/browser/renderer_host/render_view_host_impl.cc
+++ b/content/browser/renderer_host/render_view_host_impl.cc
@@ -1727,8 +1727,11 @@ void RenderViewHostImpl::OnAccessibilityEvents(
std::vector<AXEventNotificationDetails> details;
for (unsigned int i = 0; i < params.size(); ++i) {
const AccessibilityHostMsg_EventParams& param = params[i];
- AXEventNotificationDetails detail(
- param.nodes, param.event_type, param.id, GetRoutingID());
+ AXEventNotificationDetails detail(param.nodes,
+ param.event_type,
+ param.id,
+ GetProcess()->GetID(),
+ GetRoutingID());
details.push_back(detail);
}
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/203753002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/203753002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/203753002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
extensions/browser/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file extensions/browser/extension_function_histogram_value.h
Hunk #1 FAILED at 768.
1 out of 1 hunk FAILED -- saving rejects to file
extensions/browser/extension_function_histogram_value.h.rej
Patch: extensions/browser/extension_function_histogram_value.h
Index: extensions/browser/extension_function_histogram_value.h
diff --git a/extensions/browser/extension_function_histogram_value.h
b/extensions/browser/extension_function_histogram_value.h
index
2db96d7579ec051df8d7ab3152f83398961047e7..d7257633e8fa2099b694ec752194b9ef9a78025d
100644
--- a/extensions/browser/extension_function_histogram_value.h
+++ b/extensions/browser/extension_function_histogram_value.h
@@ -768,6 +768,7 @@ enum HistogramValue {
BLUETOOTHPRIVATE_ENABLEPAIRING,
BLUETOOTHPRIVATE_DISABLEPAIRING,
BLUETOOTHPRIVATE_SETPAIRINGRESPONSE,
+ AUTOMATIONINTERNAL_PERFORMACTION,
// Last entry: Add new entries above and ensure to update
// tools/metrics/histograms/histograms/histograms.xml.
ENUM_BOUNDARY
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/203753002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
(still lgtm, just following up) https://codereview.chromium.org/203753002/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/203753002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:88: axTreeUpdate->SetString("eventType", ToString(iter->event_type)); On 2014/04/03 23:05:19, David Tseng wrote: > On 2014/04/03 18:57:13, kalman wrote: > > you should be able to construct these arguments without involving the Value > API, > > too: > > > > AXEventParams args; > > params.process_id = event.process_id; > > params.routing_id = event.routing_id; > > params.event_type = ...; > > for (...) { > > linked_ptr<AXNodeData> node(new AXNodeData()); > > ... > > args.nodes.push_back(node); > > } > > DispatchEvent(browser_context_, > > OnAccessibilityEvent::kEventName, > > OnAccessibilityEvent::Create(args)); > > I have this pretty much ready for am putting it off for the next cl so I can > unblock other changes. > > One question though (not affecting this cl), after creating args above, it looks > like we'll need to convert back to base Values again to populate the event. > Wouldn't it be more efficient to just populate a base Values in the first place > like we do now? (obviously, the hand rolled attribute names are bad, but it > seems less wasteful). You're saying that with the current code you go accessibility model --> base::Value and if you were to use the compiler you'd go accessibility model --> compiled model --> base::Value for which the "compiled model" step is wasteful? Yes I think that's true, and if it's a lot of data that you're frequently ferrying across it might make a small difference, but without profiling it's hard to tell. I'd say the other serialization/deserialization that goes on across the various IPC boundaries would be a significantly more costly operation, and by going through the "compiled model" step the code becomes simpler and guaranteed to be structurally correct. An overall win IMO. https://codereview.chromium.org/203753002/diff/180001/chrome/common/extension... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/203753002/diff/180001/chrome/common/extension... chrome/common/extensions/api/automation_internal.idl:77: object opt_args); On 2014/04/03 23:05:19, David Tseng wrote: > On 2014/04/03 18:57:13, kalman wrote: > > you could use object? here to actually indicate it's optional. > > The schema compiler doesn't like this: > chrome/common/extensions/api/automation_internal.idl could not be parsed: > /usr/local/google/home/dtseng/projects/chrome/src/chrome/common/extensions/api/automation_internal.idl(77) > : Error : Unexpected "?" after symbol object. > > I'm guessing it's only valid within dictionaries? Oh interesting. JSON schema supports optional parameters but I guess IDL mustn't.
(still lgtm, just following up)
https://codereview.chromium.org/203753002/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/203753002/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:88: axTreeUpdate->SetString("eventType", ToString(iter->event_type)); On 2014/04/04 16:30:57, kalman wrote: > On 2014/04/03 23:05:19, David Tseng wrote: > > On 2014/04/03 18:57:13, kalman wrote: > > > you should be able to construct these arguments without involving the Value > > API, > > > too: > > > > > > AXEventParams args; > > > params.process_id = event.process_id; > > > params.routing_id = event.routing_id; > > > params.event_type = ...; > > > for (...) { > > > linked_ptr<AXNodeData> node(new AXNodeData()); > > > ... > > > args.nodes.push_back(node); > > > } > > > DispatchEvent(browser_context_, > > > OnAccessibilityEvent::kEventName, > > > OnAccessibilityEvent::Create(args)); > > > > I have this pretty much ready for am putting it off for the next cl so I can > > unblock other changes. > > > > One question though (not affecting this cl), after creating args above, it > looks > > like we'll need to convert back to base Values again to populate the event. > > Wouldn't it be more efficient to just populate a base Values in the first > place > > like we do now? (obviously, the hand rolled attribute names are bad, but it > > seems less wasteful). > > You're saying that with the current code you go > > accessibility model --> base::Value > > and if you were to use the compiler you'd go > > accessibility model --> compiled model --> base::Value > > for which the "compiled model" step is wasteful? Correct. Yes I think that's true, and if > it's a lot of data that you're frequently ferrying across it might make a small > difference, but without profiling it's hard to tell. It's potentially a lot of data (mostly on "load_complete" or load of a page). I'd say the other > serialization/deserialization that goes on across the various IPC boundaries > would be a significantly more costly operation, and by going through the > "compiled model" step the code becomes simpler and guaranteed to be structurally > correct. An overall win IMO. That makes sense; inter-process communication in it of itself (+ unserializing/serializing) is probably the larger cost here, so I'll followup with the suggested change in the next cl.
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/203753002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/203753002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/203753002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/203753002/280001
Message was sent while issue was closed.
Change committed as 261975 |
