Explicitly initialize secondary renderers for layout tests.

Explicitly initialize secondary renderers for layout tests.

Secondary renderers used to be implicitly initized for layout tests in
BlinkTestRunner::Navigate method, relying on comparisons with
LayoutTestRenderProcessObserver::GetInstance()->main_test_runner().
Such initialization is brittle:
1. it relies on correct singling out of a specific BlinkTestRunner and
   designating it as a main_test_runner (right now this is the instance
   that happens to be created first in the given renderer process)
2. it reinitializes the test after each navigation, which may clobber
   previous test state changes which is especially worrisome if another
   BlinkTestRunner is present in the same process (with OOPIFs a
   BlinkTestRunner for secondary and main test window can coexist in the
   same renderer process).

This CL starts to explicitly initialize all secondary renderers via
ShellViewMsg_SetupSecondaryRenderer message, tracking them on the
browser-side in BrowserTestController::HandleNewRenderFrameHost.
This helps avoid the brittleness described above.  Additionally
explicit initialization helps to guarantee that a secondary renderer
is initialized before propagating to such renderer changes of
LayoutTestRuntimeFlags (planned in https://crrev.com/1878863002).

Cleaning up BlinkTestRunner::Navigate method (and fixing up the target
BlinkTestRunner used by LayoytTestRenderFrameObserver) enables to remove
one occurence of a global field holding (an essentially non-global)
BlinkTestRunner - we can now remove
LayoutTestRenderProcessObserver::main_test_runner_ field (and then
also remove LayoutTestRenderProcessObserver::SetMainWindow which
can be simply inlined into LayoutTestContentRendererClient).

BUG=595089
Committed: https://crrev.com/8e550a0c01885ed449fd7ea4540f2d6f18762d16
Cr-Commit-Position: refs/heads/master@{#387963}

[Łukasz Anforowicz, 2016-04-15 20:15:28]

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

[Łukasz Anforowicz, 2016-04-15 20:15:29]

jochen@, could you please take a look?

https://codereview.chromium.org/1890223002/diff/20001/content/shell/browser/l...
File content/shell/browser/layout_test/blink_test_controller.h (right):

https://codereview.chromium.org/1890223002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/blink_test_controller.h:273:
std::set<RenderProcessHost*> all_observed_render_process_hosts_;
This field is somewhat redundant - render_process_host_observer_ already
contains the same set of RenderProcessHosts, but it is locked inside
ScopedObserver.  I guess I could try to add an accessor to ScopedObserver, but
that would mean pushback from //base owners unless I find 2-3 other cases that
would need an accessor...  Path of least resistance...

https://codereview.chromium.org/1890223002/diff/20001/content/shell/renderer/...
File content/shell/renderer/layout_test/layout_test_content_renderer_client.cc
(right):

https://codereview.chromium.org/1890223002/diff/20001/content/shell/renderer/...
content/shell/renderer/layout_test/layout_test_content_renderer_client.cc:116:
// when it gets notified by the browser that it is the main window?
This "todo" should be a long-term goal, but doing it today would introduce
(accidental) test failures, because this changes PrintMessage's behavior by
routing it through a different TestRunner::delegate_ (possibly swapped-out
and/or possible not listened to on the browser side).  Changing PrintMessage to
go through process/control message is tricky as discussed some time ago over
email.

https://codereview.chromium.org/1890223002/diff/20001/content/shell/renderer/...
content/shell/renderer/layout_test/layout_test_content_renderer_client.cc:121:
->SetWebView(render_view->GetWebView(), proxy);
This just inlines what used to be in
LayoutTestRenderProcessObserver::SetMainWindow

[jochen (gone - plz use gerrit), 2016-04-17 17:09:08]

lgtm

[Łukasz Anforowicz, 2016-04-18 14:40:46]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-04-18 14:40:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1890223002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1890223002/20001

[commit-bot: I haz the power, 2016-04-18 14:43:20]

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

[commit-bot: I haz the power, 2016-04-18 14:43:22]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2016-04-18 17:20:33]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-04-18 17:20:33]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1890223002/#ps40001
(title: "Rebasing...")

[commit-bot: I haz the power, 2016-04-18 17:21:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1890223002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1890223002/40001

[commit-bot: I haz the power, 2016-04-18 18:34:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-18 18:37:04]

Description was changed from

==========
Explicitly initialize secondary renderers for layout tests.

Secondary renderers used to be implicitly initized for layout tests in
BlinkTestRunner::Navigate method, relying on comparisons with
LayoutTestRenderProcessObserver::GetInstance()->main_test_runner().
Such initialization is brittle:
1. it relies on correct singling out of a specific BlinkTestRunner and
   designating it as a main_test_runner (right now this is the instance
   that happens to be created first in the given renderer process)
2. it reinitializes the test after each navigation, which may clobber
   previous test state changes which is especially worrisome if another
   BlinkTestRunner is present in the same process (with OOPIFs a
   BlinkTestRunner for secondary and main test window can coexist in the
   same renderer process).

This CL starts to explicitly initialize all secondary renderers via
ShellViewMsg_SetupSecondaryRenderer message, tracking them on the
browser-side in BrowserTestController::HandleNewRenderFrameHost.
This helps avoid the brittleness described above.  Additionally
explicit initialization helps to guarantee that a secondary renderer
is initialized before propagating to such renderer changes of
LayoutTestRuntimeFlags (planned in https://crrev.com/1878863002).

Cleaning up BlinkTestRunner::Navigate method (and fixing up the target
BlinkTestRunner used by LayoytTestRenderFrameObserver) enables to remove
one occurence of a global field holding (an essentially non-global)
BlinkTestRunner - we can now remove
LayoutTestRenderProcessObserver::main_test_runner_ field (and then
also remove LayoutTestRenderProcessObserver::SetMainWindow which
can be simply inlined into LayoutTestContentRendererClient).

BUG=595089
==========

to

==========
Explicitly initialize secondary renderers for layout tests.

Secondary renderers used to be implicitly initized for layout tests in
BlinkTestRunner::Navigate method, relying on comparisons with
LayoutTestRenderProcessObserver::GetInstance()->main_test_runner().
Such initialization is brittle:
1. it relies on correct singling out of a specific BlinkTestRunner and
   designating it as a main_test_runner (right now this is the instance
   that happens to be created first in the given renderer process)
2. it reinitializes the test after each navigation, which may clobber
   previous test state changes which is especially worrisome if another
   BlinkTestRunner is present in the same process (with OOPIFs a
   BlinkTestRunner for secondary and main test window can coexist in the
   same renderer process).

This CL starts to explicitly initialize all secondary renderers via
ShellViewMsg_SetupSecondaryRenderer message, tracking them on the
browser-side in BrowserTestController::HandleNewRenderFrameHost.
This helps avoid the brittleness described above.  Additionally
explicit initialization helps to guarantee that a secondary renderer
is initialized before propagating to such renderer changes of
LayoutTestRuntimeFlags (planned in https://crrev.com/1878863002).

Cleaning up BlinkTestRunner::Navigate method (and fixing up the target
BlinkTestRunner used by LayoytTestRenderFrameObserver) enables to remove
one occurence of a global field holding (an essentially non-global)
BlinkTestRunner - we can now remove
LayoutTestRenderProcessObserver::main_test_runner_ field (and then
also remove LayoutTestRenderProcessObserver::SetMainWindow which
can be simply inlined into LayoutTestContentRendererClient).

BUG=595089

Committed: https://crrev.com/8e550a0c01885ed449fd7ea4540f2d6f18762d16
Cr-Commit-Position: refs/heads/master@{#387963}
==========

[commit-bot: I haz the power, 2016-04-18 18:37:05]

Patchset 3 (id:??) landed as
https://crrev.com/8e550a0c01885ed449fd7ea4540f2d6f18762d16
Cr-Commit-Position: refs/heads/master@{#387963}