SiteDetailsBrowserTest: prepare to add proxy information

SiteDetailsBrowserTest: prepare to add proxy information, by
reworking how our matchers work and making the test run all
the modes by default.

SiteDetails: introduce structs to hold the per-browsing-instance
info. Make it a little more clear when we're talking about site
instance vs. browsing instance.

The follow-up CL will add additional members to the structs to
count the number of proxies in each browsing instance.

BUG=

Committed: https://crrev.com/6c28ecc923b5e88b8d5941d0e1e39b7aa6c70da5
Cr-Commit-Position: refs/heads/master@{#374476}

[ncarter (slow), 2016-02-05 22:21:23]

Description was changed from

==========
SiteDetailsBrowserTest: prepare to add proxy information, by
reworking how our matchers work, and adding structs to hold
the per-browsing-instance info.

The follow-up CL will add additional members to the structs.

BUG=
==========

to

==========
SiteDetailsBrowserTest: prepare to add proxy information, by
reworking how our matchers work and making the test run all
the modes by default.

SiteDetails: introduce structs to hold the per-browsing-instance
info. Make it a little more clear when we're talking about site
instance vs. browsing instance.

The follow-up CL will add additional members to the structs to
count the number of proxies in each browsing instance.

BUG=
==========

[ncarter (slow), 2016-02-08 18:05:04]

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

[nasko, 2016-02-08 18:27:19]

https://codereview.chromium.org/1674873002/diff/40001/chrome/browser/site_det...
File chrome/browser/site_details.h (right):

https://codereview.chromium.org/1674873002/diff/40001/chrome/browser/site_det...
chrome/browser/site_details.h:28: std::set<content::SiteInstance*> sites;
nit: Having sites as the name of the parameter in this and the above structs
made it a bit harder to read when used. Why not use site_urls and site_instances
or something similar, so it is clear which type of struct we are dealing with?

https://codereview.chromium.org/1674873002/diff/40001/chrome/browser/site_det...
chrome/browser/site_details.h:63: // Global list of all BrowsingInstances, used
for de-duping related instances.
The latter part of the sentence ", used for ..." is somewhat implied by the fact
that this is a list of BrowsingInstances, which by definition group related
SiteInstances. I think the whole comment can be simplified a bit.

[ncarter (slow), 2016-02-08 23:38:18]

PTAL

https://codereview.chromium.org/1674873002/diff/40001/chrome/browser/site_det...
File chrome/browser/site_details.h (right):

https://codereview.chromium.org/1674873002/diff/40001/chrome/browser/site_det...
chrome/browser/site_details.h:20: std::set<GURL> sites;
Left as "sites"

https://codereview.chromium.org/1674873002/diff/40001/chrome/browser/site_det...
chrome/browser/site_details.h:28: std::set<content::SiteInstance*> sites;
On 2016/02/08 18:27:19, nasko wrote:
> nit: Having sites as the name of the parameter in this and the above structs
> made it a bit harder to read when used. Why not use site_urls and
site_instances
> or something similar, so it is clear which type of struct we are dealing with?

Renamed to site_instances

https://codereview.chromium.org/1674873002/diff/40001/chrome/browser/site_det...
chrome/browser/site_details.h:50: std::set<GURL> sites;
Renamed to "all_sites"

https://codereview.chromium.org/1674873002/diff/40001/chrome/browser/site_det...
chrome/browser/site_details.h:63: // Global list of all BrowsingInstances, used
for de-duping related instances.
On 2016/02/08 18:27:19, nasko wrote:
> The latter part of the sentence ", used for ..." is somewhat implied by the
fact
> that this is a list of BrowsingInstances, which by definition group related
> SiteInstances. I think the whole comment can be simplified a bit.

Clarified comment.

https://codereview.chromium.org/1674873002/diff/40001/chrome/browser/site_det...
chrome/browser/site_details.h:66: BrowsingInstanceMap instances;
Renamed to 'browsing_instances'. Note that this is now a dupe name of the data
member in IsolationScenario; let me know if you have a better suggestion.

[nasko, 2016-02-08 23:59:56]

Thanks a lot for the renames, it makes is a lot more readable/understandable!

LGTM

[ncarter (slow), 2016-02-09 00:03:32]

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

[ncarter (slow), 2016-02-09 00:03:33]

+thestig for chrome/ rubber stamp

[Lei Zhang, 2016-02-09 00:33:33]

lgtm with nits:

https://codereview.chromium.org/1674873002/diff/80001/chrome/browser/site_det...
File chrome/browser/site_details.cc (right):

https://codereview.chromium.org/1674873002/diff/80001/chrome/browser/site_det...
chrome/browser/site_details.cc:181: const SiteData* site_data =
&site_data_map_entry.second;
const SiteData& instead? Ditto for line 185 and 192.

https://codereview.chromium.org/1674873002/diff/80001/chrome/browser/site_det...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/1674873002/diff/80001/chrome/browser/site_det...
chrome/browser/site_details_browsertest.cc:116: GetCurrentPolicy() ==
ISOLATE_NOTHING
This is a bit hairy. Will gtest let you put a function here that takes all the
isolate_foo arguments instead?

[ncarter (slow), 2016-02-09 01:06:32]

Will get back to you on the other stuff.

https://codereview.chromium.org/1674873002/diff/80001/chrome/browser/site_det...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/1674873002/diff/80001/chrome/browser/site_det...
chrome/browser/site_details_browsertest.cc:116: GetCurrentPolicy() ==
ISOLATE_NOTHING
On 2016/02/09 00:33:33, Lei Zhang wrote:
> This is a bit hairy. Will gtest let you put a function here that takes all the
> isolate_foo arguments instead?

I tried, and the problem is that the print function has to be polymorphic, and
if you templatize it, C++ isn't able to infer the type of isolate_nothing in
this context.

The other way to do this is to abandon MATCHER_P and implement this in terms of
MatcherInterface directly. There are pros and cons to that -- I wouldn't be
constrained to using PrintToString() anymore, so I wouldn't need the PrintTo()
methods below. But I actually think that the MATCHER_P macros are more readable
to people who don't deeply understand the guts of gmock matchers, since the
complicated template magic stays mostly hidden.

[Lei Zhang, 2016-02-09 01:20:16]

https://codereview.chromium.org/1674873002/diff/80001/chrome/browser/site_det...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/1674873002/diff/80001/chrome/browser/site_det...
chrome/browser/site_details_browsertest.cc:116: GetCurrentPolicy() ==
ISOLATE_NOTHING
On 2016/02/09 01:06:32, ncarter wrote:
> On 2016/02/09 00:33:33, Lei Zhang wrote:
> > This is a bit hairy. Will gtest let you put a function here that takes all
the
> > isolate_foo arguments instead?
> 
> I tried, and the problem is that the print function has to be polymorphic, and
> if you templatize it, C++ isn't able to infer the type of isolate_nothing in
> this context.

Ack. This is not the end of the world. :)

[ncarter (slow), 2016-02-09 19:32:52]

https://codereview.chromium.org/1674873002/diff/80001/chrome/browser/site_det...
File chrome/browser/site_details.cc (right):

https://codereview.chromium.org/1674873002/diff/80001/chrome/browser/site_det...
chrome/browser/site_details.cc:181: const SiteData* site_data =
&site_data_map_entry.second;
On 2016/02/09 00:33:33, Lei Zhang wrote:
> const SiteData& instead? Ditto for line 185 and 192.

Done.

[ncarter (slow), 2016-02-09 19:32:56]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2016-02-09 19:32:56]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1674873002/#ps100001
(title: "thestig's fixes")

[commit-bot: I haz the power, 2016-02-09 19:38:28]

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

[commit-bot: I haz the power, 2016-02-09 20:12:41]

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

[commit-bot: I haz the power, 2016-02-09 20:12:41]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[ncarter (slow), 2016-02-09 20:54:56]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2016-02-09 20:55:49]

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

[commit-bot: I haz the power, 2016-02-09 21:30:22]

Description was changed from

==========
SiteDetailsBrowserTest: prepare to add proxy information, by
reworking how our matchers work and making the test run all
the modes by default.

SiteDetails: introduce structs to hold the per-browsing-instance
info. Make it a little more clear when we're talking about site
instance vs. browsing instance.

The follow-up CL will add additional members to the structs to
count the number of proxies in each browsing instance.

BUG=
==========

to

==========
SiteDetailsBrowserTest: prepare to add proxy information, by
reworking how our matchers work and making the test run all
the modes by default.

SiteDetails: introduce structs to hold the per-browsing-instance
info. Make it a little more clear when we're talking about site
instance vs. browsing instance.

The follow-up CL will add additional members to the structs to
count the number of proxies in each browsing instance.

BUG=
==========

[commit-bot: I haz the power, 2016-02-09 21:30:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-09 21:31:39]

Description was changed from

==========
SiteDetailsBrowserTest: prepare to add proxy information, by
reworking how our matchers work and making the test run all
the modes by default.

SiteDetails: introduce structs to hold the per-browsing-instance
info. Make it a little more clear when we're talking about site
instance vs. browsing instance.

The follow-up CL will add additional members to the structs to
count the number of proxies in each browsing instance.

BUG=
==========

to

==========
SiteDetailsBrowserTest: prepare to add proxy information, by
reworking how our matchers work and making the test run all
the modes by default.

SiteDetails: introduce structs to hold the per-browsing-instance
info. Make it a little more clear when we're talking about site
instance vs. browsing instance.

The follow-up CL will add additional members to the structs to
count the number of proxies in each browsing instance.

BUG=

Committed: https://crrev.com/6c28ecc923b5e88b8d5941d0e1e39b7aa6c70da5
Cr-Commit-Position: refs/heads/master@{#374476}
==========

[commit-bot: I haz the power, 2016-02-09 21:31:40]

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