Remove DOMAutomationController::automation_id_

Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL (e.g. can get confused
   with other calls to domAutomationController.send(...)).  To account
   for this a new ExecuteScriptAsync function has been provided in
   this CL.

   Ideally ExecuteScript would become ExecuteScriptAsync, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteScriptAsync.  Therefore, ExecuteScriptAsync is
   a lesser evil, that minimizes amount of changes needed right now.

   To make sure that ExecuteScript stops waiting after the call to
   |domAutomationController.send(...)| that is added by ExecuteScript
   (and not by some other, accidental send) this CL sends a GUID that
   is unique in each call to ExecuteScript and verifies that the same
   GUID has been received back.  This change has flushed out a few
   additional cases where ExecuteScriptAsync needs to be used
   as well as cases where a call to domAutomationController.send
   can be removed.

   The following callers of ExecuteScript had to be switched to
   call ExecuteScriptAsync instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon).
     Additionally, it turned out that client_renderer.js and
     media_internals.js (used by WebUIResourceBrowserTest tests) were
     always erroring out before this CL (this has gone unnoticed,
     because the "FAILURE" string would have been eaten by
     ExecuteScript).

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteScriptAsync.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

   - ChromeSitePerProcessTest.PopupWindowFocus - this would conflict
     with domAutomationController.send from
     chrome/test/data/page_with_focus_events.html

   The following callers of ExecuteScript were needlessly calling
   domAutomationController.send from the script:

   - ProxyAuthOnUserBoardScreenTest.ProxyAuthDialogOnUserBoardScreen
   - WizardControllerTest::JSExecute
   - NavigationControllerBrowserTest.PostInSubframe
   - SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from the
   geoSuccessCallbackWithResponse callback (from
   chrome/test/data/geolocation/basic_geolocation.js)
   and once from a checkIfGeopositionUpdated function.

   There is no need to resolve the race via checkIfGeopositionUpdated if
   we start listening (via DOMMessageQueue) *before* trigerring an
   update in the geo position.  This way we can keep just the callback
   and remove the no longer needed checkIfGeopositionUpdated function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2478803003
Cr-Commit-Position: refs/heads/master@{#485461}
Committed: https://chromium.googlesource.com/chromium/src/+/c7d6bd3d4561ae85c1e2baf25136e930aa33b055

[Łukasz Anforowicz, 2016-11-04 22:24:33]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 22:25:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 23:03:32]

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

[commit-bot: I haz the power, 2016-11-04 23:03:33]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Łukasz Anforowicz, 2016-11-11 21:16:00]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 21:16:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 21:57:20]

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

[commit-bot: I haz the power, 2016-11-11 21:57:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2016-11-14 23:39:41]

Description was changed from

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc) used
   to append "; domAutomationController.send(0);" to the script provided
   by the caller and call ExecuteScriptHelper which would prepend
   "domAutomationController.setAutomationId(0); " to the script
   and pump message loop until the "send" is received in the browser.

   This can all be simplified, by just executing the original script
   provided by the caller.

   This is important, because otherwise spurious 0s would be received
   in the browser (breaking for example some WebUIResourceBrowserTest
   tests that where going through ExecuteWebUIResourceTest).

2. CaptivePortalBrowserTest.ShowCaptivePortalInterstitialOnCertError
   was relying on message pumping done by ExecuteScriptHelper.
   This was fragile - the timing of exiting the message pump
   was not related to the timing of the event the test needs to
   wait for.  This was broken after not going through
   ExecuteScriptHelper anymore as described in the previous item.

   This is fixed by introducing an explicit TabActivationWaiter.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
==========

to

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
==========

[Łukasz Anforowicz, 2016-11-14 23:42:47]

Description was changed from

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
==========

to

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
==========

[Łukasz Anforowicz, 2016-11-14 23:43:33]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 23:44:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 03:04:09]

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

[commit-bot: I haz the power, 2016-11-15 03:04:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Michael van Ouwerkerk, 2017-04-13 10:55:38]

Description was changed from

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
==========

to

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
==========

[Łukasz Anforowicz, 2017-06-20 22:21:03]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 22:21:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 22:51:13]

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

[commit-bot: I haz the power, 2017-06-20 22:51:14]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Łukasz Anforowicz, 2017-06-21 19:11:00]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 19:11:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-06-21 19:14:53]

Description was changed from

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
==========

to

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
==========

[commit-bot: I haz the power, 2017-06-21 19:54:50]

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

[commit-bot: I haz the power, 2017-06-21 19:54:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2017-06-22 21:03:02]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 21:03:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-06-22 21:05:00]

Description was changed from

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
==========

to

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
==========

[commit-bot: I haz the power, 2017-06-22 22:18:15]

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

[commit-bot: I haz the power, 2017-06-22 22:18:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2017-06-22 22:42:04]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 22:42:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-23 00:24:35]

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

[commit-bot: I haz the power, 2017-06-23 00:24:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2017-06-23 23:03:32]

Description was changed from

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
==========

to

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
==========

[Łukasz Anforowicz, 2017-06-23 23:03:42]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 23:04:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-24 00:23:28]

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

[commit-bot: I haz the power, 2017-06-24 00:23:31]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-06-26 16:29:08]

lukasza@chromium.org changed reviewers:
+ nick@chromium.org

[Łukasz Anforowicz, 2017-06-26 16:29:10]

nick@, could you PTAL?

This CL makes domAutomationController.setAutomationId a no-op and paves way for
removing all the callsites in a follow-up CL (cooking @
https://codereview.chromium.org/2503453003).


The main downside of this CL is that it has to bifurcate ExecuteScript test
function into
- ExecuteScript - behaves as before this CL:
    - It appends ";domAutomationController.send(0);" to the end of the script
provided by the user.  and ExecuteUnmodifiedScript
    - It pumps the message loop until the "0" is received back by the browser.
- ExecuteUnmodifiedScript
    - It sends the original script for execution to the renderer and returns
immediately (without waiting for anything / without pumping the message loop).

Ideally, there would only be ExecuteScript (and it would behave as
ExecuteUnmodifiedScript does), but many tests don't wait for the right thing and
accidentally depend on the extra message loop pumping instead - getting rid of
the pumping would make these tests flaky.

ExecuteUnmodifiedScript is a bit of an eyesore - maybe you'd have some [naming?]
suggestions on how to make it more palatable.


I feel that even with ExecuteUnmodifiedScript, this CL moves things in the right
direction, making the test code less magical (after this CL
domAutomationController.send *always* sends stuff back to the browser, without
depending on the "armed/disarmed" state of domAutomationController, which is
global and makes action-at-a-distance (e.g. weird interactions between distant
parts of a test) very much possible.


WDYT?

[Łukasz Anforowicz, 2017-06-26 17:33:30]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 17:33:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 19:02:31]

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

[commit-bot: I haz the power, 2017-06-26 19:02:33]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2017-07-05 20:30:45]

lgtm

https://codereview.chromium.org/2478803003/diff/160001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2478803003/diff/160001/content/public/test/br...
content/public/test/browser_test_utils.cc:423:
window.domAutomationController.send('has-video-input-device');
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');
}

https://codereview.chromium.org/2478803003/diff/160001/content/public/test/br...
content/public/test/browser_test_utils.cc:828: // should wait on a more specific
thing instead).
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-05 22:10:19]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-05 22:10:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-07-05 22:23:49]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-05 22:24:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-05 22:55:09]

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

[commit-bot: I haz the power, 2017-07-05 22:55:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2017-07-06 00:04:16]

Description was changed from

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
==========

to

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2017-07-06 00:04:25]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 00:04:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 00:41:37]

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

[commit-bot: I haz the power, 2017-07-06 00:41:38]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[Łukasz Anforowicz, 2017-07-06 04:09:35]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 04:09:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-07-06 04:13:49]

Description was changed from

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   To make sure that ExecuteScript stops waiting after the call to
   |domAutomationController.send(...)| that is added by ExecuteScript
   (and not by some other, accidental send) this CL sends a GUID that
   is unique to each call to ExecuteScript and verifies that the same
   GUID has been received back.  This change has flushed out a few
   additional cases where ExecuteUnmodifiedScript needs to be used
   as well as cases where a call to domAutomationController.send
   can be removed.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-07-06 05:30:27]

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

[commit-bot: I haz the power, 2017-07-06 05:30:29]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-07-06 15:47:36]

Description was changed from

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL.  To account for this
   a new ExecuteUnmodifiedScript function has been provided in this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   To make sure that ExecuteScript stops waiting after the call to
   |domAutomationController.send(...)| that is added by ExecuteScript
   (and not by some other, accidental send) this CL sends a GUID that
   is unique to each call to ExecuteScript and verifies that the same
   GUID has been received back.  This change has flushed out a few
   additional cases where ExecuteUnmodifiedScript needs to be used
   as well as cases where a call to domAutomationController.send
   can be removed.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon)

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL (e.g. can get confused
   with other calls to domAutomationController.send(...)).  To account
   for this a new ExecuteUnmodifiedScript function has been provided in
   this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   To make sure that ExecuteScript stops waiting after the call to
   |domAutomationController.send(...)| that is added by ExecuteScript
   (and not by some other, accidental send) this CL sends a GUID that
   is unique in each call to ExecuteScript and verifies that the same
   GUID has been received back.  This change has flushed out a few
   additional cases where ExecuteUnmodifiedScript needs to be used
   as well as cases where a call to domAutomationController.send
   can be removed.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon).
     Additionally, it turned out that client_renderer.js and
     media_internals.js (used by WebUIResourceBrowserTest tests) were
     always erroring out before this CL (this has gone unnoticed,
     because the "FAILURE" string would have been eaten by
     ExecuteScript).

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

   - ChromeSitePerProcessTest.PopupWindowFocus - this would conflict
     with domAutomationController.send from
     chrome/test/data/page_with_focus_events.html

   The following callers of ExecuteScript were needlessly calling
   domAutomationController.send from the script:

   - ProxyAuthOnUserBoardScreenTest.ProxyAuthDialogOnUserBoardScreen
   - WizardControllerTest::JSExecute
   - NavigationControllerBrowserTest.PostInSubframe
   - SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

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

https://codereview.chromium.org/2478803003/diff/160001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2478803003/diff/160001/content/public/test/br...
content/public/test/browser_test_utils.cc:423:
window.domAutomationController.send('has-video-input-device');
On 2017/07/05 20:30:44, ncarter (slow) wrote:
> 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');
> }

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

https://codereview.chromium.org/2478803003/diff/160001/content/public/test/br...
content/public/test/browser_test_utils.cc:828: // should wait on a more specific
thing instead).
On 2017/07/05 20:30:44, ncarter (slow) wrote:
> 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.

Done.

> I think it's fine for tests to rely on the ordering guarantees of
ExecuteScript
> doing an IPC round trip,

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.

>  but I'm glad that you're adding an async variant too.

Ok.  Any feedback on the name (ExecuteUnmodifiedScript)?

https://codereview.chromium.org/2478803003/diff/240001/content/browser/resour...
File content/browser/resources/media/client_renderer.js (right):

https://codereview.chromium.org/2478803003/diff/240001/content/browser/resour...
content/browser/resources/media/client_renderer.js:16: this.logTable =
logElement.querySelector('tbody');
This change is pretty interesting.

WebUIResourceBrowserTest ends up running the code above on an empty page (e.g.
on content/test/data/media/webui/player_info_test.html).  Without the change
above, the code above would error out (e.g. with "Uncaught TypeError: Cannot
read property 'querySelector' of null") and would trigger window.onerror event
handler in ui/webui/resources/js/webui_resource_test.js which would call
endTests(false) sending "FAILURE" string via domAutomationController.  The
"FAILURE" string would be eaten by ExecuteScript before the
|DCHECK_EQ(expected_response, actual_response)| assertion added in one of the
recent patchsets to content/public/test/browser_test_utils.cc.

[Łukasz Anforowicz, 2017-07-06 15:51:21]

mathp@, alemate@, benwells@, mmenke@, could you PTAL for an OWNERS review?

mathp:
  chrome/browser/autofill/autofill_interactive_uitest.cc
alemate:
  chrome/browser/chromeos/login/proxy_auth_dialog_browsertest.cc
  chrome/browser/chromeos/login/saml/saml_browsertest.cc
  chrome/browser/chromeos/login/wizard_controller_browsertest.cc
benwells:
  chrome/browser/extensions/browsertest_util.cc
  chrome/browser/geolocation/geolocation_browsertest.cc
mmenke:
  chrome/browser/captive_portal/captive_portal_browsertest.cc

nick:
  chrome/browser/chrome_site_per_process_browsertest.cc
  content/browser/frame_host/navigation_controller_impl_browsertest.cc
  content/browser/resources/media/client_renderer.js
  content/browser/resources/media/media_internals.js
  content/browser/security_exploit_browsertest.cc
  content/browser/site_per_process_browsertest.cc
  content/public/test/browser_test_utils.cc
  content/public/test/browser_test_utils.h
  content/renderer/dom_automation_controller.cc
  content/renderer/dom_automation_controller.h

[Łukasz Anforowicz, 2017-07-06 15:52:11]

lukasza@chromium.org changed reviewers:
+ alemate@chromium.org, benwells@chromium.org, mathp@chromium.org,
mmenke@chromium.org

[Łukasz Anforowicz, 2017-07-06 15:52:12]

mathp@, alemate@, benwells@, mmenke@, could you PTAL for an OWNERS review?

mathp:
  chrome/browser/autofill/autofill_interactive_uitest.cc
alemate:
  chrome/browser/chromeos/login/proxy_auth_dialog_browsertest.cc
  chrome/browser/chromeos/login/saml/saml_browsertest.cc
  chrome/browser/chromeos/login/wizard_controller_browsertest.cc
benwells:
  chrome/browser/extensions/browsertest_util.cc
  chrome/browser/geolocation/geolocation_browsertest.cc
mmenke:
  chrome/browser/captive_portal/captive_portal_browsertest.cc

nick:
  chrome/browser/chrome_site_per_process_browsertest.cc
  content/browser/frame_host/navigation_controller_impl_browsertest.cc
  content/browser/resources/media/client_renderer.js
  content/browser/resources/media/media_internals.js
  content/browser/security_exploit_browsertest.cc
  content/browser/site_per_process_browsertest.cc
  content/public/test/browser_test_utils.cc
  content/public/test/browser_test_utils.h
  content/renderer/dom_automation_controller.cc
  content/renderer/dom_automation_controller.h

[mmenke, 2017-07-06 16:10:34]

https://codereview.chromium.org/2478803003/diff/260001/chrome/browser/captive...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

https://codereview.chromium.org/2478803003/diff/260001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_browsertest.cc:905: void
WaitUntilActiveTabChanges() {
Suggestion:  Wait for a single change, and fail if there are more than one.  If
we cared about waiting for more than one, we'd presumably be interested in what
tab was focused in between the first and last focus, and we couldn't check that
with this class, anyways.  So I think we'd need to beef things up in that case,
so best only to support what this class can usefully support.

https://codereview.chromium.org/2478803003/diff/260001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2478803003/diff/260001/content/public/test/br...
content/public/test/browser_test_utils.h:212: // the below functions. Returns
true on success.
While you're here, mind documenting what "success" is?  From the comment in
ExecuteUnmodifiedScript, I assume it at least waits for the script to finish
running, but I'm still not sure what success is.  If the script throws an
exception, is that success?

https://codereview.chromium.org/2478803003/diff/260001/content/public/test/br...
content/public/test/browser_test_utils.h:223: // immediately (unlike
ExecuteScript above).
I guess ExecuteScript waits for completion?  That should be documented with
ExecuteScript.

https://codereview.chromium.org/2478803003/diff/260001/content/public/test/br...
content/public/test/browser_test_utils.h:227: void ExecuteUnmodifiedScript(const
ToRenderFrameHost& adapter,
ExecuteUnmodifiedScript?  Does ExecuteScript modify the script?  If so, perhaps
that should be documented, as it's not clear what makes this script not
modified.

[Łukasz Anforowicz, 2017-07-06 16:46:56]

mmenke@ - thanks for the comments.  Can you take another look please?

https://codereview.chromium.org/2478803003/diff/260001/chrome/browser/captive...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

https://codereview.chromium.org/2478803003/diff/260001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_browsertest.cc:905: void
WaitUntilActiveTabChanges() {
On 2017/07/06 16:10:34, mmenke wrote:
> Suggestion:  Wait for a single change, and fail if there are more than one. 
If
> we cared about waiting for more than one, we'd presumably be interested in
what
> tab was focused in between the first and last focus, and we couldn't check
that
> with this class, anyways.  So I think we'd need to beef things up in that
case,
> so best only to support what this class can usefully support.

Good point.  Done.

https://codereview.chromium.org/2478803003/diff/260001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2478803003/diff/260001/content/public/test/br...
content/public/test/browser_test_utils.h:212: // the below functions. Returns
true on success.
On 2017/07/06 16:10:34, mmenke wrote:
> While you're here, mind documenting what "success" is?  From the comment in
> ExecuteUnmodifiedScript, I assume it at least waits for the script to finish
> running, but I'm still not sure what success is.  If the script throws an
> exception, is that success?

Done.

https://codereview.chromium.org/2478803003/diff/260001/content/public/test/br...
content/public/test/browser_test_utils.h:223: // immediately (unlike
ExecuteScript above).
On 2017/07/06 16:10:34, mmenke wrote:
> I guess ExecuteScript waits for completion?  That should be documented with
> ExecuteScript.

Done.

https://codereview.chromium.org/2478803003/diff/260001/content/public/test/br...
content/public/test/browser_test_utils.h:227: void ExecuteUnmodifiedScript(const
ToRenderFrameHost& adapter,
On 2017/07/06 16:10:34, mmenke wrote:
> ExecuteUnmodifiedScript?  Does ExecuteScript modify the script?  If so,
perhaps
> that should be documented, as it's not clear what makes this script not
> modified.

Done.

[Łukasz Anforowicz, 2017-07-06 16:47:10]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 16:47:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-07-06 17:10:38]

captive_portal LGTM!

[Mathieu, 2017-07-06 17:25:16]

autofill lgtm, could this have previously made things flaky? we've been having
difficulties with the tests in this file.

[Łukasz Anforowicz, 2017-07-06 17:34:23]

On 2017/07/06 17:25:16, Mathieu wrote:
> autofill lgtm, could this have previously made things flaky? we've been having
> difficulties with the tests in this file.

Possibly.  I am pretty sure that before this CL there was a race between 2
domAutomationController.send calls / IPCs.  OTOH, I don't know if this race
could be contributing to flakiness (i.e. in some cases I've seen despite the
race things worked fine, because the exact values sent back didn't matter).

[commit-bot: I haz the power, 2017-07-06 18:12:42]

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

[commit-bot: I haz the power, 2017-07-06 18:12:43]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2017-07-06 18:14:31]

https://codereview.chromium.org/2478803003/diff/280001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2478803003/diff/280001/content/public/test/br...
content/public/test/browser_test_utils.cc:843: std::string new_script = script +
";window.domAutomationController.send(0);";
Should this match what ExecuteScript does with the GUID?

https://codereview.chromium.org/2478803003/diff/280001/content/public/test/br...
content/public/test/browser_test_utils.cc:849: const std::string& script) {
You asked for feedback on the name.

I would be tempted to call this ExecuteScriptWithoutWaiting(),
ExecuteScriptAsync(), or EnqueueScriptExecution(), or something else to
emphasize the async vs. sync nature, rather than emphasizing the fact that the
script isn't modified in the process.

[Łukasz Anforowicz, 2017-07-06 18:29:28]

Description was changed from

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL (e.g. can get confused
   with other calls to domAutomationController.send(...)).  To account
   for this a new ExecuteUnmodifiedScript function has been provided in
   this CL.

   Ideally ExecuteScript would become ExecuteUnmodifiedScript, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteUnmodifiedScript.  Therefore, ExecuteUnmodifiedScript is
   a lesser evil, that minimizes amount of changes needed right now.

   To make sure that ExecuteScript stops waiting after the call to
   |domAutomationController.send(...)| that is added by ExecuteScript
   (and not by some other, accidental send) this CL sends a GUID that
   is unique in each call to ExecuteScript and verifies that the same
   GUID has been received back.  This change has flushed out a few
   additional cases where ExecuteUnmodifiedScript needs to be used
   as well as cases where a call to domAutomationController.send
   can be removed.

   The following callers of ExecuteScript had to be switched to
   call ExecuteUnmodifiedScript instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon).
     Additionally, it turned out that client_renderer.js and
     media_internals.js (used by WebUIResourceBrowserTest tests) were
     always erroring out before this CL (this has gone unnoticed,
     because the "FAILURE" string would have been eaten by
     ExecuteScript).

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteUnmodifiedScript.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

   - ChromeSitePerProcessTest.PopupWindowFocus - this would conflict
     with domAutomationController.send from
     chrome/test/data/page_with_focus_events.html

   The following callers of ExecuteScript were needlessly calling
   domAutomationController.send from the script:

   - ProxyAuthOnUserBoardScreenTest.ProxyAuthDialogOnUserBoardScreen
   - WizardControllerTest::JSExecute
   - NavigationControllerBrowserTest.PostInSubframe
   - SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL (e.g. can get confused
   with other calls to domAutomationController.send(...)).  To account
   for this a new ExecuteScriptAsync function has been provided in
   this CL.

   Ideally ExecuteScript would become ExecuteScriptAsync, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteScriptAsync.  Therefore, ExecuteScriptAsync is
   a lesser evil, that minimizes amount of changes needed right now.

   To make sure that ExecuteScript stops waiting after the call to
   |domAutomationController.send(...)| that is added by ExecuteScript
   (and not by some other, accidental send) this CL sends a GUID that
   is unique in each call to ExecuteScript and verifies that the same
   GUID has been received back.  This change has flushed out a few
   additional cases where ExecuteScriptAsync needs to be used
   as well as cases where a call to domAutomationController.send
   can be removed.

   The following callers of ExecuteScript had to be switched to
   call ExecuteScriptAsync instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon).
     Additionally, it turned out that client_renderer.js and
     media_internals.js (used by WebUIResourceBrowserTest tests) were
     always erroring out before this CL (this has gone unnoticed,
     because the "FAILURE" string would have been eaten by
     ExecuteScript).

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteScriptAsync.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

   - ChromeSitePerProcessTest.PopupWindowFocus - this would conflict
     with domAutomationController.send from
     chrome/test/data/page_with_focus_events.html

   The following callers of ExecuteScript were needlessly calling
   domAutomationController.send from the script:

   - ProxyAuthOnUserBoardScreenTest.ProxyAuthDialogOnUserBoardScreen
   - WizardControllerTest::JSExecute
   - NavigationControllerBrowserTest.PostInSubframe
   - SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2017-07-06 18:31:22]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 18:31:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 19:56:08]

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

[commit-bot: I haz the power, 2017-07-06 19:56:09]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-07-06 20:11:02]

Thanks Nick.

https://codereview.chromium.org/2478803003/diff/280001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2478803003/diff/280001/content/public/test/br...
content/public/test/browser_test_utils.cc:843: std::string new_script = script +
";window.domAutomationController.send(0);";
On 2017/07/06 18:14:31, ncarter (slow) wrote:
> Should this match what ExecuteScript does with the GUID?

Doh... :-/  You're right - these checks should apply to both.  I've moved the
common code to a helper function in the anonymous namespace.

https://codereview.chromium.org/2478803003/diff/280001/content/public/test/br...
content/public/test/browser_test_utils.cc:849: const std::string& script) {
On 2017/07/06 18:14:30, ncarter (slow) wrote:
> You asked for feedback on the name.
> 
> I would be tempted to call this ExecuteScriptWithoutWaiting(),
> ExecuteScriptAsync(), or EnqueueScriptExecution(), or something else to
> emphasize the async vs. sync nature, rather than emphasizing the fact that the
> script isn't modified in the process.

Thanks for the feedback.  I've picked ExecuteScriptAsync for the name.

[ncarter (slow), 2017-07-06 21:40:11]

lgtm

thanks again for taking on this longstanding wart

[Łukasz Anforowicz, 2017-07-07 22:22:26]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-07 22:22:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-07 23:36:18]

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

[commit-bot: I haz the power, 2017-07-07 23:36:19]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-07-10 05:30:50]

lgtm with a little nit

https://codereview.chromium.org/2478803003/diff/300001/chrome/browser/geoloca...
File chrome/browser/geolocation/geolocation_browsertest.cc (right):

https://codereview.chromium.org/2478803003/diff/300001/chrome/browser/geoloca...
chrome/browser/geolocation/geolocation_browsertest.cc:416:
content::DOMMessageQueue dom_message_queue;
Nit: can you move this declaration down to just before it is used?

[Łukasz Anforowicz, 2017-07-10 14:52:34]

Description was changed from

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL (e.g. can get confused
   with other calls to domAutomationController.send(...)).  To account
   for this a new ExecuteScriptAsync function has been provided in
   this CL.

   Ideally ExecuteScript would become ExecuteScriptAsync, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteScriptAsync.  Therefore, ExecuteScriptAsync is
   a lesser evil, that minimizes amount of changes needed right now.

   To make sure that ExecuteScript stops waiting after the call to
   |domAutomationController.send(...)| that is added by ExecuteScript
   (and not by some other, accidental send) this CL sends a GUID that
   is unique in each call to ExecuteScript and verifies that the same
   GUID has been received back.  This change has flushed out a few
   additional cases where ExecuteScriptAsync needs to be used
   as well as cases where a call to domAutomationController.send
   can be removed.

   The following callers of ExecuteScript had to be switched to
   call ExecuteScriptAsync instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon).
     Additionally, it turned out that client_renderer.js and
     media_internals.js (used by WebUIResourceBrowserTest tests) were
     always erroring out before this CL (this has gone unnoticed,
     because the "FAILURE" string would have been eaten by
     ExecuteScript).

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteScriptAsync.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

   - ChromeSitePerProcessTest.PopupWindowFocus - this would conflict
     with domAutomationController.send from
     chrome/test/data/page_with_focus_events.html

   The following callers of ExecuteScript were needlessly calling
   domAutomationController.send from the script:

   - ProxyAuthOnUserBoardScreenTest.ProxyAuthDialogOnUserBoardScreen
   - WizardControllerTest::JSExecute
   - NavigationControllerBrowserTest.PostInSubframe
   - SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from a callback
   and once from a checkIfGeopositionUpdated function.

   This is fixed by only listening for messages coming from the callback
   and avoiding calling the (no longer needed) checkIfGeopositionUpdated
   function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL (e.g. can get confused
   with other calls to domAutomationController.send(...)).  To account
   for this a new ExecuteScriptAsync function has been provided in
   this CL.

   Ideally ExecuteScript would become ExecuteScriptAsync, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteScriptAsync.  Therefore, ExecuteScriptAsync is
   a lesser evil, that minimizes amount of changes needed right now.

   To make sure that ExecuteScript stops waiting after the call to
   |domAutomationController.send(...)| that is added by ExecuteScript
   (and not by some other, accidental send) this CL sends a GUID that
   is unique in each call to ExecuteScript and verifies that the same
   GUID has been received back.  This change has flushed out a few
   additional cases where ExecuteScriptAsync needs to be used
   as well as cases where a call to domAutomationController.send
   can be removed.

   The following callers of ExecuteScript had to be switched to
   call ExecuteScriptAsync instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon).
     Additionally, it turned out that client_renderer.js and
     media_internals.js (used by WebUIResourceBrowserTest tests) were
     always erroring out before this CL (this has gone unnoticed,
     because the "FAILURE" string would have been eaten by
     ExecuteScript).

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteScriptAsync.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

   - ChromeSitePerProcessTest.PopupWindowFocus - this would conflict
     with domAutomationController.send from
     chrome/test/data/page_with_focus_events.html

   The following callers of ExecuteScript were needlessly calling
   domAutomationController.send from the script:

   - ProxyAuthOnUserBoardScreenTest.ProxyAuthDialogOnUserBoardScreen
   - WizardControllerTest::JSExecute
   - NavigationControllerBrowserTest.PostInSubframe
   - SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from the
   geoSuccessCallbackWithResponse callback
   and once from a checkIfGeopositionUpdated function.

   There is no need to resolve the race via checkIfGeopositionUpdated if
   we start listening (via DOMMessageQueue) *before* trigerring an
   update in the geo position.  This way we can keep just the callback
   and remove the no longer needed checkIfGeopositionUpdated function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2017-07-10 14:53:50]

Description was changed from

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL (e.g. can get confused
   with other calls to domAutomationController.send(...)).  To account
   for this a new ExecuteScriptAsync function has been provided in
   this CL.

   Ideally ExecuteScript would become ExecuteScriptAsync, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteScriptAsync.  Therefore, ExecuteScriptAsync is
   a lesser evil, that minimizes amount of changes needed right now.

   To make sure that ExecuteScript stops waiting after the call to
   |domAutomationController.send(...)| that is added by ExecuteScript
   (and not by some other, accidental send) this CL sends a GUID that
   is unique in each call to ExecuteScript and verifies that the same
   GUID has been received back.  This change has flushed out a few
   additional cases where ExecuteScriptAsync needs to be used
   as well as cases where a call to domAutomationController.send
   can be removed.

   The following callers of ExecuteScript had to be switched to
   call ExecuteScriptAsync instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon).
     Additionally, it turned out that client_renderer.js and
     media_internals.js (used by WebUIResourceBrowserTest tests) were
     always erroring out before this CL (this has gone unnoticed,
     because the "FAILURE" string would have been eaten by
     ExecuteScript).

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteScriptAsync.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

   - ChromeSitePerProcessTest.PopupWindowFocus - this would conflict
     with domAutomationController.send from
     chrome/test/data/page_with_focus_events.html

   The following callers of ExecuteScript were needlessly calling
   domAutomationController.send from the script:

   - ProxyAuthOnUserBoardScreenTest.ProxyAuthDialogOnUserBoardScreen
   - WizardControllerTest::JSExecute
   - NavigationControllerBrowserTest.PostInSubframe
   - SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from the
   geoSuccessCallbackWithResponse callback
   and once from a checkIfGeopositionUpdated function.

   There is no need to resolve the race via checkIfGeopositionUpdated if
   we start listening (via DOMMessageQueue) *before* trigerring an
   update in the geo position.  This way we can keep just the callback
   and remove the no longer needed checkIfGeopositionUpdated function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL (e.g. can get confused
   with other calls to domAutomationController.send(...)).  To account
   for this a new ExecuteScriptAsync function has been provided in
   this CL.

   Ideally ExecuteScript would become ExecuteScriptAsync, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteScriptAsync.  Therefore, ExecuteScriptAsync is
   a lesser evil, that minimizes amount of changes needed right now.

   To make sure that ExecuteScript stops waiting after the call to
   |domAutomationController.send(...)| that is added by ExecuteScript
   (and not by some other, accidental send) this CL sends a GUID that
   is unique in each call to ExecuteScript and verifies that the same
   GUID has been received back.  This change has flushed out a few
   additional cases where ExecuteScriptAsync needs to be used
   as well as cases where a call to domAutomationController.send
   can be removed.

   The following callers of ExecuteScript had to be switched to
   call ExecuteScriptAsync instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon).
     Additionally, it turned out that client_renderer.js and
     media_internals.js (used by WebUIResourceBrowserTest tests) were
     always erroring out before this CL (this has gone unnoticed,
     because the "FAILURE" string would have been eaten by
     ExecuteScript).

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteScriptAsync.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

   - ChromeSitePerProcessTest.PopupWindowFocus - this would conflict
     with domAutomationController.send from
     chrome/test/data/page_with_focus_events.html

   The following callers of ExecuteScript were needlessly calling
   domAutomationController.send from the script:

   - ProxyAuthOnUserBoardScreenTest.ProxyAuthDialogOnUserBoardScreen
   - WizardControllerTest::JSExecute
   - NavigationControllerBrowserTest.PostInSubframe
   - SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from the
   geoSuccessCallbackWithResponse callback (from
   chrome/test/data/geolocation/basic_geolocation.js)
   and once from a checkIfGeopositionUpdated function.

   There is no need to resolve the race via checkIfGeopositionUpdated if
   we start listening (via DOMMessageQueue) *before* trigerring an
   update in the geo position.  This way we can keep just the callback
   and remove the no longer needed checkIfGeopositionUpdated function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2017-07-10 14:54:18]

Thanks Ben, and my apologies for not providing enough detail in the CL
description.  I've tried to tweak the CL description to better explain the
changes you were concerned about.

https://codereview.chromium.org/2478803003/diff/300001/chrome/browser/geoloca...
File chrome/browser/geolocation/geolocation_browsertest.cc (right):

https://codereview.chromium.org/2478803003/diff/300001/chrome/browser/geoloca...
chrome/browser/geolocation/geolocation_browsertest.cc:416:
content::DOMMessageQueue dom_message_queue;
On 2017/07/10 05:30:50, benwells wrote:
> Nit: can you move this declaration down to just before it is used?

I've tried to explain this in the CL description (in item #3) - the
geoSuccessCallbackWithResponse function in
chrome/test/data/geolocation/basic_geolocation.js will send
'geoposition-updated' string via domAutomationController in response to the
ui_test_utils::OverrideGeolocation call below.  To have the test listen for this
value, we need to start listening (and therefore declare
content::DOMMessageQueue variable) *before* calling
ui_test_utils::OverrideGeolocation.

I've tried to add a bit more details to the CL description.

[Alexander Alekseev, 2017-07-10 22:32:33]

lgtm

[Łukasz Anforowicz, 2017-07-10 22:52:44]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2017-07-10 22:52:45]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
mmenke@chromium.org, alemate@chromium.org, nick@chromium.org,
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2478803003/#ps320001
(title: "Rebasing...")

[commit-bot: I haz the power, 2017-07-10 22:52:57]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 00:19:34]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1499727164684760,
"parent_rev": "2b0d6d14ecc0b3d2302901efcb457868522790db", "commit_rev":
"fff83c83f53761407fe28df29c8eff5cdcbace9d"}

[commit-bot: I haz the power, 2017-07-11 00:19:57]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1499727164684760,
"parent_rev": "fc015703812e1675523d49ceca98e7ac9bf55748", "commit_rev":
"c7d6bd3d4561ae85c1e2baf25136e930aa33b055"}

[commit-bot: I haz the power, 2017-07-11 00:20:11]

Description was changed from

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL (e.g. can get confused
   with other calls to domAutomationController.send(...)).  To account
   for this a new ExecuteScriptAsync function has been provided in
   this CL.

   Ideally ExecuteScript would become ExecuteScriptAsync, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteScriptAsync.  Therefore, ExecuteScriptAsync is
   a lesser evil, that minimizes amount of changes needed right now.

   To make sure that ExecuteScript stops waiting after the call to
   |domAutomationController.send(...)| that is added by ExecuteScript
   (and not by some other, accidental send) this CL sends a GUID that
   is unique in each call to ExecuteScript and verifies that the same
   GUID has been received back.  This change has flushed out a few
   additional cases where ExecuteScriptAsync needs to be used
   as well as cases where a call to domAutomationController.send
   can be removed.

   The following callers of ExecuteScript had to be switched to
   call ExecuteScriptAsync instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon).
     Additionally, it turned out that client_renderer.js and
     media_internals.js (used by WebUIResourceBrowserTest tests) were
     always erroring out before this CL (this has gone unnoticed,
     because the "FAILURE" string would have been eaten by
     ExecuteScript).

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteScriptAsync.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

   - ChromeSitePerProcessTest.PopupWindowFocus - this would conflict
     with domAutomationController.send from
     chrome/test/data/page_with_focus_events.html

   The following callers of ExecuteScript were needlessly calling
   domAutomationController.send from the script:

   - ProxyAuthOnUserBoardScreenTest.ProxyAuthDialogOnUserBoardScreen
   - WizardControllerTest::JSExecute
   - NavigationControllerBrowserTest.PostInSubframe
   - SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from the
   geoSuccessCallbackWithResponse callback (from
   chrome/test/data/geolocation/basic_geolocation.js)
   and once from a checkIfGeopositionUpdated function.

   There is no need to resolve the race via checkIfGeopositionUpdated if
   we start listening (via DOMMessageQueue) *before* trigerring an
   update in the geo position.  This way we can keep just the callback
   and remove the no longer needed checkIfGeopositionUpdated function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove DOMAutomationController::automation_id_

Main change in this CL
======================

For a long time now the DOMAutomationController::automation_id_ value has
not been used for its original purpose (associating sends with
particular "jobs" / "automations" / render_views).  Right now, the
argument of domAutomationController::send is always routed back to
WebContents (not to any more granular/specific destination).

Right now the DOMAutomationController::automation_id_ field is still
used to enable/disable domAutomationController::send - the send only
works if setAutomationId was called previously (with a value different
than MSG_ROUTING_NONE).  In fact - in almost all cases, the scripts
say:
  domAutomationController.setAutomationId(0)
  domAutomationController.send(...)

This CL removes DOMAutomationController::automation_id_ field and
makes domAutomationController.send work unconditionally.

After this CL lands, we will be able to remove all calls to
setAutomationId (after this CL they will be no-ops).


Other changes needed by this CL
===============================

The CL needed to make the following, additional fixes:

1. ExecuteScript (in content/public/test/browser_test_utils.h / .cc)
   appends "; domAutomationController.send(0);" to the script provided
   by the caller.  This extra value would be usually ignored before
   this CL, but gets in the way after this CL (e.g. can get confused
   with other calls to domAutomationController.send(...)).  To account
   for this a new ExecuteScriptAsync function has been provided in
   this CL.

   Ideally ExecuteScript would become ExecuteScriptAsync, but
   there are multiple tests that depend on the extra pumping of the
   message loop that happens in ExecuteScript and not in
   ExecuteScriptAsync.  Therefore, ExecuteScriptAsync is
   a lesser evil, that minimizes amount of changes needed right now.

   To make sure that ExecuteScript stops waiting after the call to
   |domAutomationController.send(...)| that is added by ExecuteScript
   (and not by some other, accidental send) this CL sends a GUID that
   is unique in each call to ExecuteScript and verifies that the same
   GUID has been received back.  This change has flushed out a few
   additional cases where ExecuteScriptAsync needs to be used
   as well as cases where a call to domAutomationController.send
   can be removed.

   The following callers of ExecuteScript had to be switched to
   call ExecuteScriptAsync instead:

   - ExecuteWebUIResourceTest (consumes messages via DOMMessageQueue
     and relies on the fact that this queue is not stumped upon).
     Additionally, it turned out that client_renderer.js and
     media_internals.js (used by WebUIResourceBrowserTest tests) were
     always erroring out before this CL (this has gone unnoticed,
     because the "FAILURE" string would have been eaten by
     ExecuteScript).

   - SecurityExploitBrowserTest.InterstitialCommandFromUnderlyingContent
     (otherwise the artificual "0" ends up in interstitial->last_command()).

   - captive_portal_browsertest.cc (otherwise we trigger a CHECK in
     captive_portal_blocking_page.cc: Command 0 isn't handled by the
     captive portal interstitial).

     Note that CaptivePortalBrowserTest /
     ShowCaptivePortalInterstitialOnCertError was relying on message
     pumping done by ExecuteScript.  This was fragile - the timing of
     exiting the message pump was not related to the timing of the event
     the test needs to wait for.  This was broken after going through
     ExecuteScriptAsync.  This is fixed by introducing an explicit
     TabActivationWaiter.

   - ExecuteScriptInBackgroundPageNoWait (otherwise DOMMessageQueue
     used by ProcessManagerBrowserTest.ExtensionProcessReuse would
     have unexpectedly gotten a value sent by ExecuteScript, rather
     than the value send by the script itself;  additionally, calling
     Execute*Unmodified*Script here makes sense in a function named
     "NoWait" - ExecuteScript would have waited).

   - saml_browsertest.cc - there was undesired interaction with
     listening to the value sent by SetupAuthFlowChangeListener.

   - ChromeSitePerProcessTest.PopupWindowFocus - this would conflict
     with domAutomationController.send from
     chrome/test/data/page_with_focus_events.html

   The following callers of ExecuteScript were needlessly calling
   domAutomationController.send from the script:

   - ProxyAuthOnUserBoardScreenTest.ProxyAuthDialogOnUserBoardScreen
   - WizardControllerTest::JSExecute
   - NavigationControllerBrowserTest.PostInSubframe
   - SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs

2. autofill_interactive_uitest.cc used to call
   domAutomationController.send(true) from onfocus DOM event handlers.
   This would confuse expectations of ExecuteScriptAndExtractInt running
   "domAutomationController.send(42)" (it would sometimes receive a
   boolean rather than int back).  This is fixed by using a one-time
   onfocus handlers (rather than permanent handlers) from
   FocusFieldByName method.

3. After the changes some GeolocationBrowserTest would receive
   "geoposition-updated" message twice - once from the
   geoSuccessCallbackWithResponse callback (from
   chrome/test/data/geolocation/basic_geolocation.js)
   and once from a checkIfGeopositionUpdated function.

   There is no need to resolve the race via checkIfGeopositionUpdated if
   we start listening (via DOMMessageQueue) *before* trigerring an
   update in the geo position.  This way we can keep just the callback
   and remove the no longer needed checkIfGeopositionUpdated function.

4. content::IsWebcamAvailableOnSystem (via kHasVideoInputDeviceOnSystem)
   would call domAutomationController.send(...) twice before this CL.
   After this CL it is only called once.


Other, opportunistic changes
============================

I've noticed that DOMMessageQueue::message_loop_runner_ can be null
and therefore uses of this field need to be guarded.  This required
remembering result of DOMMessageQueue::RenderProcessGone, so that
it still has an effect when there was no runner when the renderer
crash notification happened.


BUG=662543
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2478803003
Cr-Commit-Position: refs/heads/master@{#485461}
Committed:
https://chromium.googlesource.com/chromium/src/+/c7d6bd3d4561ae85c1e2baf25136...
==========

[commit-bot: I haz the power, 2017-07-11 00:20:13]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/c7d6bd3d4561ae85c1e2baf25136...