Add more flexible handling of what is considered a google home page.

Add more flexible handling of what is considered a google home page.

BUG=http:b/5609053
TEST=See unit tests for valid and invalid URLs.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112726

[Roger Tawa OOO till Jul 10th, 2011-11-28 20:49:25]

Hi Steve,

Can you please review?  Thanks.

[SteveT, 2011-11-28 21:04:49]

A few small comments but mostly looking good...

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
File chrome/browser/google/google_util.cc (right):

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
chrome/browser/google/google_util.cc:85: bool IsGoogleHomePageUrl(const
std::string& url) {
Nit: Move down to roughly match header order.

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
chrome/browser/google/google_util.cc:95: if (!original_url.port().empty())
Do we want to allow :80 and :443 for http and https, respectively, or do we want
the port to be explicitly empty?

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
File chrome/browser/google/google_util.h (right):

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
chrome/browser/google/google_util.h:33: bool IsGoogleHomePageUrl(const
std::string& url);
Nit: Please move down after GetReactivationBrand, where it'll be in roughly
alphabetical order (even if some other methods aren't).

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
File chrome/browser/google/google_util_unittest.cc (right):

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
chrome/browser/google/google_util_unittest.cc:51: // Must be http or https.
Add a case for "www.google.com" (schemeless but valid TLD and subdomain)?

[Roger Tawa OOO till Jul 10th, 2011-11-29 16:00:07]

Thanks Steve, please take another look.  This list of good/bad URLs was passed
by PSO, includes a couple of changes.

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
File chrome/browser/google/google_util.cc (right):

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
chrome/browser/google/google_util.cc:85: bool IsGoogleHomePageUrl(const
std::string& url) {
On 2011/11/28 21:04:49, SteveT wrote:
> Nit: Move down to roughly match header order.

Done.

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
chrome/browser/google/google_util.cc:95: if (!original_url.port().empty())
On 2011/11/28 21:04:49, SteveT wrote:
> Do we want to allow :80 and :443 for http and https, respectively, or do we
want
> the port to be explicitly empty?

GURL does not make it easy to detect default port numbers for a given scheme, so
as long as the port number are the expected ones, I don't think it matters. 
However, I'm getting the PSO folks to double check the good and bad URL
mentioned in the test, so I'll ask them this too.

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
File chrome/browser/google/google_util.h (right):

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
chrome/browser/google/google_util.h:33: bool IsGoogleHomePageUrl(const
std::string& url);
On 2011/11/28 21:04:49, SteveT wrote:
> Nit: Please move down after GetReactivationBrand, where it'll be in roughly
> alphabetical order (even if some other methods aren't).

Done.

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
File chrome/browser/google/google_util_unittest.cc (right):

http://codereview.chromium.org/8728004/diff/5001/chrome/browser/google/google...
chrome/browser/google/google_util_unittest.cc:51: // Must be http or https.
On 2011/11/28 21:04:49, SteveT wrote:
> Add a case for "www.google.com" (schemeless but valid TLD and subdomain)?

Done.

[SteveT, 2011-11-29 16:08:50]

LGTM. If PSO is cool with all the URLs, feel free to commit!

[commit-bot: I haz the power, 2011-12-02 16:20:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/8728004/4006

[commit-bot: I haz the power, 2011-12-02 17:39:38]

Change committed as 112726