Revert "Revert of [NoStatePrefetch] Kill renderer after preload scanning (patchset #12 id:240001 of…

Reland: [NoStatePrefetch] Kill renderer after preload scanning

Remove the inherently flaky test
NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch and revert the
following revert:

> Reason for revert:
> "NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch" became flaky
> shortly after this landed.
>
> BUG=659697
> Original issue's description:
> > [NoStatePrefetch] Kill renderer after preload scanning
> >
> > The design doc is linked in the first bug. Main goal: remove Prerender.
> >
> > Currently NoState Prefetch uses the same logic to manage renderers as the
> > PrerenderManager. Prefetch code is still living in PrerenderManager, separating
> > the code from there is planned after we finalize on how much of the
> > functionality of PrerenderManager is needed.
> >
> > This is the first tweak in the renderer lifetime: the renderer asks to be killed
> > as soon as the main resource is fully preload-scanned and all possible
> > subresources are requested, special Prerendering FinalStatus is recorded to allow:
> > * testing
> > * provide a hint that a new prefetch can be started soon
> >
> > Browsertest changes:
> >
> > * TestPrerender now keeps FinalStatus for longer, to be able to verify that it
> >   is correct even after the TestPrerenderContents is destroyed.
> >
> > * Prefetch browsertests wait only for creation of PrerenderContents, Prerender
> >   tests have more waiting for page loads on top of that
> >
> > * PrerenderTestUrlImpl is removed to eliminate one hop through protected method
> >   in the parent class, which also simplified the prefetch side of the tests: no
> >   need to mention how many page loads to wait for
> >
> > * Disabled two tests that load a non-HTML document, correctly killing the
> >   renderer will be done in later changes.
> >
> > BUG=632361, 649632
> >
> > Committed: https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538
> > Cr-Commit-Position: refs/heads/master@{#427678}
> Committed: https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4
> Cr-Commit-Position: refs/heads/master@{#427974}

TBR=jochen@chromium.org,csharrison@chromium.org,mattcary@chromium.org,mkwst@chromium.org,pasko@chromium.org
BUG=632361, 649632
Committed: https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d
Cr-Commit-Position: refs/heads/master@{#428027}

[pasko, 2016-10-27 11:55:17]

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

[commit-bot: I haz the power, 2016-10-27 11:55:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pasko, 2016-10-27 11:55:53]

Description was changed from

==========
Revert "Revert of [NoStatePrefetch] Kill renderer after preload scanning
(patchset #12 id:240001 of https://codereview.chromium.org/2411863002/ )"

Removing the inherently flaky test
NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch and reverting the
following revert:

> Reason for revert:
> "NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch" became flaky
> shortly after this landed.
>
> BUG=659697
> Original issue's description:
> > [NoStatePrefetch] Kill renderer after preload scanning
> >
> > The design doc is linked in the first bug. Main goal: remove Prerender.
> >
> > Currently NoState Prefetch uses the same logic to manage renderers as the
> > PrerenderManager. Prefetch code is still living in PrerenderManager,
separating
> > the code from there is planned after we finalize on how much of the
> > functionality of PrerenderManager is needed.
> >
> > This is the first tweak in the renderer lifetime: the renderer asks to be
killed
> > as soon as the main resource is fully preload-scanned and all possible
> > subresources are requested, special Prerendering FinalStatus is recorded to
allow:
> > * testing
> > * provide a hint that a new prefetch can be started soon
> >
> > Browsertest changes:
> >
> > * TestPrerender now keeps FinalStatus for longer, to be able to verify that
it
> >   is correct even after the TestPrerenderContents is destroyed.
> >
> > * Prefetch browsertests wait only for creation of PrerenderContents,
Prerender
> >   tests have more waiting for page loads on top of that
> >
> > * PrerenderTestUrlImpl is removed to eliminate one hop through protected
method
> >   in the parent class, which also simplified the prefetch side of the tests:
no
> >   need to mention how many page loads to wait for
> >
> > * Disabled two tests that load a non-HTML document, correctly killing the
> >   renderer will be done in later changes.
> >
> > BUG=632361, 649632
> >
> > Committed: https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538
> > Cr-Commit-Position: refs/heads/master@{#427678}
> Committed: https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4
> Cr-Commit-Position: refs/heads/master@{#427974}

TBR=jochen@chromium.org,csharrison@chromium.org,droger@chromium.org,mattcary@...
BUG=632361, 649632
==========

to

==========
Revert "Revert of [NoStatePrefetch] Kill renderer after preload scanning
(patchset #12 id:240001 of https://codereview.chromium.org/2411863002/ )"

Removing the inherently flaky test
NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch and reverting the
following revert:

> Reason for revert:
> "NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch" became flaky
> shortly after this landed.
>
> BUG=659697
> Original issue's description:
> > [NoStatePrefetch] Kill renderer after preload scanning
> >
> > The design doc is linked in the first bug. Main goal: remove Prerender.
> >
> > Currently NoState Prefetch uses the same logic to manage renderers as the
> > PrerenderManager. Prefetch code is still living in PrerenderManager,
separating
> > the code from there is planned after we finalize on how much of the
> > functionality of PrerenderManager is needed.
> >
> > This is the first tweak in the renderer lifetime: the renderer asks to be
killed
> > as soon as the main resource is fully preload-scanned and all possible
> > subresources are requested, special Prerendering FinalStatus is recorded to
allow:
> > * testing
> > * provide a hint that a new prefetch can be started soon
> >
> > Browsertest changes:
> >
> > * TestPrerender now keeps FinalStatus for longer, to be able to verify that
it
> >   is correct even after the TestPrerenderContents is destroyed.
> >
> > * Prefetch browsertests wait only for creation of PrerenderContents,
Prerender
> >   tests have more waiting for page loads on top of that
> >
> > * PrerenderTestUrlImpl is removed to eliminate one hop through protected
method
> >   in the parent class, which also simplified the prefetch side of the tests:
no
> >   need to mention how many page loads to wait for
> >
> > * Disabled two tests that load a non-HTML document, correctly killing the
> >   renderer will be done in later changes.
> >
> > BUG=632361, 649632
> >
> > Committed: https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538
> > Cr-Commit-Position: refs/heads/master@{#427678}
> Committed: https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4
> Cr-Commit-Position: refs/heads/master@{#427974}

TBR=jochen@chromium.org,csharrison@chromium.org,mattcary@chromium.org,mkwst@c...
BUG=632361, 649632
==========

[pasko, 2016-10-27 11:56:58]

droger: please do a sanity check
others: this will be trivial and TBR

[droger, 2016-10-27 11:58:59]

lgtm

Why remove over disable? There is no hope to make it work?

[pasko, 2016-10-27 12:20:00]

On 2016/10/27 11:58:59, droger wrote:
> lgtm
> 
> Why remove over disable? There is no hope to make it work?

Sry, I should have clarified. I think the test is not really adding a lot of
value (having the process in the table is not of big importance to users, and
now we have the FinalStatus to confirm that the prefetch was running).

It seems there is always a chance that the prerender will be too brief for the
task manager to display it. I am not 100% sure what guarantees the
WaitForTaskManagerRows() provides wrt processes created for brief amounts of
time, and I am not sure a strong guarantee is reasonable to maintain. In case
there is a guarantee that process appearance will always reach the TaskManager,
we would need to immediately WaitForTaskManagerRows() without waiting for the
prefetch to finish, which is not a big change in the test.

[pasko, 2016-10-27 12:42:56]

The CQ bit was unchecked by pasko@chromium.org

[pasko, 2016-10-27 12:43:03]

The CQ bit was checked by pasko@chromium.org

[commit-bot: I haz the power, 2016-10-27 12:43:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 13:18:05]

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

[commit-bot: I haz the power, 2016-10-27 13:18:07]

Failed to apply patch for chrome/browser/prerender/prerender_final_status.cc:
While running git apply --index -3 -p1;
  error: patch failed: chrome/browser/prerender/prerender_final_status.cc:12
  Falling back to three-way merge...
  Applied patch to 'chrome/browser/prerender/prerender_final_status.cc' with
conflicts.
  U chrome/browser/prerender/prerender_final_status.cc

Patch:       chrome/browser/prerender/prerender_final_status.cc
Index: chrome/browser/prerender/prerender_final_status.cc
diff --git a/chrome/browser/prerender/prerender_final_status.cc
b/chrome/browser/prerender/prerender_final_status.cc
index
8418e8499356a4d79d8c2ffa825bba8ddc8a3d3a..6ad7e8da0c011bd1ac04778c1aeede2fc811e7c9
100644
--- a/chrome/browser/prerender/prerender_final_status.cc
+++ b/chrome/browser/prerender/prerender_final_status.cc
@@ -12,62 +12,63 @@ namespace prerender {
 namespace {
 
 const char* kFinalStatusNames[] = {
-  "Used",
-  "Timed Out",
-  "Evicted",
-  "Manager Shutdown",
-  "Closed",
-  "Create New Window",
-  "Profile Destroyed",
-  "App Terminating",
-  "Javascript Alert",
-  "Auth Needed",
-  "HTTPS",
-  "Download",
-  "Memory Limit Exceeded",
-  "JS Out Of Memory",
-  "Renderer Unresponsive",
-  "Too many processes",
-  "Rate Limit Exceeded",
-  "Pending Skipped",
-  "Control Group",
-  "HTML5 Media",
-  "Source Render View Closed",
-  "Renderer Crashed",
-  "Unsupported Scheme",
-  "Invalid HTTP Method",
-  "window.print",
-  "Recently Visited",
-  "window.opener",
-  "Page ID Conflict",
-  "Safe Browsing",
-  "Fragment Mismatch",
-  "SSL Client Certificate Requested",
-  "Cache or History Cleared",
-  "Cancelled",
-  "SSL Error",
-  "Cross-Site Navigation Pending",
-  "DevTools Attached To The Tab",
-  "Session Storage Namespace Mismatch",
-  "No Use Group",
-  "Match Complete Dummy",
-  "Duplicate",
-  "OpenURL",
-  "WouldHaveBeenUsed",
-  "Register Protocol Handler",
-  "Creating Audio Stream",
-  "Page Being Captured",
-  "Bad Deferred Redirect",
-  "Navigation Uncommitted",
-  "New Navigation Entry",
-  "Cookie Store Not Loaded",
-  "Cookie Conflict",
-  "Non-Empty Browsing Instance",
-  "Navigation Intercepted",
-  "Prerendering Disabled",
-  "Cellular Network",
-  "Block Third Party Cookies",
-  "Max",
+    "Used",
+    "Timed Out",
+    "Evicted",
+    "Manager Shutdown",
+    "Closed",
+    "Create New Window",
+    "Profile Destroyed",
+    "App Terminating",
+    "Javascript Alert",
+    "Auth Needed",
+    "HTTPS",
+    "Download",
+    "Memory Limit Exceeded",
+    "JS Out Of Memory",
+    "Renderer Unresponsive",
+    "Too many processes",
+    "Rate Limit Exceeded",
+    "Pending Skipped",
+    "Control Group",
+    "HTML5 Media",
+    "Source Render View Closed",
+    "Renderer Crashed",
+    "Unsupported Scheme",
+    "Invalid HTTP Method",
+    "window.print",
+    "Recently Visited",
+    "window.opener",
+    "Page ID Conflict",
+    "Safe Browsing",
+    "Fragment Mismatch",
+    "SSL Client Certificate Requested",
+    "Cache or History Cleared",
+    "Cancelled",
+    "SSL Error",
+    "Cross-Site Navigation Pending",
+    "DevTools Attached To The Tab",
+    "Session Storage Namespace Mismatch",
+    "No Use Group",
+    "Match Complete Dummy",
+    "Duplicate",
+    "OpenURL",
+    "WouldHaveBeenUsed",
+    "Register Protocol Handler",
+    "Creating Audio Stream",
+    "Page Being Captured",
+    "Bad Deferred Redirect",
+    "Navigation Uncommitted",
+    "New Navigation Entry",
+    "Cookie Store Not Loaded",
+    "Cookie Conflict",
+    "Non-Empty Browsing Instance",
+    "Navigation Intercepted",
+    "Prerendering Disabled",
+    "Cellular Network",
+    "Block Third Party Cookies",
+    "NoStatePrefetch Finished",
+    "Max",
 };
 static_assert(arraysize(kFinalStatusNames) == FINAL_STATUS_MAX + 1,
               "status name count mismatch");

[pasko, 2016-10-27 13:28:47]

Description was changed from

==========
Revert "Revert of [NoStatePrefetch] Kill renderer after preload scanning
(patchset #12 id:240001 of https://codereview.chromium.org/2411863002/ )"

Removing the inherently flaky test
NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch and reverting the
following revert:

> Reason for revert:
> "NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch" became flaky
> shortly after this landed.
>
> BUG=659697
> Original issue's description:
> > [NoStatePrefetch] Kill renderer after preload scanning
> >
> > The design doc is linked in the first bug. Main goal: remove Prerender.
> >
> > Currently NoState Prefetch uses the same logic to manage renderers as the
> > PrerenderManager. Prefetch code is still living in PrerenderManager,
separating
> > the code from there is planned after we finalize on how much of the
> > functionality of PrerenderManager is needed.
> >
> > This is the first tweak in the renderer lifetime: the renderer asks to be
killed
> > as soon as the main resource is fully preload-scanned and all possible
> > subresources are requested, special Prerendering FinalStatus is recorded to
allow:
> > * testing
> > * provide a hint that a new prefetch can be started soon
> >
> > Browsertest changes:
> >
> > * TestPrerender now keeps FinalStatus for longer, to be able to verify that
it
> >   is correct even after the TestPrerenderContents is destroyed.
> >
> > * Prefetch browsertests wait only for creation of PrerenderContents,
Prerender
> >   tests have more waiting for page loads on top of that
> >
> > * PrerenderTestUrlImpl is removed to eliminate one hop through protected
method
> >   in the parent class, which also simplified the prefetch side of the tests:
no
> >   need to mention how many page loads to wait for
> >
> > * Disabled two tests that load a non-HTML document, correctly killing the
> >   renderer will be done in later changes.
> >
> > BUG=632361, 649632
> >
> > Committed: https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538
> > Cr-Commit-Position: refs/heads/master@{#427678}
> Committed: https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4
> Cr-Commit-Position: refs/heads/master@{#427974}

TBR=jochen@chromium.org,csharrison@chromium.org,mattcary@chromium.org,mkwst@c...
BUG=632361, 649632
==========

to

==========
Reland: [NoStatePrefetch] Kill renderer after preload scanning

Remove the inherently flaky test
NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch and revert the
following revert:

> Reason for revert:
> "NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch" became flaky
> shortly after this landed.
>
> BUG=659697
> Original issue's description:
> > [NoStatePrefetch] Kill renderer after preload scanning
> >
> > The design doc is linked in the first bug. Main goal: remove Prerender.
> >
> > Currently NoState Prefetch uses the same logic to manage renderers as the
> > PrerenderManager. Prefetch code is still living in PrerenderManager,
separating
> > the code from there is planned after we finalize on how much of the
> > functionality of PrerenderManager is needed.
> >
> > This is the first tweak in the renderer lifetime: the renderer asks to be
killed
> > as soon as the main resource is fully preload-scanned and all possible
> > subresources are requested, special Prerendering FinalStatus is recorded to
allow:
> > * testing
> > * provide a hint that a new prefetch can be started soon
> >
> > Browsertest changes:
> >
> > * TestPrerender now keeps FinalStatus for longer, to be able to verify that
it
> >   is correct even after the TestPrerenderContents is destroyed.
> >
> > * Prefetch browsertests wait only for creation of PrerenderContents,
Prerender
> >   tests have more waiting for page loads on top of that
> >
> > * PrerenderTestUrlImpl is removed to eliminate one hop through protected
method
> >   in the parent class, which also simplified the prefetch side of the tests:
no
> >   need to mention how many page loads to wait for
> >
> > * Disabled two tests that load a non-HTML document, correctly killing the
> >   renderer will be done in later changes.
> >
> > BUG=632361, 649632
> >
> > Committed: https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538
> > Cr-Commit-Position: refs/heads/master@{#427678}
> Committed: https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4
> Cr-Commit-Position: refs/heads/master@{#427974}

TBR=jochen@chromium.org,csharrison@chromium.org,mattcary@chromium.org,mkwst@c...
BUG=632361, 649632
==========

[pasko, 2016-10-27 13:29:58]

The CQ bit was checked by pasko@chromium.org

[pasko, 2016-10-27 13:29:58]

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

[commit-bot: I haz the power, 2016-10-27 13:30:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 15:03:05]

Description was changed from

==========
Reland: [NoStatePrefetch] Kill renderer after preload scanning

Remove the inherently flaky test
NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch and revert the
following revert:

> Reason for revert:
> "NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch" became flaky
> shortly after this landed.
>
> BUG=659697
> Original issue's description:
> > [NoStatePrefetch] Kill renderer after preload scanning
> >
> > The design doc is linked in the first bug. Main goal: remove Prerender.
> >
> > Currently NoState Prefetch uses the same logic to manage renderers as the
> > PrerenderManager. Prefetch code is still living in PrerenderManager,
separating
> > the code from there is planned after we finalize on how much of the
> > functionality of PrerenderManager is needed.
> >
> > This is the first tweak in the renderer lifetime: the renderer asks to be
killed
> > as soon as the main resource is fully preload-scanned and all possible
> > subresources are requested, special Prerendering FinalStatus is recorded to
allow:
> > * testing
> > * provide a hint that a new prefetch can be started soon
> >
> > Browsertest changes:
> >
> > * TestPrerender now keeps FinalStatus for longer, to be able to verify that
it
> >   is correct even after the TestPrerenderContents is destroyed.
> >
> > * Prefetch browsertests wait only for creation of PrerenderContents,
Prerender
> >   tests have more waiting for page loads on top of that
> >
> > * PrerenderTestUrlImpl is removed to eliminate one hop through protected
method
> >   in the parent class, which also simplified the prefetch side of the tests:
no
> >   need to mention how many page loads to wait for
> >
> > * Disabled two tests that load a non-HTML document, correctly killing the
> >   renderer will be done in later changes.
> >
> > BUG=632361, 649632
> >
> > Committed: https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538
> > Cr-Commit-Position: refs/heads/master@{#427678}
> Committed: https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4
> Cr-Commit-Position: refs/heads/master@{#427974}

TBR=jochen@chromium.org,csharrison@chromium.org,mattcary@chromium.org,mkwst@c...
BUG=632361, 649632
==========

to

==========
Reland: [NoStatePrefetch] Kill renderer after preload scanning

Remove the inherently flaky test
NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch and revert the
following revert:

> Reason for revert:
> "NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch" became flaky
> shortly after this landed.
>
> BUG=659697
> Original issue's description:
> > [NoStatePrefetch] Kill renderer after preload scanning
> >
> > The design doc is linked in the first bug. Main goal: remove Prerender.
> >
> > Currently NoState Prefetch uses the same logic to manage renderers as the
> > PrerenderManager. Prefetch code is still living in PrerenderManager,
separating
> > the code from there is planned after we finalize on how much of the
> > functionality of PrerenderManager is needed.
> >
> > This is the first tweak in the renderer lifetime: the renderer asks to be
killed
> > as soon as the main resource is fully preload-scanned and all possible
> > subresources are requested, special Prerendering FinalStatus is recorded to
allow:
> > * testing
> > * provide a hint that a new prefetch can be started soon
> >
> > Browsertest changes:
> >
> > * TestPrerender now keeps FinalStatus for longer, to be able to verify that
it
> >   is correct even after the TestPrerenderContents is destroyed.
> >
> > * Prefetch browsertests wait only for creation of PrerenderContents,
Prerender
> >   tests have more waiting for page loads on top of that
> >
> > * PrerenderTestUrlImpl is removed to eliminate one hop through protected
method
> >   in the parent class, which also simplified the prefetch side of the tests:
no
> >   need to mention how many page loads to wait for
> >
> > * Disabled two tests that load a non-HTML document, correctly killing the
> >   renderer will be done in later changes.
> >
> > BUG=632361, 649632
> >
> > Committed: https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538
> > Cr-Commit-Position: refs/heads/master@{#427678}
> Committed: https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4
> Cr-Commit-Position: refs/heads/master@{#427974}

TBR=jochen@chromium.org,csharrison@chromium.org,mattcary@chromium.org,mkwst@c...
BUG=632361, 649632

Committed: https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d
Cr-Commit-Position: refs/heads/master@{#428027}
==========

[commit-bot: I haz the power, 2016-10-27 15:03:07]

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

[commit-bot: I haz the power, 2016-10-27 15:03:13]

Failed to apply the patch.
On branch working_branch
Your branch is up-to-date with 'origin/refs/pending/heads/master'.
nothing to commit, working tree clean