predictors: Add browsertest that tests prefetching.

predictors: Add browsertest that tests prefetching.

Before this we only had tests for filling predictor database. This CL
adds test that prefills database and then activates prefetching on the
same web page. Then it checks that cache actually has been populated by
prefetched resources.

BUG=650253
Review-Url: https://codereview.chromium.org/2553083002
Committed: https://chromium.googlesource.com/chromium/src/+/984fbd599358277897f5ad24e1d51bd56678a487

[alexilin, 2016-12-06 12:44:16]

alexilin@chromium.org changed reviewers:
+ lizeb@chromium.org

[alexilin, 2016-12-06 12:44:17]

Hi!
This is a draft of a test for prefetching.
I'm waiting for your comments and suggestions.

https://codereview.chromium.org/2553083002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2553083002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:552:
NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath));
Here we reuse interceptors of URLRequests that ResourcePrefetchPredictor has and
check that all resources has actually been served from cache.

https://codereview.chromium.org/2553083002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

https://codereview.chromium.org/2553083002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetcher_manager.cc:91:
BrowserThread::PostTask(
Probably we don't want to send this message to other thread for non-testing
code. WDYT?

https://codereview.chromium.org/2553083002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetcher_manager.cc:94:
base::Unretained(predictor_), main_frame_url));
I'm not sure that base::Unretained is safe here. PTAL.

[alexilin, 2016-12-16 15:52:29]

Hi,

We want to test prefetching also for various html pages. So we had two choices:
1) Create different prefetching test for each test page.
2) Make all existing tests testing both learning and prefetching.

The second way is more preferable because of resource savings and less code
duplication.

HttpToHttpsRedirect test has particular problems (see crbug.com/674914) so it's
testing learning only for a while.

[Benoit L, 2016-12-19 12:32:00]

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:79: //
ResourcePrefetchPredictor is initialized before observer creation.
nit: before the observer creation.

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:283:
predictor_->StartPrefetching(main_frame_url, PrefetchOrigin::NAVIGATION);
nit: What you're doing here maps more closely to "EXTERNAL" conceptually, since
you prefetch independently from a navigation.

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:518: //
TestLearningAndPrefetching(GetURL(kRedirectPath));
nit: Don't put commented-out code. However the comment is fine.

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_manager.cc:85: const GURL
main_frame_url = resource_prefetcher->main_frame_url();
nit: const GURL&

Might avoid one copy.

[alexilin, 2016-12-19 12:58:09]

Top-level comment:
Is it okay to rename all these tests (as I did)? At first I wanted to follow
learning/prefetching naming rule but now it makes no sense.

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:79: //
ResourcePrefetchPredictor is initialized before observer creation.
On 2016/12/19 12:32:00, Benoit L wrote:
> nit: before the observer creation.

Done.

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:283:
predictor_->StartPrefetching(main_frame_url, PrefetchOrigin::NAVIGATION);
On 2016/12/19 12:32:00, Benoit L wrote:
> nit: What you're doing here maps more closely to "EXTERNAL" conceptually,
since
> you prefetch independently from a navigation.

I hope that the behavior of predictor in this two cases won't diverge? I'm
curious if we'll need special test cases for both.

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:518: //
TestLearningAndPrefetching(GetURL(kRedirectPath));
On 2016/12/19 12:32:00, Benoit L wrote:
> nit: Don't put commented-out code. However the comment is fine.

Done.
Is there some rule in styleguide about that or is this just a bad style?

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_manager.cc:85: const GURL
main_frame_url = resource_prefetcher->main_frame_url();
On 2016/12/19 12:32:00, Benoit L wrote:
> nit: const GURL&
> 
> Might avoid one copy.

Bad idea: we pass it to another thread below.

[alexilin, 2016-12-19 15:03:42]

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_manager.cc:85: const GURL
main_frame_url = resource_prefetcher->main_frame_url();
On 2016/12/19 12:58:09, alexilin wrote:
> On 2016/12/19 12:32:00, Benoit L wrote:
> > nit: const GURL&
> > 
> > Might avoid one copy.
> 
> Bad idea: we pass it to another thread below.

It would cause Segmentation Fault by another reason: resource_prefetcher object
is deleted before BrowserThread::PostTask call.
I reordered calls to get rid of copy.

[Benoit L, 2016-12-19 15:19:06]

lgtm with one minor comment about a comment.

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:283:
predictor_->StartPrefetching(main_frame_url, PrefetchOrigin::NAVIGATION);
On 2016/12/19 12:58:09, alexilin wrote:
> On 2016/12/19 12:32:00, Benoit L wrote:
> > nit: What you're doing here maps more closely to "EXTERNAL" conceptually,
> since
> > you prefetch independently from a navigation.
> 
> I hope that the behavior of predictor in this two cases won't diverge? I'm
> curious if we'll need special test cases for both.

Good point.
I'm sure that if either of us change this, we will add a test... or suffer the
wrath of the other one in code review.

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:518: //
TestLearningAndPrefetching(GetURL(kRedirectPath));
On 2016/12/19 12:58:09, alexilin wrote:
> On 2016/12/19 12:32:00, Benoit L wrote:
> > nit: Don't put commented-out code. However the comment is fine.
> 
> Done.
> Is there some rule in styleguide about that or is this just a bad style?

I'm not sure, but I'm quite confident that no-one likes commented-out code. For
code that was there, we have git. For code that will be there, this is code that
has never ran, may not even compile, so has little value compared to not writing
it. And that's without taking into account all the times where I wrote code that
I thought "would be useful at some point"...


Can you update the comment as well? Something like "Test learning and
prefetching once crbug.com/650246 is fixed."

[alexilin, 2016-12-19 15:36:14]

Committing is coming...

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(right):

https://codereview.chromium.org/2553083002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:518: //
TestLearningAndPrefetching(GetURL(kRedirectPath));
On 2016/12/19 15:19:05, Benoit L wrote:
> On 2016/12/19 12:58:09, alexilin wrote:
> > On 2016/12/19 12:32:00, Benoit L wrote:
> > > nit: Don't put commented-out code. However the comment is fine.
> > 
> > Done.
> > Is there some rule in styleguide about that or is this just a bad style?
> 
> I'm not sure, but I'm quite confident that no-one likes commented-out code.
For
> code that was there, we have git. For code that will be there, this is code
that
> has never ran, may not even compile, so has little value compared to not
writing
> it. And that's without taking into account all the times where I wrote code
that
> I thought "would be useful at some point"...
> 
> 
> Can you update the comment as well? Something like "Test learning and
> prefetching once crbug.com/650246 is fixed."

Well, now even this never commited commented-out code is stored "forever" in the
code review tool.
Done.

[alexilin, 2016-12-19 15:36:26]

The CQ bit was checked by alexilin@chromium.org

[alexilin, 2016-12-19 15:36:26]

The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/2553083002/#ps80001
(title: "Fix TODO comment.")

[commit-bot: I haz the power, 2016-12-19 15:36:42]

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-12-19 15:38:25]

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

[commit-bot: I haz the power, 2016-12-19 15:38:26]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[alexilin, 2016-12-19 16:45:25]

The CQ bit was checked by alexilin@chromium.org

[alexilin, 2016-12-19 16:45:25]

The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/2553083002/#ps100001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-12-19 16:45:39]

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-12-19 17:31:32]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1482165925397170,
"parent_rev": "9b24124b7e3716a817a4168904b4fb2935bbcdc9", "commit_rev":
"984fbd599358277897f5ad24e1d51bd56678a487"}

[commit-bot: I haz the power, 2016-12-19 17:33:35]

Description was changed from

==========
predictors: Add browsertest that tests prefetching.

Before this we only had tests for filling predictor database. This CL
adds test that prefills database and then activates prefetching on the
same web page. Then it checks that cache actually has been populated by
prefetched resources.

BUG=650253
==========

to

==========
predictors: Add browsertest that tests prefetching.

Before this we only had tests for filling predictor database. This CL
adds test that prefills database and then activates prefetching on the
same web page. Then it checks that cache actually has been populated by
prefetched resources.

BUG=650253

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

[commit-bot: I haz the power, 2016-12-19 17:33:36]

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

[commit-bot: I haz the power, 2016-12-19 17:33:42]

Description was changed from

==========
predictors: Add browsertest that tests prefetching.

Before this we only had tests for filling predictor database. This CL
adds test that prefills database and then activates prefetching on the
same web page. Then it checks that cache actually has been populated by
prefetched resources.

BUG=650253

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

to

==========
predictors: Add browsertest that tests prefetching.

Before this we only had tests for filling predictor database. This CL
adds test that prefills database and then activates prefetching on the
same web page. Then it checks that cache actually has been populated by
prefetched resources.

BUG=650253

Review-Url: https://codereview.chromium.org/2553083002
Committed:
https://chromium.googlesource.com/chromium/src/+/984fbd599358277897f5ad24e1d5...
==========

[commit-bot: I haz the power, 2016-12-19 17:33:46]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/984fbd599358277897f5ad24e1d5...