Replicating LayoutDumpFlags across OOPIFs.

Replicating LayoutDumpFlags across OOPIFs.

This CL makes sure that LayoutDumpFLags are replicated across OOPIFs.
This allows to get rid of all the incorrect "missing" expectations
from third_party/WebKit/LayoutTests/FlagExpectations/site-per-process.

Replication is done by messages passed between browser and renderers.
TestRunner that changes LayoutDumpFlags notifies BlinkTestRunner
which sends a message to the browser.  The browser 1) broadcasts
the changes to all the renderers and 2) stashes accumulated changes
so it can send them to new renderers in the future.

BUG=587175
Committed: https://crrev.com/c9cbe71dd85bad04bf4f0b516614be820bb82b32
Cr-Commit-Position: refs/heads/master@{#380967}

[Łukasz Anforowicz, 2016-02-18 22:17:21]

Description was changed from

==========
Proof of concept for TrackedDictionary approach.

BUG=587175
==========

to

==========
Proof of concept for TrackedDictionary approach.

Doesn't build today.  Code in header files needs to be moved to an
implementation file.  Needs formatting.  Did I say that it doesn't even build
today?

BUG=587175
==========

[Łukasz Anforowicz, 2016-03-03 21:46:32]

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

[Łukasz Anforowicz, 2016-03-03 21:46:34]

jochen@, can you please take a look?  Feel free to review this CL last (after
the other related CLs - https://codereview.chromium.org/1746393002/,
https://codereview.chromium.org/1750063002/,
https://codereview.chromium.org/1755123004/) - this CL depends on the other ones
+ is bigger + might need more discussion (i.e. I am confident that the other
ones are ready to land, I *think* this one might be ready, but I am not 100%
sure - see my comments below).

https://codereview.chromium.org/1715573002/diff/120001/components/test_runner...
File components/test_runner/layout_dump_flags.h (left):

https://codereview.chromium.org/1715573002/diff/120001/components/test_runner...
components/test_runner/layout_dump_flags.h:22: is_printing(is_printing) {}
I’ve removed the LayoutDumpFlags constructor taking values of all the flags
(that was suggested / discussed in
https://codereview.chromium.org/1689283003/#msg13).  Values passed to the
constructor were always clobbered by TestRunner::Reset = the constructor is not
really needed.  I think it is more useful to take the default values hardcoded
in TestRunner::Reset and move them into LayoutDumpFlags::Reset (and call this in
the default constructor of LayoutDumpFlags, because now we can have a default
constructor - we don’t need to keep LayoutDumpFlags as POD-only anymore).

https://codereview.chromium.org/1715573002/diff/120001/components/test_runner...
File components/test_runner/layout_dump_flags.h (right):

https://codereview.chromium.org/1715573002/diff/120001/components/test_runner...
components/test_runner/layout_dump_flags.h:37: void set_##name(bool new_value) {
dict_.SetBoolean(#name, new_value); }
I am not very happy about using macros here, but I think this is ok (I think I
would be less happy with the alternatives - writing the code manually OR
generating the code via a script).  WDYT?

https://codereview.chromium.org/1715573002/diff/120001/components/test_runner...
File components/test_runner/test_runner.cc (left):

https://codereview.chromium.org/1715573002/diff/120001/components/test_runner...
components/test_runner/test_runner.cc:3155: }
Replication means that we no longer need to pass LayoutDumpFlags in the payload
of ShellViewMsg_LayoutDumpRequest and ShellViewHostMsg_InitiateLayoutDump
(yay!).  OTOH, this also means that TestRunner::CheckResponseMimeType has to be
hardened against a remote main frame (because now *all* frames call
TestRuner::GetLayoutDumpFlags which calls CheckResponseMimeType).

BTW: calling TestFinished in the main frame [i.e. crrev.com/1746393002] is still
needed despite the hardening - we want the right CheckResponseMimeType
calculation to happen before starting the layout dump.

https://codereview.chromium.org/1715573002/diff/120001/components/test_runner...
File components/test_runner/test_runner.h (left):

https://codereview.chromium.org/1715573002/diff/120001/components/test_runner...
components/test_runner/test_runner.h:707: bool generate_pixel_results_;
I think that extra flags (i.e. policy_delegate_enabled_,
dump_frame_load_callbacks_, etc.) can be moved from TestRunner into to
LayoutDumpFlags (aka LayoutTestRuntimeFlags) in a separate CL.  This CL
arbitrarily moves 1) generate_pixel_results to showcase that it fixes a bunch of
“missing expectation” failures with --site-per-process and 2) wait_until_done_
(because otherwise extra tests start failing with --site-per-process - for
example http/tests/security/originHeader/origin-header-for-https.html).

https://codereview.chromium.org/1715573002/diff/120001/components/test_runner...
File components/test_runner/tracked_dictionary.h (right):

https://codereview.chromium.org/1715573002/diff/120001/components/test_runner...
components/test_runner/tracked_dictionary.h:55: #endif  //
COMPONENTS_TEST_RUNNER_TRACKED_DICTIONARY_H_
I like the TrackedDictionary approach:

- This avoids browser->layout_dump_flags.h dependency - the browser can treat
base::DictionaryValue more or less opaquely (more-or-less, because of the single
call to MergeDictionary - I think this is still pretty opaque / oblivious to
what actual values are allowed+stored in the dictionary).  This benefit would be
gone with optional<T> and shared-memory approach (although in shared-memory
approach only sizeof(LayoutDumpFlags) is needed).

- Usage of base::DictionaryValue instead of introducing optional<T> to Chromium
already has a precedent in remoting::PolicyWatcher::PolicyUpdatedCallback
(disclaimer: I touched this code, but I didn’t introduce usage of the
DictionaryValue [and I did think about introducing optional<T> for
remoting::PolicyWatcher but decided that it would be just a bit of busywork with
no big benefit]).

- At one point I thought that shared memory approach (i.e.
https://codereview.chromium.org/1736353002/) might be cleaner, but dcheng@
rightfully pointed out that I haven’t thought about the need to use volatile and
memory fences and that even with those the semantics/safety is not clear.

- BTW: I use base::DictionaryValue rather than std::map<std::string, bool>,
because in the future LayoutDumpFlags might also contain other data types -
custom_text_output, http_headers_to_clear, audio_data, tooltip_text, etc.

https://codereview.chromium.org/1715573002/diff/120001/content/shell/renderer...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/1715573002/diff/120001/content/shell/renderer...
content/shell/renderer/layout_test/blink_test_runner.cc:1023: return;
We want to avoid consuming change notification broadcasts in the renderer that
originated the change (otherwise the broadcast can clobber subsequent changes in
the originator).  This is done via |change_originator_guid| in the CL.

This results in 1 avoidable IPC from browser to renderer (but then
correctness>performance + this is just test code?).  Plus, I don’t know how to
do things more efficiently or cleanly:

- I don’t know how the browser can identify RPH associated with the sender (this
might be doable if the notification was a RFH rather than RVH message, but
renderer side inherently gets change notifications at RV level [AFAICT some flag
changes cannot be attributed to a specific frame]).

- I don’t know how the renderer could send its RPH->GetID in the payload (there
is no RP::GetID method).

[Łukasz Anforowicz, 2016-03-03 21:52:07]

Description was changed from

==========
Proof of concept for TrackedDictionary approach.

Doesn't build today.  Code in header files needs to be moved to an
implementation file.  Needs formatting.  Did I say that it doesn't even build
today?

BUG=587175
==========

to

==========
Replicating LayoutDumpFlags across OOPIFs.

This CL makes sure that LayoutDumpFLags are replicated across OOPIFs.
This allows to get rid of all the incorrect "missing" expectations
from third_party/WebKit/LayoutTests/FlagExpectations/site-per-process.

Replication is done by messages passed between browser and renderers.
TestRunner that changes LayoutDumpFlags notifies BlinkTestRunner
which sends a message to the browser.  The browser 1) broadcasts
the changes to all the renderers and 2) stashes accumulated changes
so it can send them to new renderers in the future.

BUG=587175
==========

[Łukasz Anforowicz, 2016-03-03 21:55:02]

Description was changed from

==========
Replicating LayoutDumpFlags across OOPIFs.

This CL makes sure that LayoutDumpFLags are replicated across OOPIFs.
This allows to get rid of all the incorrect "missing" expectations
from third_party/WebKit/LayoutTests/FlagExpectations/site-per-process.

Replication is done by messages passed between browser and renderers.
TestRunner that changes LayoutDumpFlags notifies BlinkTestRunner
which sends a message to the browser.  The browser 1) broadcasts
the changes to all the renderers and 2) stashes accumulated changes
so it can send them to new renderers in the future.

BUG=587175
==========

to

==========
Replicating LayoutDumpFlags across OOPIFs.

This CL makes sure that LayoutDumpFLags are replicated across OOPIFs.
This allows to get rid of all the incorrect "missing" expectations
from third_party/WebKit/LayoutTests/FlagExpectations/site-per-process.

Replication is done by messages passed between browser and renderers.
TestRunner that changes LayoutDumpFlags notifies BlinkTestRunner
which sends a message to the browser.  The browser 1) broadcasts
the changes to all the renderers and 2) stashes accumulated changes
so it can send them to new renderers in the future.

BUG=587175
==========

[Łukasz Anforowicz, 2016-03-03 21:55:28]

<updating review title and CL description>

[Łukasz Anforowicz, 2016-03-03 22:34:33]

+dcheng@ to CC

[jochen (gone - plz use gerrit), 2016-03-04 12:50:37]

https://codereview.chromium.org/1715573002/diff/120001/content/shell/browser/...
File content/shell/browser/layout_test/blink_test_controller.cc (right):

https://codereview.chromium.org/1715573002/diff/120001/content/shell/browser/...
content/shell/browser/layout_test/blink_test_controller.cc:401: bool
BlinkTestController::OnMessageReceived(const IPC::Message& message) {
just override OnMessageReceived(const IPC::Message& message, RenderFrameHost*
rfh) here instead. Then you know where the message came from.

[Łukasz Anforowicz, 2016-03-04 19:58:56]

https://codereview.chromium.org/1715573002/diff/120001/content/shell/browser/...
File content/shell/browser/layout_test/blink_test_controller.cc (right):

https://codereview.chromium.org/1715573002/diff/120001/content/shell/browser/...
content/shell/browser/layout_test/blink_test_controller.cc:401: bool
BlinkTestController::OnMessageReceived(const IPC::Message& message) {
On 2016/03/04 12:50:37, jochen wrote:
> just override OnMessageReceived(const IPC::Message& message, RenderFrameHost*
> rfh) here instead. Then you know where the message came from.

Done.

Thanks for pushing in this direction.  I've considered this option, but
initially rejected it because it means sending via an arbitrarily picked local
frame from BlinkTestRunner::OnLayoutDumpFlagsChanged.  OTOH, I've discussed this
with nasko@ and dcheng@ and they think guid-trick is more hacky and also prefer
detecting the sender via WebContentsObserver::OnMessageReceived(...,
RenderFrameHost*) overload.  To some extent this is consistent with the
long-term desire to make all RenderViewObservers (including BlinkTestRunner)
RenderFrameObservers (although I am skeptical when this will be possible for
BlinkTestRunner - test_runner::TestRunner seems to have a lot of things that are
inherently per-view, not per-frame [i.e. ForceRedSelectionColors or
SetPageVisibility or SetMockScreenOrientation]).

Note that I accidentally included some undesirable reverts in Patchset#8  - to
see the intended differences it is probably best to compare Patchset#9 with
Patchset#7.

[jochen (gone - plz use gerrit), 2016-03-08 16:31:47]

lgtm

https://codereview.chromium.org/1715573002/diff/200001/content/shell/DEPS
File content/shell/DEPS (left):

https://codereview.chromium.org/1715573002/diff/200001/content/shell/DEPS#old...
content/shell/DEPS:29: "+components/test_runner/layout_dump_flags.h",  # POD
only.
\o/

[Łukasz Anforowicz, 2016-03-08 18:33:18]

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

[Łukasz Anforowicz, 2016-03-08 18:33:19]

nasko@, could you take a look at content/common/swapped_out_messages.cc?

Disclaimer: jochen@ raised [1] some concerns about content -> content/shell
dependency (in a different CL), but I think the current CL is still okay to
land: I am preventing further dependency violations via
https://codereview.chromium.org/1777503003/ + the extra swapped out hole
shouldn't matter that match (especially given that swapped out will be removed
altogether).

.
.
.

Let's also think how the messages should be delivered in the long term - I try
to describe the current state below and sketch the options going forward.

Right now the following events need holes for renderer->browser message in
swapped out state:
1. PrintMessage (old exception)
2. LayoutDumpFlagsChanged (this CL)
3. TestFinishedInSecondaryRenderer (future CL)

All of the events above can happen in an OOPIF (i.e. can be trigerred by a frame
in a different renderer than the main frame).  1 can be trigerred when
shouldDumpFrameLoadCallbacks test flag is set and when
WebTestProxyBase::LoadURLExternally runs.  2. can be trigerred by almost any
testRunner calls (i.e. by javascript calling the ambient [present in all frames]
testRunner.dumpAsText()).  3. can be trigerred by a call to
testRunner.notifyDone().

Currently all the 3 events above are send via RVO::Send (which today hits the
swapped-out-filter when main frame is not local).  So - what are the options
going forward?:

- Option #1: Keep making exceptions (for remote frames instead of for swapped
out) so things will work now and when swapped out is removed altogether.

- Option #2: Send the 3 messages via random local frame (as we do today in
https://codereview.chromium.org/1715573002/patch/200001/210017).  Hope that this
will be more natural one day when/if BlinkTestRunner becomes an RFO (rather than
RVO that it is today).

- Other options?


[1]
https://codereview.chromium.org/1765623003/diff/60001/content/common/swapped_...

[nasko, 2016-03-08 22:56:31]

On 2016/03/08 18:33:19, Łukasz Anforowicz wrote:
> nasko@, could you take a look at content/common/swapped_out_messages.cc?
> 
> Disclaimer: jochen@ raised [1] some concerns about content -> content/shell
> dependency (in a different CL), but I think the current CL is still okay to
> land: I am preventing further dependency violations via
> https://codereview.chromium.org/1777503003/ + the extra swapped out hole
> shouldn't matter that match (especially given that swapped out will be removed
> altogether).
> 
> .
> .
> .
> 
> Let's also think how the messages should be delivered in the long term - I try
> to describe the current state below and sketch the options going forward.
> 
> Right now the following events need holes for renderer->browser message in
> swapped out state:
> 1. PrintMessage (old exception)
> 2. LayoutDumpFlagsChanged (this CL)
> 3. TestFinishedInSecondaryRenderer (future CL)
> 
> All of the events above can happen in an OOPIF (i.e. can be trigerred by a
frame
> in a different renderer than the main frame).  1 can be trigerred when
> shouldDumpFrameLoadCallbacks test flag is set and when
> WebTestProxyBase::LoadURLExternally runs.  2. can be trigerred by almost any
> testRunner calls (i.e. by javascript calling the ambient [present in all
frames]
> testRunner.dumpAsText()).  3. can be trigerred by a call to
> testRunner.notifyDone().
> 
> Currently all the 3 events above are send via RVO::Send (which today hits the
> swapped-out-filter when main frame is not local).  So - what are the options
> going forward?:
> 
> - Option #1: Keep making exceptions (for remote frames instead of for swapped
> out) so things will work now and when swapped out is removed altogether.
> 
> - Option #2: Send the 3 messages via random local frame (as we do today in
> https://codereview.chromium.org/1715573002/patch/200001/210017).  Hope that
this
> will be more natural one day when/if BlinkTestRunner becomes an RFO (rather
than
> RVO that it is today).
> 
> - Other options?

Make them RenderProcessHost control messages as we've chatted?

> [1]
>
https://codereview.chromium.org/1765623003/diff/60001/content/common/swapped_...

Swapped out exception LGTM.

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

Just a few drive-by comments.

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
File components/test_runner/layout_dump_flags.h (right):

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
components/test_runner/layout_dump_flags.h:68: 
#undef DEFINE_BOOL_LAYOUT_DUMP_FLAG.

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
components/test_runner/test_runner.cc:3030: if
(layout_dump_flags_.tracked_dictionary().changed_values().size() == 0)
.empty()?

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
components/test_runner/test_runner.cc:3033:
delegate_->OnLayoutDumpFlagsChanged(&layout_dump_flags_);
Nit: pass a const ref, I don't see a place that actually needs this to be an
optional value.

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
File components/test_runner/tracked_dictionary.cc (right):

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
components/test_runner/tracked_dictionary.cc:7: #include <memory>
Nit: utility (memory is for unique_ptr, which isn't allowed yet)

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
File components/test_runner/tracked_dictionary.h (right):

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
components/test_runner/tracked_dictionary.h:17: class TEST_RUNNER_EXPORT
TrackedDictionary {
Add a class comment for a summary of how this differs from
base::DictionaryValue.

https://codereview.chromium.org/1715573002/diff/200001/content/shell/browser/...
File content/shell/browser/layout_test/blink_test_controller.cc (right):

https://codereview.chromium.org/1715573002/diff/200001/content/shell/browser/...
content/shell/browser/layout_test/blink_test_controller.cc:687: std::set<int>
already_covered_process_ids;
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:19]

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

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

dcheng@, can you take another look please?

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
File components/test_runner/layout_dump_flags.h (right):

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
components/test_runner/layout_dump_flags.h:68: 
On 2016/03/08 23:09:37, dcheng wrote:
> #undef DEFINE_BOOL_LAYOUT_DUMP_FLAG.

Done.  Good point.

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
components/test_runner/test_runner.cc:3030: if
(layout_dump_flags_.tracked_dictionary().changed_values().size() == 0)
On 2016/03/08 23:09:37, dcheng wrote:
> .empty()?

Done.

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
components/test_runner/test_runner.cc:3033:
delegate_->OnLayoutDumpFlagsChanged(&layout_dump_flags_);
On 2016/03/08 23:09:37, dcheng wrote:
> Nit: pass a const ref, I don't see a place that actually needs this to be an
> optional value.

Done.  I've moved responsibility for deciding when to reset change tracking from
BlinkTestRunner to TestRunner.  I think this is ok, but please read the
explanation below to sanity check.

This cannot be "const", because BlinkTestRunner::OnLayoutDumpFlagsChanged
currently calls
|layout_dump_flags->tracked_dictionary().ResetChangeTracking();|.  Looking at it
again, I am not sure why I thought that ResetChangeTracking should be called by
BlinkTestRunner, rather than by TestRunner.  It felt safer to only reset change
tracking at the exact moment when sending the changes to the browser.  OTOH, we
only skip changes when !is_main_window (and the changes prior to that will be
cloberred anyway by ResetChangeTracking called from LayoutTestFlags::Reset).

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
File components/test_runner/tracked_dictionary.cc (right):

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
components/test_runner/tracked_dictionary.cc:7: #include <memory>
On 2016/03/08 23:09:37, dcheng wrote:
> Nit: utility (memory is for unique_ptr, which isn't allowed yet)

Done.  Thanks for pointing this out.

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
File components/test_runner/tracked_dictionary.h (right):

https://codereview.chromium.org/1715573002/diff/200001/components/test_runner...
components/test_runner/tracked_dictionary.h:17: class TEST_RUNNER_EXPORT
TrackedDictionary {
On 2016/03/08 23:09:37, dcheng wrote:
> Add a class comment for a summary of how this differs from
> base::DictionaryValue.

Done.

https://codereview.chromium.org/1715573002/diff/200001/content/shell/DEPS
File content/shell/DEPS (left):

https://codereview.chromium.org/1715573002/diff/200001/content/shell/DEPS#old...
content/shell/DEPS:29: "+components/test_runner/layout_dump_flags.h",  # POD
only.
On 2016/03/08 16:31:47, jochen wrote:
> \o/

Acknowledged.

https://codereview.chromium.org/1715573002/diff/200001/content/shell/browser/...
File content/shell/browser/layout_test/blink_test_controller.cc (right):

https://codereview.chromium.org/1715573002/diff/200001/content/shell/browser/...
content/shell/browser/layout_test/blink_test_controller.cc:687: std::set<int>
already_covered_process_ids;
On 2016/03/08 23:09:37, dcheng wrote:
> Will we have a better way of doing this in the future? Is this something
that'd
> make sense to use PageMsg for?

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]

https://codereview.chromium.org/1715573002/diff/200001/content/shell/browser/...
File content/shell/browser/layout_test/blink_test_controller.cc (right):

https://codereview.chromium.org/1715573002/diff/200001/content/shell/browser/...
content/shell/browser/layout_test/blink_test_controller.cc:687: std::set<int>
already_covered_process_ids;
On 2016/03/10 at 22:24:21, Łukasz Anforowicz wrote:
> On 2016/03/08 23:09:37, dcheng wrote:
> > Will we have a better way of doing this in the future? Is this something
that'd
> > make sense to use PageMsg for?
> 
> 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...

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-13 20:44:01]

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

[commit-bot: I haz the power, 2016-03-13 20:44:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1715573002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1715573002/260001

[commit-bot: I haz the power, 2016-03-13 22:51:59]

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

[commit-bot: I haz the power, 2016-03-13 22:52:01]

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

[Łukasz Anforowicz, 2016-03-13 23:08:23]

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

[commit-bot: I haz the power, 2016-03-13 23:08:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1715573002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1715573002/280001

[commit-bot: I haz the power, 2016-03-14 01:45:23]

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

[commit-bot: I haz the power, 2016-03-14 01:45:24]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-03-14 14:01:35]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-03-14 14:01:35]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1715573002/#ps280001
(title: "Removed unneeded include.")

[commit-bot: I haz the power, 2016-03-14 14:01:46]

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

[commit-bot: I haz the power, 2016-03-14 14:06:51]

Description was changed from

==========
Replicating LayoutDumpFlags across OOPIFs.

This CL makes sure that LayoutDumpFLags are replicated across OOPIFs.
This allows to get rid of all the incorrect "missing" expectations
from third_party/WebKit/LayoutTests/FlagExpectations/site-per-process.

Replication is done by messages passed between browser and renderers.
TestRunner that changes LayoutDumpFlags notifies BlinkTestRunner
which sends a message to the browser.  The browser 1) broadcasts
the changes to all the renderers and 2) stashes accumulated changes
so it can send them to new renderers in the future.

BUG=587175
==========

to

==========
Replicating LayoutDumpFlags across OOPIFs.

This CL makes sure that LayoutDumpFLags are replicated across OOPIFs.
This allows to get rid of all the incorrect "missing" expectations
from third_party/WebKit/LayoutTests/FlagExpectations/site-per-process.

Replication is done by messages passed between browser and renderers.
TestRunner that changes LayoutDumpFlags notifies BlinkTestRunner
which sends a message to the browser.  The browser 1) broadcasts
the changes to all the renderers and 2) stashes accumulated changes
so it can send them to new renderers in the future.

BUG=587175
==========

[commit-bot: I haz the power, 2016-03-14 14:06:53]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-03-14 14:08:15]

Description was changed from

==========
Replicating LayoutDumpFlags across OOPIFs.

This CL makes sure that LayoutDumpFLags are replicated across OOPIFs.
This allows to get rid of all the incorrect "missing" expectations
from third_party/WebKit/LayoutTests/FlagExpectations/site-per-process.

Replication is done by messages passed between browser and renderers.
TestRunner that changes LayoutDumpFlags notifies BlinkTestRunner
which sends a message to the browser.  The browser 1) broadcasts
the changes to all the renderers and 2) stashes accumulated changes
so it can send them to new renderers in the future.

BUG=587175
==========

to

==========
Replicating LayoutDumpFlags across OOPIFs.

This CL makes sure that LayoutDumpFLags are replicated across OOPIFs.
This allows to get rid of all the incorrect "missing" expectations
from third_party/WebKit/LayoutTests/FlagExpectations/site-per-process.

Replication is done by messages passed between browser and renderers.
TestRunner that changes LayoutDumpFlags notifies BlinkTestRunner
which sends a message to the browser.  The browser 1) broadcasts
the changes to all the renderers and 2) stashes accumulated changes
so it can send them to new renderers in the future.

BUG=587175

Committed: https://crrev.com/c9cbe71dd85bad04bf4f0b516614be820bb82b32
Cr-Commit-Position: refs/heads/master@{#380967}
==========

[commit-bot: I haz the power, 2016-03-14 14:08:17]

Patchset 15 (id:??) landed as
https://crrev.com/c9cbe71dd85bad04bf4f0b516614be820bb82b32
Cr-Commit-Position: refs/heads/master@{#380967}

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

Thanks for the feedback Daniel.

https://codereview.chromium.org/1715573002/diff/200001/content/shell/browser/...
File content/shell/browser/layout_test/blink_test_controller.cc (right):

https://codereview.chromium.org/1715573002/diff/200001/content/shell/browser/...
content/shell/browser/layout_test/blink_test_controller.cc:687: std::set<int>
already_covered_process_ids;
On 2016/03/12 01:21:50, dcheng wrote:
> On 2016/03/10 at 22:24:21, Łukasz Anforowicz wrote:
> > On 2016/03/08 23:09:37, dcheng wrote:
> > > Will we have a better way of doing this in the future? Is this something
> that'd
> > > make sense to use PageMsg for?
> > 
> > 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...
> 
> Please don't block this CL on my objections.

Cool, thanks. :-)

> At the same time, I would like us
> to figure this out sooner rather than later.

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

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

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

[brettw, 2016-03-22 00:04:36]

brettw@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2016-03-22 00:04:37]

From https://bugs.chromium.org/p/chromium/issues/detail?id=596736 I just filed:

https://codereview.chromium.org/1715573002
https://codereview.chromium.org/1067373005

Introduced includes from content/common to content/shell for
swapped_out_messages.cc

I am trying to make GN's header checker pass on content so we don't introduce
layering violations, and that's blocked on this laying violation.

content/shell is a test harness browser while content/common is linked into
every process in the browser. Including content_shell_messages here also brings
in other parts of content shell like shell_test_configuration.h and
leak_detection_result.h into content/common. We need to find a way to do this
that's not a layering violation.

Perhaps content_shell needs it's own superclass of the swapped out messages
filter.