New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 655467 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 348655



Sign in to add a comment

Replace <url> == GURL(<string constant>) with <url>.spec() == <string constant>

Project Member Reported by csharrison@chromium.org, Oct 13 2016

Issue description

GURL(<string constant>) will always re-parse the URL which can be slow. We do some of these equality checks many times per page load.
 
Blocking: 348655
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ebeca8e21727b975523416e9abaa2a4024ac36f5

commit ebeca8e21727b975523416e9abaa2a4024ac36f5
Author: csharrison <csharrison@chromium.org>
Date: Tue Oct 18 02:35:36 2016

Add operator==(const GURL&, const StringPiece&) to gurl.h.

This patch moves GURL::operator==(const GURL&) to a free-floating function,
and adds a new function to allow comparisons with StringPiece specs.

The new equality operator should be used when the spec of the URL is known
apriori, and parsing a GURL is not necessary for a comparison. This is
specifically designed to replace the case where we compare:
<url> = GURL(kUrlStringConstant).

One such case is replaced by this patch, and the rest will be modified in
a followup.

Note: this patch requires changes to instances of
base::Bind(&GURL::operator==...). They have been edited to use static_cast
to let Bind know that the operator== acts on GURLs.

BUG=655467

Review-Url: https://codereview.chromium.org/2421383003
Cr-Commit-Position: refs/heads/master@{#425863}

[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/chrome/browser/history/history_utils.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/chrome/browser/password_manager/native_backend_libsecret_unittest.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/chrome/browser/password_manager/password_store_mac_unittest.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/chrome/browser/search_engines/template_url_service_unittest.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/chrome/renderer/autofill/form_autofill_browsertest.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/chrome/renderer/extensions/chrome_extensions_renderer_client.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/components/domain_reliability/monitor_unittest.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/components/password_manager/core/browser/statistics_table_unittest.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/components/power/origin_power_map_unittest.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/content/browser/download/download_manager_impl_unittest.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/extensions/renderer/script_injection_manager.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/net/quic/chromium/quic_stream_factory_test.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/url/gurl.cc
[modify] https://crrev.com/ebeca8e21727b975523416e9abaa2a4024ac36f5/url/gurl.h

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0b2013bac16787ab019357a3561288fadb68b87b

commit 0b2013bac16787ab019357a3561288fadb68b87b
Author: csharrison <csharrison@chromium.org>
Date: Wed Oct 19 15:54:42 2016

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

Review-Url: https://chromiumcodereview.appspot.com/2409423005
Cr-Commit-Position: refs/heads/master@{#426207}

[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/browser/android/data_usage/data_use_ui_tab_model.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/browser/app_controller_mac.mm
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/browser/prerender/prerender_manager.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/browser/search/search.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/browser/sessions/session_restore.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/browser/sync/chrome_sync_client.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/browser/ui/search/new_tab_page_interceptor_service.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/browser/ui/search/search_tab_helper.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/browser/ui/singleton_tabs.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/browser/ui/startup/startup_browser_creator_impl.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/browser/ui/webui/uber/uber_ui.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/renderer/content_settings_observer.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/renderer/extensions/resource_request_policy.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/renderer/printing/chrome_print_web_view_helper_delegate.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/chrome/utility/importer/bookmarks_file_importer.cc
[modify] https://crrev.com/0b2013bac16787ab019357a3561288fadb68b87b/content/public/common/url_constants.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a3bd0b372da5a2dffefbf1d928db1695ae2a9939

commit a3bd0b372da5a2dffefbf1d928db1695ae2a9939
Author: csharrison <csharrison@chromium.org>
Date: Wed Oct 19 18:40:48 2016

Avoid unnecessary URL parsing for GURL comparisons in //content

This patch uses the new operator==(const GURL&, const StringPiece&)
function to optimize URL comparisons with string constants, in //content.

BUG=655467
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://chromiumcodereview.appspot.com/2424913003
Cr-Commit-Position: refs/heads/master@{#426250}

[modify] https://crrev.com/a3bd0b372da5a2dffefbf1d928db1695ae2a9939/content/browser/frame_host/debug_urls.cc
[modify] https://crrev.com/a3bd0b372da5a2dffefbf1d928db1695ae2a9939/content/browser/frame_host/frame_tree.cc
[modify] https://crrev.com/a3bd0b372da5a2dffefbf1d928db1695ae2a9939/content/browser/frame_host/frame_tree_node.cc
[modify] https://crrev.com/a3bd0b372da5a2dffefbf1d928db1695ae2a9939/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/a3bd0b372da5a2dffefbf1d928db1695ae2a9939/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/a3bd0b372da5a2dffefbf1d928db1695ae2a9939/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/a3bd0b372da5a2dffefbf1d928db1695ae2a9939/content/common/navigation_params.cc
[modify] https://crrev.com/a3bd0b372da5a2dffefbf1d928db1695ae2a9939/content/renderer/render_frame_impl.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/370250aa8528244380cc222f15ef2467d582a493

commit 370250aa8528244380cc222f15ef2467d582a493
Author: cfredric <cfredric@chromium.org>
Date: Tue Nov 15 22:38:56 2016

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

Review-Url: https://codereview.chromium.org/2485253002
Cr-Commit-Position: refs/heads/master@{#432275}

[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/android_webview/browser/aw_permission_manager_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/chrome/browser/bookmarks/managed_bookmark_service_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/chrome/browser/browser_about_handler.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/chrome/browser/browsing_data/conditional_cache_deletion_helper_browsertest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/chrome/browser/captive_portal/captive_portal_browsertest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/chrome/browser/extensions/tab_helper.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/chrome/browser/history/history_tab_helper.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/chrome/browser/printing/cloud_print/privet_notifications_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/chrome/browser/ui/search/search_tab_helper.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/chrome/browser/ui/webui/browsing_history_handler_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/components/bookmarks/managed/managed_bookmarks_tracker_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/components/data_usage/core/data_use_aggregator_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/components/drive/drive_uploader_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/components/error_page/renderer/net_error_helper_core.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/components/history/core/browser/history_querying_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/components/history/core/test/fake_web_history_service.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/components/navigation_interception/intercept_navigation_throttle_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/components/ntp_snippets/remote/ntp_snippets_fetcher.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/components/security_state/security_state_model_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/content/browser/appcache/appcache_request_handler_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/content/browser/fileapi/obfuscated_file_util_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/content/browser/quota/quota_manager_unittest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/content/public/common/url_constants.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/content/test/data/dom_storage/webcore_test_database.localstorage
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/extensions/browser/guest_view/extension_view/extension_view_guest.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/ios/chrome/browser/browser_about_rewriter.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/ios/chrome/browser/history/history_utils.cc
[modify] https://crrev.com/370250aa8528244380cc222f15ef2467d582a493/url/gurl.cc

Cc: cfredric@chromium.org
cfredric: how does the usage situation look like after your patch? Should we close the issue?
I think there's a little more work to be done before closing this. Currently there are (according to `git grep`):
0 instances of "GURL([^()]*) !="
2 instances of "GURL([^()]*) =="
0 instances of "!= GURL([^()]*)"
24 instances of "== GURL([^()]*)"

Of those 26 instances, it looks to me like at least 13 are just reparsing constants and should be fairly straightforward to fix. They're mostly in ios- or win-specific code.

3 of the 26 instances are reparsing a constant and then getting the origin: "... == GURL(kSomeConstant).GetOrigin()". Those aren't in unittests, so it seems like we might want to avoid rerunning that computation if we can.
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/89296468bc813b73a5e574e131f03f9d9c062104

commit 89296468bc813b73a5e574e131f03f9d9c062104
Author: Andrew Moylan <amoylan@chromium.org>
Date: Tue Oct 10 07:09:28 2017

Add operator== and operator!=(StringPiece, GURL)

We already have operator==(GURL, StringPiece) and operator!=(GURL,
StringPiece). The CL adds variants with the arguments the other way
around.

Motivation: Compatibility with EXPECT_EQ(expected, actual) in
googletest:

GURL gurl = ...;
EXPECT_EQ("https://www.google.com/", gurl);

Updated the motivating test case in this CL also.

Bug: 655467
Change-Id: I170bdd8c4219fabd324fc596c95a941390ec36cd
Reviewed-on: https://chromium-review.googlesource.com/680854
Commit-Queue: Andrew Moylan <amoylan@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507601}
[modify] https://crrev.com/89296468bc813b73a5e574e131f03f9d9c062104/chrome/browser/geolocation/geolocation_browsertest.cc
[modify] https://crrev.com/89296468bc813b73a5e574e131f03f9d9c062104/url/gurl.cc
[modify] https://crrev.com/89296468bc813b73a5e574e131f03f9d9c062104/url/gurl.h

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c57fe765b6d5397d59ff9effc5a2774fe69341cd

commit c57fe765b6d5397d59ff9effc5a2774fe69341cd
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Mon Jan 29 09:50:42 2018

Avoid needless parsing constants URLs.

There is a global comparison operator allowing to check whether
a GURL is equal to a constant URL. This avoid needlessly parsing
the constant to a GURL everytime the comparison is done.

Bug: 655467
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I9c876b542f09235042c36026b32977d88c2f0d0b
Reviewed-on: https://chromium-review.googlesource.com/886421
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Stepan Khapugin <stkhapugin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532373}
[modify] https://crrev.com/c57fe765b6d5397d59ff9effc5a2774fe69341cd/ios/chrome/browser/sync/ios_chrome_sync_client.mm
[modify] https://crrev.com/c57fe765b6d5397d59ff9effc5a2774fe69341cd/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/c57fe765b6d5397d59ff9effc5a2774fe69341cd/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm
[modify] https://crrev.com/c57fe765b6d5397d59ff9effc5a2774fe69341cd/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/c57fe765b6d5397d59ff9effc5a2774fe69341cd/ios/chrome/browser/ui/first_run/welcome_to_chrome_view.mm
[modify] https://crrev.com/c57fe765b6d5397d59ff9effc5a2774fe69341cd/ios/chrome/browser/ui/omnibox/chrome_omnibox_client_ios.mm
[modify] https://crrev.com/c57fe765b6d5397d59ff9effc5a2774fe69341cd/ios/chrome/browser/ui/toolbar/web_toolbar_controller.mm
[modify] https://crrev.com/c57fe765b6d5397d59ff9effc5a2774fe69341cd/ios/chrome/browser/web/browsing_prevent_default_egtest.mm
[modify] https://crrev.com/c57fe765b6d5397d59ff9effc5a2774fe69341cd/ios/web/webui/crw_web_ui_page_builder_unittest.mm

Sign in to add a comment