Implement AutomationNode.querySelector().

chrome/browser/extensions/api/automation_internal/automation_internal_api.cc

Index: chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
diff --git a/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc b/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
index 40e184bbf3b64d5776b523af8abb94ca9feb063d..e6c770ba90bfe1c1224a801a0da0d52fb554ff2d 100644
--- a/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
+++ b/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
@@ -7,6 +7,7 @@
 #include <vector>
 
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/api/automation_internal/automation_action_adapter.h"
 #include "chrome/browser/extensions/api/automation_internal/automation_util.h"
 #include "chrome/browser/extensions/api/tabs/tabs_constants.h"
@@ -19,9 +20,11 @@
 #include "content/public/browser/ax_event_notification_details.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
+#include "extensions/common/extension_messages.h"
 #include "extensions/common/permissions/permissions_data.h"
 
 #if defined(OS_CHROMEOS)
@@ -41,6 +44,64 @@ const int kDesktopRoutingID = 0;
 const char kCannotRequestAutomationOnPage[] =
     "Cannot request automation tree on url \"*\". "
     "Extension manifest must request permission to access this host.";
+const char kRendererDestroyed[] = "The tab was closed.";
+
+class QuerySelectorHandler : public content::WebContentsObserver {

[Devlin, 2014/10/28 21:05:09]

document

[aboxhall, 2014/10/28 23:43:56]

Done.

+ public:
+  QuerySelectorHandler(
+      content::WebContents* web_contents,
+      int request_id,
+      int acc_obj_id,
+      base::string16& query,

[Devlin, 2014/10/28 21:05:09]

const

[aboxhall, 2014/10/28 23:43:55]

Done.

+      extensions::AutomationInternalQuerySelectorFunction::Callback callback)

[Devlin, 2014/10/28 21:05:09]

const &

[aboxhall, 2014/10/28 23:43:56]

I don't think this makes sense.

[Devlin, 2014/10/29 16:09:03]

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.

[aboxhall, 2014/10/29 16:35:19]

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.

[aboxhall, 2014/10/29 17:05:20]

Ah right, but then I copy it again. Will fix.

[Devlin, 2014/10/29 17:05:36]

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?

+      : content::WebContentsObserver(web_contents),
+        request_id_(request_id),
+        callback_(callback) {
+    content::RenderViewHost* rvh = web_contents->GetRenderViewHost();
+    rvh->Send(new ExtensionMsg_AutomationQuerySelector(
+        rvh->GetRoutingID(), request_id, acc_obj_id, query));
+  }
+
+  virtual ~QuerySelectorHandler() {}

[Devlin, 2014/10/28 21:05:09]

nit: ~QuerySelectorHandler() override {}

[aboxhall, 2014/10/28 23:43:55]

Why override?

[Devlin, 2014/10/29 16:09:03]

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.

[aboxhall, 2014/10/29 16:35:19]

Got it, thanks.
(https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/C$2B$2...
in case anyone else is curious)

+
+  virtual bool OnMessageReceived(const IPC::Message& message) override {

[Devlin, 2014/10/28 21:05:09]

nit: no virtual (also for other methods)

[aboxhall, 2014/10/28 23:43:56]

I don't understand; this is overriding a virtual method from
WebContentsObserver.

[Devlin, 2014/10/29 16:09:03]

See above - no virtual, just override.  i.e.
bool OnMessageReceived(const IPC::Message& message) override {

[aboxhall, 2014/10/29 16:35:19]

Done.

+    if (message.type() != ExtensionHostMsg_AutomationQuerySelector_Result::ID)
+      return false;
+
+    // There may be several requests in flight; check that this response matches

[Devlin, 2014/10/28 21:05:10]

nit: End the sentence with a period.

[aboxhall, 2014/10/28 23:43:56]

I don't think it's worth wrapping onto a new line.

[Devlin, 2014/10/29 16:09:03]

Sadly, style rules. :/  (My personal preference would probably be more in line
with your own, but we need to be consistent across the codebase.)

[aboxhall, 2014/10/29 16:35:19]

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.

[Devlin, 2014/10/29 17:05:36]

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. :/

+    int unused_result_acc_obj_id;
+    int message_request_id;
+    PickleIterator iter(message);
+    CHECK(message.ReadInt(&iter, &unused_result_acc_obj_id));
+    CHECK(message.ReadInt(&iter, &message_request_id));

[Devlin, 2014/10/28 21:05:09]

I'm not super familiar with the inner workings of pickles - do we not need to
read the string (the second parameter)?

[aboxhall, 2014/10/28 23:43:55]

Good question. This was actually several flavours of wrong- should be fixed now.

+
+    if (message_request_id != request_id_)
+      return false;
+
+    IPC_BEGIN_MESSAGE_MAP(QuerySelectorHandler, message)
+      IPC_MESSAGE_HANDLER(ExtensionHostMsg_AutomationQuerySelector_Result,
+                          OnQueryResponse)
+    IPC_END_MESSAGE_MAP()
+    return true;
+  }
+
+  virtual void WebContentsDestroyed() override {

[Devlin, 2014/10/28 21:05:09]

It's good that we watch the web contents, but shouldn't we also watch the render
view to which we sent the query?

[aboxhall, 2014/10/28 23:43:56]

I'm not sure; I copied this pattern from extensions/browser/script_executor.cc
which doesn't watch the RV(H).

+    callback_.Run(kRendererDestroyed, 0);
+    delete this;
+  }
+
+ private:
+  void OnQueryResponse(int request_id,
+                       const std::string& error,
+                       int result_acc_obj_id) {
+    callback_.Run(error, result_acc_obj_id);
+    delete this;
+  }
+
+  int request_id_;
+  extensions::AutomationInternalQuerySelectorFunction::Callback callback_;

[Devlin, 2014/10/28 21:05:09]

Why not just put the class in extensions::?

[aboxhall, 2014/10/28 23:43:56]

Which class?

[Devlin, 2014/10/29 16:09:03]

QuerySelectorHandler. Basically, move the namespace extensions on line 107 to
38.  Or, better  yet, just take out line 36.

[aboxhall, 2014/10/29 16:35:19]

Moved this into extensions:: below the const declarations in the anonymous
namespace.

+};
+
 }  // namespace
 
 namespace extensions {
@@ -167,7 +228,7 @@ AutomationInternalEnableTabFunction::Run() {
   return RespondNow(
       ArgumentList(api::automation_internal::EnableTab::Results::Create(
           rfh->GetProcess()->GetID(), rfh->GetRoutingID())));
-  }
+}
 
 ExtensionFunction::ResponseAction
 AutomationInternalPerformActionFunction::Run() {
@@ -251,4 +312,49 @@ AutomationInternalEnableDesktopFunction::Run() {
 #endif  // defined(OS_CHROMEOS)
 }
 
+// static
+int AutomationInternalQuerySelectorFunction::query_request_id_counter_ = 0;

[Devlin, 2014/10/28 21:05:09]

why not put this in an anonymous namespace?  Is it going to be used outside of
this file?

[aboxhall, 2014/10/28 23:43:56]

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?

[Devlin, 2014/10/29 16:09:03]

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. :)

[aboxhall, 2014/10/29 16:35:19]

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.

+
+ExtensionFunction::ResponseAction
+AutomationInternalQuerySelectorFunction::Run() {
+  const AutomationInfo* automation_info = AutomationInfo::Get(extension());
+  EXTENSION_FUNCTION_VALIDATE(automation_info);
+
+  using api::automation_internal::QuerySelector::Params;
+  scoped_ptr<Params> params(Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  if (params->args.process_id == kDesktopProcessID &&
+      params->args.routing_id == kDesktopRoutingID) {
+    return RespondNow(
+        Error("querySelector queries may not be used on the desktop."));
+  }
+  content::RenderFrameHost* rfh = content::RenderFrameHost::FromID(
+      params->args.process_id, params->args.routing_id);
+  if (!rfh)
+    return RespondNow(Error("querySelector query sent on destroyed node"));
+
+  content::WebContents* contents =
+      content::WebContents::FromRenderFrameHost(rfh);
+
+  int request_id = query_request_id_counter_++;
+  base::string16 selector = base::UTF8ToUTF16(params->args.selector);
+
+  new QuerySelectorHandler(

[Devlin, 2014/10/28 21:05:10]

nit: comment that QuerySelectorHandler handles its own lifetime.

[aboxhall, 2014/10/28 23:43:56]

Done.

+      contents, request_id, params->args.automation_node_id, selector,

[Devlin, 2014/10/28 21:05:09]

nit: one arg per line (even though it's long)

[aboxhall, 2014/10/28 23:43:56]

This is how clang-format formats it, I'd rather leave it alone.

[Devlin, 2014/10/29 16:09:03]

Dang - they keep changing clang format's style (before, it would deliberately
take existing lines like this, and break them up).

+      base::Bind(&AutomationInternalQuerySelectorFunction::OnResponse, this));
+  return RespondLater();
+}
+
+void AutomationInternalQuerySelectorFunction::OnResponse(
+    const std::string& error,
+    int result_acc_obj_id) {
+  if (!error.empty()) {
+    Respond(Error(error));
+    return;
+  }
+
+  Respond(OneArgument(new base::FundamentalValue(result_acc_obj_id)));
+}
+
 }  // namespace extensions