Renderer-side changes for NoState Prefetch

Renderer-side changes for NoState Prefetch

NoState Prefetch design doc:
https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5EhU

This CL is the first step for NoState Prefetch implementation
in the renderer.

The code added in this CL is not called yet (it will be called
only when the PREFETCH_ONLY enum is sent through IPC).

The prefetching is implemented in the HTMLDocumentParser.
When prefetching, the parser runs in single-thread mode.
When feeded with HTML input, it runs the preload scanner on
it, and stops without actually building or executing the page.

BUG=632361
Committed: https://crrev.com/04f065d8f4417c062ec53cbc7d27ed571e6736b2
Cr-Commit-Position: refs/heads/master@{#414406}

[droger, 2016-07-21 13:33:25]

Patchset #1 (id:1) has been deleted

[droger, 2016-07-22 13:39:18]

Description was changed from

==========
NoStatePrefetch early prototype
==========

to

==========
NoStatePrefetch early prototype

The background HTML parser collects the URLs to preload and
sends them to the HTMLDocumentParser.

The HTMLDocumentParser makes the actual requests, and drops
everything else on the floor, preventing the actual
construction and execution of the page.
==========

[droger, 2016-07-22 13:41:36]

Description was changed from

==========
NoStatePrefetch early prototype

The background HTML parser collects the URLs to preload and
sends them to the HTMLDocumentParser.

The HTMLDocumentParser makes the actual requests, and drops
everything else on the floor, preventing the actual
construction and execution of the page.
==========

to

==========
NoStatePrefetch early prototype

The background HTML parser collects the URLs to preload and
sends them to the HTMLDocumentParser.

The HTMLDocumentParser makes the actual requests, and drops
everything else on the floor, preventing the actual
construction and execution of the page.

Uses the PageVisibilityState to control this behavior.
==========

[droger, 2016-07-22 13:42:56]

lizeb, mattcary, pasko: FYI this is what I'm experimenting with for prefetching
on the renderer side.

[droger, 2016-07-22 13:43:27]

Description was changed from

==========
NoStatePrefetch early prototype

The background HTML parser collects the URLs to preload and
sends them to the HTMLDocumentParser.

The HTMLDocumentParser makes the actual requests, and drops
everything else on the floor, preventing the actual
construction and execution of the page.

Uses the PageVisibilityState to control this behavior.
==========

to

==========
NoStatePrefetch early prototype

The background HTML parser collects the URLs to preload and
sends them to the HTMLDocumentParser.

The HTMLDocumentParser makes the actual prefetching
requests, and drops everything else on the floor,
preventing the actual construction and execution of the
page.

Uses the PageVisibilityState to control this behavior.
==========

[droger, 2016-07-25 14:33:18]

Using the PageVisibilityState is probably not the right way to know if we're
doing a prefetch.
In particular this enum is part of the web platform, and adding a new value to
it is probably not easy and not what we want.

I'm trying to figure out the plumbing, how we could distinguish between a
prefetch and a renderer.

[droger, 2016-07-25 14:34:29]

On 2016/07/25 14:33:18, droger wrote:
> prefetch and a renderer.

Oops, should be:

prefetch and a prerender.

[blink-reviews, 2016-07-25 14:40:57]

OK, I just started looking into this as well.

Good to know that adding the enum to PageVisibilityState isn't practical,
that was one of my alternatives.

On Mon, Jul 25, 2016 at 4:34 PM, <droger@chromium.org> wrote:

> On 2016/07/25 14:33:18, droger wrote:
> > prefetch and a renderer.
>
> Oops, should be:
>
> prefetch and a prerender.
>
>
>
> https://codereview.chromium.org/2172613002/
>

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

[chromium-reviews, 2016-07-25 14:41:04]

OK, I just started looking into this as well.

Good to know that adding the enum to PageVisibilityState isn't practical,
that was one of my alternatives.

On Mon, Jul 25, 2016 at 4:34 PM, <droger@chromium.org> wrote:

> On 2016/07/25 14:33:18, droger wrote:
> > prefetch and a renderer.
>
> Oops, should be:
>
> prefetch and a prerender.
>
>
>
> https://codereview.chromium.org/2172613002/
>

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

[droger, 2016-07-26 11:34:19]

Added some more plumbing to get the prefetching bit from the prerender helper

[droger, 2016-07-27 14:23:41]

Finished plumbing to the IPC.

As per offline discussion, it may be possible to avoid the change to the
Document API by fiddling with the many various Prerender clients and helpers,
but it requires at least a static cast (or two) and almost certainly exposing
more implementation details.

[droger, 2016-07-27 15:01:08]

Description was changed from

==========
NoStatePrefetch early prototype

The background HTML parser collects the URLs to preload and
sends them to the HTMLDocumentParser.

The HTMLDocumentParser makes the actual prefetching
requests, and drops everything else on the floor,
preventing the actual construction and execution of the
page.

Uses the PageVisibilityState to control this behavior.
==========

to

==========
NoStatePrefetch early prototype

The background HTML parser collects the URLs to preload and
sends them to the HTMLDocumentParser.

The HTMLDocumentParser makes the actual prefetching
requests, and drops everything else on the floor,
preventing the actual construction and execution of the
page.
==========

[droger, 2016-07-27 15:01:35]

+CC csharrison

[Charlie Harrison, 2016-07-27 15:27:38]

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

[Charlie Harrison, 2016-07-27 15:27:39]

+kouhei

Hm I haven't taken a long look at this but it seems like we could do something
simpler? Using HTMLDocumentParser in full seems overkill, especially if we
aren't actually parsing / running scripts / etc.

I wonder if you can turn off the threaded parser and just use HTMLPreloadScanner
strings from HTMLDocumentParser::append. This avoids doing unnecessary
multithreading / XSS scanning that the BackgroundHTMLParser does. There's a ton
of overhead there.

[pasko, 2016-07-27 16:17:46]

On 2016/07/27 15:27:39, csharrison wrote:
> +kouhei
> 
> Hm I haven't taken a long look at this but it seems like we could do something
> simpler? Using HTMLDocumentParser in full seems overkill, especially if we
> aren't actually parsing / running scripts / etc.
> 
> I wonder if you can turn off the threaded parser and just use
HTMLPreloadScanner
> strings from HTMLDocumentParser::append. This avoids doing unnecessary
> multithreading / XSS scanning that the BackgroundHTMLParser does. There's a
ton
> of overhead there.

Thanks for input!

Yeah, I think we should get to something of this sort. Though maybe slightly
later. The overhead is arguably not on the critical path, we'll be able to
optimize it later. The aim of this patch is to make a quick broken prototype (no
security checks, etc.), and piggybacking on BackgroundHTMLParser seems indeed
easier to cook.

[Charlie Harrison, 2016-07-27 16:35:59]

On 2016/07/27 16:17:46, pasko wrote:
> On 2016/07/27 15:27:39, csharrison wrote:
> > +kouhei
> > 
> > Hm I haven't taken a long look at this but it seems like we could do
something
> > simpler? Using HTMLDocumentParser in full seems overkill, especially if we
> > aren't actually parsing / running scripts / etc.
> > 
> > I wonder if you can turn off the threaded parser and just use
> HTMLPreloadScanner
> > strings from HTMLDocumentParser::append. This avoids doing unnecessary
> > multithreading / XSS scanning that the BackgroundHTMLParser does. There's a
> ton
> > of overhead there.
> 
> Thanks for input!
> 
> Yeah, I think we should get to something of this sort. Though maybe slightly
> later. The overhead is arguably not on the critical path, we'll be able to
> optimize it later. The aim of this patch is to make a quick broken prototype
(no
> security checks, etc.), and piggybacking on BackgroundHTMLParser seems indeed
> easier to cook.

Sure, whatever works. Though I think the suggestion would not necessarily be
more complicated.

For a prototype, the simplest implementation SGTM.

[droger, 2016-07-27 16:38:59]

On 2016/07/27 15:27:39, csharrison wrote:
> +kouhei
> 
> Hm I haven't taken a long look at this but it seems like we could do something
> simpler? Using HTMLDocumentParser in full seems overkill, especially if we
> aren't actually parsing / running scripts / etc.
> 
> I wonder if you can turn off the threaded parser and just use
HTMLPreloadScanner
> strings from HTMLDocumentParser::append. This avoids doing unnecessary
> multithreading / XSS scanning that the BackgroundHTMLParser does. There's a
ton
> of overhead there.

Thanks. I'll look into this and see how it goes!

[droger, 2016-07-28 13:24:18]

Patchset #6 (id:120001) has been deleted

[droger, 2016-07-28 13:26:08]

On 2016/07/27 16:35:59, csharrison wrote:
> 
> Sure, whatever works. Though I think the suggestion would not necessarily be
> more complicated.
> 

I uploaded a new version (patch 6) implementing the suggestion, let me know what
you think!

[droger, 2016-07-28 13:28:16]

Patchset #6 (id:140001) has been deleted

[Charlie Harrison, 2016-07-28 14:11:01]

On 2016/07/28 13:26:08, droger wrote:
> On 2016/07/27 16:35:59, csharrison wrote:
> > 
> > Sure, whatever works. Though I think the suggestion would not necessarily be
> > more complicated.
> > 
> 
> I uploaded a new version (patch 6) implementing the suggestion, let me know
what
> you think!

Yes! This is exactly what I was thinking! Thanks for implementing it :)

What is the plan with this CL? Do you want a full review?

[droger, 2016-07-28 16:09:33]

On 2016/07/28 14:11:01, csharrison wrote:
> On 2016/07/28 13:26:08, droger wrote:
> > On 2016/07/27 16:35:59, csharrison wrote:
> > > 
> > > Sure, whatever works. Though I think the suggestion would not necessarily
be
> > > more complicated.
> > > 
> > 
> > I uploaded a new version (patch 6) implementing the suggestion, let me know
> what
> > you think!
> 
> Yes! This is exactly what I was thinking! Thanks for implementing it :)
> 
> What is the plan with this CL? Do you want a full review?

Yes, the plan is to implement the feature, so you can do a full review.
This code path will not be used yet, as we also need to make corresponding
changes on the browser side.

I'm not familiar with webkit, but maybe I should consider writing some tests?

Thanks

[Charlie Harrison, 2016-07-28 16:25:39]

On 2016/07/28 16:09:33, droger wrote:
> On 2016/07/28 14:11:01, csharrison wrote:
> > On 2016/07/28 13:26:08, droger wrote:
> > > On 2016/07/27 16:35:59, csharrison wrote:
> > > > 
> > > > Sure, whatever works. Though I think the suggestion would not
necessarily
> be
> > > > more complicated.
> > > > 
> > > 
> > > I uploaded a new version (patch 6) implementing the suggestion, let me
know
> > what
> > > you think!
> > 
> > Yes! This is exactly what I was thinking! Thanks for implementing it :)
> > 
> > What is the plan with this CL? Do you want a full review?
> 
> Yes, the plan is to implement the feature, so you can do a full review.
> This code path will not be used yet, as we also need to make corresponding
> changes on the browser side.
> 
> I'm not familiar with webkit, but maybe I should consider writing some tests?
> 
> Thanks

Okay cool. I am not an owner, so I will review in addition to another Blink
owner (I've cced Kouhei, who is an owner).
Tests would be good. Unit tests are pretty easy in Blink, though they are not as
common as they should be. Browser tests would also work if that's doable with
the current code.

Would you mind expanding on the CL description and say how this is all being
hooked up?

[droger, 2016-08-02 10:05:01]

Description was changed from

==========
NoStatePrefetch early prototype

The background HTML parser collects the URLs to preload and
sends them to the HTMLDocumentParser.

The HTMLDocumentParser makes the actual prefetching
requests, and drops everything else on the floor,
preventing the actual construction and execution of the
page.
==========

to

==========
Renderer-side changes for NoState Prefetch

NoState Prefetch designe doc:
https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5EhU

This CL is the first step for NoState Prefetch implementation
in the renderer.

The code added in this CL is not called yet (it will be called
only when the PREFETCH_ONLY enum is sent through IPC).

The prefetching is implemented in the HTMLDocumentParser.
When prefetching, the parser runs in single-thread mode.
When feeded with HTML input, it runs the preload scanner on
it, and stops without actually building or executing the page.
==========

[droger, 2016-08-02 11:55:07]

Description was changed from

==========
Renderer-side changes for NoState Prefetch

NoState Prefetch designe doc:
https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5EhU

This CL is the first step for NoState Prefetch implementation
in the renderer.

The code added in this CL is not called yet (it will be called
only when the PREFETCH_ONLY enum is sent through IPC).

The prefetching is implemented in the HTMLDocumentParser.
When prefetching, the parser runs in single-thread mode.
When feeded with HTML input, it runs the preload scanner on
it, and stops without actually building or executing the page.
==========

to

==========
Renderer-side changes for NoState Prefetch

NoState Prefetch design doc:
https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5EhU

This CL is the first step for NoState Prefetch implementation
in the renderer.

The code added in this CL is not called yet (it will be called
only when the PREFETCH_ONLY enum is sent through IPC).

The prefetching is implemented in the HTMLDocumentParser.
When prefetching, the parser runs in single-thread mode.
When feeded with HTML input, it runs the preload scanner on
it, and stops without actually building or executing the page.
==========

[droger, 2016-08-03 13:56:17]

Patchset #7 (id:180001) has been deleted

[droger, 2016-08-03 13:57:08]

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

[droger, 2016-08-03 13:57:21]

Added unit tests. Please take a look.

[commit-bot: I haz the power, 2016-08-03 13:57:31]

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

[Charlie Harrison, 2016-08-03 14:40:04]

Thanks! cc shivanisha@ for the "fire and forget" question.

It would be nice to also see (potentially in a follow up):
 - A browser test that successfully observes requests going through the network
stack.
 - A test that confirms that we aren't actually parsing the document.

https://codereview.chromium.org/2172613002/diff/200001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2172613002/diff/200001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:489: RenderFrame* main_frame =
Optional: It might make sense to hide some of this logic into
PrerendererHelper's constructor (i.e. getting the mode) to simplify construction
sites.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:355: bool isPrefetchOnly() const;
Please document this method, especially since "Prefetch" is such an overloaded
term.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:93:
HTMLDocumentParser* HTMLDocumentParser::create(
I have a slight preference for passing the sync policy from the callsite,
instead of reaching into the document in threadingAllowedForDocument.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:727:
m_insertionPreloadScanner->scanAndPreload(
nit: Unnecessary line break.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:814:
m_preloadScanner->scanAndPreload(m_preloader.get(),
document()->validBaseElementURL(), nullptr);
This just preloads the tokens normally. Do we want to add load flags like
LOAD_PREFETCH so all resources are forced into the HTTP cache?

Also, if we aren't using this render process for the final page load, this will
bloat the MemoryCache with in-memory resources that will never be used.

+shivanisha@ is thinking of implementing a "fire-and-forget" system in Blink
that will send requests but ignore results (make sure they don't land in the
memory cache). It might make sense to collaborate on this.

(note: I understand this is just a prototype, I just want to understand what the
final design will look like.)

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:1066: if
(!length || isStopped())
Can you DCHECK(!document()->isPrefetchOnly()) here?

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:82: // Exposed
for testing.
Append "ForTesting" to the end of methods that should only be used for testing.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp
(right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:18: 
So, it is against Blink style to indent in a nested namespace. This is really
confusing because I think clang format does this sometimes. Hopefully this will
all be fixed once chromium style is merged... :(

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:99:
EXPECT_TRUE(scriptRunnerHost->hasPreloadScanner());
might make more sense to be checking m_haveBackgroundParser. hasPreloadScanner()
is less clear.

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

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

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

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[droger, 2016-08-03 16:45:50]

Thanks for the review!

Re: adding a test to check that the document is not parsed.
This is what I was intending to test by checking the test of the tokenizer in
the unittest. Were you thinking of another way of testing this?

https://codereview.chromium.org/2172613002/diff/200001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2172613002/diff/200001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:489: RenderFrame* main_frame =
On 2016/08/03 14:40:03, csharrison wrote:
> Optional: It might make sense to hide some of this logic into
> PrerendererHelper's constructor (i.e. getting the mode) to simplify
construction
> sites.

Done.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:355: bool isPrefetchOnly() const;
On 2016/08/03 14:40:03, csharrison wrote:
> Please document this method, especially since "Prefetch" is such an overloaded
> term.

Done.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:93:
HTMLDocumentParser* HTMLDocumentParser::create(
On 2016/08/03 14:40:03, csharrison wrote:
> I have a slight preference for passing the sync policy from the callsite,
> instead of reaching into the document in threadingAllowedForDocument.

Done.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:727:
m_insertionPreloadScanner->scanAndPreload(
On 2016/08/03 14:40:03, csharrison wrote:
> nit: Unnecessary line break.

Done.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:814:
m_preloadScanner->scanAndPreload(m_preloader.get(),
document()->validBaseElementURL(), nullptr);
On 2016/08/03 14:40:03, csharrison wrote:
> This just preloads the tokens normally. Do we want to add load flags like
> LOAD_PREFETCH so all resources are forced into the HTTP cache?
> 

This uses the same code as the standard preload scanner. I would think that it
uses the LOAD_PREFETCH flags accordingly, and (at first at least) we want to
have the same behavior for prefetch.

> Also, if we aren't using this render process for the final page load, this
will
> bloat the MemoryCache with in-memory resources that will never be used.
> 

Yes, this is a good point. Disabling the memory cache would probably be nice.

> +shivanisha@ is thinking of implementing a "fire-and-forget" system in Blink
> that will send requests but ignore results (make sure they don't land in the
> memory cache). It might make sense to collaborate on this.
> 
> (note: I understand this is just a prototype, I just want to understand what
the
> final design will look like.)

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:1066: if
(!length || isStopped())
On 2016/08/03 14:40:03, csharrison wrote:
> Can you DCHECK(!document()->isPrefetchOnly()) here?

Why? Is this because isStopped() should never be true when prefetching?

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp
(right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:18: 
On 2016/08/03 14:40:04, csharrison wrote:
> So, it is against Blink style to indent in a nested namespace. This is really
> confusing because I think clang format does this sometimes. Hopefully this
will
> all be fixed once chromium style is merged... :(

Yes, `git cl format` did this.
Done.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:99:
EXPECT_TRUE(scriptRunnerHost->hasPreloadScanner());
On 2016/08/03 14:40:04, csharrison wrote:
> might make more sense to be checking m_haveBackgroundParser.
hasPreloadScanner()
> is less clear.

Why checking the background parser? Since we are in synchronous mode, there
should not be one.

I'm checking the hasPreloadScanner as a way to check that prefetching has been
done.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:100:
EXPECT_EQ(HTMLTokenizer::DataState, parser->tokenizer()->getState());
This is intended to check that the page has is not being parsed (as DataState
seems to be the default idle state for the Tokenizer).

[Charlie Harrison, 2016-08-03 17:15:15]

Thanks for the clarification!

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:814:
m_preloadScanner->scanAndPreload(m_preloader.get(),
document()->validBaseElementURL(), nullptr);
On 2016/08/03 16:45:50, droger wrote:
> On 2016/08/03 14:40:03, csharrison wrote:
> > This just preloads the tokens normally. Do we want to add load flags like
> > LOAD_PREFETCH so all resources are forced into the HTTP cache?
> > 
> 
> This uses the same code as the standard preload scanner. I would think that it
> uses the LOAD_PREFETCH flags accordingly, and (at first at least) we want to
> have the same behavior for prefetch.
> 
> > Also, if we aren't using this render process for the final page load, this
> will
> > bloat the MemoryCache with in-memory resources that will never be used.
> > 
> 
> Yes, this is a good point. Disabling the memory cache would probably be nice.
> 
> > +shivanisha@ is thinking of implementing a "fire-and-forget" system in Blink
> > that will send requests but ignore results (make sure they don't land in the
> > memory cache). It might make sense to collaborate on this.
> > 
> > (note: I understand this is just a prototype, I just want to understand what
> the
> > final design will look like.)
> 

Nope, LOAD_PREFETCH afaik is only used for the link rel="prefetch" feature.
These loads will look like normal requests from the net stack, and will not
follow special caching behavior like LOAD_PREFETCH.

I did a brief trace and you can see that the load flag is added when the request
has RequestContext::RequestContextPrefetch which only happens for link rel
prefetches. One thing that you could do in this patch to solve this problem is
to inspect the preloads that are being scanned, and set their type to
LinkRelPrefetch. This would require:
1. Adding "setForPrefetch" to PreloadRequest.h which sets the resource type to
LinkRelPrefetch.
2. one of:
 a. Modifying HTMLPreloadScanner in some way to do this for you.
 b. Using a TokenPreloadScanner instead of HTMLPreloadScanner (look at
HTMLPreloadScanner for how to do this)

If you'd like LOAD_PREFETCH on your requests, you'll need to do this otherwise
you will double download no-store resources, etc.

This technique sounds reasonable to me, but again, I am not an owner so you'll
need to get sign-off from some blink owner :)

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:1066: if
(!length || isStopped())
On 2016/08/03 16:45:50, droger wrote:
> On 2016/08/03 14:40:03, csharrison wrote:
> > Can you DCHECK(!document()->isPrefetchOnly()) here?
> 
> Why? Is this because isStopped() should never be true when prefetching?

This method should not run in the non-threaded model (pretty sure).

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp
(right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:99:
EXPECT_TRUE(scriptRunnerHost->hasPreloadScanner());
On 2016/08/03 16:45:50, droger wrote:
> On 2016/08/03 14:40:04, csharrison wrote:
> > might make more sense to be checking m_haveBackgroundParser.
> hasPreloadScanner()
> > is less clear.
> 
> Why checking the background parser? Since we are in synchronous mode, there
> should not be one.
> 
> I'm checking the hasPreloadScanner as a way to check that prefetching has been
> done.

Ah nevermind, makes sense.

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:100:
EXPECT_EQ(HTMLTokenizer::DataState, parser->tokenizer()->getState());
On 2016/08/03 16:45:50, droger wrote:
> This is intended to check that the page has is not being parsed (as DataState
> seems to be the default idle state for the Tokenizer).

Nice! Yeah that works.

[pasko, 2016-08-04 12:23:07]

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

[pasko, 2016-08-04 12:23:09]

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:814:
m_preloadScanner->scanAndPreload(m_preloader.get(),
document()->validBaseElementURL(), nullptr);
On 2016/08/03 17:15:14, csharrison wrote:
> On 2016/08/03 16:45:50, droger wrote:
> > On 2016/08/03 14:40:03, csharrison wrote:
> > > This just preloads the tokens normally. Do we want to add load flags like
> > > LOAD_PREFETCH so all resources are forced into the HTTP cache?
> > > 
> > 
> > This uses the same code as the standard preload scanner. I would think that
it
> > uses the LOAD_PREFETCH flags accordingly, and (at first at least) we want to
> > have the same behavior for prefetch.
> > 
> > > Also, if we aren't using this render process for the final page load, this
> > will
> > > bloat the MemoryCache with in-memory resources that will never be used.
> > > 
> > 
> > Yes, this is a good point. Disabling the memory cache would probably be
nice.
> > 
> > > +shivanisha@ is thinking of implementing a "fire-and-forget" system in
Blink
> > > that will send requests but ignore results (make sure they don't land in
the
> > > memory cache). It might make sense to collaborate on this.
> > > 
> > > (note: I understand this is just a prototype, I just want to understand
what
> > the
> > > final design will look like.)
> > 
> 
> Nope, LOAD_PREFETCH afaik is only used for the link rel="prefetch" feature.
> These loads will look like normal requests from the net stack, and will not
> follow special caching behavior like LOAD_PREFETCH.
> 
> I did a brief trace and you can see that the load flag is added when the
request
> has RequestContext::RequestContextPrefetch which only happens for link rel
> prefetches. One thing that you could do in this patch to solve this problem is
> to inspect the preloads that are being scanned, and set their type to
> LinkRelPrefetch. This would require:
> 1. Adding "setForPrefetch" to PreloadRequest.h which sets the resource type to
> LinkRelPrefetch.
> 2. one of:
>  a. Modifying HTMLPreloadScanner in some way to do this for you.
>  b. Using a TokenPreloadScanner instead of HTMLPreloadScanner (look at
> HTMLPreloadScanner for how to do this)
> 
> If you'd like LOAD_PREFETCH on your requests, you'll need to do this otherwise
> you will double download no-store resources, etc.
> 
> This technique sounds reasonable to me, but again, I am not an owner so you'll
> need to get sign-off from some blink owner :)

csharrison: thank you for investigation and the info. Let's handle load flags
properly in a separate patch. I indeed see that LOAD_PREFETCH is not set in the
net stack, but strangely the following page load still fetches from cache
ignoring validators. Hmm.. I thought only LOAD_PREFETCH can do that..

[Charlie Harrison, 2016-08-04 12:26:01]

On 2016/08/04 12:23:09, pasko wrote:
>
https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
(right):
> 
>
https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:814:
> m_preloadScanner->scanAndPreload(m_preloader.get(),
> document()->validBaseElementURL(), nullptr);
> On 2016/08/03 17:15:14, csharrison wrote:
> > On 2016/08/03 16:45:50, droger wrote:
> > > On 2016/08/03 14:40:03, csharrison wrote:
> > > > This just preloads the tokens normally. Do we want to add load flags
like
> > > > LOAD_PREFETCH so all resources are forced into the HTTP cache?
> > > > 
> > > 
> > > This uses the same code as the standard preload scanner. I would think
that
> it
> > > uses the LOAD_PREFETCH flags accordingly, and (at first at least) we want
to
> > > have the same behavior for prefetch.
> > > 
> > > > Also, if we aren't using this render process for the final page load,
this
> > > will
> > > > bloat the MemoryCache with in-memory resources that will never be used.
> > > > 
> > > 
> > > Yes, this is a good point. Disabling the memory cache would probably be
> nice.
> > > 
> > > > +shivanisha@ is thinking of implementing a "fire-and-forget" system in
> Blink
> > > > that will send requests but ignore results (make sure they don't land in
> the
> > > > memory cache). It might make sense to collaborate on this.
> > > > 
> > > > (note: I understand this is just a prototype, I just want to understand
> what
> > > the
> > > > final design will look like.)
> > > 
> > 
> > Nope, LOAD_PREFETCH afaik is only used for the link rel="prefetch" feature.
> > These loads will look like normal requests from the net stack, and will not
> > follow special caching behavior like LOAD_PREFETCH.
> > 
> > I did a brief trace and you can see that the load flag is added when the
> request
> > has RequestContext::RequestContextPrefetch which only happens for link rel
> > prefetches. One thing that you could do in this patch to solve this problem
is
> > to inspect the preloads that are being scanned, and set their type to
> > LinkRelPrefetch. This would require:
> > 1. Adding "setForPrefetch" to PreloadRequest.h which sets the resource type
to
> > LinkRelPrefetch.
> > 2. one of:
> >  a. Modifying HTMLPreloadScanner in some way to do this for you.
> >  b. Using a TokenPreloadScanner instead of HTMLPreloadScanner (look at
> > HTMLPreloadScanner for how to do this)
> > 
> > If you'd like LOAD_PREFETCH on your requests, you'll need to do this
otherwise
> > you will double download no-store resources, etc.
> > 
> > This technique sounds reasonable to me, but again, I am not an owner so
you'll
> > need to get sign-off from some blink owner :)
> 
> csharrison: thank you for investigation and the info. Let's handle load flags
> properly in a separate patch. I indeed see that LOAD_PREFETCH is not set in
the
> net stack, but strangely the following page load still fetches from cache
> ignoring validators. Hmm.. I thought only LOAD_PREFETCH can do that..

Try killing the tab before loading the page again, they may be still in
MemoryCache?

[droger, 2016-08-04 12:33:37]

On 2016/08/04 12:23:09, pasko wrote:
> csharrison: thank you for investigation and the info. Let's handle load flags
> properly in a separate patch. I indeed see that LOAD_PREFETCH is not set in
the
> net stack, but strangely the following page load still fetches from cache
> ignoring validators. Hmm.. I thought only LOAD_PREFETCH can do that..

I agree, lets do this in a follow-up. This patch is already very large.

[droger, 2016-08-04 12:33:55]

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

[commit-bot: I haz the power, 2016-08-04 12:34:07]

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

[Charlie Harrison, 2016-08-04 12:35:46]

SGTM. The patch as-is looks good to me as long as we add TODOs to add the load
flags. I will try to a final pass today or Monday.

Very exciting work!

[pasko, 2016-08-04 12:37:11]

On 2016/08/04 12:26:01, csharrison wrote:
> > csharrison: thank you for investigation and the info. Let's handle load
flags
> > properly in a separate patch. I indeed see that LOAD_PREFETCH is not set in
> the
> > net stack, but strangely the following page load still fetches from cache
> > ignoring validators. Hmm.. I thought only LOAD_PREFETCH can do that..
> 
> Try killing the tab before loading the page again, they may be still in
> MemoryCache?

yah, good idea, checking this. I assumed that my browser-initiated navigation
would choose another process, but it could be that omnibox swapped in our
prerender.

[droger, 2016-08-04 12:42:29]

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:1066: if
(!length || isStopped())
On 2016/08/03 17:15:14, csharrison wrote:
> On 2016/08/03 16:45:50, droger wrote:
> > On 2016/08/03 14:40:03, csharrison wrote:
> > > Can you DCHECK(!document()->isPrefetchOnly()) here?
> > 
> > Why? Is this because isStopped() should never be true when prefetching?
> 
> This method should not run in the non-threaded model (pretty sure).

Is "This method" appendBytes()? Or isStopped()?

appendBytes() is run for me, and looking at the "if (shouldUseThreading())" line
1069 below, I think this is expected.

[Charlie Harrison, 2016-08-04 12:44:41]

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2172613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:1066: if
(!length || isStopped())
On 2016/08/04 12:42:29, droger wrote:
> On 2016/08/03 17:15:14, csharrison wrote:
> > On 2016/08/03 16:45:50, droger wrote:
> > > On 2016/08/03 14:40:03, csharrison wrote:
> > > > Can you DCHECK(!document()->isPrefetchOnly()) here?
> > > 
> > > Why? Is this because isStopped() should never be true when prefetching?
> > 
> > This method should not run in the non-threaded model (pretty sure).
> 
> Is "This method" appendBytes()? Or isStopped()?
> 
> appendBytes() is run for me, and looking at the "if (shouldUseThreading())"
line
> 1069 below, I think this is expected.

Ah you're right, I was misremember the control flow for the non-threaded case.
Apologies.

[droger, 2016-08-04 14:24:18]

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

[commit-bot: I haz the power, 2016-08-04 14:24:39]

Dry run: 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-04 16:34:37]

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

[commit-bot: I haz the power, 2016-08-04 16:34:39]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[droger, 2016-08-05 14:52:14]

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

[commit-bot: I haz the power, 2016-08-05 14:52:32]

Dry run: 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-05 14:55:17]

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

[commit-bot: I haz the power, 2016-08-05 14:55:18]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[droger, 2016-08-05 15:02:53]

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

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

Dry run: 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-05 17:22:09]

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

[commit-bot: I haz the power, 2016-08-05 17:22:10]

Dry run: This issue passed the CQ dry run.

[droger, 2016-08-17 12:36:42]

csharrison: ping.
Does this look good? What else is missing before landing this CL?

[Charlie Harrison, 2016-08-17 14:23:03]

Sorry for the delay.

This generally looks good, but I'd like to make sure we have a path forward for 
 - CSP checks
 - adding the load prefetch flag
 - evicting the resource from memory cache

I'm nervous about landing this until we at least know there are no hidden
gotchas that will make the full implementation tricky or half broken.

[Charlie Harrison, 2016-08-17 14:23:03]

Sorry for the delay.

This generally looks good, but I'd like to make sure we have a path forward for 
 - CSP checks
 - adding the load prefetch flag
 - evicting the resource from memory cache

I'm nervous about landing this until we at least know there are no hidden
gotchas that will make the full implementation tricky or half broken.

[Charlie Harrison, 2016-08-17 14:24:58]

Sorry I sent that a little early.

Can you give some background on the prerender code? What is the document
associated with the prerender? Is it a new document or is it the document that
initiated the prerender?

Right now, if a document has CSP checks in place and makes a prerender, do we
ignore those CSP checks? Do we apply CSP in the headers of the prerender?

[pasko, 2016-08-17 14:54:26]

On 2016/08/17 14:24:58, Charlie Harrison wrote:
> Sorry I sent that a little early.
> 
> Can you give some background on the prerender code? What is the document
> associated with the prerender? Is it a new document or is it the document that
> initiated the prerender?
> 
> Right now, if a document has CSP checks in place and makes a prerender, do we
> ignore those CSP checks? Do we apply CSP in the headers of the prerender?

I believe Prerender ignores CSP rules from the document that initiated the
prerender. Prerender creates a completely new WebContents with a URL and no
other context whatsoever.

CSP checks inside the prerender webcontents are respected as usual, I believe,
modulo bugs that we will be looking after in separate changes.

[Charlie Harrison, 2016-08-17 14:55:29]

Thanks, will do another full pass today.

[pasko, 2016-08-17 14:59:37]

On 2016/08/17 14:23:03, Charlie Harrison wrote:
> Sorry for the delay.
> 
> This generally looks good, but I'd like to make sure we have a path forward
for 
>  - CSP checks
>  - adding the load prefetch flag
>  - evicting the resource from memory cache

the latter two I think are doable in separate changes. Do you suspect that their
design will conflict a lot with the choices made in this change? I don't have
intuition here, but sounds OK to make mistakes here, unblock other changes, and
redo parts if it appears to be needed...

[pasko, 2016-08-17 15:00:32]

On 2016/08/17 14:55:29, Charlie Harrison wrote:
> Thanks, will do another full pass today.

Thank you, Charlie

[Charlie Harrison, 2016-08-17 15:04:07]

On 2016/08/17 14:59:37, pasko wrote:
> On 2016/08/17 14:23:03, Charlie Harrison wrote:
> > Sorry for the delay.
> > 
> > This generally looks good, but I'd like to make sure we have a path forward
> for 
> >  - CSP checks
> >  - adding the load prefetch flag
> >  - evicting the resource from memory cache
> 
> the latter two I think are doable in separate changes. Do you suspect that
their
> design will conflict a lot with the choices made in this change? I don't have
> intuition here, but sounds OK to make mistakes here, unblock other changes,
and
> redo parts if it appears to be needed...

No it won't conflict a lot. I mainly wanted to figure this out because I'm
nervous the full implementation will need to make a new ResourceClient and do a
bunch of core/fetch type stuff wrt evicting from memory cache and manually
calling fetch() on resources rather than using the preload infrastructure. If
that's the case then I'd like to make sure it's clear in comments that the code
that just calls into HTMLPreloadScanner is temporary.

I think the code as-is is pretty landable though as a v0.5.

[droger, 2016-08-17 16:22:08]

On 2016/08/17 15:04:07, Charlie Harrison wrote:
> > >  - adding the load prefetch flag

I discussed this with Yoav, and he suggested adding this flag at the chrome
layer instead at the blink layer.
Basically Blink would not know that it is in prefetch mode (with respect to the
load flags), but instead Chrome would add the flags to all the requests coming
out of blink.

I'll dig into this more tomorrow.

[droger, 2016-08-18 14:31:16]

On 2016/08/17 14:23:03, Charlie Harrison wrote:
>  - evicting the resource from memory cache

I've started looking at this and does not seem trivial. It's probably not
required at first though and could be considered an optimization.

The approaches I've been considering are:
1) make the browser silently drop the responses and actually never send them to
the renderer
2) avoid calling memoryCache()->add() in 
ResourceFetcher::createResourceForLoading()
3) set the size of the memory cache to zero for the whole renderer process

1 and 2 look hard to do, and 3 seems hacky (and not trivial either).

[shivanisha, 2016-08-18 15:08:21]

On 2016/08/18 at 14:31:16, droger wrote:
> On 2016/08/17 14:23:03, Charlie Harrison wrote:
> >  - evicting the resource from memory cache
> 
> I've started looking at this and does not seem trivial. It's probably not
required at first though and could be considered an optimization.
> 
> The approaches I've been considering are:
> 1) make the browser silently drop the responses and actually never send them
to the renderer
> 2) avoid calling memoryCache()->add() in 
ResourceFetcher::createResourceForLoading()
> 3) set the size of the memory cache to zero for the whole renderer process
> 
> 1 and 2 look hard to do, and 3 seems hacky (and not trivial either).

I am solving a similar problem of evicting a resource from the memory cache. I
am basically doing it in ScriptLoader->notifyFinished as my case is script
specific. Though my implementation is currently running into other issues which
I am debugging. Once that works, I will update it.

[Charlie Harrison, 2016-08-18 21:14:43]

The core/html/parser changes lgtm. I wonder if we can do some layout tests or
browser tests in a followup?

In particular it would be great to have
 - Basic tests that we are actually making requests to the network.
 - test that a nostate prefetch with CSP rules in the header are properly
enfored

Thanks for the unit tests!

[droger, 2016-08-19 10:04:31]

droger@chromium.org changed reviewers:
+ mmenke@chromium.org, yoav@yoav.ws

[droger, 2016-08-19 10:04:32]

yoav: can you do OWNERS review for blink changes? Note that Charlie is owner for
parser, but you can review it too if you wish, in particular because the rest of
the CL is only plumbing for the parser change.

mmenke: can you review changes in chrome/renderer/prerender?

[mmenke, 2016-08-19 16:42:50]

https://codereview.chromium.org/2172613002/diff/320001/chrome/renderer/preren...
File chrome/renderer/prerender/prerender_helper.cc (right):

https://codereview.chromium.org/2172613002/diff/320001/chrome/renderer/preren...
chrome/renderer/prerender/prerender_helper.cc:49:
sub_frame->GetRenderView()->GetMainRenderFrame())) {
DCHECK that render_frame is not the main render frame?

https://codereview.chromium.org/2172613002/diff/320001/chrome/renderer/preren...
File chrome/renderer/prerender/prerenderer_client.h (right):

https://codereview.chromium.org/2172613002/diff/320001/chrome/renderer/preren...
chrome/renderer/prerender/prerenderer_client.h:27: bool isPrefetchOnly()
override;
Should this return the PrenderMode instead?  Seems a little more general, and
since we have an enum, seems like we might as well use it.

[droger, 2016-08-22 09:00:56]

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

[commit-bot: I haz the power, 2016-08-22 09:01:12]

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

[droger, 2016-08-22 09:30:06]

Thanks.

https://codereview.chromium.org/2172613002/diff/320001/chrome/renderer/preren...
File chrome/renderer/prerender/prerender_helper.cc (right):

https://codereview.chromium.org/2172613002/diff/320001/chrome/renderer/preren...
chrome/renderer/prerender/prerender_helper.cc:49:
sub_frame->GetRenderView()->GetMainRenderFrame())) {
On 2016/08/19 16:42:50, mmenke wrote:
> DCHECK that render_frame is not the main render frame?

Done.

https://codereview.chromium.org/2172613002/diff/320001/chrome/renderer/preren...
File chrome/renderer/prerender/prerenderer_client.h (right):

https://codereview.chromium.org/2172613002/diff/320001/chrome/renderer/preren...
chrome/renderer/prerender/prerenderer_client.h:27: bool isPrefetchOnly()
override;
On 2016/08/19 16:42:50, mmenke wrote:
> Should this return the PrenderMode instead?  Seems a little more general, and
> since we have an enum, seems like we might as well use it.

This is a blink::WebPrerendererClient interface, which does not know about the
prerender mode.

Do you think it is worth creating the enum in blink?

[commit-bot: I haz the power, 2016-08-22 10:59:47]

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

[commit-bot: I haz the power, 2016-08-22 10:59:57]

Dry run: This issue passed the CQ dry run.

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

On 2016/08/22 09:30:06, droger wrote:
> Thanks.
> 
>
https://codereview.chromium.org/2172613002/diff/320001/chrome/renderer/preren...
> File chrome/renderer/prerender/prerender_helper.cc (right):
> 
>
https://codereview.chromium.org/2172613002/diff/320001/chrome/renderer/preren...
> chrome/renderer/prerender/prerender_helper.cc:49:
> sub_frame->GetRenderView()->GetMainRenderFrame())) {
> On 2016/08/19 16:42:50, mmenke wrote:
> > DCHECK that render_frame is not the main render frame?
> 
> Done.
> 
>
https://codereview.chromium.org/2172613002/diff/320001/chrome/renderer/preren...
> File chrome/renderer/prerender/prerenderer_client.h (right):
> 
>
https://codereview.chromium.org/2172613002/diff/320001/chrome/renderer/preren...
> chrome/renderer/prerender/prerenderer_client.h:27: bool isPrefetchOnly()
> override;
> On 2016/08/19 16:42:50, mmenke wrote:
> > Should this return the PrenderMode instead?  Seems a little more general,
and
> > since we have an enum, seems like we might as well use it.
> 
> This is a blink::WebPrerendererClient interface, which does not know about the
> prerender mode.
> 
> Do you think it is worth creating the enum in blink?

Oops!  Sorry, I was thinking that file was prerender_contents.  I'll defer to
you guys on whether that is worth it - if we're getting rid of non-prefetch-only
prerenders, probably not worth the effort.

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

LGTM (Largely rubberstamp, not familiar with how things work in the renderer)

https://codereview.chromium.org/2172613002/diff/340001/chrome/renderer/preren...
File chrome/renderer/prerender/prerender_helper.cc (right):

https://codereview.chromium.org/2172613002/diff/340001/chrome/renderer/preren...
chrome/renderer/prerender/prerender_helper.cc:50:
DCHECK(!render_frame()->IsMainFrame());
Is it worth creating one of these for every frame, or should GetPRerenderMode
below just be
PrerenderHelper::Get(render_frame->GetRenderView()->GetMainRenderFrame())?

I'm not advocating either option here, as I'm not at all sure of the tradeoffs,
just thought I'd ask.

[droger, 2016-08-22 14:31:01]

https://codereview.chromium.org/2172613002/diff/340001/chrome/renderer/preren...
File chrome/renderer/prerender/prerender_helper.cc (right):

https://codereview.chromium.org/2172613002/diff/340001/chrome/renderer/preren...
chrome/renderer/prerender/prerender_helper.cc:50:
DCHECK(!render_frame()->IsMainFrame());
On 2016/08/22 14:22:08, mmenke wrote:
> Is it worth creating one of these for every frame, or should GetPRerenderMode
> below just be
> PrerenderHelper::Get(render_frame->GetRenderView()->GetMainRenderFrame())?
> 
> I'm not advocating either option here, as I'm not at all sure of the
tradeoffs,
> just thought I'd ask.

I kept the existing behavior which was to create one helper per frame.
I don't know if it is necessary though.

This is probably something we can revisit later if/once we remove the prerender
codepath entirely.

[droger, 2016-08-23 10:00:42]

yoav: ping

[Yoav Weiss, 2016-08-23 20:03:58]

Apologies for the delayed response (on vacation...)

LGTM with a couple nits/questions.

https://codereview.chromium.org/2172613002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DecodedDataDocumentParser.h (right):

https://codereview.chromium.org/2172613002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DecodedDataDocumentParser.h:36: class
CORE_EXPORT DecodedDataDocumentParser : public DocumentParser {
Do we require these CORE_EXPORT because HTMLDocumentParser is exposed to a test?
Same goes for ScriptableDocumentParser

https://codereview.chromium.org/2172613002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp
(right):

https://codereview.chromium.org/2172613002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:43:
HTMLDocumentParser* CreateParser(HTMLDocument& document)
nit: s/CreateParser/createParser/ in order to match Blink style

[droger, 2016-08-24 11:33:44]

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

[droger, 2016-08-24 11:33:45]

Thanks.

+jochen as OWNER for (simple) changes in:
- chrome/renderer/chrome_render_frame_observer.cc
- third_party/WebKit/Source/web/

https://codereview.chromium.org/2172613002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DecodedDataDocumentParser.h (right):

https://codereview.chromium.org/2172613002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DecodedDataDocumentParser.h:36: class
CORE_EXPORT DecodedDataDocumentParser : public DocumentParser {
On 2016/08/23 20:03:58, Yoav Weiss wrote:
> Do we require these CORE_EXPORT because HTMLDocumentParser is exposed to a
test?
> Same goes for ScriptableDocumentParser

Yes, this is because of the HTMLDocumentParser unit test.
To be able to instantiate it in a test, it must be exported as well as all its
super classes (which there are many of).

https://codereview.chromium.org/2172613002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp
(right):

https://codereview.chromium.org/2172613002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp:43:
HTMLDocumentParser* CreateParser(HTMLDocument& document)
On 2016/08/23 20:03:58, Yoav Weiss wrote:
> nit: s/CreateParser/createParser/ in order to match Blink style 

Done.

[jochen (gone - plz use gerrit), 2016-08-24 20:57:04]

lgtm

[jochen (gone - plz use gerrit), 2016-08-24 20:58:48]

please ping the design doc to chrome-design-docs@ or as part of an
intent-to-implement to blink-dev to make sure it gets seen

[droger, 2016-08-25 08:52:28]

On 2016/08/24 20:58:48, jochen wrote:
> please ping the design doc to chrome-design-docs@ or as part of an
> intent-to-implement to blink-dev to make sure it gets seen

Thanks. It made a round on chrome-loading, but we will send it to a broader
audience.

[droger, 2016-08-25 08:53:30]

The CQ bit was checked by droger@chromium.org

[droger, 2016-08-25 08:53:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org, yoav@yoav.ws, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2172613002/#ps360001
(title: "Fix style")

[commit-bot: I haz the power, 2016-08-25 08:53:54]

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

[droger, 2016-08-25 08:57:03]

Description was changed from

==========
Renderer-side changes for NoState Prefetch

NoState Prefetch design doc:
https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5EhU

This CL is the first step for NoState Prefetch implementation
in the renderer.

The code added in this CL is not called yet (it will be called
only when the PREFETCH_ONLY enum is sent through IPC).

The prefetching is implemented in the HTMLDocumentParser.
When prefetching, the parser runs in single-thread mode.
When feeded with HTML input, it runs the preload scanner on
it, and stops without actually building or executing the page.
==========

to

==========
Renderer-side changes for NoState Prefetch

NoState Prefetch design doc:
https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5EhU

This CL is the first step for NoState Prefetch implementation
in the renderer.

The code added in this CL is not called yet (it will be called
only when the PREFETCH_ONLY enum is sent through IPC).

The prefetching is implemented in the HTMLDocumentParser.
When prefetching, the parser runs in single-thread mode.
When feeded with HTML input, it runs the preload scanner on
it, and stops without actually building or executing the page.

BUG=632361
==========

[commit-bot: I haz the power, 2016-08-25 09:50:01]

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

[commit-bot: I haz the power, 2016-08-25 09:50:02]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[droger, 2016-08-25 10:02:10]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2016-08-25 10:02:47]

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-25 12:29:11]

Description was changed from

==========
Renderer-side changes for NoState Prefetch

NoState Prefetch design doc:
https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5EhU

This CL is the first step for NoState Prefetch implementation
in the renderer.

The code added in this CL is not called yet (it will be called
only when the PREFETCH_ONLY enum is sent through IPC).

The prefetching is implemented in the HTMLDocumentParser.
When prefetching, the parser runs in single-thread mode.
When feeded with HTML input, it runs the preload scanner on
it, and stops without actually building or executing the page.

BUG=632361
==========

to

==========
Renderer-side changes for NoState Prefetch

NoState Prefetch design doc:
https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5EhU

This CL is the first step for NoState Prefetch implementation
in the renderer.

The code added in this CL is not called yet (it will be called
only when the PREFETCH_ONLY enum is sent through IPC).

The prefetching is implemented in the HTMLDocumentParser.
When prefetching, the parser runs in single-thread mode.
When feeded with HTML input, it runs the preload scanner on
it, and stops without actually building or executing the page.

BUG=632361
==========

[commit-bot: I haz the power, 2016-08-25 12:29:14]

Committed patchset #15 (id:360001)

[commit-bot: I haz the power, 2016-08-25 12:32:54]

Description was changed from

==========
Renderer-side changes for NoState Prefetch

NoState Prefetch design doc:
https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5EhU

This CL is the first step for NoState Prefetch implementation
in the renderer.

The code added in this CL is not called yet (it will be called
only when the PREFETCH_ONLY enum is sent through IPC).

The prefetching is implemented in the HTMLDocumentParser.
When prefetching, the parser runs in single-thread mode.
When feeded with HTML input, it runs the preload scanner on
it, and stops without actually building or executing the page.

BUG=632361
==========

to

==========
Renderer-side changes for NoState Prefetch

NoState Prefetch design doc:
https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5EhU

This CL is the first step for NoState Prefetch implementation
in the renderer.

The code added in this CL is not called yet (it will be called
only when the PREFETCH_ONLY enum is sent through IPC).

The prefetching is implemented in the HTMLDocumentParser.
When prefetching, the parser runs in single-thread mode.
When feeded with HTML input, it runs the preload scanner on
it, and stops without actually building or executing the page.

BUG=632361

Committed: https://crrev.com/04f065d8f4417c062ec53cbc7d27ed571e6736b2
Cr-Commit-Position: refs/heads/master@{#414406}
==========

[commit-bot: I haz the power, 2016-08-25 12:32:56]

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