[Prerender] Cleanup prerender_util

[Prerender] Cleanup prerender_util

IsGoogleDomain() and IsGoogleSearchResultURL() are bad
re-implementations of functions that already exist in google_util.

This CL removes these functions and uses the ones from
google_util that are not bugged, widely used and more maintained.

ALLOW_NON_STANDARD_PORT is used, as it matches existing behavior, and is
required by integration tests. It would certainly be good to disallow
such ports as future work.

Some dead code is removed as well.

Review-Url: https://codereview.chromium.org/2645713003
Cr-Commit-Position: refs/heads/master@{#445374}
Committed: https://chromium.googlesource.com/chromium/src/+/4239cebc352889db1bd41ba9abbc17d9261a5769

[droger, 2017-01-19 13:48:57]

Description was changed from

==========
[Prerender] Do not reimplement google_util functions

IsGoogleDomain() and
==========

to

==========
[Prerender] Do not reimplement google_util functions

IsGoogleDomain() and IsGoogleSearchResultURL() are bad
re-implementations of functions that already exist in google_util.

This CL removes these functions and uses the ones from
google_util that are not bugged, widely used and more maintained.
==========

[droger, 2017-01-19 13:56:02]

Description was changed from

==========
[Prerender] Do not reimplement google_util functions

IsGoogleDomain() and IsGoogleSearchResultURL() are bad
re-implementations of functions that already exist in google_util.

This CL removes these functions and uses the ones from
google_util that are not bugged, widely used and more maintained.
==========

to

==========
[Prerender] Cleanup prerender_util

IsGoogleDomain() and IsGoogleSearchResultURL() are bad
re-implementations of functions that already exist in google_util.

This CL removes these functions and uses the ones from
google_util that are not bugged, widely used and more maintained.

Some dead code is removed as well.
==========

[droger, 2017-01-19 14:01:46]

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

[droger, 2017-01-19 14:01:54]

Patchset #1 (id:1) has been deleted

[droger, 2017-01-19 14:01:59]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-01-19 14:02:18]

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-19 14:41:13]

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

[commit-bot: I haz the power, 2017-01-19 14:41:13]

Dry run: 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_...)

[droger, 2017-01-19 16:56:09]

Patchset #2 (id:60001) has been deleted

[droger, 2017-01-19 17:01:13]

Patchset #2 (id:80001) has been deleted

[droger, 2017-01-19 17:01:19]

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

[commit-bot: I haz the power, 2017-01-19 17:02:02]

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-19 17:44:27]

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

[commit-bot: I haz the power, 2017-01-19 17:44:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[droger, 2017-01-20 10:24:42]

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

[commit-bot: I haz the power, 2017-01-20 10:24:52]

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-20 11:15:41]

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

[commit-bot: I haz the power, 2017-01-20 11:15:41]

Dry run: This issue passed the CQ dry run.

[droger, 2017-01-20 11:59:47]

Description was changed from

==========
[Prerender] Cleanup prerender_util

IsGoogleDomain() and IsGoogleSearchResultURL() are bad
re-implementations of functions that already exist in google_util.

This CL removes these functions and uses the ones from
google_util that are not bugged, widely used and more maintained.

Some dead code is removed as well.
==========

to

==========
[Prerender] Cleanup prerender_util

IsGoogleDomain() and IsGoogleSearchResultURL() are bad
re-implementations of functions that already exist in google_util.

This CL removes these functions and uses the ones from
google_util that are not bugged, widely used and more maintained.

ALLOW_NON_STANDARD_PORT is used, as it matches existing behavior, and is
required by integration tests. It would certainly be good to disallow
such ports as future work.

Some dead code is removed as well.
==========

[droger, 2017-01-20 12:07:30]

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

[droger, 2017-01-20 12:07:30]

mattcary for general review

I'll file a bug about removing the ALLOW_NON_STANDARD_HOST as future work.

CC pkasting, isherman FYI: in case you want to double check the use of the
google_util functions.

[droger, 2017-01-20 12:08:17]

Description was changed from

==========
[Prerender] Cleanup prerender_util

IsGoogleDomain() and IsGoogleSearchResultURL() are bad
re-implementations of functions that already exist in google_util.

This CL removes these functions and uses the ones from
google_util that are not bugged, widely used and more maintained.

ALLOW_NON_STANDARD_PORT is used, as it matches existing behavior, and is
required by integration tests. It would certainly be good to disallow
such ports as future work.

Some dead code is removed as well.
==========

to

==========
[Prerender] Cleanup prerender_util

IsGoogleDomain() and IsGoogleSearchResultURL() are bad
re-implementations of functions that already exist in google_util.

This CL removes these functions and uses the ones from
google_util that are not bugged, widely used and more maintained.

ALLOW_NON_STANDARD_PORT is used, as it matches existing behavior, and is
required by integration tests. It would certainly be good to disallow
such ports as future work.

Some dead code is removed as well.
==========

[mattcary, 2017-01-20 13:24:21]

lgtm

https://codereview.chromium.org/2645713003/diff/120001/chrome/test/data/prere...
File chrome/test/data/prerender/prerender_loader_with_replace_state.html
(right):

https://codereview.chromium.org/2645713003/diff/120001/chrome/test/data/prere...
chrome/test/data/prerender/prerender_loader_with_replace_state.html:8:
history.replaceState(null, "Preloader", "/");
What's going on here?

[droger, 2017-01-20 13:41:39]

https://codereview.chromium.org/2645713003/diff/120001/chrome/test/data/prere...
File chrome/test/data/prerender/prerender_loader_with_replace_state.html
(right):

https://codereview.chromium.org/2645713003/diff/120001/chrome/test/data/prere...
chrome/test/data/prerender/prerender_loader_with_replace_state.html:8:
history.replaceState(null, "Preloader", "/");
On 2017/01/20 13:24:21, mattcary wrote:
> What's going on here?

www.google.com/search is actually not a valid search URL.

It has to either have a query parameter, like:
www.google.com/search?q=hi
or the empty path
www.google.com/

However, because of the embedded server the URL would actually look like
www.google.com:49054/search?q=hi
It would actually not work here in the test, because there is still a
non-standard port check that I could not remove in this case. 

So I went with the empty path, which specifically allows non standard ports.


The cleaner way would be to inject the "IsGoogleSearchUrl()" function somewhere,
so that production code would disallow non-standard port, but tests would allow
it.

[droger, 2017-01-20 13:45:00]

While I'm not 100% sure, I think the reason why the empty path is allowed is
because of referrer policies.

This function is called on urls passed in the "referrer" HTTP header, and
sometimes the path is stripped.

[droger, 2017-01-20 13:45:03]

While I'm not 100% sure, I think the reason why the empty path is allowed is
because of referrer policies.

This function is called on urls passed in the "referrer" HTTP header, and
sometimes the path is stripped.

[Ilya Sherman, 2017-01-21 00:21:08]

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

[Ilya Sherman, 2017-01-21 00:21:09]

Use of google_utils LGTM.  A couple of nits inline:

https://codereview.chromium.org/2645713003/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_util.cc (right):

https://codereview.chromium.org/2645713003/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_util.cc:38: bool IsGWSOriginURL(const GURL&
origin_url) {
nit: "GWS" is not a commonly understood acronym outside of Google.  Please use
something that folks are more likely to easily understand.

https://codereview.chromium.org/2645713003/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_util.cc:39: // ALLOW_NON_STANDARD_PORTS for
integration tests with the embedded server.
Optional: I'm not a fan of loosening production code guarantees for the sake of
testing code.  Could this be handled as an explicit dependency instead?

[droger, 2017-01-23 10:51:17]

Thanks.

https://codereview.chromium.org/2645713003/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_util.cc (right):

https://codereview.chromium.org/2645713003/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_util.cc:38: bool IsGWSOriginURL(const GURL&
origin_url) {
On 2017/01/21 00:21:08, Ilya Sherman wrote:
> nit: "GWS" is not a commonly understood acronym outside of Google.  Please use
> something that folks are more likely to easily understand.

Done.

Note: I think it was OK because "gws" is consistent with the prerender::Origin
type and UMA histograms (we have ORIGIN_GWS_PRERENDER and a "gws" family of
prerender histograms). But another name works too.

https://codereview.chromium.org/2645713003/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_util.cc:39: // ALLOW_NON_STANDARD_PORTS for
integration tests with the embedded server.
On 2017/01/21 00:21:09, Ilya Sherman wrote:
> Optional: I'm not a fan of loosening production code guarantees for the sake
of
> testing code.  Could this be handled as an explicit dependency instead?

I agree completely. This is not a new behavior introduced in this CL though, and
is non-trivial to fix.

I'm filing bug https://crbug.com/683853 for follow up.

[droger, 2017-01-23 10:51:23]

The CQ bit was checked by droger@chromium.org

[droger, 2017-01-23 10:51:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, mattcary@chromium.org
Link to the patchset: https://codereview.chromium.org/2645713003/#ps140001
(title: "Review comments")

[commit-bot: I haz the power, 2017-01-23 10:51:38]

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-23 11:46:01]

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

[commit-bot: I haz the power, 2017-01-23 11:46:02]

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

[droger, 2017-01-23 13:00:34]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2017-01-23 13:00:44]

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-23 15:15:53]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485176434467120,
"parent_rev": "5dc22a61fc8e68b59cc7e82d2f616daa185c6507", "commit_rev":
"4239cebc352889db1bd41ba9abbc17d9261a5769"}

[commit-bot: I haz the power, 2017-01-23 15:17:40]

Description was changed from

==========
[Prerender] Cleanup prerender_util

IsGoogleDomain() and IsGoogleSearchResultURL() are bad
re-implementations of functions that already exist in google_util.

This CL removes these functions and uses the ones from
google_util that are not bugged, widely used and more maintained.

ALLOW_NON_STANDARD_PORT is used, as it matches existing behavior, and is
required by integration tests. It would certainly be good to disallow
such ports as future work.

Some dead code is removed as well.
==========

to

==========
[Prerender] Cleanup prerender_util

IsGoogleDomain() and IsGoogleSearchResultURL() are bad
re-implementations of functions that already exist in google_util.

This CL removes these functions and uses the ones from
google_util that are not bugged, widely used and more maintained.

ALLOW_NON_STANDARD_PORT is used, as it matches existing behavior, and is
required by integration tests. It would certainly be good to disallow
such ports as future work.

Some dead code is removed as well.

Review-Url: https://codereview.chromium.org/2645713003
Cr-Commit-Position: refs/heads/master@{#445374}
Committed:
https://chromium.googlesource.com/chromium/src/+/4239cebc352889db1bd41ba9abbc...
==========

[commit-bot: I haz the power, 2017-01-23 15:17:41]

Committed patchset #4 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/4239cebc352889db1bd41ba9abbc...