Replicating LayoutDumpFlags across OOPIFs.

content/shell/browser/layout_test/blink_test_controller.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/shell/browser/layout_test/blink_test_controller.h"
 
 #include <stddef.h>
 
 #include <iostream>
+#include <set>
 #include <utility>
 
 #include "base/base64.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 242 matching lines @@
   DCHECK(CalledOnValidThread());
   test_phase_ = DURING_TEST;
   current_working_directory_ = current_working_directory;
   enable_pixel_dumping_ = enable_pixel_dumping;
   expected_pixel_hash_ = expected_pixel_hash;
   test_url_ = test_url;
   need_to_send_test_configuration_to_renderer_ = true;
   printer_->reset();
   frame_to_layout_dump_map_.clear();
   render_process_host_observer_.RemoveAll();
+  accumulated_layout_dump_flags_changes_.Clear();
   ShellBrowserContext* browser_context =
       ShellContentBrowserClient::Get()->browser_context();
   if (test_url.spec().find("compositing/") != std::string::npos)
     is_compositing_test_ = true;
   initial_size_ = Shell::GetShellDefaultSize();
   // The W3C SVG layout tests use a different size than the other layout tests.
   if (test_url.spec().find("W3C-SVG-1.1") != std::string::npos)
     initial_size_ = gfx::Size(kTestSVGWindowWidthDip, kTestSVGWindowHeightDip);
   if (!main_window_) {
     main_window_ = content::Shell::CreateNewWindow(
@@ skipping 151 matching lines @@
 
   return handled;
 }
 
 bool BlinkTestController::OnMessageReceived(
     const IPC::Message& message,
     RenderFrameHost* render_frame_host) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP_WITH_PARAM(BlinkTestController, message,
                                    render_frame_host)
+    IPC_MESSAGE_HANDLER(ShellViewHostMsg_LayoutDumpFlagsChanged,
+                        OnLayoutDumpFlagsChanged)
     IPC_MESSAGE_HANDLER(ShellViewHostMsg_LayoutDumpResponse,
                         OnLayoutDumpResponse)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 void BlinkTestController::PluginCrashed(const base::FilePath& plugin_path,
                                         base::ProcessId plugin_pid) {
   DCHECK(CalledOnValidThread());
@@ skipping 141 matching lines @@
           switches::kAllowExternalPages);
   params.expected_pixel_hash = expected_pixel_hash_;
   params.initial_size = initial_size_;
 
   if (need_to_send_test_configuration_to_renderer_) {
     need_to_send_test_configuration_to_renderer_ = false;
     frame->Send(
         new ShellViewMsg_SetTestConfiguration(frame->GetRoutingID(), params));
   } else {
     frame->Send(new ShellViewMsg_ReplicateTestConfiguration(
-        frame->GetRoutingID(), params));
+        frame->GetRoutingID(), params, accumulated_layout_dump_flags_changes_));
   }
 }
 
 void BlinkTestController::OnTestFinished() {
   test_phase_ = CLEAN_UP;
   if (!printer_->output_finished())
     printer_->PrintImageFooter();
   RenderViewHost* render_view_host =
       main_window_->web_contents()->GetRenderViewHost();
   main_window_->web_contents()->ExitFullscreen(/*will_cause_resize=*/false);
@@ skipping 45 matching lines @@
   printer_->PrintAudioBlock(dump);
   printer_->PrintAudioFooter();
 }
 
 void BlinkTestController::OnTextDump(const std::string& dump) {
   printer_->PrintTextHeader();
   printer_->PrintTextBlock(dump);
   printer_->PrintTextFooter();
 }
 
-void BlinkTestController::OnInitiateLayoutDump(
-    const test_runner::LayoutDumpFlags& layout_dump_flags) {
-  DCHECK(layout_dump_flags.dump_child_frames());
+void BlinkTestController::OnInitiateLayoutDump() {
   pending_layout_dumps_ = main_window_->web_contents()->SendToAllFrames(
-      new ShellViewMsg_LayoutDumpRequest(MSG_ROUTING_NONE, layout_dump_flags));
+      new ShellViewMsg_LayoutDumpRequest(MSG_ROUTING_NONE));
+}
+
+void BlinkTestController::OnLayoutDumpFlagsChanged(
+    RenderFrameHost* sender,
+    const base::DictionaryValue& changed_layout_dump_flags) {
+  // Stash the changes for future renderers.
+  accumulated_layout_dump_flags_changes_.MergeDictionary(
+      &changed_layout_dump_flags);
+
+  // Only need to send the propagation message once per renderer process.
+  std::set<int> already_covered_process_ids;

[dcheng, 2016/03/08 23:09:37]

Will we have a better way of doing this in the future? Is this something that'd
make sense to use PageMsg for?

[Łukasz Anforowicz, 2016/03/10 22:24:21]

This is a very good question.  I don't know the answer.  Possible partial
answers:

- RF-vs-RV-vs-RP msgs is orthogonal to the deduping mechanism.  If we won't want
to dedupe like above (which I agree is ugly), then we may need to reconsider
guid-based deduping (maybe the guid should move to LayoutTestFlags).  Guid-based
deduping mechanism seems to be usable regardless if messages are sent to RF / RP
/ RV).

- Currently page-level messages (i.e. https://crrev.com/1685213002/) are not
usable from outside of content internals (because PageMsgStart is internal to
content - see how dispatching to RV is done in
https://codereview.chromium.org/1685213002/patch/200001/210011).


Also - your question is about browser -> renderer direction.  There is a
complementary question about renderer -> browser direction which causes trouble
(i.e. around swapped out exception ugliness) in
https://codereview.chromium.org/1777503003/.


I guess the summary is:

1. I don't have a confident vision for how things should look like in the future
(i.e. maybe parts of TestRunner have to be moved to a separate, an
instance-per-frame class).  Without having a confident vision, I am hesitant to
start working on any particular direction of changes.

2. Without a confident long-term vision we are left with ugly hacks as above. 
But hey - 50+ more passing tests / 50+ less test exceptions for
--site-per-process is desirable...

[dcheng, 2016/03/12 01:21:50]

Please don't block this CL on my objections. At the same time, I would like us
to figure this out sooner rather than later.

So current layering makes this not work, but I feel like the model we should be
using is similar to the model we use to propagate changes to security origin,
opener, etc: we iterate through the RFH / RFPHs and update the ones where
SiteInstance doesn't match the source SiteInstance. It feels like a more natural
way of deduping.

I guess this may be problematic for things like noopener popup windows, but
maybe we don't need to care about those: do the contents of popup windows ever
show up directly in the dumped layout.

[Łukasz Anforowicz, 2016/03/14 16:23:13]

Cool, thanks. :-)
I probably should move this discussion somewhere outside of this CL, but right
now I am starting to think that the trouble here is caused by attempts to
shoehorn BlinkTestRunner/Controller communication into the artificial
restriction of having to deal with only 3 kinds of routable objects (frame /
view / process).  In short - I claim that PrintMessage is not frame-y, or
view-y, or process-y, but is something aimed directly at BlinkTestController
(and current piggybacking on RVO/WCO is accidental).  Given above I think I want
to try exploring how BTC/BTR communication would look like in Mojo world (and
one of my first questions will be around ordering guarantees [i.e. guaranteeing
that LayoutDumpFlagsChanged comes before any subsequent navigation requests
and/or OOPIF creations]).
 
Hmmm... Alex mentioned this as well.  OTOH, me and Alex weren't sure of the
SiteInstance <-> Process affinity.  For example - the "third party renderer
process" might host frames from multiple SiteInstances, right?  In this case
there will still be a single RenderView in the process, right?  (otherwise
having multiple RenderViews seems wasteful).  And at any rate - I think there
should be and will be only a single BlinkTestRunner in this case (despite
potentially mulitple SiteInstances per RenderProcessHost).
I don't know.  OTOH, let me note that we care not only about contents of a
layout dump, but also about other runtime test flags [i.e. wait_until_done]
which I think are more likely to be needed in popup windows [as long as popup
windows can run javascript?  it is possible that my lack of knowledge of
html/js/... is showing through here...]).

+
+  // No need to propagate the changes back to the process that originated them.
+  // (propagating them back could also clobber subsequent changes in the
+  // originator).
+  already_covered_process_ids.insert(sender->GetProcess()->GetID());
+
+  // Propagate the changes to all the renderer processes associated with the
+  // main window.
+  for (RenderFrameHost* frame : main_window_->web_contents()->GetAllFrames()) {
+    bool inserted_new_item =
+        already_covered_process_ids.insert(frame->GetProcess()->GetID()).second;
+    if (inserted_new_item) {
+      frame->Send(new ShellViewMsg_ReplicateLayoutDumpFlagsChanges(
+          frame->GetRoutingID(), changed_layout_dump_flags));
+    }
+  }
 }
 
 void BlinkTestController::OnLayoutDumpResponse(RenderFrameHost* sender,
                                                const std::string& dump) {
   // Store the result.
   auto pair = frame_to_layout_dump_map_.insert(
       std::make_pair(sender->GetFrameTreeNodeId(), dump));
   bool insertion_took_place = pair.second;
   DCHECK(insertion_took_place);
 
@@ skipping 193 matching lines @@
   } else {
     printer_->AddErrorMessage(base::StringPrintf(
         "FAIL: Unexpected sendBluetoothManualChooserEvent() event name '%s'.",
         event_name.c_str()));
     return;
   }
   bluetooth_chooser_factory_->SendEvent(event, argument);
 }
 
 }  // namespace content