Prerender: NoStatePrefetch browser side changes.

Prerender: NoStatePrefetch browser side changes.

Enables NoState Prefetch through the browser via a command line flag.

This is a no-op until droger@'s 2172613002 goes in, hence no tests are currently affected.

BUG=631930
Committed: https://crrev.com/d4a8c77fcc1ba0fdf2b4c381b80318a2f25fc062
Cr-Commit-Position: refs/heads/master@{#413776}

[mattcary, 2016-07-27 08:35:07]

Description was changed from

==========
Prerender: NoStatePrefetch browser side changes.

BUG=631930
==========

to

==========
Prerender: NoStatePrefetch browser side changes.

BUG=631930
==========

[mattcary, 2016-08-04 14:36:42]

Description was changed from

==========
Prerender: NoStatePrefetch browser side changes.

BUG=631930
==========

to

==========
Prerender: NoStatePrefetch browser side changes.

Enables NoState Prefetch through the browser via a command line flag.

BUG=631930
==========

[mattcary, 2016-08-04 14:37:23]

mattcary@chromium.org changed reviewers:
+ droger@chromium.org, pasko@chromium.org

[mattcary, 2016-08-04 14:37:24]

Browser-side nostate prefetch changes.

Note this currently fails testing on the following assert
    DCHECK(!document.isPrefetchOnly() || !shouldUseThreading());

on line 133 in HTMLDocumentParser.cpp.

[droger, 2016-08-04 15:09:43]

Looking good so far. I'll do another pass once the test is fixed and the CL is
rebased on top of the IPC change.

[mattcary, 2016-08-05 07:56:35]

On 2016/08/04 15:09:43, droger wrote:
> Looking good so far. I'll do another pass once the test is fixed and the CL is
> rebased on top of the IPC change.

Ack, I rebased but neglected to upload, sorry.

[mattcary, 2016-08-05 08:45:58]

On 2016/08/04 15:09:43, droger wrote:
> Looking good so far. I'll do another pass once the test is fixed and the CL is
> rebased on top of the IPC change.

So it looks like the issue is that the parser is created
(Document::createParser) before  ChromeRenderFrameObserver::OnSetIsPrerendering
is called. Note that PrerenderContents sends the SetIsPrerendering message
before the parser is created (PrerenderContents::RenderFrameCreated). So this
might be a race? The prerender_browser test may end up calling things in a
different order than a browser instance.

I'm continuing to dig, but do you have any ideas?

[mattcary, 2016-08-05 14:31:46]

So here is a suggestion to the problem I was seeing. The issue seems to have
been that the test, for whatever reason, involved a TextDocument rather than an
HTML document, and force synchronous was only being done in the case of an
HTMLDocument. I've moved it to Document::implicitOpen here; note that when
merged with David's change, his logic in Document::createParser will be
redundant.

The fact that the test is using a TextDocument may be a problem in the test, but
maybe a fortuitous one.

[droger, 2016-08-05 14:43:31]

On 2016/08/05 14:31:46, mattcary wrote:
> So here is a suggestion to the problem I was seeing. The issue seems to have
> been that the test, for whatever reason, involved a TextDocument rather than
an
> HTML document, and force synchronous was only being done in the case of an
> HTMLDocument. I've moved it to Document::implicitOpen here; note that when
> merged with David's change, his logic in Document::createParser will be
> redundant.
> 
> The fact that the test is using a TextDocument may be a problem in the test,
but
> maybe a fortuitous one.

I agree that it looks like a good approach. In particular, in my CL, there is a
discrepency between the value of m_parserSyncPolicy and the actual policy that
is used.
Your solution solves this.

Should I update my CL with this new code?

[mattcary, 2016-08-05 14:44:10]

On 2016/08/05 14:43:31, droger wrote:
> On 2016/08/05 14:31:46, mattcary wrote:
> > So here is a suggestion to the problem I was seeing. The issue seems to have
> > been that the test, for whatever reason, involved a TextDocument rather than
> an
> > HTML document, and force synchronous was only being done in the case of an
> > HTMLDocument. I've moved it to Document::implicitOpen here; note that when
> > merged with David's change, his logic in Document::createParser will be
> > redundant.
> > 
> > The fact that the test is using a TextDocument may be a problem in the test,
> but
> > maybe a fortuitous one.
> 
> I agree that it looks like a good approach. In particular, in my CL, there is
a
> discrepency between the value of m_parserSyncPolicy and the actual policy that
> is used.
> Your solution solves this.
> 
> Should I update my CL with this new code?

SGTM!

[droger, 2016-08-16 10:01:55]

lgtm

https://codereview.chromium.org/2184963002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2184963002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.cc:222:
DCHECK(!prerendering_has_started_);
Optional:
To remove the need for the DCHECK and the comment, pass the prerender mode to
the constructor, and remove the setter.

https://codereview.chromium.org/2184963002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2184963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:2435: } else if
(parserSyncPolicy == AllowAsynchronousParsing && isPrefetchOnly()) {
This probably belongs to another CL.

[mattcary, 2016-08-17 20:27:32]

https://codereview.chromium.org/2184963002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2184963002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.cc:222:
DCHECK(!prerendering_has_started_);
On 2016/08/16 10:01:54, droger wrote:
> Optional:
> To remove the need for the DCHECK and the comment, pass the prerender mode to
> the constructor, and remove the setter.

That's not a bad idea, but I think that would end up bloating the API. It would
necessitate widening PrerenderManager::CreatePrerenderContents which is
overridden in tests, etc, and so seems like it would cause longer-term
maintenance headaches.

https://codereview.chromium.org/2184963002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2184963002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:2435: } else if
(parserSyncPolicy == AllowAsynchronousParsing && isPrefetchOnly()) {
On 2016/08/16 10:01:54, droger wrote:
> This probably belongs to another CL.

Yes, I think this leaked over from your CL. Removed.

[mattcary, 2016-08-17 20:28:31]

Egor and/or David---

Any suggestions for proper owner review?

[pasko, 2016-08-18 10:00:49]

On 2016/08/17 20:28:31, mattcary wrote:
> Egor and/or David---
> 
> Any suggestions for proper owner review?

mmenke@, after the tests pass. I did not see changes addressing the tests, so I
auto-assume that they are failing, let me know if it is not the case.

[chromium-reviews, 2016-08-20 01:20:31]

Thanks. I'll double-check, but I think the tests all pass as they don't
exercise the new code path.

I have some new tests (partially at this point) written once David's CL
goes in. But this change, by itself, is a no-op until David's CL goes.

On Aug 18, 2016 5:00 AM, <pasko@chromium.org> wrote:

> On 2016/08/17 20:28:31, mattcary wrote:
> > Egor and/or David---
> >
> > Any suggestions for proper owner review?
>
> mmenke@, after the tests pass. I did not see changes addressing the
> tests, so I
> auto-assume that they are failing, let me know if it is not the case.
>
> https://codereview.chromium.org/2184963002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pasko, 2016-08-22 09:47:05]

OK, if tests pass, let's land it sooner, since we are experimenting on top of
this patch already. If we need to postpone new tests to land this earlier, let's
prefer to land and add tests later?

[mattcary, 2016-08-22 13:43:10]

On 2016/08/22 09:47:05, pasko wrote:
> OK, if tests pass, let's land it sooner, since we are experimenting on top of
> this patch already. If we need to postpone new tests to land this earlier,
let's
> prefer to land and add tests later?

All prerender unittests and browser tests pass. I'll pass this along to mmenke@.

[mattcary, 2016-08-22 13:44:14]

Description was changed from

==========
Prerender: NoStatePrefetch browser side changes.

Enables NoState Prefetch through the browser via a command line flag.

BUG=631930
==========

to

==========
Prerender: NoStatePrefetch browser side changes.

Enables NoState Prefetch through the browser via a command line flag.

This is a no-op until droger@'s 2172613002 goes in, hence no tests are currently
affected.

BUG=631930
==========

[mattcary, 2016-08-22 13:44:37]

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

[mattcary, 2016-08-22 13:45:24]

Matt---

As owner can you review this change?

Thanks

[mmenke, 2016-08-22 14:17:58]

https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.cc:522:
render_frame_host->GetRoutingID(), prerender_mode_));
Is this hooked up, renderer-side yet?  If so, we should have a browser test for
it, I think.

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

https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:326: }
nit:  Remove braces

https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:580: return "_NoStatePrefetch";
Need to update histograms.xml for this addition.

https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:691: WebContents* contents =
prerender->contents()->prerender_contents();
So we do create a WebContents in the non-full prerender case, but we don't
return it here?  Why not?

If this is important behavior, should have a test for it.

https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:955: }
nit:  Remove braces

[mattcary, 2016-08-23 10:53:56]

https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.cc:522:
render_frame_host->GetRoutingID(), prerender_mode_));
On 2016/08/22 14:17:58, mmenke wrote:
> Is this hooked up, renderer-side yet?  If so, we should have a browser test
for
> it, I think.

No, that's in droger@'s cl. I'm working on the browser tests right now in
parallel :)

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

https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:326: }
On 2016/08/22 14:17:58, mmenke wrote:
> nit:  Remove braces

Done.

https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:580: return "_NoStatePrefetch";
On 2016/08/22 14:17:58, mmenke wrote:
> Need to update histograms.xml for this addition.

Done.

I see that the PrerenderMode histogram in histograms.xml was marked deprecated
in 16 Nov 2012. Should I un-deprecate or do anything else?

https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:691: WebContents* contents =
prerender->contents()->prerender_contents();
On 2016/08/22 14:17:58, mmenke wrote:
> So we do create a WebContents in the non-full prerender case, but we don't
> return it here?  Why not?
> 
> If this is important behavior, should have a test for it.

AFAIK, we will have a WebContents for the prefetching render because we will
have a renderer process for actually loading & preload scanning the main
resource. But we never want to swap into this renderer, because no renderering
has been done.

The only non-test use of GetAllPrerenderingContents is in tab_android.cc where
by context we're only interested in prerenders that have finished loading. We
won't want a prefetching prerenderer to appear in that case so making the
semantics of GetAllPrerenderingContents return only full prerenderers seems like
the right things to do.

There is a test use of this ftn in extensions/activity_log that looks like it
will probably break when/if full prerendering is deprecated (as it expects an
omnibox-initiated prerender appear in the result of this ftn). If that happens,
though, we will probably want to remove or change all uses of this ftn anyway as
prerendering will mean something quite different from what it used to.

I looked into adding a test to prerender_unittests.cc, but
GetAllPrerenderingContents requires WebContent instances, not just
PrerenderContents, and creating one of those in a unittest context seems
impractical (I played around with WebContentsTester and TestBrowserContext, but
it appears the TestBrowserContext doesn't do enough for the TestWebContents to
init with seg faulting).

https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:955: }
On 2016/08/22 14:17:58, mmenke wrote:
> nit:  Remove braces

Done.

[mmenke, 2016-08-23 14:35:27]

LGTM

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

https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:580: return "_NoStatePrefetch";
On 2016/08/23 10:53:56, mattcary wrote:
> On 2016/08/22 14:17:58, mmenke wrote:
> > Need to update histograms.xml for this addition.
> 
> Done.
> 
> I see that the PrerenderMode histogram in histograms.xml was marked deprecated
> in 16 Nov 2012. Should I un-deprecate or do anything else?

Hrm...you're right.  I'd say no.  Looks like a lot of the prerender histograms
(Including all that use this method...) should just be removed.

[mattcary, 2016-08-23 15:51:11]

On 2016/08/23 14:35:27, mmenke wrote:
> LGTM
> 
>
https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
> File chrome/browser/prerender/prerender_manager.cc (right):
> 
>
https://codereview.chromium.org/2184963002/diff/140001/chrome/browser/prerend...
> chrome/browser/prerender/prerender_manager.cc:580: return "_NoStatePrefetch";
> On 2016/08/23 10:53:56, mattcary wrote:
> > On 2016/08/22 14:17:58, mmenke wrote:
> > > Need to update histograms.xml for this addition.
> > 
> > Done.
> > 
> > I see that the PrerenderMode histogram in histograms.xml was marked
deprecated
> > in 16 Nov 2012. Should I un-deprecate or do anything else?
> 
> Hrm...you're right.  I'd say no.  Looks like a lot of the prerender histograms
> (Including all that use this method...) should just be removed.

Thanks. After discussing with Egor I'll just tear out those histograms in a
separate CL.

[mattcary, 2016-08-23 15:56:23]

The CQ bit was checked by mattcary@chromium.org

[mattcary, 2016-08-23 15:56:23]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2184963002/#ps180001
(title: "remove histogram change as they will all be going away.")

[commit-bot: I haz the power, 2016-08-23 15:56:51]

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-08-23 17:33:02]

Description was changed from

==========
Prerender: NoStatePrefetch browser side changes.

Enables NoState Prefetch through the browser via a command line flag.

This is a no-op until droger@'s 2172613002 goes in, hence no tests are currently
affected.

BUG=631930
==========

to

==========
Prerender: NoStatePrefetch browser side changes.

Enables NoState Prefetch through the browser via a command line flag.

This is a no-op until droger@'s 2172613002 goes in, hence no tests are currently
affected.

BUG=631930
==========

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

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-08-23 17:36:29]

Description was changed from

==========
Prerender: NoStatePrefetch browser side changes.

Enables NoState Prefetch through the browser via a command line flag.

This is a no-op until droger@'s 2172613002 goes in, hence no tests are currently
affected.

BUG=631930
==========

to

==========
Prerender: NoStatePrefetch browser side changes.

Enables NoState Prefetch through the browser via a command line flag.

This is a no-op until droger@'s 2172613002 goes in, hence no tests are currently
affected.

BUG=631930

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

[commit-bot: I haz the power, 2016-08-23 17:36:30]

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