Compare GURLs to kUrlConstants with .spec() and string equality

Compare URL constants with .spec() and string equality

This patch replaces instances of "<url> == GURL(<string constant>)" with
"<url> == <string constant>" for string constants that are valid
GURL specs, in //chrome, using the new operator== defined for both
GURL and StringPiece.

This prevents needlessly reparsing the constant GURL, which is slow.

Follow-up changes will change these comparisons in other portions of
the code base.

BUG=655467
Committed: https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b
Cr-Commit-Position: refs/heads/master@{#426207}

[Charlie Harrison, 2016-10-13 04:24:12]

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

[commit-bot: I haz the power, 2016-10-13 04:24:25]

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-10-13 05:32:19]

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

[commit-bot: I haz the power, 2016-10-13 05:32:22]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-10-13 12:48:27]

csharrison@chromium.org changed reviewers:
+ jochen@chromium.org, sky@chromium.org

[Charlie Harrison, 2016-10-13 12:48:28]

sky: ptal at 
 - chrome/browser/ui
 - chrome/browser/history
 - chrome/browser/sessions

jochen: would you ptal at the rest? I can find more specific OWNERS but it would
end up being a big list for such a small change.

Note: the main thing to check in this CL is that the associated URL constant is
equal to the spec it would produce after GURL construction. Usually this means
it has a trailing /.

[sky, 2016-10-13 17:47:05]

sky@chromium.org changed reviewers:
+ brettw@chromium.org

[sky, 2016-10-13 17:47:06]

sky->brettw

[jochen (gone - plz use gerrit), 2016-10-14 09:15:15]

lgtm

[brettw, 2016-10-14 18:01:24]

What do you think about defining operator== between a GURL and a StringPiece
(assuming this will get automatically created from a char[] for the operator).

I have some concerns about some of the callers not being valid and throwing
exceptions in debug builds. When we wrote GURL, the style guide had a stronger
prohibition on operator overloading.

https://codereview.chromium.org/2409423005/diff/20001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/2409423005/diff/20001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:719: startupContent->GetVisibleURL().spec()
== chrome::kChromeUINewTabURL) {
Is the visible URL always guaranteed to be valid? What happens if you have an
invalid URL on the command line?

[Charlie Harrison, 2016-10-14 18:13:55]

I'm supportive of adding operator== for StringPiece, if you are. That would
avoid the issue you noticed with GetVisibleURL(). It improves the ergonomics of
the API so consumers don't have to worry about calling spec() or invalid_spec().

https://codereview.chromium.org/2409423005/diff/20001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/2409423005/diff/20001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:719: startupContent->GetVisibleURL().spec()
== chrome::kChromeUINewTabURL) {
On 2016/10/14 18:01:24, brettw (ping on IM after 24h) wrote:
> Is the visible URL always guaranteed to be valid? What happens if you have an
> invalid URL on the command line?

You're right, it can be invalid (in a whole bunch of different ways).

[brettw, 2016-10-14 19:15:00]

On 2016/10/14 18:13:55, Charlie Harrison wrote:
> I'm supportive of adding operator== for StringPiece, if you are. That would
> avoid the issue you noticed with GetVisibleURL(). It improves the ergonomics
of
> the API so consumers don't have to worry about calling spec() or
invalid_spec().

Okay, let's do the operators. I think the current best practice is to define
free-floating operator==, so maybe we want to move the existing member
operator== to the bottom of the file with the new ones and the existing stream
operator.

[Charlie Harrison, 2016-10-14 19:23:09]

On 2016/10/14 19:15:00, brettw (ping on IM after 24h) wrote:
> On 2016/10/14 18:13:55, Charlie Harrison wrote:
> > I'm supportive of adding operator== for StringPiece, if you are. That would
> > avoid the issue you noticed with GetVisibleURL(). It improves the ergonomics
> of
> > the API so consumers don't have to worry about calling spec() or
> invalid_spec().
> 
> Okay, let's do the operators. I think the current best practice is to define
> free-floating operator==, so maybe we want to move the existing member
> operator== to the bottom of the file with the new ones and the existing stream
> operator.

SGTM. I noticed a bunch of unit tests wanting to use
base::Bind(GURL::operator==,...)
which we will probably have to change. I'll see how big the change is, and
possibly
send out a separate CL that this one will depend on.

[Charlie Harrison, 2016-10-17 18:59:44]

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

[commit-bot: I haz the power, 2016-10-17 19:00:13]

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-10-17 20:05:41]

This patch is now based on a dependent PS that adds operator==(const GURL&,
const base::StringPiece&) to gurl.h. Apologies jochen but this caused (trivial)
changes in all the files you reviewed.

[Charlie Harrison, 2016-10-17 20:16:48]

Description was changed from

==========
Compare URL constants with .spec() and string equality

This patch replaces instances of "<url> == GURL(<string constant>)" with
"<url>.spec() == <string constant>" for string constants that are valid
GURL specs, in //chrome.

This prevents needlessly reparsing the constant GURL, which is slow.

BUG=655467
==========

to

==========
Compare URL constants with .spec() and string equality

This patch replaces instances of "<url> == GURL(<string constant>)" with
"<url> == <string constant>" for string constants that are valid
GURL specs, in //chrome, using the new operator== defined for both
GURL and StringPiece.


This prevents needlessly reparsing the constant GURL, which is slow.

Follow-up changes will change these comparisons in other portions of
the code base.

BUG=655467
==========

[commit-bot: I haz the power, 2016-10-17 22:55:40]

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

[commit-bot: I haz the power, 2016-10-17 22:55:42]

Dry run: Exceeded global retry quota

[brettw, 2016-10-17 23:01:09]

lgtm

[Charlie Harrison, 2016-10-17 23:08:47]

Thanks, I'll wait for a second pass from jochen@ before landing.

[jochen (gone - plz use gerrit), 2016-10-19 14:35:07]

still lgtm

[Charlie Harrison, 2016-10-19 15:00:08]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-10-19 15:00:31]

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-10-19 15:55:05]

Description was changed from

==========
Compare URL constants with .spec() and string equality

This patch replaces instances of "<url> == GURL(<string constant>)" with
"<url> == <string constant>" for string constants that are valid
GURL specs, in //chrome, using the new operator== defined for both
GURL and StringPiece.


This prevents needlessly reparsing the constant GURL, which is slow.

Follow-up changes will change these comparisons in other portions of
the code base.

BUG=655467
==========

to

==========
Compare URL constants with .spec() and string equality

This patch replaces instances of "<url> == GURL(<string constant>)" with
"<url> == <string constant>" for string constants that are valid
GURL specs, in //chrome, using the new operator== defined for both
GURL and StringPiece.


This prevents needlessly reparsing the constant GURL, which is slow.

Follow-up changes will change these comparisons in other portions of
the code base.

BUG=655467
==========

[commit-bot: I haz the power, 2016-10-19 15:55:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-21 13:08:54]

Description was changed from

==========
Compare URL constants with .spec() and string equality

This patch replaces instances of "<url> == GURL(<string constant>)" with
"<url> == <string constant>" for string constants that are valid
GURL specs, in //chrome, using the new operator== defined for both
GURL and StringPiece.


This prevents needlessly reparsing the constant GURL, which is slow.

Follow-up changes will change these comparisons in other portions of
the code base.

BUG=655467
==========

to

==========
Compare URL constants with .spec() and string equality

This patch replaces instances of "<url> == GURL(<string constant>)" with
"<url> == <string constant>" for string constants that are valid
GURL specs, in //chrome, using the new operator== defined for both
GURL and StringPiece.

This prevents needlessly reparsing the constant GURL, which is slow.

Follow-up changes will change these comparisons in other portions of
the code base.

BUG=655467

Committed: https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b
Cr-Commit-Position: refs/heads/master@{#426207}
==========

[commit-bot: I haz the power, 2016-10-21 13:08:55]

Patchset 3 (id:??) landed as
https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b
Cr-Commit-Position: refs/heads/master@{#426207}