predictors: browser test for client-side redirect.

predictors: browser test for client-side redirect.

ResourcePrefetchPredictor doesn't receive the information from observers about
redirects initiated from javascript. However such redirects initiate the new
navigation that treated by predictor as usual.
This browser test documents the current behavior that could be a subject of
change.

Also, this CL changes all existing redirect tests to be cross-host. Redirect to
the same host could be ambiguous because of host-based learning.

BUG=650253
Review-Url: https://codereview.chromium.org/2609403007
Cr-Commit-Position: refs/heads/master@{#442259}
Committed: https://chromium.googlesource.com/chromium/src/+/7e5dcb16fb03140fb729fbf58b03bb02ebe9df0e

[alexilin, 2017-01-05 17:03:56]

Description was changed from

==========
predictors: browser test for the client-side redirect.

ResourcePrefetchPredictor doesn't receive the information from obsevers about
redirects initiated from javascript. However such redirects initiate the new
navigation that treated by predictor as usual.
This browser test documents the current behaviour that could be a subject of
change.

Also, this CL changes all existing redirect tests to be cross-host. Redirect to
the same host could be ambiguous because of host-based learning.

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

to

==========
predictors: browser test for client-side redirect.

ResourcePrefetchPredictor doesn't receive the information from observers about
redirects initiated from javascript. However such redirects initiate the new
navigation that treated by predictor as usual.
This browser test documents the current behavior that could be a subject of
change.

Also, this CL changes all existing redirect tests to be cross-host. Redirect to
the same host could be ambiguous because of host-based learning.

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

[alexilin, 2017-01-05 17:11:45]

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

[alexilin, 2017-01-05 17:11:46]

Hi Benoit,

I hope you rest well! As you can remember we've discussed with Camille that
client-side redirects behave differently. I wrote a test to describe current
behavior.

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

https://codereview.chromium.org/2609403007/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:194: GURL
GetRequestURL(const net::test_server::HttpRequest& request) {
Extracted from HandleResourceRequest

[Benoit L, 2017-01-09 14:31:47]

Thanks!

Only nits.

Can you also add a comment in the predictor to note that client-side redirects
are not supported?

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

https://codereview.chromium.org/2609403007/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:389:
predictor_->StartPrefetching(main_frame_url, PrefetchOrigin::EXTERNAL);
Isn't this function only used in one place?
If so, please keep the call inline.

https://codereview.chromium.org/2609403007/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:840: //
Client-side redirect initiates a new navigation to the redirect destination
nit: A client-side redirect... so that the OnLoad event...

https://codereview.chromium.org/2609403007/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:843:
JavascriptRedirect) {
nit: JavascriptRedirectsAreNotHandled?

https://codereview.chromium.org/2609403007/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:858: // Two
navigations will occur. LearningObserver will get event only for the
nit: events

https://codereview.chromium.org/2609403007/diff/1/chrome/test/data/predictors...
File chrome/test/data/predictors/html_subresources.html (right):

https://codereview.chromium.org/2609403007/diff/1/chrome/test/data/predictors...
chrome/test/data/predictors/html_subresources.html:15: <script
src="/handled-by-test/script.js"></script>
Why moving this to <head>?

https://codereview.chromium.org/2609403007/diff/1/chrome/test/data/predictors...
File chrome/test/data/predictors/javascript_redirect.html (right):

https://codereview.chromium.org/2609403007/diff/1/chrome/test/data/predictors...
chrome/test/data/predictors/javascript_redirect.html:5: <script
src="javascript_redirect.js"></script>
tiny nit: Can you inline the script? One less indirection level to follow.
Up to you.

https://codereview.chromium.org/2609403007/diff/1/chrome/test/data/predictors...
File chrome/test/data/predictors/javascript_redirect.js (right):

https://codereview.chromium.org/2609403007/diff/1/chrome/test/data/predictors...
chrome/test/data/predictors/javascript_redirect.js:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
nit: 2017 is the new 2016.

[alexilin, 2017-01-09 15:08:16]

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

https://codereview.chromium.org/2609403007/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:389:
predictor_->StartPrefetching(main_frame_url, PrefetchOrigin::EXTERNAL);
On 2017/01/09 14:31:47, Benoit L wrote:
> Isn't this function only used in one place?
> If so, please keep the call inline.

Well, the problem is that |predictor_| is private. I didn't want to make it
public.

https://codereview.chromium.org/2609403007/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:840: //
Client-side redirect initiates a new navigation to the redirect destination
On 2017/01/09 14:31:47, Benoit L wrote:
> nit: A client-side redirect... so that the OnLoad event...

Done.

https://codereview.chromium.org/2609403007/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:843:
JavascriptRedirect) {
On 2017/01/09 14:31:47, Benoit L wrote:
> nit: JavascriptRedirectsAreNotHandled?

Done.

https://codereview.chromium.org/2609403007/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:858: // Two
navigations will occur. LearningObserver will get event only for the
On 2017/01/09 14:31:47, Benoit L wrote:
> nit: events

Done.

https://codereview.chromium.org/2609403007/diff/1/chrome/test/data/predictors...
File chrome/test/data/predictors/html_subresources.html (right):

https://codereview.chromium.org/2609403007/diff/1/chrome/test/data/predictors...
chrome/test/data/predictors/html_subresources.html:15: <script
src="/handled-by-test/script.js"></script>
On 2017/01/09 14:31:47, Benoit L wrote:
> Why moving this to <head>?

The reason is strictly aesthetic. A <script> outside <head> and <body> looks
malformed. Browser implicitly moves it into <head> section so I just tried to
make it nicer.
Can get rid of it if you complain.

https://codereview.chromium.org/2609403007/diff/1/chrome/test/data/predictors...
File chrome/test/data/predictors/javascript_redirect.html (right):

https://codereview.chromium.org/2609403007/diff/1/chrome/test/data/predictors...
chrome/test/data/predictors/javascript_redirect.html:5: <script
src="javascript_redirect.js"></script>
On 2017/01/09 14:31:47, Benoit L wrote:
> tiny nit: Can you inline the script? One less indirection level to follow.
> Up to you.

We chose to use files for these tests earlier, so let's be consistent.

https://codereview.chromium.org/2609403007/diff/1/chrome/test/data/predictors...
File chrome/test/data/predictors/javascript_redirect.js (right):

https://codereview.chromium.org/2609403007/diff/1/chrome/test/data/predictors...
chrome/test/data/predictors/javascript_redirect.js:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/01/09 14:31:47, Benoit L wrote:
> nit: 2017 is the new 2016.

Happy New Year!

[Benoit L, 2017-01-09 15:21:03]

Thanks!

lgtm

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

https://codereview.chromium.org/2609403007/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:389:
predictor_->StartPrefetching(main_frame_url, PrefetchOrigin::EXTERNAL);
On 2017/01/09 15:08:15, alexilin wrote:
> On 2017/01/09 14:31:47, Benoit L wrote:
> > Isn't this function only used in one place?
> > If so, please keep the call inline.
> 
> Well, the problem is that |predictor_| is private. I didn't want to make it
> public.

Acknowledged.

[alexilin, 2017-01-09 15:23:12]

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

[commit-bot: I haz the power, 2017-01-09 15:23:21]

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, 2017-01-09 16:14:46]

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

[commit-bot: I haz the power, 2017-01-09 16:14:47]

Dry run: This issue passed the CQ dry run.

[alexilin, 2017-01-09 16:15:14]

The CQ bit was checked by alexilin@chromium.org

[commit-bot: I haz the power, 2017-01-09 16:15:36]

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, 2017-01-09 16:20:05]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483978514153090,
"parent_rev": "74e3405dd042c4e4a3fcded953ec6fc5a87f2c16", "commit_rev":
"7e5dcb16fb03140fb729fbf58b03bb02ebe9df0e"}

[commit-bot: I haz the power, 2017-01-09 16:20:34]

Description was changed from

==========
predictors: browser test for client-side redirect.

ResourcePrefetchPredictor doesn't receive the information from observers about
redirects initiated from javascript. However such redirects initiate the new
navigation that treated by predictor as usual.
This browser test documents the current behavior that could be a subject of
change.

Also, this CL changes all existing redirect tests to be cross-host. Redirect to
the same host could be ambiguous because of host-based learning.

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

to

==========
predictors: browser test for client-side redirect.

ResourcePrefetchPredictor doesn't receive the information from observers about
redirects initiated from javascript. However such redirects initiate the new
navigation that treated by predictor as usual.
This browser test documents the current behavior that could be a subject of
change.

Also, this CL changes all existing redirect tests to be cross-host. Redirect to
the same host could be ambiguous because of host-based learning.

BUG=650253

Review-Url: https://codereview.chromium.org/2609403007
Cr-Commit-Position: refs/heads/master@{#442259}
Committed:
https://chromium.googlesource.com/chromium/src/+/7e5dcb16fb03140fb729fbf58b03...
==========

[commit-bot: I haz the power, 2017-01-09 16:20:35]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/7e5dcb16fb03140fb729fbf58b03...