Prerender: Remove MatchComplete

Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete"), but does
not remove the MatchCompleteStatus yet - postponed for a followup change.

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

BUG=609105
Committed: https://crrev.com/3e302bd544e4ac9ac1b119424a5b4d100340c70b
Cr-Commit-Position: refs/heads/master@{#392952}

[pasko, 2016-04-20 09:17:55]

Description was changed from

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete").

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reason for removing: the current code probably cannot be reused for future
experiments, makes Prerender lifetime simpler.

BUG=none
==========

to

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete").

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

BUG=none
==========

[pasko, 2016-04-20 12:51:13]

Description was changed from

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete").

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

BUG=none
==========

to

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete").

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

Other non-obvious manipulations:
* Removed PrerenderBrowserTest.PrerenderDeferredSynchronousXHR because it
  requires setting up PrerenderResourceThrottle, which is not done in
  prerender_browsertest, and anyway the case is tested in
  PrerenderResourceThrottleTest.RedirectSyncXHR.

BUG=none
==========

[pasko, 2016-04-20 13:59:40]

Description was changed from

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete").

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

Other non-obvious manipulations:
* Removed PrerenderBrowserTest.PrerenderDeferredSynchronousXHR because it
  requires setting up PrerenderResourceThrottle, which is not done in
  prerender_browsertest, and anyway the case is tested in
  PrerenderResourceThrottleTest.RedirectSyncXHR.

BUG=none
==========

to

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete").

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

Other non-obvious manipulations:
* Removed PrerenderBrowserTest.PrerenderDeferredSynchronousXHR because it
  requires setting up PrerenderResourceThrottle, which is not done in
  prerender_browsertest, and anyway the case is tested in
  PrerenderResourceThrottleTest.RedirectSyncXHR.
* Disabled the PrerenderBrowserTestWithExtensions.StreamsTest, it has been
  relying on MatchComplete in a way pasko@ does not yet understand.

BUG=none
==========

[pasko, 2016-05-04 13:25:30]

Description was changed from

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete").

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

Other non-obvious manipulations:
* Removed PrerenderBrowserTest.PrerenderDeferredSynchronousXHR because it
  requires setting up PrerenderResourceThrottle, which is not done in
  prerender_browsertest, and anyway the case is tested in
  PrerenderResourceThrottleTest.RedirectSyncXHR.
* Disabled the PrerenderBrowserTestWithExtensions.StreamsTest, it has been
  relying on MatchComplete in a way pasko@ does not yet understand.

BUG=none
==========

to

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete").

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

Other non-obvious manipulations:
* Removed PrerenderBrowserTest.PrerenderDeferredSynchronousXHR because it
  requires setting up PrerenderResourceThrottle, which is not done in
  prerender_browsertest, and anyway the case is tested in
  PrerenderResourceThrottleTest.RedirectSyncXHR.
* In the PrerenderBrowserTestWithExtensions.StreamsTest removed the check for
  NavigateToDestURL(application/msword URL) because it hits an assert for
  non-null GetPrerenderContents() in NavigateToURLImpl(). I think this worked
  before by using the fake MatchComplete WebContents.

BUG=none
==========

[pasko, 2016-05-04 13:45:22]

Description was changed from

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete").

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

Other non-obvious manipulations:
* Removed PrerenderBrowserTest.PrerenderDeferredSynchronousXHR because it
  requires setting up PrerenderResourceThrottle, which is not done in
  prerender_browsertest, and anyway the case is tested in
  PrerenderResourceThrottleTest.RedirectSyncXHR.
* In the PrerenderBrowserTestWithExtensions.StreamsTest removed the check for
  NavigateToDestURL(application/msword URL) because it hits an assert for
  non-null GetPrerenderContents() in NavigateToURLImpl(). I think this worked
  before by using the fake MatchComplete WebContents.

BUG=none
==========

to

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete").

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

Other non-obvious manipulations:
* Removed PrerenderBrowserTest.PrerenderDeferredSynchronousXHR because it
  requires setting up PrerenderResourceThrottle, which is not done in
  prerender_browsertest, and anyway the case is tested in
  PrerenderResourceThrottleTest.RedirectSyncXHR.

BUG=609105
==========

[pasko, 2016-05-04 13:59:46]

Description was changed from

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete").

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

Other non-obvious manipulations:
* Removed PrerenderBrowserTest.PrerenderDeferredSynchronousXHR because it
  requires setting up PrerenderResourceThrottle, which is not done in
  prerender_browsertest, and anyway the case is tested in
  PrerenderResourceThrottleTest.RedirectSyncXHR.

BUG=609105
==========

to

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete"), but does
not remove the MatchCompleteStatus yet - postponed for a followup change.

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

Other non-obvious manipulations:
* Removed PrerenderBrowserTest.PrerenderDeferredSynchronousXHR because it
  requires setting up PrerenderResourceThrottle, which is not done in
  prerender_browsertest, and anyway the case is tested in
  PrerenderResourceThrottleTest.RedirectSyncXHR.

BUG=609105
==========

[pasko, 2016-05-04 16:41:00]

pasko@chromium.org changed reviewers:
+ mmenke@chromium.org

[pasko, 2016-05-04 16:41:01]

mmenke: PTaL

[mmenke, 2016-05-04 19:21:53]

Great to see this code path removed!  It's been a pain to deal with ever since
it was added.

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3765:
PrerenderManager::PRERENDER_MODE_EXPERIMENT_MATCH_COMPLETE_GROUP);
Why does this test require this call?  Seems like the two lines should work,
regardless.

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3445:
current_browser()->OpenURL(params);
Can you just use NavigateToURL(browser(), dest_url())?

Not strictly needed, since the catcher does its own waiting, but it's a little
simpler than using OpenURLParams.

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.h:99: // TODO(pasko): Remove the
deadcode associated with non-default
nit:  "dead code"

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:581: case PRERENDER_MODE_MAX:
Should update histograms.xml to account for removing this group.

[pasko, 2016-05-09 16:07:51]

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3765:
PrerenderManager::PRERENDER_MODE_EXPERIMENT_MATCH_COMPLETE_GROUP);
On 2016/05/04 19:21:52, mmenke wrote:
> Why does this test require this call?  Seems like the two lines should work,
> regardless.

As in the StreamsTest, we cannot use NavigateToDestURL() because it expects a
non-null GetPrerenderContents(). I think the lines were added as a quick fix
without investigating the reasons. The patchset says "Update tests":

https://codereview.chromium.org/550453002

I can make it a ui_test_utils::NavigateToURL(current_browser(), dest_url()), but
it seems to require to set up PrerenderResourceThrottle, which is not done in
prerender_browsertest, afaict.

PrerenderResourceThrottleTest.RedirectSyncXHR seems to cover the exact case we
wanted to cover here, and does it with PrerenderResourceThrottle, so I prefer to
remove this test.

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3445:
current_browser()->OpenURL(params);
On 2016/05/04 19:21:52, mmenke wrote:
> Can you just use NavigateToURL(browser(), dest_url())?
> 
> Not strictly needed, since the catcher does its own waiting, but it's a little
> simpler than using OpenURLParams.

Done. Thank you, I did not know about the existence of
ui_test_utils::NavigateToURL. The implementation looks a bit different than
NavigateToURLImpl here, but duplicated in parts, which confuses me. Do you know
the history behind the two?

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.h:99: // TODO(pasko): Remove the
deadcode associated with non-default
On 2016/05/04 19:21:52, mmenke wrote:
> nit:  "dead code"

Done.

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:581: case PRERENDER_MODE_MAX:
On 2016/05/04 19:21:52, mmenke wrote:
> Should update histograms.xml to account for removing this group.

I cannot find these suffixes with leading underscore in histograms.xml, were
they renamed or never put there?

I see stuff like _PrerenderMulti, _PrerenderNoUse, _PrerenderMatchComplete via
affected-histogram, but those are already marked as obsolete.

Histogram cleanup looks like a separate pass/CL for me.

[mmenke, 2016-05-09 20:04:39]

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3765:
PrerenderManager::PRERENDER_MODE_EXPERIMENT_MATCH_COMPLETE_GROUP);
On 2016/05/09 16:07:51, pasko wrote:
> On 2016/05/04 19:21:52, mmenke wrote:
> > Why does this test require this call?  Seems like the two lines should work,
> > regardless.
> 
> As in the StreamsTest, we cannot use NavigateToDestURL() because it expects a
> non-null GetPrerenderContents(). I think the lines were added as a quick fix
> without investigating the reasons. The patchset says "Update tests":
> 
> https://codereview.chromium.org/550453002
> 
> I can make it a ui_test_utils::NavigateToURL(current_browser(), dest_url()),
but
> it seems to require to set up PrerenderResourceThrottle, which is not done in
> prerender_browsertest, afaict.
> 
> PrerenderResourceThrottleTest.RedirectSyncXHR seems to cover the exact case we
> wanted to cover here, and does it with PrerenderResourceThrottle, so I prefer
to
> remove this test.

I want to try and get some idea what's going on there before signing off on this
CL - I generally prefer integration tests, because of cobbled together a lot of
these network APIs are, and because the underlying implementation of some of
this stuff changes over time, and if we ever removed the throttle, this test
should still pass.

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3445:
current_browser()->OpenURL(params);
On 2016/05/09 16:07:51, pasko wrote:
> On 2016/05/04 19:21:52, mmenke wrote:
> > Can you just use NavigateToURL(browser(), dest_url())?
> > 
> > Not strictly needed, since the catcher does its own waiting, but it's a
little
> > simpler than using OpenURLParams.
> 
> Done. Thank you, I did not know about the existence of
> ui_test_utils::NavigateToURL. The implementation looks a bit different than
> NavigateToURLImpl here, but duplicated in parts, which confuses me. Do you
know
> the history behind the two?
> 

OpenURL is the production code method to start a navigation.

NavigateToURL is the test method to navigate to a page and spin the message loop
until the navigation has finished.  Since this code uses catcher.GetNextResult()
on the next line, the waiting isn't strictly needed, but it doesn't hurt, and
it's a simpler API.

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:581: case PRERENDER_MODE_MAX:
On 2016/05/09 16:07:51, pasko wrote:
> On 2016/05/04 19:21:52, mmenke wrote:
> > Should update histograms.xml to account for removing this group.
> 
> I cannot find these suffixes with leading underscore in histograms.xml, were
> they renamed or never put there?
> 
> I see stuff like _PrerenderMulti, _PrerenderNoUse, _PrerenderMatchComplete via
> affected-histogram, but those are already marked as obsolete.
> 
> Histogram cleanup looks like a separate pass/CL for me.

The histograms team seem to prefer histograms to be cleaned up in the same CL
that removes code to record the histogram.  That having been said...Yea, I don't
see any corresponding histograms in the XML file.  I have no idea why that is.

[pasko, 2016-05-10 13:32:06]

Description was changed from

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete"), but does
not remove the MatchCompleteStatus yet - postponed for a followup change.

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

Other non-obvious manipulations:
* Removed PrerenderBrowserTest.PrerenderDeferredSynchronousXHR because it
  requires setting up PrerenderResourceThrottle, which is not done in
  prerender_browsertest, and anyway the case is tested in
  PrerenderResourceThrottleTest.RedirectSyncXHR.

BUG=609105
==========

to

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete"), but does
not remove the MatchCompleteStatus yet - postponed for a followup change.

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

BUG=609105
==========

[pasko, 2016-05-10 13:52:26]

PTAL

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3765:
PrerenderManager::PRERENDER_MODE_EXPERIMENT_MATCH_COMPLETE_GROUP);
On 2016/05/09 20:04:38, mmenke wrote:
> On 2016/05/09 16:07:51, pasko wrote:
> > On 2016/05/04 19:21:52, mmenke wrote:
> > > Why does this test require this call?  Seems like the two lines should
work,
> > > regardless.
> > 
> > As in the StreamsTest, we cannot use NavigateToDestURL() because it expects
a
> > non-null GetPrerenderContents(). I think the lines were added as a quick fix
> > without investigating the reasons. The patchset says "Update tests":
> > 
> > https://codereview.chromium.org/550453002
> > 
> > I can make it a ui_test_utils::NavigateToURL(current_browser(), dest_url()),
> but
> > it seems to require to set up PrerenderResourceThrottle, which is not done
in
> > prerender_browsertest, afaict.
> > 
> > PrerenderResourceThrottleTest.RedirectSyncXHR seems to cover the exact case
we
> > wanted to cover here, and does it with PrerenderResourceThrottle, so I
prefer
> to
> > remove this test.
> 
> I want to try and get some idea what's going on there before signing off on
this
> CL - I generally prefer integration tests, because of cobbled together a lot
of
> these network APIs are, and because the underlying implementation of some of
> this stuff changes over time, and if we ever removed the throttle, this test
> should still pass.

The argument for more integration-kind of tests is something I did not think
about, and it is good.

Also, I was wrong when I said that PrerenderResourceThrottle is not set up with
prerender_browsertest. I checked more carefully and saw that it actually sets
FINAL_STATUS_BAD_DEFERRED_REDIRECT in this test.

So I restored the test. Thanks.

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:3445:
current_browser()->OpenURL(params);
On 2016/05/09 20:04:38, mmenke wrote:
> On 2016/05/09 16:07:51, pasko wrote:
> > On 2016/05/04 19:21:52, mmenke wrote:
> > > Can you just use NavigateToURL(browser(), dest_url())?
> > > 
> > > Not strictly needed, since the catcher does its own waiting, but it's a
> little
> > > simpler than using OpenURLParams.
> > 
> > Done. Thank you, I did not know about the existence of
> > ui_test_utils::NavigateToURL. The implementation looks a bit different than
> > NavigateToURLImpl here, but duplicated in parts, which confuses me. Do you
> know
> > the history behind the two?
> > 
> 
> OpenURL is the production code method to start a navigation.
> 
> NavigateToURL is the test method to navigate to a page and spin the message
loop
> until the navigation has finished.  Since this code uses
catcher.GetNextResult()
> on the next line, the waiting isn't strictly needed, but it doesn't hurt, and
> it's a simpler API.

Acknowledged. Thank you.

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/1896403002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:581: case PRERENDER_MODE_MAX:
On 2016/05/09 20:04:39, mmenke wrote:
> On 2016/05/09 16:07:51, pasko wrote:
> > On 2016/05/04 19:21:52, mmenke wrote:
> > > Should update histograms.xml to account for removing this group.
> > 
> > I cannot find these suffixes with leading underscore in histograms.xml, were
> > they renamed or never put there?
> > 
> > I see stuff like _PrerenderMulti, _PrerenderNoUse, _PrerenderMatchComplete
via
> > affected-histogram, but those are already marked as obsolete.
> > 
> > Histogram cleanup looks like a separate pass/CL for me.
> 
> The histograms team seem to prefer histograms to be cleaned up in the same CL
> that removes code to record the histogram.  That having been said...Yea, I
don't
> see any corresponding histograms in the XML file.  I have no idea why that is.

What I tried to say about a separate CL is: "My preference is also to do
histograms.xml change in the same change that modifies histogram collection, but
in this case I did not find the corresponding histograms.xml parts for this
code. Also, since this is dead code, it's somewhat safe to leave it this way
even if my reading of histograms.xml is wrong".

A separate CL for histograms I envision is about:
1. removing dead code that does not record anything
2. remove code that records useless histograms

So more like 2 CLs, or even more than that. But I did not plan them for short
term.

[mmenke, 2016-05-11 15:45:19]

LGTM!  Great to finally get rid of this stuff.

[pasko, 2016-05-11 15:57:02]

Thank you for review, Matt

[pasko, 2016-05-11 15:57:08]

The CQ bit was checked by pasko@chromium.org

[commit-bot: I haz the power, 2016-05-11 15:57:25]

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

[commit-bot: I haz the power, 2016-05-11 16:38:07]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-05-11 16:39:35]

Description was changed from

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete"), but does
not remove the MatchCompleteStatus yet - postponed for a followup change.

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

BUG=609105
==========

to

==========
Prerender: Remove MatchComplete

This removes the control group emulation logic (i.e. "MatchComplete"), but does
not remove the MatchCompleteStatus yet - postponed for a followup change.

The problem being solved was:
1. Prerendering in the Control Group pretends that it performs prerenders as
   usual, but actually is a no-op.
2. In the Control Group there is no knowledge of cases when Prerender gives up
   early.
3. To match the FinalStatus of the Control Group the new MATCH_COMPLETE_GROUP
   was introduced. It worked as a usual prerender, and in case of giving up
   early it provided a "dummy" prerender and recorded use/no-use for the dummy
   in the lifetime.

Reasons for removing:
1. the current code probably cannot be reused for future experiments,
2. makes Prerender lifetime simpler.

BUG=609105

Committed: https://crrev.com/3e302bd544e4ac9ac1b119424a5b4d100340c70b
Cr-Commit-Position: refs/heads/master@{#392952}
==========

[commit-bot: I haz the power, 2016-05-11 16:39:36]

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