Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
655273005:
Implement AutomationNode.querySelector(). (Closed)
|
Created:
6 years, 1 month ago by aboxhall Modified:
6 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, not at google - send to devlin, dmazzoni Base URL:
https://chromium.googlesource.com/chromium/src@master Project:
chromium Visibility:
Public. |
DescriptionImplement AutomationNode.querySelector().
BUG=404710
Committed: https://crrev.com/7abfb3db96dc7d87c9f1282b5fc81328ec9c787c
Cr-Commit-Position: refs/heads/master@{#302944}
Patch Set 1 #
Total comments: 69
Patch Set 2 : Address comments #
Total comments: 32
Patch Set 3 : Address review comments and flesh out error and edge case handling #
Total comments: 30
Patch Set 4 : Address comments #Patch Set 5 : Use assertEq instead of assertTrue in testQuerySelectorFromRemovedNode() #
Total comments: 10
Patch Set 6 : dmazzoni comments #Patch Set 7 : devlin review comments #
Total comments: 8
Patch Set 8 : Use enums instead of strings for error messages #
Total comments: 4
Patch Set 9 : Use correct path for automation_api_helper in BUILD.gn #Patch Set 10 : palmer nits #Patch Set 11 : rebase #Patch Set 12 : License typo #Patch Set 13 : Add complex.html #Patch Set 14 : Fix heap-use-after-free issue by not keeping a scoped_ptr to automation_api_helper in extension_hel… #Messages
Total messages: 60 (10 generated)
aboxhall@chromium.org changed reviewers: + dtseng@chromium.org, rdevlin.cronin@chromium.org
Not quite ready for commit yet (e.g. need to do some more robust error handling), but looking for feedback at this stage on API (dtseng@ in particular) and implementation design (everyone).
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
Rather than making the implementation only part of the automation API, I'd prefer we just make it another accessibility API that's potentially exposed to other platforms - it seems like this or something like it could be useful on other platforms too, even if the API doesn't exist today. At a minimum, it seems potentially really useful for our own unit tests! Specifically, that means moving the IPC to accessibility_messages.h, along with SetFocus, DoDefaultAction, and so on - and implementing it in renderer_accessibility_complete.cc. https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... File extensions/renderer/api/automation/automation_api_helper.cc (right): https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.cc:63: result_acc_obj_id = ax_obj.axID(); Need to check if ax_obj is valid - the best way is to call isDetached(), which tells you if it's NULL (i.e. it was never assigned to anything), or if it's detached (meaning it used to point to something, but the object it pointed to is now NULL, and it only exists until all references to it go away).
What happens if the AutomationNode you fire this on is a Desktop node? It's fine if it does nothing, but it might be nice to make sure it doesn't send a useless IPC and have a test for this to make sure it doesn't crash.
One meta-comment - I think this is a cool API and really useful for automation - but my one worry is that it potentially makes it easy to lose out on the benefits of the accessibility tree abstraction. Even for the simple case of a button, you'd have to write "button,[role='button']" in order to handle ARIA buttons too - and that can be increasingly error-prone. It'd be cool if we could provide a way to do queries on the accessibility tree - like look for an element with a role of "button". Even without full querySelector syntax I think adding something like that would be important - not only for ChromeVox but to encourage developers to use the most abstract possible query when possible. https://codereview.chromium.org/655273005/diff/1/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/automation/tests/tabs/query.js (right): https://codereview.chromium.org/655273005/diff/1/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/automation/tests/tabs/query.js:15: rootNode.query({'selector': 'button'}, assertCorrectResult); Please add a test that does some crazy CSS selector just to demonstrate that it's giving you the full power of CSS here - like foo not(#bar) > .baz:nth_child(2) or something like that
+1 making this more generic to accessibility and making desktop accessibility possible to support via a subset of selectors. The latter seems along the same lines as Dominic's suggestion of abstractions and using attributes specific to the accessibility/automation tree. https://codereview.chromium.org/655273005/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/655273005/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/automation.idl:225: DOMString? querySelector; Is this optional? https://codereview.chromium.org/655273005/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/automation.idl:226: }; Will there be additional info members? If not, then just expose the query info as a string. https://codereview.chromium.org/655273005/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/automation.idl:303: nit: Docs?
Maybe this one should be renamed domQuerySelector to indicate that it uses DOM syntax? On Mon, Oct 27, 2014 at 1:44 PM, <dtseng@chromium.org> wrote: > +1 making this more generic to accessibility and making desktop > accessibility > possible to support via a subset of selectors. The latter seems along the > same > lines as Dominic's suggestion of abstractions and using attributes > specific to > the accessibility/automation tree. > > > https://codereview.chromium.org/655273005/diff/1/chrome/ > common/extensions/api/automation.idl > File chrome/common/extensions/api/automation.idl (right): > > https://codereview.chromium.org/655273005/diff/1/chrome/ > common/extensions/api/automation.idl#newcode225 > chrome/common/extensions/api/automation.idl:225: DOMString? > querySelector; > Is this optional? > > https://codereview.chromium.org/655273005/diff/1/chrome/ > common/extensions/api/automation.idl#newcode226 > chrome/common/extensions/api/automation.idl:226: }; > Will there be additional info members? If not, then just expose the > query info as a string. > > https://codereview.chromium.org/655273005/diff/1/chrome/ > common/extensions/api/automation.idl#newcode303 > chrome/common/extensions/api/automation.idl:303: > nit: Docs? > > https://codereview.chromium.org/655273005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
First quick look-through. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:49: class QuerySelectorHandler : public content::WebContentsObserver { document https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:55: base::string16& query, const https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:56: extensions::AutomationInternalQuerySelectorFunction::Callback callback) const & https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:65: virtual ~QuerySelectorHandler() {} nit: ~QuerySelectorHandler() override {} https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:67: virtual bool OnMessageReceived(const IPC::Message& message) override { nit: no virtual (also for other methods) https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:71: // There may be several requests in flight; check that this response matches nit: End the sentence with a period. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:76: CHECK(message.ReadInt(&iter, &message_request_id)); I'm not super familiar with the inner workings of pickles - do we not need to read the string (the second parameter)? https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:88: virtual void WebContentsDestroyed() override { It's good that we watch the web contents, but shouldn't we also watch the render view to which we sent the query? https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:102: extensions::AutomationInternalQuerySelectorFunction::Callback callback_; Why not just put the class in extensions::? https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:316: int AutomationInternalQuerySelectorFunction::query_request_id_counter_ = 0; why not put this in an anonymous namespace? Is it going to be used outside of this file? https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:343: new QuerySelectorHandler( nit: comment that QuerySelectorHandler handles its own lifetime. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:344: contents, request_id, params->args.automation_node_id, selector, nit: one arg per line (even though it's long) https://codereview.chromium.org/655273005/diff/1/extensions/common/extension_... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/655273005/diff/1/extensions/common/extension_... extensions/common/extension_messages.h:768: int /* result_acc_obj_id */) This order looks different than the one you use in the api.cc file... https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... File extensions/renderer/api/automation/automation_api_helper.h (right): https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.h:15: class AutomationApiHelper : public content::RenderViewObserver { document https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.h:18: virtual ~AutomationApiHelper(); nit: ~AutomationApiHelper() override; https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.h:22: virtual bool OnMessageReceived(const IPC::Message& message) override; nit: no virtual https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.h:23: void OnQuerySelector(int acc_obj_id, new line + comments https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.h:26: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/655273005/diff/1/extensions/renderer/extensio... File extensions/renderer/extension_helper.h (right): https://codereview.chromium.org/655273005/diff/1/extensions/renderer/extensio... extensions/renderer/extension_helper.h:26: class Dispatcher; nit: alphabetize (within sections) https://codereview.chromium.org/655273005/diff/1/extensions/renderer/extensio... extensions/renderer/extension_helper.h:104: // Renderer hooks for the Automation API nit: .
On 2014/10/27 18:47:36, dmazzoni wrote: > Rather than making the implementation only part of the automation API, I'd > prefer we just make it another accessibility API that's potentially exposed to > other platforms - it seems like this or something like it could be useful on > other platforms too, even if the API doesn't exist today. > > At a minimum, it seems potentially really useful for our own unit tests! > > Specifically, that means moving the IPC to accessibility_messages.h, along with > SetFocus, DoDefaultAction, and so on - and implementing it in > renderer_accessibility_complete.cc. As discussed offline: I spent some time doing this, since I think it would be good as well, but it's going to be a more major change than it initially appears, so we agreed to leave this change as-is for now, and revisit later when it makes sense (e.g. when we add hooks for something like hitTest into the API).
On 2014/10/27 18:50:42, dmazzoni wrote: > What happens if the AutomationNode you fire this on is a Desktop node? It's fine > if it does nothing, but it might be nice to make sure it doesn't send a useless > IPC and have a test for this to make sure it doesn't crash. As it stands in this CL, we check for the desktop process/routing ID on line 327 of https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... and respond with an error. I can add a test as well.
On 2014/10/27 18:50:42, dmazzoni wrote: > What happens if the AutomationNode you fire this on is a Desktop node? It's fine > if it does nothing, but it might be nice to make sure it doesn't send a useless > IPC and have a test for this to make sure it doesn't crash. As it stands in this CL, we check for the desktop process/routing ID on line 327 of https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... and respond with an error. I can add a test as well.
On 2014/10/27 18:57:09, dmazzoni wrote: > One meta-comment - I think this is a cool API and really useful for automation - > but my one worry is that it potentially makes it easy to lose out on the > benefits of the accessibility tree abstraction. > > Even for the simple case of a button, you'd have to write > "button,[role='button']" in order to handle ARIA buttons too - and that can be > increasingly error-prone. It'd be cool if we could provide a way to do queries > on the accessibility tree - like look for an element with a role of "button". > Even without full querySelector syntax I think adding something like that would > be important - not only for ChromeVox but to encourage developers to use the > most abstract possible query when possible. Right, this is why the full API proposal in the linked bug also includes the ability to query by role and state - I definitely agree we want to be able to query over the accessibility tree.
On 2014/10/27 20:44:35, David Tseng wrote: > +1 making this more generic to accessibility and making desktop accessibility > possible to support via a subset of selectors. The latter seems along the same > lines as Dominic's suggestion of abstractions and using attributes specific to > the accessibility/automation tree. Agreed, see inline comment to follow.
https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:49: class QuerySelectorHandler : public content::WebContentsObserver { On 2014/10/28 21:05:09, Devlin wrote: > document Done. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:55: base::string16& query, On 2014/10/28 21:05:09, Devlin wrote: > const Done. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:56: extensions::AutomationInternalQuerySelectorFunction::Callback callback) On 2014/10/28 21:05:09, Devlin wrote: > const & I don't think this makes sense. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:65: virtual ~QuerySelectorHandler() {} On 2014/10/28 21:05:09, Devlin wrote: > nit: ~QuerySelectorHandler() override {} Why override? https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:67: virtual bool OnMessageReceived(const IPC::Message& message) override { On 2014/10/28 21:05:09, Devlin wrote: > nit: no virtual (also for other methods) I don't understand; this is overriding a virtual method from WebContentsObserver. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:71: // There may be several requests in flight; check that this response matches On 2014/10/28 21:05:10, Devlin wrote: > nit: End the sentence with a period. I don't think it's worth wrapping onto a new line. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:76: CHECK(message.ReadInt(&iter, &message_request_id)); On 2014/10/28 21:05:09, Devlin wrote: > I'm not super familiar with the inner workings of pickles - do we not need to > read the string (the second parameter)? Good question. This was actually several flavours of wrong- should be fixed now. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:88: virtual void WebContentsDestroyed() override { On 2014/10/28 21:05:09, Devlin wrote: > It's good that we watch the web contents, but shouldn't we also watch the render > view to which we sent the query? I'm not sure; I copied this pattern from extensions/browser/script_executor.cc which doesn't watch the RV(H). https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:102: extensions::AutomationInternalQuerySelectorFunction::Callback callback_; On 2014/10/28 21:05:09, Devlin wrote: > Why not just put the class in extensions::? Which class? https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:316: int AutomationInternalQuerySelectorFunction::query_request_id_counter_ = 0; On 2014/10/28 21:05:09, Devlin wrote: > why not put this in an anonymous namespace? Is it going to be used outside of > this file? It's scoped and private to the class which uses it, and this way I can define it near where it's used. What would the benefit be of moving it into an anonymous namespace? https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:343: new QuerySelectorHandler( On 2014/10/28 21:05:10, Devlin wrote: > nit: comment that QuerySelectorHandler handles its own lifetime. Done. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:344: contents, request_id, params->args.automation_node_id, selector, On 2014/10/28 21:05:09, Devlin wrote: > nit: one arg per line (even though it's long) This is how clang-format formats it, I'd rather leave it alone. https://codereview.chromium.org/655273005/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/655273005/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/automation.idl:225: DOMString? querySelector; On 2014/10/27 20:44:35, David Tseng wrote: > Is this optional? See below. https://codereview.chromium.org/655273005/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/automation.idl:226: }; On 2014/10/27 20:44:35, David Tseng wrote: > Will there be additional info members? If not, then just expose the query info > as a string. Right, I guess my CL description was a little misleading :) This is the proposed API in the linked bug (https://code.google.com/p/chromium/issues/detail?id=404710) /** * Gets the first node in this node's subtree which matches |queryInfo|. * @param {Object} queryInfo A query object with any of the following keys: * - selector {string} a CSS querySelector string * - state {StateType} for example ‘focused’ * - role {RoleType} * @return {chrome.automation.AutomationNode} */ chrome.automation.AutomationNode.query(query); This change just implements 'selector' (which I somewhat arbitrarily renamed querySelector here). My idea was that we could implement the filters which act over the automation tree in JavaScript, unless there's some more performant way to do it in C++. I'd love it if you had any suggestions for other attributes we could query on, too. https://codereview.chromium.org/655273005/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/automation.idl:303: On 2014/10/27 20:44:35, David Tseng wrote: > nit: Docs? Done. https://codereview.chromium.org/655273005/diff/1/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/automation/tests/tabs/query.js (right): https://codereview.chromium.org/655273005/diff/1/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/automation/tests/tabs/query.js:15: rootNode.query({'selector': 'button'}, assertCorrectResult); On 2014/10/27 18:57:08, dmazzoni wrote: > Please add a test that does some crazy CSS selector just to demonstrate that > it's giving you the full power of CSS here - like foo not(#bar) > > .baz:nth_child(2) or something like that Yeah, this is actually a terrible example anyway since you'd almost never execute querySelector('button') anyway, even in regular DOM (you'd probably either want all the buttons, or a specific button that you'd select by ID or class or something) https://codereview.chromium.org/655273005/diff/1/extensions/common/extension_... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/655273005/diff/1/extensions/common/extension_... extensions/common/extension_messages.h:768: int /* result_acc_obj_id */) On 2014/10/28 21:05:10, Devlin wrote: > This order looks different than the one you use in the api.cc file... Actually the _helper.cc file had it in the wrong order for the other IPC, but yeah. https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... File extensions/renderer/api/automation/automation_api_helper.cc (right): https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.cc:63: result_acc_obj_id = ax_obj.axID(); On 2014/10/27 18:47:36, dmazzoni wrote: > Need to check if ax_obj is valid - the best way is to call isDetached(), which > tells you if it's NULL (i.e. it was never assigned to anything), or if it's > detached (meaning it used to point to something, but the object it pointed to is > now NULL, and it only exists until all references to it go away). Done. https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... File extensions/renderer/api/automation/automation_api_helper.h (right): https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.h:15: class AutomationApiHelper : public content::RenderViewObserver { On 2014/10/28 21:05:10, Devlin wrote: > document Done. https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.h:18: virtual ~AutomationApiHelper(); On 2014/10/28 21:05:10, Devlin wrote: > nit: ~AutomationApiHelper() override; Similarly, I don't understand why. https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.h:22: virtual bool OnMessageReceived(const IPC::Message& message) override; On 2014/10/28 21:05:10, Devlin wrote: > nit: no virtual Why? https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.h:23: void OnQuerySelector(int acc_obj_id, On 2014/10/28 21:05:10, Devlin wrote: > new line + comments Added new line; I don't think I can add any comments which aren't tautological. https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.h:26: }; On 2014/10/28 21:05:10, Devlin wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/655273005/diff/1/extensions/renderer/extensio... File extensions/renderer/extension_helper.h (right): https://codereview.chromium.org/655273005/diff/1/extensions/renderer/extensio... extensions/renderer/extension_helper.h:26: class Dispatcher; On 2014/10/28 21:05:10, Devlin wrote: > nit: alphabetize (within sections) Done. https://codereview.chromium.org/655273005/diff/1/extensions/renderer/extensio... extensions/renderer/extension_helper.h:104: // Renderer hooks for the Automation API On 2014/10/28 21:05:10, Devlin wrote: > nit: . Done.
https://codereview.chromium.org/655273005/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/655273005/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/automation.idl:226: }; On 2014/10/28 23:43:56, aboxhall wrote: > On 2014/10/27 20:44:35, David Tseng wrote: > > Will there be additional info members? If not, then just expose the query info > > as a string. > > Right, I guess my CL description was a little misleading :) > > This is the proposed API in the linked bug > (https://code.google.com/p/chromium/issues/detail?id=404710) My vote is to have two separate APIs. I'll comment on the bug. https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... File extensions/renderer/api/automation/automation_api_helper.cc (right): https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.cc:55: node = obj.node(); One subtle nuance here - not every AXObject corresponds to a node - it's possible that an AXObject is CSS generated content, an anonymous render block, or a scroll view, or a spinner button, or one of many objects that aren't nodes. You should check for a null node here - but also consider that it might be confusing if this query fails on some ax objects but succeeds on others - maybe walk up the ancestors and run the query on the nearest ancestor that corresponds to a node?
Just answering quick questions - will do a full review this afternoon. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:56: extensions::AutomationInternalQuerySelectorFunction::Callback callback) On 2014/10/28 23:43:56, aboxhall wrote: > On 2014/10/28 21:05:09, Devlin wrote: > > const & > > I don't think this makes sense. Sure it does - you can copy callbacks just like anything else. You are making a copy of it when you assign it to |callback_|, so right now you make a copy when passing to the constructor, and another copy when assigning to |callback_| (unless clang detects that |callback| is never actually used, and does crazy optimization). Let's eliminate one of those copies. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:65: virtual ~QuerySelectorHandler() {} On 2014/10/28 23:43:55, aboxhall wrote: > On 2014/10/28 21:05:09, Devlin wrote: > > nit: ~QuerySelectorHandler() override {} > > Why override? C++ 11 style. TL;DR: virtual + override is redundant (can't override a non-virtual method), and the destructor of subclasses is actually "overriding" the parent destructor, so should be override, not virtual. Most patches are linked against crbug.com/417463, which has the basic idea. Posts on chromium-dev go a lot more into it, if you wanted to know more. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:67: virtual bool OnMessageReceived(const IPC::Message& message) override { On 2014/10/28 23:43:56, aboxhall wrote: > On 2014/10/28 21:05:09, Devlin wrote: > > nit: no virtual (also for other methods) > > I don't understand; this is overriding a virtual method from > WebContentsObserver. See above - no virtual, just override. i.e. bool OnMessageReceived(const IPC::Message& message) override { https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:71: // There may be several requests in flight; check that this response matches On 2014/10/28 23:43:56, aboxhall wrote: > On 2014/10/28 21:05:10, Devlin wrote: > > nit: End the sentence with a period. > > I don't think it's worth wrapping onto a new line. Sadly, style rules. :/ (My personal preference would probably be more in line with your own, but we need to be consistent across the codebase.) https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:102: extensions::AutomationInternalQuerySelectorFunction::Callback callback_; On 2014/10/28 23:43:56, aboxhall wrote: > On 2014/10/28 21:05:09, Devlin wrote: > > Why not just put the class in extensions::? > > Which class? QuerySelectorHandler. Basically, move the namespace extensions on line 107 to 38. Or, better yet, just take out line 36. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:316: int AutomationInternalQuerySelectorFunction::query_request_id_counter_ = 0; On 2014/10/28 23:43:56, aboxhall wrote: > On 2014/10/28 21:05:09, Devlin wrote: > > why not put this in an anonymous namespace? Is it going to be used outside of > > this file? > > It's scoped and private to the class which uses it, and this way I can define it > near where it's used. What would the benefit be of moving it into an anonymous > namespace? Reduces the header import size by 0.000001%. :P More importantly, it makes it clear that it's only used in this file, and serves as a kind of visual queue to readers (my opinion). If you feel strongly, I won't fight you on it. :) https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:344: contents, request_id, params->args.automation_node_id, selector, On 2014/10/28 23:43:56, aboxhall wrote: > On 2014/10/28 21:05:09, Devlin wrote: > > nit: one arg per line (even though it's long) > > This is how clang-format formats it, I'd rather leave it alone. Dang - they keep changing clang format's style (before, it would deliberately take existing lines like this, and break them up).
https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:56: extensions::AutomationInternalQuerySelectorFunction::Callback callback) On 2014/10/29 16:09:03, Devlin wrote: > On 2014/10/28 23:43:56, aboxhall wrote: > > On 2014/10/28 21:05:09, Devlin wrote: > > > const & > > > > I don't think this makes sense. > > Sure it does - you can copy callbacks just like anything else. You are making a > copy of it when you assign it to |callback_|, so right now you make a copy when > passing to the constructor, and another copy when assigning to |callback_| > (unless clang detects that |callback| is never actually used, and does crazy > optimization). Let's eliminate one of those copies. I realise I'm making a copy, but I think that's the right thing to do here - it's only two pointers, and it's allocated on the stack in the calling method, which this class outlives. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:65: virtual ~QuerySelectorHandler() {} On 2014/10/29 16:09:03, Devlin wrote: > On 2014/10/28 23:43:55, aboxhall wrote: > > On 2014/10/28 21:05:09, Devlin wrote: > > > nit: ~QuerySelectorHandler() override {} > > > > Why override? > > C++ 11 style. TL;DR: virtual + override is redundant (can't override a > non-virtual method), and the destructor of subclasses is actually "overriding" > the parent destructor, so should be override, not virtual. > > Most patches are linked against crbug.com/417463, which has the basic idea. > Posts on chromium-dev go a lot more into it, if you wanted to know more. Got it, thanks. (https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/C$2B$2... in case anyone else is curious) https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:67: virtual bool OnMessageReceived(const IPC::Message& message) override { On 2014/10/29 16:09:03, Devlin wrote: > On 2014/10/28 23:43:56, aboxhall wrote: > > On 2014/10/28 21:05:09, Devlin wrote: > > > nit: no virtual (also for other methods) > > > > I don't understand; this is overriding a virtual method from > > WebContentsObserver. > > See above - no virtual, just override. i.e. > bool OnMessageReceived(const IPC::Message& message) override { Done. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:71: // There may be several requests in flight; check that this response matches On 2014/10/29 16:09:03, Devlin wrote: > On 2014/10/28 23:43:56, aboxhall wrote: > > On 2014/10/28 21:05:10, Devlin wrote: > > > nit: End the sentence with a period. > > > > I don't think it's worth wrapping onto a new line. > > Sadly, style rules. :/ (My personal preference would probably be more in line > with your own, but we need to be consistent across the codebase.) I don't see it in the style guide. I don't think wrapping and adding a period would aid readability in any way. If you really insist, I'll reword. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:102: extensions::AutomationInternalQuerySelectorFunction::Callback callback_; On 2014/10/29 16:09:03, Devlin wrote: > On 2014/10/28 23:43:56, aboxhall wrote: > > On 2014/10/28 21:05:09, Devlin wrote: > > > Why not just put the class in extensions::? > > > > Which class? > > QuerySelectorHandler. Basically, move the namespace extensions on line 107 to > 38. Or, better yet, just take out line 36. Moved this into extensions:: below the const declarations in the anonymous namespace. https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:316: int AutomationInternalQuerySelectorFunction::query_request_id_counter_ = 0; On 2014/10/29 16:09:03, Devlin wrote: > On 2014/10/28 23:43:56, aboxhall wrote: > > On 2014/10/28 21:05:09, Devlin wrote: > > > why not put this in an anonymous namespace? Is it going to be used outside > of > > > this file? > > > > It's scoped and private to the class which uses it, and this way I can define > it > > near where it's used. What would the benefit be of moving it into an anonymous > > namespace? > > Reduces the header import size by 0.000001%. :P More importantly, it makes it > clear that it's only used in this file, and serves as a kind of visual queue to > readers (my opinion). If you feel strongly, I won't fight you on it. :) I don't feel particularly strongly, but I do think it's easier to understand what it's for when it's scoped to the class, so I'll leave it.
On 2014/10/29 00:00:10, dmazzoni wrote: > https://codereview.chromium.org/655273005/diff/1/chrome/common/extensions/api... > File chrome/common/extensions/api/automation.idl (right): > > https://codereview.chromium.org/655273005/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/automation.idl:226: }; > On 2014/10/28 23:43:56, aboxhall wrote: > > On 2014/10/27 20:44:35, David Tseng wrote: > > > Will there be additional info members? If not, then just expose the query > info > > > as a string. > > > > Right, I guess my CL description was a little misleading :) > > > > This is the proposed API in the linked bug > > (https://code.google.com/p/chromium/issues/detail?id=404710) > > My vote is to have two separate APIs. I'll comment on the bug. > > https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... > File extensions/renderer/api/automation/automation_api_helper.cc (right): > > https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... > extensions/renderer/api/automation/automation_api_helper.cc:55: node = > obj.node(); > One subtle nuance here - not every AXObject corresponds to a node - it's > possible that an AXObject is CSS generated content, an anonymous render block, > or a scroll view, or a spinner button, or one of many objects that aren't nodes. > > You should check for a null node here - but also consider that it might be > confusing if this query fails on some ax objects but succeeds on others - maybe > walk up the ancestors and run the query on the nearest ancestor that corresponds > to a node? This is very interesting. Is there ever a risk that by walking up the tree we're going to end up including siblings which we don't want to include in the query scope?
https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:56: extensions::AutomationInternalQuerySelectorFunction::Callback callback) On 2014/10/29 16:35:19, aboxhall wrote: > On 2014/10/29 16:09:03, Devlin wrote: > > On 2014/10/28 23:43:56, aboxhall wrote: > > > On 2014/10/28 21:05:09, Devlin wrote: > > > > const & > > > > > > I don't think this makes sense. > > > > Sure it does - you can copy callbacks just like anything else. You are making > a > > copy of it when you assign it to |callback_|, so right now you make a copy > when > > passing to the constructor, and another copy when assigning to |callback_| > > (unless clang detects that |callback| is never actually used, and does crazy > > optimization). Let's eliminate one of those copies. > > I realise I'm making a copy, but I think that's the right thing to do here - > it's only two pointers, and it's allocated on the stack in the calling method, > which this class outlives. Ah right, but then I copy it again. Will fix.
https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:56: extensions::AutomationInternalQuerySelectorFunction::Callback callback) On 2014/10/29 16:35:19, aboxhall wrote: > On 2014/10/29 16:09:03, Devlin wrote: > > On 2014/10/28 23:43:56, aboxhall wrote: > > > On 2014/10/28 21:05:09, Devlin wrote: > > > > const & > > > > > > I don't think this makes sense. > > > > Sure it does - you can copy callbacks just like anything else. You are making > a > > copy of it when you assign it to |callback_|, so right now you make a copy > when > > passing to the constructor, and another copy when assigning to |callback_| > > (unless clang detects that |callback| is never actually used, and does crazy > > optimization). Let's eliminate one of those copies. > > I realise I'm making a copy, but I think that's the right thing to do here - > it's only two pointers, and it's allocated on the stack in the calling method, > which this class outlives. One copy is good, but right now there could be two. If you make the constructor QuerySelectorHandler( ... const AutomationInternalQuerySelectorFunction::Callback& callback) : ... callback_(callback) { } you still make a copy when you assign from |callback| into |callback_|. There's no reason to (potentially, depending on the site) make another when assigning from the call site into |callback|, right? https://codereview.chromium.org/655273005/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:71: // There may be several requests in flight; check that this response matches On 2014/10/29 16:35:19, aboxhall wrote: > On 2014/10/29 16:09:03, Devlin wrote: > > On 2014/10/28 23:43:56, aboxhall wrote: > > > On 2014/10/28 21:05:10, Devlin wrote: > > > > nit: End the sentence with a period. > > > > > > I don't think it's worth wrapping onto a new line. > > > > Sadly, style rules. :/ (My personal preference would probably be more in line > > with your own, but we need to be consistent across the codebase.) > > I don't see it in the style guide. I don't think wrapping and adding a period > would aid readability in any way. If you really insist, I'll reword. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Comment_Style "Comments should be as readable as narrative text, with proper capitalization and punctuation... Although it can be frustrating to have a code reviewer point out that you are using a comma when you should be using a semicolon, it is very important that source code maintain a high level of clarity and readability. Proper punctuation, spelling, and grammar help with that goal." Sorry to be a stickler. :/
Mostly just small stuff. https://codereview.chromium.org/655273005/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:54: class QuerySelectorHandler : public content::WebContentsObserver { This class should still be in an anonymous namespace (to prevent name potential name collisions, even if they're unlikely), but that anonymous namespace can be in the extensions namespace. I.e., namespace extensions { namespace { class QuerySelectorHandler... } // namespace ... } // namespace extensions https://codereview.chromium.org/655273005/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:355: return; To me, this "early" return seems kind of silly. Why not just if (!error.empty()) Respond(Error(error)); else Respond(OneArgument(...))); ? https://codereview.chromium.org/655273005/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.h (right): https://codereview.chromium.org/655273005/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.h:81: virtual ~AutomationInternalQuerySelectorFunction() {} same comments on virtual/override here. https://codereview.chromium.org/655273005/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/655273005/diff/20001/chrome/common/extensions... chrome/common/extensions/api/automation.idl:304: // Gets the first node in this node's subtree which matches the query specified more than 80 characters https://codereview.chromium.org/655273005/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/655273005/diff/20001/chrome/common/extensions... chrome/common/extensions/api/automation_internal.idl:88: dictionary QuerySelectorRequiredParams { comments. https://codereview.chromium.org/655273005/diff/20001/chrome/common/extensions... chrome/common/extensions/api/automation_internal.idl:117: // Performs a query selector query. Calls the callback with a request ID to It's odd that this is the only place you use the term "request ID" instead of "automationNodeID" - if they're the same thing, stick to one name (if they're not, what is a request id?) https://codereview.chromium.org/655273005/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/automation/sites/index.html (right): https://codereview.chromium.org/655273005/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/automation/sites/index.html:6: 9<html> typo? https://codereview.chromium.org/655273005/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/automation/tests/tabs/query.js (right): https://codereview.chromium.org/655273005/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/automation/tests/tabs/query.js:9: console.log("cancelButton: " + cancelButton.toString()); These logs should be removed before committing (just making a note, no need to remove them now). https://codereview.chromium.org/655273005/diff/20001/extensions/common/extens... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/655273005/diff/20001/extensions/common/extens... extensions/common/extension_messages.h:763: // AXid is the accessibility tree ID of the result element; 0 indicates no AXid doesn't refer to a variable name. https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... File extensions/renderer/api/automation/automation_api_helper.cc (right): https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.cc:45: blink::WebDocument document = I don't know blink accessibility code well enough to fully grok from here to line 65, so I'll trust the other reviewers here. https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... File extensions/renderer/api/automation/automation_api_helper.h (right): https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.h:8: #include <string> don't need this. https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.h:19: AutomationApiHelper(content::RenderView* render_view); explicit https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.h:20: virtual ~AutomationApiHelper(); Same virtual/override comments https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.h:33: } nit: newline https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.h:33: } nit: // namespace extensions
On Wed, Oct 29, 2014 at 9:58 AM, <aboxhall@chromium.org> wrote: > https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/ > automation/automation_api_helper.cc#newcode55 > >> extensions/renderer/api/automation/automation_api_helper.cc:55: node = >> obj.node(); >> One subtle nuance here - not every AXObject corresponds to a node - it's >> possible that an AXObject is CSS generated content, an anonymous render >> block, >> or a scroll view, or a spinner button, or one of many objects that aren't >> > nodes. > > You should check for a null node here - but also consider that it might be >> confusing if this query fails on some ax objects but succeeds on others - >> > maybe > >> walk up the ancestors and run the query on the nearest ancestor that >> > corresponds > >> to a node? >> > > This is very interesting. Is there ever a risk that by walking up the tree > we're > going to end up including siblings which we don't want to include in the > query > scope? > Yes, that's quite possible. Here's an example - given this html: <button>Button</button> <button>Button 2</button> <p>Para</p> You get this AX tree: AXWebArea AXGroup AXButton AXTitle='Button' AXButton AXTitle='Button 2' AXGroup AXStaticText AXValue='Para' The two buttons are grouped together because they're both inline, so an anonymous block has to be created around them during layout. Given the AX tree, you'd think that it'd be possible to do a query from the first AXGroup and search for the button you want - but as you can see from the DOM, that's not possible - the buttons and the paragraph are actually siblings in the DOM. I don't think there's any way around this, just pick a reasonable interpretation and document it. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/655273005/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:54: class QuerySelectorHandler : public content::WebContentsObserver { On 2014/10/29 21:23:43, Devlin wrote: > This class should still be in an anonymous namespace (to prevent name potential > name collisions, even if they're unlikely), but that anonymous namespace can be > in the extensions namespace. I.e., > > namespace extensions { > > namespace { > > class QuerySelectorHandler... > > } // namespace > > ... > > } // namespace extensions Done. https://codereview.chromium.org/655273005/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.h (right): https://codereview.chromium.org/655273005/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.h:81: virtual ~AutomationInternalQuerySelectorFunction() {} On 2014/10/29 21:23:43, Devlin wrote: > same comments on virtual/override here. Done. https://codereview.chromium.org/655273005/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/655273005/diff/20001/chrome/common/extensions... chrome/common/extensions/api/automation.idl:304: // Gets the first node in this node's subtree which matches the query specified On 2014/10/29 21:23:43, Devlin wrote: > more than 80 characters Done. https://codereview.chromium.org/655273005/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/automation_internal.idl (right): https://codereview.chromium.org/655273005/diff/20001/chrome/common/extensions... chrome/common/extensions/api/automation_internal.idl:88: dictionary QuerySelectorRequiredParams { On 2014/10/29 21:23:43, Devlin wrote: > comments. Done. https://codereview.chromium.org/655273005/diff/20001/chrome/common/extensions... chrome/common/extensions/api/automation_internal.idl:117: // Performs a query selector query. Calls the callback with a request ID to On 2014/10/29 21:23:43, Devlin wrote: > It's odd that this is the only place you use the term "request ID" instead of > "automationNodeID" - if they're the same thing, stick to one name (if they're > not, what is a request id?) Ah, that comment is out of date - fixed. https://codereview.chromium.org/655273005/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/automation/sites/index.html (right): https://codereview.chromium.org/655273005/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/automation/sites/index.html:6: 9<html> On 2014/10/29 21:23:43, Devlin wrote: > typo? Done. https://codereview.chromium.org/655273005/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/automation/sites/index.html:6: 9<html> On 2014/10/29 21:23:43, Devlin wrote: > typo? Done. https://codereview.chromium.org/655273005/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/automation/tests/tabs/query.js (right): https://codereview.chromium.org/655273005/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/automation/tests/tabs/query.js:9: console.log("cancelButton: " + cancelButton.toString()); On 2014/10/29 21:23:43, Devlin wrote: > These logs should be removed before committing (just making a note, no need to > remove them now). Acknowledged. https://codereview.chromium.org/655273005/diff/20001/extensions/common/extens... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/655273005/diff/20001/extensions/common/extens... extensions/common/extension_messages.h:763: // AXid is the accessibility tree ID of the result element; 0 indicates no On 2014/10/29 21:23:43, Devlin wrote: > AXid doesn't refer to a variable name. Done. https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... File extensions/renderer/api/automation/automation_api_helper.cc (right): https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.cc:45: blink::WebDocument document = On 2014/10/29 21:23:44, Devlin wrote: > I don't know blink accessibility code well enough to fully grok from here to > line 65, so I'll trust the other reviewers here. Acknowledged. https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... File extensions/renderer/api/automation/automation_api_helper.h (right): https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.h:8: #include <string> On 2014/10/29 21:23:44, Devlin wrote: > don't need this. Done. https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.h:19: AutomationApiHelper(content::RenderView* render_view); On 2014/10/29 21:23:44, Devlin wrote: > explicit Done. https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.h:20: virtual ~AutomationApiHelper(); On 2014/10/29 21:23:44, Devlin wrote: > Same virtual/override comments Done. https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.h:33: } On 2014/10/29 21:23:44, Devlin wrote: > nit: newline Done. https://codereview.chromium.org/655273005/diff/20001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.h:33: } On 2014/10/29 21:23:44, Devlin wrote: > nit: // namespace extensions Done.
https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... File extensions/renderer/api/automation/automation_api_helper.cc (right): https://codereview.chromium.org/655273005/diff/1/extensions/renderer/api/auto... extensions/renderer/api/automation/automation_api_helper.cc:55: node = obj.node(); On 2014/10/29 00:00:10, dmazzoni wrote: > One subtle nuance here - not every AXObject corresponds to a node - it's > possible that an AXObject is CSS generated content, an anonymous render block, > or a scroll view, or a spinner button, or one of many objects that aren't nodes. > > You should check for a null node here - but also consider that it might be > confusing if this query fails on some ax objects but succeeds on others - maybe > walk up the ancestors and run the query on the nearest ancestor that corresponds > to a node? PTAL - I've tried to address this.
Ping?
I'm mostly happy. I'll hold off on the lg until dmazzoni and dtseng confirm that there's nothing else large needed. https://codereview.chromium.org/655273005/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:355: return; On 2014/10/29 21:23:43, Devlin wrote: > To me, this "early" return seems kind of silly. Why not just > if (!error.empty()) > Respond(Error(error)); > else > Respond(OneArgument(...))); > ? Looks like this comment went unaddressed? https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:78: int message_request_id; nit: initialize to 0. https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:137: void DoDefault(int32 id) override { rfh_->AccessibilityDoDefaultAction(id); } use int32_t and include stdint.h https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:153: }; nit: newline https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:154: } // anonymous namespace nit: typically just "namespace" is enough. https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:156: nit: just one newline. :) https://codereview.chromium.org/655273005/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/automation/tests/tabs/queryselector.js (right): https://codereview.chromium.org/655273005/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/automation/tests/tabs/queryselector.js:9: function assertCorrectResult(queryResult) { I'm not too passionate about it, but is there a reason to not inline these in the api call? https://codereview.chromium.org/655273005/diff/40001/extensions/common/extens... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/655273005/diff/40001/extensions/common/extens... extensions/common/extension_messages.h:753: nit: remove extra newline. https://codereview.chromium.org/655273005/diff/40001/extensions/extensions.gyp File extensions/extensions.gyp (right): https://codereview.chromium.org/655273005/diff/40001/extensions/extensions.gy... extensions/extensions.gyp:839: 'renderer/api/automation/automation_api_helper.cc', These should be duplicated in the GN build, I think.
https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:337: return RespondNow(Error("querySelector query sent on destroyed node")); nit: this is really "destroyed tree" or "destroyed web contents" or something like that https://codereview.chromium.org/655273005/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/655273005/diff/40001/chrome/common/extensions... chrome/common/extensions/api/automation.idl:310: // ancestor of this node. nit: ancestor -> descendant https://codereview.chromium.org/655273005/diff/40001/chrome/common/extensions... chrome/common/extensions/api/automation.idl:313: // automation node (for example an element within an ARIA widget, where the Your example doesn't make sense, we don't remove nodes from aria widgets from the automation tree. The simplest example would be nodes that are hidden or not displayed - they'll be returned by querySelector but won't have an automation node. https://codereview.chromium.org/655273005/diff/40001/chrome/renderer/resource... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/655273005/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/automation/automation_node.js:234: if (!resultNode) { Do you want to return here? It looks like you're calling userCallback(null) if the result is not in the tree. https://codereview.chromium.org/655273005/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/automation/tests/tabs/queryselector.js (right): https://codereview.chromium.org/655273005/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/automation/tests/tabs/queryselector.js:5: var allTests = [ How about a test where the query string doesn't return a node? https://codereview.chromium.org/655273005/diff/40001/extensions/renderer/api/... File extensions/renderer/api/automation/automation_api_helper.cc (right): https://codereview.chromium.org/655273005/diff/40001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.cc:55: while (node.isNull()) { I'm worried this could give you an endless loop if document.accessibilityObjectFromID returns you a null object above. You should also terminate the loop if obj.isNull() Maybe you should write a test that gets an automation node for an element, then has the page remove that element from the page, then tries to do a query on that node. https://codereview.chromium.org/655273005/diff/40001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.cc:68: ax_obj = ax_obj.parentObject(); nit: you can just call ax_obj.parentObjectUnignored() and make it an if instead of a while loop.
https://codereview.chromium.org/655273005/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:355: return; On 2014/10/30 23:26:53, Devlin wrote: > On 2014/10/29 21:23:43, Devlin wrote: > > To me, this "early" return seems kind of silly. Why not just > > if (!error.empty()) > > Respond(Error(error)); > > else > > Respond(OneArgument(...))); > > ? > > Looks like this comment went unaddressed? I prefer this style because it makes the normal vs. error case look different. https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:78: int message_request_id; On 2014/10/30 23:26:53, Devlin wrote: > nit: initialize to 0. Done, although note that this is not the norm: https://code.google.com/p/chromium/codesearch#search/&q=ReadInt%5C(&p=3&sq=pa... - and that the CHECK guarantees that this int will never be read uninitialized (unless something goes very wrong with ReadInt, in which case we probably have other problems). https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:137: void DoDefault(int32 id) override { rfh_->AccessibilityDoDefaultAction(id); } On 2014/10/30 23:26:53, Devlin wrote: > use int32_t and include stdint.h This isn't new code. https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:153: }; On 2014/10/30 23:26:53, Devlin wrote: > nit: newline Done. https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:154: } // anonymous namespace On 2014/10/30 23:26:53, Devlin wrote: > nit: typically just "namespace" is enough. Done. https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:156: On 2014/10/30 23:26:53, Devlin wrote: > nit: just one newline. :) Done. https://codereview.chromium.org/655273005/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:337: return RespondNow(Error("querySelector query sent on destroyed node")); On 2014/10/30 23:32:42, dmazzoni wrote: > nit: this is really "destroyed tree" or "destroyed web contents" or something > like that Good point, fixed. https://codereview.chromium.org/655273005/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/655273005/diff/40001/chrome/common/extensions... chrome/common/extensions/api/automation.idl:310: // ancestor of this node. On 2014/10/30 23:32:42, dmazzoni wrote: > nit: ancestor -> descendant Oops, done. https://codereview.chromium.org/655273005/diff/40001/chrome/common/extensions... chrome/common/extensions/api/automation.idl:313: // automation node (for example an element within an ARIA widget, where the On 2014/10/30 23:32:42, dmazzoni wrote: > Your example doesn't make sense, we don't remove nodes from aria widgets from > the automation tree. The simplest example would be nodes that are hidden or not > displayed - they'll be returned by querySelector but won't have an automation > node. The are ignoredForAccessibility and don't end up in the tree, as per my test, but I added comments about hidden elements as well. I can remove the ARIA widget example if I've missed something... https://codereview.chromium.org/655273005/diff/40001/chrome/renderer/resource... File chrome/renderer/resources/extensions/automation/automation_node.js (right): https://codereview.chromium.org/655273005/diff/40001/chrome/renderer/resource... chrome/renderer/resources/extensions/automation/automation_node.js:234: if (!resultNode) { On 2014/10/30 23:32:42, dmazzoni wrote: > Do you want to return here? It looks like you're calling userCallback(null) if > the result is not in the tree. I think that's WAI - they need to know that they don't need to wait any longer. I thought about setting an error in chrome.lastError, but I think this is really an implementation detail - the user wouldn't be able to act on the error. However, writing the test for removing the node demonstrated that this was calling the callback with undefined rather than null, so I fixed that here. https://codereview.chromium.org/655273005/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/automation/tests/tabs/queryselector.js (right): https://codereview.chromium.org/655273005/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/automation/tests/tabs/queryselector.js:5: var allTests = [ On 2014/10/30 23:32:42, dmazzoni wrote: > How about a test where the query string doesn't return a node? Good idea, done. https://codereview.chromium.org/655273005/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/automation/tests/tabs/queryselector.js:9: function assertCorrectResult(queryResult) { On 2014/10/30 23:26:53, Devlin wrote: > I'm not too passionate about it, but is there a reason to not inline these in > the api call? No real reason here, but I'm in the habit of doing it this way because in these tests we often need to chain asynchronous calls, and it gets confusing/unreadable very quickly if you declare functions inline. https://codereview.chromium.org/655273005/diff/40001/extensions/common/extens... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/655273005/diff/40001/extensions/common/extens... extensions/common/extension_messages.h:753: On 2014/10/30 23:26:53, Devlin wrote: > nit: remove extra newline. Done. https://codereview.chromium.org/655273005/diff/40001/extensions/common/extens... extensions/common/extension_messages.h:753: On 2014/10/30 23:26:53, Devlin wrote: > nit: remove extra newline. Done. https://codereview.chromium.org/655273005/diff/40001/extensions/renderer/api/... File extensions/renderer/api/automation/automation_api_helper.cc (right): https://codereview.chromium.org/655273005/diff/40001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.cc:55: while (node.isNull()) { On 2014/10/30 23:32:42, dmazzoni wrote: > I'm worried this could give you an endless loop if > document.accessibilityObjectFromID returns you a null object above. You should > also terminate the loop if obj.isNull() Good point. I think it's actually an error if that happens (i.e. we're never going to find a good DOM node here) so I just returned an error here. > Maybe you should write a test that gets an automation node for an element, then > has the page remove that element from the page, then tries to do a query on that > node. Great idea! Done. https://codereview.chromium.org/655273005/diff/40001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.cc:68: ax_obj = ax_obj.parentObject(); On 2014/10/30 23:32:42, dmazzoni wrote: > nit: you can just call ax_obj.parentObjectUnignored() and make it an if instead > of a while loop. It's not in the public api ;_;
lgtm https://codereview.chromium.org/655273005/diff/80001/chrome/common/extensions... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/655273005/diff/80001/chrome/common/extensions... chrome/common/extensions/api/automation.idl:224: dictionary QueryInfo { unused? https://codereview.chromium.org/655273005/diff/80001/extensions/renderer/api/... File extensions/renderer/api/automation/automation_api_helper.cc (right): https://codereview.chromium.org/655273005/diff/80001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.cc:74: while (ax_obj.accessibilityIsIgnored()) nit: indentation
aboxhall@chromium.org changed reviewers: + palmer@chromium.org
+palmer for extension_messages.h https://codereview.chromium.org/655273005/diff/80001/chrome/common/extensions... File chrome/common/extensions/api/automation.idl (right): https://codereview.chromium.org/655273005/diff/80001/chrome/common/extensions... chrome/common/extensions/api/automation.idl:224: dictionary QueryInfo { On 2014/10/31 22:02:07, dmazzoni wrote: > unused? Done. https://codereview.chromium.org/655273005/diff/80001/extensions/renderer/api/... File extensions/renderer/api/automation/automation_api_helper.cc (right): https://codereview.chromium.org/655273005/diff/80001/extensions/renderer/api/... extensions/renderer/api/automation/automation_api_helper.cc:74: while (ax_obj.accessibilityIsIgnored()) On 2014/10/31 22:02:07, dmazzoni wrote: > nit: indentation Done.
extensions lgtm https://codereview.chromium.org/655273005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:343: base::string16 selector = base::UTF8ToUTF16(params->args.selector); #include "base/strings/string16.h" https://codereview.chromium.org/655273005/diff/80001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/automation/tests/tabs/queryselector.js (right): https://codereview.chromium.org/655273005/diff/80001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/automation/tests/tabs/queryselector.js:30: console.log('main: ' + main.toString()); reminder: Remove these logs. https://codereview.chromium.org/655273005/diff/80001/extensions/extensions.gyp File extensions/extensions.gyp (right): https://codereview.chromium.org/655273005/diff/80001/extensions/extensions.gy... extensions/extensions.gyp:839: 'renderer/api/automation/automation_api_helper.cc', Still need these in gn, right?
https://codereview.chromium.org/655273005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:343: base::string16 selector = base::UTF8ToUTF16(params->args.selector); On 2014/10/31 22:11:28, Devlin wrote: > #include "base/strings/string16.h" Done. https://codereview.chromium.org/655273005/diff/80001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/automation/tests/tabs/queryselector.js (right): https://codereview.chromium.org/655273005/diff/80001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/automation/tests/tabs/queryselector.js:30: console.log('main: ' + main.toString()); On 2014/10/31 22:11:28, Devlin wrote: > reminder: Remove these logs. Done. https://codereview.chromium.org/655273005/diff/80001/extensions/extensions.gyp File extensions/extensions.gyp (right): https://codereview.chromium.org/655273005/diff/80001/extensions/extensions.gy... extensions/extensions.gyp:839: 'renderer/api/automation/automation_api_helper.cc', On 2014/10/31 22:11:28, Devlin wrote: > Still need these in gn, right? Oops, done.
https://codereview.chromium.org/655273005/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:140: void Focus(int32 id) override { rfh_->AccessibilitySetFocus(id); } Are these |id|s the same things as are declared as ints in the messages.h file? Probably best to be consistent, and call them int32s in messages.h too. https://codereview.chromium.org/655273005/diff/120001/extensions/common/exten... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/655273005/diff/120001/extensions/common/exten... extensions/common/extension_messages.h:759: base::string16 /* selector */) What is the lexical/syntactic structure of these strings? Size limits, et c.? (I have not read the other files in this CL yet.) Are they CSS selectors? Can we limit their size, and verify that they correctly parse as CSS selectors? https://codereview.chromium.org/655273005/diff/120001/extensions/common/exten... extensions/common/extension_messages.h:766: std::string /* error */, What's the range of these errors? Constrained, or infinite/large? My instinct is normally to pass an error enum and then have the recipient stringify it with a switch/case or an array of strings. (Benefits: Smaller and faster IPC messages, less potentially-hostile data in the recipient's address space, strongly-typed errors.)
https://codereview.chromium.org/655273005/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:140: void Focus(int32 id) override { rfh_->AccessibilitySetFocus(id); } On 2014/11/01 00:20:48, Chromium Palmer wrote: > Are these |id|s the same things as are declared as ints in the messages.h file? > Probably best to be consistent, and call them int32s in messages.h too. Note that this isn't new code. Also, the code which actually creates the message takes an int (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...), so I'd have to change the entire call stack, which seems out of scope for this change. I can make a follow-up change to fix it if you like. https://codereview.chromium.org/655273005/diff/120001/extensions/common/exten... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/655273005/diff/120001/extensions/common/exten... extensions/common/extension_messages.h:759: base::string16 /* selector */) On 2014/11/01 00:20:48, Chromium Palmer wrote: > What is the lexical/syntactic structure of these strings? Size limits, et c.? (I > have not read the other files in this CL yet.) > > Are they CSS selectors? Can we limit their size, and verify that they correctly > parse as CSS selectors? Yes, they're CSS selectors. Where should the size limits and verification go? https://codereview.chromium.org/655273005/diff/120001/extensions/common/exten... extensions/common/extension_messages.h:766: std::string /* error */, On 2014/11/01 00:20:48, Chromium Palmer wrote: > What's the range of these errors? Constrained, or infinite/large? > > My instinct is normally to pass an error enum and then have the recipient > stringify it with a switch/case or an array of strings. (Benefits: Smaller and > faster IPC messages, less potentially-hostile data in the recipient's address > space, strongly-typed errors.) An enum sounds good; should it live in this file?
> > Are these |id|s the same things as are declared as ints in the messages.h > file? > > Probably best to be consistent, and call them int32s in messages.h too. > > Note that this isn't new code. Also, the code which actually creates the message > takes an int > (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...), > so I'd have to change the entire call stack, which seems out of scope for this > change. I can make a follow-up change to fix it if you like. Sure sounds good. It's not like a burning security freakout; call it a low-priority code cleanup. > Yes, they're CSS selectors. Where should the size limits and verification go? For security: On the browser side, when taking in the IPC message. To avoid having innocent renderers mistakenly send messages the browser will reject, you could optionally also check on the renderer side just before sending the messages. > > My instinct is normally to pass an error enum and then have the recipient > > stringify it with a switch/case or an array of strings. (Benefits: Smaller and > > faster IPC messages, less potentially-hostile data in the recipient's address > > space, strongly-typed errors.) > > An enum sounds good; should it live in this file? Anywhere that makes sense to you. It's probably the case that both sender and receiver need to know the enum, so it should go in some place they share; only the receiver needs to care about the strings, so that can be private.
https://codereview.chromium.org/655273005/diff/120001/extensions/common/exten... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/655273005/diff/120001/extensions/common/exten... extensions/common/extension_messages.h:759: base::string16 /* selector */) On 2014/11/03 17:27:11, aboxhall wrote: > On 2014/11/01 00:20:48, Chromium Palmer wrote: > > What is the lexical/syntactic structure of these strings? Size limits, et c.? > (I > > have not read the other files in this CL yet.) > > > > Are they CSS selectors? Can we limit their size, and verify that they > correctly > > parse as CSS selectors? > > Yes, they're CSS selectors. Where should the size limits and verification go? Actually, after looking into this further, I'm not sure this is technically feasible. The CSS parser is in Blink, so I think I'd have to send an IPC to parse it anyway, and I don't think there's any obvious value to pick for the size length. I found at least one other point where we (eventually) send a querySelector through an extension_messages IPC message: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... - so at least this isn't a regression? :) https://codereview.chromium.org/655273005/diff/120001/extensions/common/exten... extensions/common/extension_messages.h:766: std::string /* error */, On 2014/11/03 17:27:11, aboxhall wrote: > On 2014/11/01 00:20:48, Chromium Palmer wrote: > > What's the range of these errors? Constrained, or infinite/large? > > > > My instinct is normally to pass an error enum and then have the recipient > > stringify it with a switch/case or an array of strings. (Benefits: Smaller and > > faster IPC messages, less potentially-hostile data in the recipient's address > > space, strongly-typed errors.) > > An enum sounds good; should it live in this file? Done.
palmer: ping?
This is failing to build on linux gn with the message: ninja: error: '../../extensions/renderer/automation_api_helper.cc', needed by 'obj/extensions/renderer/renderer.automation_api_helper.o', missing and no known rule to make it (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) I added it to extensions/renderer/BUILD.gn already - did I do it wrong?
On 2014/11/04 19:12:10, aboxhall wrote: > This is failing to build on linux gn with the message: > ninja: error: '../../extensions/renderer/automation_api_helper.cc', needed by > 'obj/extensions/renderer/renderer.automation_api_helper.o', missing and no known > rule to make it > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > I added it to extensions/renderer/BUILD.gn already - did I do it wrong? Isn't the path api/automation/automation_api_helper, not just automation_api_helper?
On 2014/11/04 19:26:44, Devlin wrote: > On 2014/11/04 19:12:10, aboxhall wrote: > > This is failing to build on linux gn with the message: > > ninja: error: '../../extensions/renderer/automation_api_helper.cc', needed by > > 'obj/extensions/renderer/renderer.automation_api_helper.o', missing and no > known > > rule to make it > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > > > I added it to extensions/renderer/BUILD.gn already - did I do it wrong? > > Isn't the path api/automation/automation_api_helper, not just > automation_api_helper? That'd be it. Thanks!
LGTM, but you probably don't want to crash the browser process unnecessarily. https://codereview.chromium.org/655273005/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:86: CHECK(message.ReadInt(&iter, &message_request_id)); Browser crash on bad IPC? Better to just return false? https://codereview.chromium.org/655273005/diff/140001/extensions/renderer/api... File extensions/renderer/api/automation/automation_api_helper.cc (right): https://codereview.chromium.org/655273005/diff/140001/extensions/renderer/api... extensions/renderer/api/automation/automation_api_helper.cc:57: blink::WebAXObject obj = document.accessibilityObjectFromID(acc_obj_id); Nit: "accessibility object" is sometimes "acc_obj" and sometimes "ax_obj". Might be good to standardize on 1.
https://codereview.chromium.org/655273005/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_internal_api.cc (right): https://codereview.chromium.org/655273005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:86: CHECK(message.ReadInt(&iter, &message_request_id)); On 2014/11/04 19:39:22, Chromium Palmer wrote: > Browser crash on bad IPC? Better to just return false? Excellent point, done. https://codereview.chromium.org/655273005/diff/140001/extensions/renderer/api... File extensions/renderer/api/automation/automation_api_helper.cc (right): https://codereview.chromium.org/655273005/diff/140001/extensions/renderer/api... extensions/renderer/api/automation/automation_api_helper.cc:57: blink::WebAXObject obj = document.accessibilityObjectFromID(acc_obj_id); On 2014/11/04 19:39:22, Chromium Palmer wrote: > Nit: "accessibility object" is sometimes "acc_obj" and sometimes "ax_obj". Might > be good to standardize on 1. Done.
The CQ bit was checked by aboxhall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/655273005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by aboxhall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/655273005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...) linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by aboxhall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/655273005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by aboxhall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/655273005/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/7abfb3db96dc7d87c9f1282b5fc81328ec9c787c Cr-Commit-Position: refs/heads/master@{#302944} |
