Remove DOMAutomationController::automation_id_

content/public/test/browser_test_utils.cc

Index: content/public/test/browser_test_utils.cc
diff --git a/content/public/test/browser_test_utils.cc b/content/public/test/browser_test_utils.cc
index ec91284fda82f772e28cf9f8769ecf2e129d3a40..5b44b1df3b8ac98742492154747bff72f47278d7 100644
--- a/content/public/test/browser_test_utils.cc
+++ b/content/public/test/browser_test_utils.cc
@@ -413,19 +413,23 @@ void AppendGzippedResource(const base::RefCountedMemory& encoded,
 //
 // Returns has-video-input-device to the test if there is a webcam available,
 // no-video-input-devices otherwise.
-const char kHasVideoInputDeviceOnSystem[] =
-    "(function() {"
-      "navigator.mediaDevices.enumerateDevices()"
-      ".then(function(devices) {"
-        "devices.forEach(function(device) {"
-          "if (device.kind == 'videoinput') {"
-            "window.domAutomationController.send('has-video-input-device');"
-            "return;"
-          "}"
-        "});"
-        "window.domAutomationController.send('no-video-input-devices');"
-      "});"
-    "})()";
+const char kHasVideoInputDeviceOnSystem[] = R"(
+    (function() {
+      navigator.mediaDevices.enumerateDevices()
+      .then(function(devices) {
+        var found = false;
+        devices.forEach(function(device) {
+          if (device.kind == 'videoinput') {
+            window.domAutomationController.send('has-video-input-device');

[ncarter (slow), 2017/07/05 20:30:44]

looks like there could still be multiple sends() here if there are multiple
devices in the result with type 'videoinput'.

This can be fixed by either moving the 'has-video-input-device' send outside the
forEach. You could also use Array.prototype.some instead of
Array.prototype.forEach here:

if (devices.some((device) => device.kind == 'videoinput')) {
  window.domAutomationController.send('has-video-input-device');
} else {
  window.domAutomationController.send('no-video-input-devices');
}

[Łukasz Anforowicz, 2017/07/06 15:48:02]

Good catch.  Thanks for the "devices.some" suggestion - this is what I've
picked.

+            found = true;
+          }
+        });
+        if (!found) {
+          window.domAutomationController.send('no-video-input-devices');
+        }
+      });
+    })()
+)";
 
 const char kHasVideoInputDevice[] = "has-video-input-device";
 
@@ -816,6 +820,12 @@ RenderFrameHost* ConvertToRenderFrameHost(RenderFrameHost* render_frame_host) {
 
 bool ExecuteScript(const ToRenderFrameHost& adapter,
                    const std::string& script) {
+  // TODO(lukasza): ExecuteScript should just call
+  // ExecuteJavaScriptWithUserGestureForTests and avoid modifying the original
+  // script (and at that point we should remove ExecuteUnmodifiedScript).  This
+  // is difficult to change, because many tests depend on the message loop
+  // pumping done by ExecuteScriptHelper below (this is fragile - these tests
+  // should wait on a more specific thing instead).

[ncarter (slow), 2017/07/05 20:30:44]

We could make this more robust by generating a GUID here, making this delegate
to ExecuteScriptAndExtractString, and then asserting that the value we get back
is equal to the GUID.

I think it's fine for tests to rely on the ordering guarantees of ExecuteScript
doing an IPC round trip, but I'm glad that you're adding an async variant too.

[Łukasz Anforowicz, 2017/07/06 15:48:02]

Done.
Ok.  I forgot examples that I found long time ago, where the test could made for
a new window (via WebContentsAddedObserver), rather than depend on the
accidental timing/pumping inside ExecuteScript.  Still - I think the situation
after this CL is ok.
Ok.  Any feedback on the name (ExecuteUnmodifiedScript)?

   std::string new_script =
       script + ";window.domAutomationController.send(0);";
   return ExecuteScriptHelper(adapter.render_frame_host(), new_script, true,
@@ -829,6 +839,12 @@ bool ExecuteScriptWithoutUserGesture(const ToRenderFrameHost& adapter,
                              nullptr);
 }
 
+void ExecuteUnmodifiedScript(const ToRenderFrameHost& adapter,
+                             const std::string& script) {
+  adapter.render_frame_host()->ExecuteJavaScriptWithUserGestureForTests(
+      base::UTF8ToUTF16(script));
+}
+
 bool ExecuteScriptAndExtractDouble(const ToRenderFrameHost& adapter,
                                    const std::string& script, double* result) {
   DCHECK(result);
@@ -1003,8 +1019,7 @@ bool ExecuteWebUIResourceTest(WebContents* web_contents,
     return false;
 
   DOMMessageQueue message_queue;
-  if (!ExecuteScript(web_contents, "runTests()"))
-    return false;
+  ExecuteUnmodifiedScript(web_contents, "runTests()");
 
   std::string message;
   do {
@@ -1492,7 +1507,7 @@ void DOMMessageQueue::Observe(int type,
                               const NotificationDetails& details) {
   Details<std::string> dom_op_result(details);
   message_queue_.push(*dom_op_result.ptr());
-  if (message_loop_runner_.get())
+  if (message_loop_runner_)
     message_loop_runner_->Quit();
 }
 
@@ -1503,6 +1518,7 @@ void DOMMessageQueue::RenderProcessGone(base::TerminationStatus status) {
     case base::TERMINATION_STATUS_STILL_RUNNING:
       break;
     default:
+      renderer_crashed_ = true;
       if (message_loop_runner_.get())
         message_loop_runner_->Quit();
       break;
@@ -1515,7 +1531,7 @@ void DOMMessageQueue::ClearQueue() {
 
 bool DOMMessageQueue::WaitForMessage(std::string* message) {
   DCHECK(message);
-  if (message_queue_.empty()) {
+  if (!renderer_crashed_ && message_queue_.empty()) {
     // This will be quit when a new message comes in.
     message_loop_runner_ =
         new MessageLoopRunner(MessageLoopRunner::QuitMode::IMMEDIATE);
@@ -1526,7 +1542,7 @@ bool DOMMessageQueue::WaitForMessage(std::string* message) {
 
 bool DOMMessageQueue::PopMessage(std::string* message) {
   DCHECK(message);
-  if (message_queue_.empty())
+  if (renderer_crashed_ || message_queue_.empty())
     return false;
   *message = message_queue_.front();
   message_queue_.pop();