Remove unnecessary calls to GURL()

Remove unnecessary calls to GURL().

This follows on to https://codereview.chromium.org/2421383003, and updates a few
more usages of operator==(const GURL& x, const GURL& y) to use operator==(const
GURL& x, const base::StringPiece& spec) instead. This updates usages of the
appropriate operator!= as well.

BUG=655467
Committed: https://crrev.com/370250aa8528244380cc222f15ef2467d582a493
Cr-Commit-Position: refs/heads/master@{#432275}

[cfredric, 2016-11-08 21:23:38]

Description was changed from

==========
Remove unnecessary calls to GURL()

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

to

==========
Remove unnecessary calls to GURL()

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

[cfredric, 2016-11-08 21:24:16]

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

[commit-bot: I haz the power, 2016-11-08 21:25:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cfredric, 2016-11-08 21:30:25]

Description was changed from

==========
Remove unnecessary calls to GURL()

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

to

==========
This is currently a work in progress.

Remove unnecessary calls to GURL(). This follows on to
https://codereview.chromium.org/2421383003, and updates a few more usages of
operator==(const GURL& x, const GURL& y) to use operator==(const GURL& x, const
base::StringPiece& spec) instead.

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

[commit-bot: I haz the power, 2016-11-08 22:04:07]

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

[commit-bot: I haz the power, 2016-11-08 22:04:08]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[cfredric, 2016-11-09 18:11:39]

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

[commit-bot: I haz the power, 2016-11-09 18:12:19]

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-11-09 18:48:34]

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

[commit-bot: I haz the power, 2016-11-09 18:48:35]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[cfredric, 2016-11-09 19:17:16]

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

[commit-bot: I haz the power, 2016-11-09 19:17:40]

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-11-09 20:13:33]

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

[commit-bot: I haz the power, 2016-11-09 20:13:34]

Dry run: This issue passed the CQ dry run.

[cfredric, 2016-11-09 20:22:27]

Description was changed from

==========
This is currently a work in progress.

Remove unnecessary calls to GURL(). This follows on to
https://codereview.chromium.org/2421383003, and updates a few more usages of
operator==(const GURL& x, const GURL& y) to use operator==(const GURL& x, const
base::StringPiece& spec) instead.

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

to

==========
Remove unnecessary calls to GURL(). This follows on to
https://codereview.chromium.org/2421383003, and updates a few more usages of
operator==(const GURL& x, const GURL& y) to use operator==(const GURL& x, const
base::StringPiece& spec) instead. This updates usages of the appropriate
operator!= as well.

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

[cfredric, 2016-11-09 21:00:16]

cfredric@chromium.org changed reviewers:
+ boliu@chromium.org, csharrison@chromium.org, esprehn@chromium.org,
estark@chromium.org, eugenebut@chromium.org, fukino@chromium.org,
juliatuttle@chromium.org, michaelbai@chromium.org, rockot@chromium.org,
sclittle@chromium.org, sky@chromium.org, treib@chromium.org,
tsergeant@chromium.org, zhongyi@chromium.org

[cfredric, 2016-11-09 21:00:16]

rockot@chromium.org: Please review changes in
chrome/browser/android/data_usage/data_use_ui_tab_model.cc.

treib@chromium.org: Please review changes in
components/ntp_snippets/remote/ntp_snippets_fetcher.cc.

fukino@chromium.org: Please review changes in
components/drive/drive_uploader_unittest.cc

tsergeant@chromium.org: Please review changes in
chrome/browser/ui/webui/browsing_history_handler_unittest.cc

zhongyi@chromium.org: Please review changes in net/url_request/url_request.cc

estark@chromium.org: Please review changes in
components/security_state/security_state_model_unittest.cc

esprehn@chromium.org: Please review changes in
content/renderer/render_frame_impl.cc,
content/renderer/render_view_browsertest.cc

michaelbai@chromium.org: Please review changes in
android_webview/browser/aw_permission_manager_unittest.cc,
components/navigation_interception/intercept_navigation_throttle_unittest.cc

sclittle@chromium.org: Please review changes in
chrome/browser/android/data_usage/data_use_ui_tab_model.cc,
components/data_usage/core/data_use_aggregator_unittest.cc

eugenebut@chromium.org: Please review changes in
ios/chrome/browser/browser_about_rewriter.cc,
ios/chrome/browser/history/history_utils.cc

juliatuttle@chromium.org: Please review changes in
components/error_page/renderer/net_error_helper_core.cc

boliu@chromium.org: Please review changes in
content/browser/appcache/appcache_request_handler_unittest.cc,
content/browser/fileapi/obfuscated_file_util_unittest.cc,
content/browser/quota/quota_manager_unittest.cc

sky@chromium.org: Please review changes in
chrome/browser/android/data_usage/data_use_ui_tab_model.cc,
chrome/browser/bookmarks/managed_bookmark_service_unittest.cc,
chrome/browser/browser_about_handler.cc,
chrome/browser/browsing_data/conditional_cache_deletion_helper_browsertest.cc,
chrome/browser/captive_portal/captive_portal_browsertest.cc,
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc,
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc,
chrome/browser/extensions/tab_helper.cc,
chrome/browser/history/history_tab_helper.cc,
chrome/browser/net/predictor_unittest.cc,
chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc,
chrome/browser/printing/cloud_print/privet_notifications_unittest.cc,
chrome/browser/ui/search/search_tab_helper.cc,
components/bookmarks/managed/managed_bookmarks_tracker_unittest.cc,
components/history/core/browser/history_querying_unittest.cc, 
components/history/core/test/fake_web_history_service.cc

[esprehn, 2016-11-09 21:05:49]

Description was changed from

==========
Remove unnecessary calls to GURL(). This follows on to
https://codereview.chromium.org/2421383003, and updates a few more usages of
operator==(const GURL& x, const GURL& y) to use operator==(const GURL& x, const
base::StringPiece& spec) instead. This updates usages of the appropriate
operator!= as well.

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

to

==========
Remove unnecessary calls to GURL().

This follows on to https://codereview.chromium.org/2421383003, and updates a few
more usages of operator==(const GURL& x, const GURL& y) to use operator==(const 
GURL& x, const base::StringPiece& spec) instead. This updates usages of the 
appropriate operator!= as well.

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

[esprehn, 2016-11-09 21:09:25]

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/android/...
File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right):

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_ui_tab_model.cc:195:
DCHECK(!navigation_entry || (navigation_entry->GetURL() == url));
why don't we need to canonicalize url?

https://codereview.chromium.org/2485253002/diff/40001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/2485253002/diff/40001/net/url_request/url_req...
net/url_request/url_request.cc:646: url()) != referrer_) {
ditto, why do we not need to canonicalize referrer_?

[Charlie Harrison, 2016-11-09 21:12:59]

Generally looks good, but I'm wary of using this operator on dynamic values
unless we are sure (enough to DCHECK) that they are already canonicalized.

Brief first pass:

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/android/...
File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right):

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_ui_tab_model.cc:185: GURL(url),
&transition_type)) {
Note that we GURL(url) here. Let's just use that, as I'm not sure |url| will
always be canonicalized here.

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/managed_bookmark_service_unittest.cc (right):

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/managed_bookmark_service_unittest.cc:129: if
(!dict->GetString("url", &url) || node->url() != url)
Will |url| always be canonicalized here?

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:328:
GURL(content::kAboutSrcDocURL) ||
Can you change GURL(content::kAboutSrcDocURL) too?

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/net/pred...
File chrome/browser/net/predictor_unittest.cc (right):

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/net/pred...
chrome/browser/net/predictor_unittest.cc:70: if (motivation == existing_spec)
Let's keep the GURL() here, or at least DCHECK that GURL(existing_spec) ==
existing_spec.

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/net/pred...
chrome/browser/net/predictor_unittest.cc:141: if (subresource == url_spec) {
same here. I'm generally nervous about using this operator on dynamic values.

https://codereview.chromium.org/2485253002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_request_handler_unittest.cc (right):

https://codereview.chromium.org/2485253002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_request_handler_unittest.cc:1005:
EXPECT_TRUE(host_->blocked_manifest_url_ == "http://blah/manifest/");
Use EXPECT_EQ instead of EXPECT_TRUE.

[Eugene But (OOO till 7-30), 2016-11-09 21:18:26]

ios lgtm

[esprehn, 2016-11-09 21:21:25]

Can we assert inside the operator that the value must be canonicalized?

bool operator==(const GURL& a, StringPiece other) {
  DCHECK_EQ(a, GURL(other));
  ...
}

[cfredric, 2016-11-09 22:09:59]

On 2016/11/09 21:21:25, esprehn wrote:
> Can we assert inside the operator that the value must be canonicalized?
> 
> bool operator==(const GURL& a, StringPiece other) {
>   DCHECK_EQ(a, GURL(other));
>   ...
> }

Sounds like a good idea to me. @csharrison, what do you think?

[cfredric, 2016-11-09 22:12:46]

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/android/...
File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right):

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_ui_tab_model.cc:185: GURL(url),
&transition_type)) {
On 2016/11/09 21:12:58, Charlie Harrison wrote:
> Note that we GURL(url) here. Let's just use that, as I'm not sure |url| will
> always be canonicalized here.

Done.

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_ui_tab_model.cc:195:
DCHECK(!navigation_entry || (navigation_entry->GetURL() == url));
On 2016/11/09 21:09:25, esprehn wrote:
> why don't we need to canonicalize url?

I'm not sure that we don't. Changed back.

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/bookmark...
File chrome/browser/bookmarks/managed_bookmark_service_unittest.cc (right):

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/bookmark...
chrome/browser/bookmarks/managed_bookmark_service_unittest.cc:129: if
(!dict->GetString("url", &url) || node->url() != url)
On 2016/11/09 21:12:58, Charlie Harrison wrote:
> Will |url| always be canonicalized here?

Currently, yes. The only usage of DictionaryValue::SetString with a key of "url"
uses GURL(url).spec() as the value.

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:328:
GURL(content::kAboutSrcDocURL) ||
On 2016/11/09 21:12:58, Charlie Harrison wrote:
> Can you change GURL(content::kAboutSrcDocURL) too?

Done.

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/net/pred...
File chrome/browser/net/predictor_unittest.cc (right):

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/net/pred...
chrome/browser/net/predictor_unittest.cc:70: if (motivation == existing_spec)
On 2016/11/09 21:12:58, Charlie Harrison wrote:
> Let's keep the GURL() here, or at least DCHECK that GURL(existing_spec) ==
> existing_spec.

Done.

https://codereview.chromium.org/2485253002/diff/40001/chrome/browser/net/pred...
chrome/browser/net/predictor_unittest.cc:141: if (subresource == url_spec) {
On 2016/11/09 21:12:58, Charlie Harrison wrote:
> same here. I'm generally nervous about using this operator on dynamic values.

Done.

https://codereview.chromium.org/2485253002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_request_handler_unittest.cc (right):

https://codereview.chromium.org/2485253002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_request_handler_unittest.cc:1005:
EXPECT_TRUE(host_->blocked_manifest_url_ == "http://blah/manifest/");
On 2016/11/09 21:12:58, Charlie Harrison wrote:
> Use EXPECT_EQ instead of EXPECT_TRUE.

Done.

https://codereview.chromium.org/2485253002/diff/40001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/2485253002/diff/40001/net/url_request/url_req...
net/url_request/url_request.cc:646: url()) != referrer_) {
On 2016/11/09 21:09:25, esprehn wrote:
> ditto, why do we not need to canonicalize referrer_?

I had thought it was already canonicalized here. Changed back.

Is it worth saving the canonicalized value so we can reuse it in the next if
condition? Or is that memory we don't want to use?

[tsergeant, 2016-11-09 23:10:38]

browsing_history_handler_unittest lgtm

[boliu, 2016-11-09 23:15:48]

a_w lgtm

[sclittle, 2016-11-09 23:24:40]

components/data_usage lgtm

[sky, 2016-11-10 00:01:38]

LGTM

[Charlie Harrison, 2016-11-10 00:05:54]

On 2016/11/09 22:09:59, cfredric wrote:
> On 2016/11/09 21:21:25, esprehn wrote:
> > Can we assert inside the operator that the value must be canonicalized?
> > 
> > bool operator==(const GURL& a, StringPiece other) {
> >   DCHECK_EQ(a, GURL(other));
> >   ...
> > }
> 
> Sounds like a good idea to me. @csharrison, what do you think?

Yeah let's do it! Good idea esprehn@.

[estark, 2016-11-10 00:58:12]

components/security_state/security_state_model_unittest.cc lgtm

[esprehn, 2016-11-10 00:58:46]

lgtm

[michaelbai, 2016-11-10 02:01:08]

android_webview and components/navigation_interception LGTM

[Marc Treib, 2016-11-10 09:11:51]

components/ntp_snippets/ lgtm, assuming we get that DCHECK in the operator==
implementation

[cfredric, 2016-11-10 15:29:14]

cfredric@chromium.org changed reviewers:
+ abarth@chromium.org, clamy@chromium.org

[cfredric, 2016-11-10 15:29:15]

clamy@chromium.org: Please review changes in
content/public/common/url_constants.cc.

abarth@chromium.org: Please review changes in url/gurl.cc.

estark@chromium.org: Please review changes in
components/security_state/security_state_model_unittest.cc.

[cfredric, 2016-11-10 15:30:36]

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

[commit-bot: I haz the power, 2016-11-10 15:31:01]

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-11-10 15:54:48]

//url non-owners LGTM

[commit-bot: I haz the power, 2016-11-10 16:49:00]

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

[commit-bot: I haz the power, 2016-11-10 16:49:02]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-11-10 17:20:01]

lgtm

[Zhongyi Shi, 2016-11-10 18:50:24]

lgtm

[Charlie Harrison, 2016-11-11 16:02:28]

csharrison@chromium.org changed reviewers:
+ brettw@chromium.org
- abarth@chromium.org

[Charlie Harrison, 2016-11-11 16:02:29]

-abarth, +brettw for DCHECK in gurl.

[Julia Tuttle, 2016-11-14 18:54:34]

components/error_page lgtm.

[brettw, 2016-11-14 19:14:42]

url lgtm

[cfredric, 2016-11-15 20:01:59]

The CQ bit was checked by cfredric@chromium.org

[cfredric, 2016-11-15 20:02:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, sky@chromium.org, sclittle@chromium.org,
michaelbai@chromium.org, esprehn@chromium.org, boliu@chromium.org,
tsergeant@chromium.org, estark@chromium.org, treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2485253002/#ps80001
(title: "Assert that StringPiece must always be canonicalized. Fix some
constants.")

[commit-bot: I haz the power, 2016-11-15 20:02:26]

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-11-15 22:39:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-15 22:46:29]

Description was changed from

==========
Remove unnecessary calls to GURL().

This follows on to https://codereview.chromium.org/2421383003, and updates a few
more usages of operator==(const GURL& x, const GURL& y) to use operator==(const 
GURL& x, const base::StringPiece& spec) instead. This updates usages of the 
appropriate operator!= as well.

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

to

==========
Remove unnecessary calls to GURL().

This follows on to https://codereview.chromium.org/2421383003, and updates a few
more usages of operator==(const GURL& x, const GURL& y) to use operator==(const
GURL& x, const base::StringPiece& spec) instead. This updates usages of the
appropriate operator!= as well.

BUG=655467

Committed: https://crrev.com/370250aa8528244380cc222f15ef2467d582a493
Cr-Commit-Position: refs/heads/master@{#432275}
==========

[commit-bot: I haz the power, 2016-11-15 22:46:31]

Patchset 5 (id:??) landed as
https://crrev.com/370250aa8528244380cc222f15ef2467d582a493
Cr-Commit-Position: refs/heads/master@{#432275}