Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(61)

Issue 2695883003: Change uses of base::JoinString to pass StringPieces where possible. (Closed)

Created:
3 years, 10 months ago by Matt Giuca
Modified:
3 years, 9 months ago
CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, vmpstr+watch_chromium.org, nektar+watch_chromium.org, browser-components-watch_chromium.org, dmazzoni+watch_chromium.org, tracing+reviews_chromium.org, stevenjb+watch_chromium.org, timvolodine, extensions-reviews_chromium.org, cbentzel+watch_chromium.org, aboxhall+watch_chromium.org, sebsg+autofillwatch_chromium.org, tzik, net-reviews_chromium.org, nhiroki, nona+watch_chromium.org, je_julie, rlp+watch_chromium.org, rogerm+autofillwatch_chromium.org, groby+spellwatch_chromium.org, chromium-apps-reviews_chromium.org, tbansal+watch-data-reduction-proxy_chromium.org, derat+watch_chromium.org, rouslan+autofill_chromium.org, yuzo+watch_chromium.org, vabr+watchlistautofill_chromium.org, rouslan+spell_chromium.org, oshima+watch_chromium.org, kalyank, mathp+autofillwatch_chromium.org, wfh+watch_chromium.org, shuchen+watch_chromium.org, dtseng+watch_chromium.org, estade+watch_chromium.org, kinuko+fileapi, davemoore+watch_chromium.org, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Change uses of base::JoinString to pass StringPieces where possible. The calling code has been hand-modified to, wherever possible, create vectors of StringPieces (rather than strings) for use with base::JoinString. This reduces unnecessary string copying in the calling code (sometimes updating surrounding logic to also use StringPieces). BUG=348241 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2695883003 Cr-Commit-Position: refs/heads/master@{#459983} Committed: https://chromium.googlesource.com/chromium/src/+/30f7588201a275c75d0382ad4d63d4329a2fd9e8

Patch Set 1 #

Patch Set 2 : Rebase. #

Patch Set 3 : Convert lots more to JoinStringPiece. #

Patch Set 4 : Finished converting all occurrences I can find with XRefs. #

Patch Set 5 : Include what you use. #

Patch Set 6 : Cleanup. #

Patch Set 7 : Break autofill_data_util and translate_prefs_unittest into separate CLs. #

Patch Set 8 : Rebase. #

Patch Set 9 : Fix MultilingualSpellCheckTest (dangling pointer). #

Patch Set 10 : Fix android_webview compilation (and made FeatureList::SplitFeatureListString take StringPiece). #

Total comments: 10

Patch Set 11 : Fixed chromeos compilation (input_method_syncer). #

Patch Set 12 : Rebase. #

Patch Set 13 : chrome_browser_main: Update for new usage of JoinString (added since rebase). #

Patch Set 14 : Rebase. #

Patch Set 15 : Rebase. #

Patch Set 16 : Use by-reference iteration following discussion. #

Patch Set 17 : Revert use of auto at reviewer's request. #

Patch Set 18 : Remove dependency CL. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+243 lines, -186 lines) Patch
M android_webview/browser/command_line_helper.cc View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -2 lines 0 comments Download
M base/feature_list.h View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -3 lines 0 comments Download
M base/feature_list.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +5 lines, -5 lines 0 comments Download
M base/feature_list_unittest.cc View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M base/test/launcher/test_launcher.cc View 1 2 3 4 5 6 7 2 chunks +4 lines, -3 lines 0 comments Download
M base/trace_event/trace_log.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/chrome_browser_main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/chromeos/accessibility/accessibility_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +7 lines, -6 lines 0 comments Download
M chrome/browser/chromeos/input_method/input_method_syncer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 5 chunks +38 lines, -30 lines 0 comments Download
M chrome/browser/chromeos/power/peripheral_battery_observer.cc View 1 2 3 4 5 6 7 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc View 1 2 3 4 5 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/extensions/chrome_content_verifier_delegate.cc View 1 2 3 4 5 2 chunks +7 lines, -7 lines 0 comments Download
M chrome/browser/extensions/content_capabilities_browsertest.cc View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/extensions/webstore_installer.cc View 1 2 3 4 5 2 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/net/safe_search_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +6 lines, -6 lines 0 comments Download
M chrome/browser/spellchecker/spellcheck_service_browsertest.cc View 1 2 3 4 5 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/sync_file_system/drive_backend/metadata_database.cc View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +2 lines, -1 line 0 comments Download
M chrome/common/extensions/command.cc View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M chromeos/network/onc/onc_validator.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M components/autofill/content/renderer/autofill_agent.cc View 1 2 3 4 5 6 7 2 chunks +4 lines, -3 lines 0 comments Download
M components/autofill/core/browser/autofill_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +4 lines, -3 lines 0 comments Download
M components/autofill/core/browser/autofill_profile_comparator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 5 chunks +6 lines, -7 lines 0 comments Download
M components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc View 1 2 3 4 5 6 7 2 chunks +3 lines, -2 lines 0 comments Download
M components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc View 1 2 3 4 5 2 chunks +4 lines, -3 lines 0 comments Download
M components/flags_ui/flags_state.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +6 lines, -5 lines 0 comments Download
M components/history/core/browser/top_sites_database.cc View 1 2 3 4 5 2 chunks +4 lines, -3 lines 0 comments Download
M components/nacl/renderer/platform_info.cc View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M components/password_manager/core/browser/password_ui_utils.cc View 1 2 3 4 5 2 chunks +2 lines, -3 lines 0 comments Download
M components/policy/core/browser/policy_error_map.cc View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M components/safe_browsing_db/safe_browsing_prefs_unittest.cc View 1 2 3 4 5 6 7 2 chunks +3 lines, -2 lines 0 comments Download
M components/search_engines/keyword_table.cc View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M components/search_provider_logos/google_logo_api.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +7 lines, -3 lines 0 comments Download
M components/spellcheck/renderer/spellcheck_multilingual_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +5 lines, -3 lines 0 comments Download
M components/task_scheduler_util/common/variations_util.cc View 1 2 3 4 5 2 chunks +1 line, -2 lines 0 comments Download
M components/translate/core/browser/translate_prefs.cc View 1 2 3 4 5 6 7 3 chunks +10 lines, -8 lines 0 comments Download
M content/browser/gpu/gpu_data_manager_impl_private.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +14 lines, -9 lines 0 comments Download
M content/renderer/input/main_thread_event_queue_unittest.cc View 1 2 3 4 5 6 7 2 chunks +3 lines, -2 lines 0 comments Download
M extensions/browser/api/bluetooth/bluetooth_private_api.cc View 1 2 3 4 5 6 7 2 chunks +3 lines, -3 lines 0 comments Download
M extensions/browser/api/networking_private/networking_cast_private_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -3 lines 0 comments Download
M extensions/common/csp_validator.cc View 1 2 3 4 5 5 chunks +7 lines, -4 lines 0 comments Download
M extensions/common/features/feature_provider.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M extensions/common/permissions/socket_permission_entry.cc View 1 2 3 4 5 2 chunks +4 lines, -3 lines 0 comments Download
M extensions/common/url_pattern.cc View 1 2 3 4 5 2 chunks +1 line, -2 lines 0 comments Download
M google_apis/gaia/fake_gaia.cc View 1 2 3 4 5 2 chunks +3 lines, -2 lines 0 comments Download
M gpu/config/gpu_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +14 lines, -8 lines 0 comments Download
M remoting/base/capabilities.cc View 1 2 3 4 5 2 chunks +9 lines, -9 lines 0 comments Download
M remoting/base/capabilities_unittest.cc View 1 2 3 4 5 3 chunks +5 lines, -4 lines 0 comments Download
M ui/gl/gl_implementation.cc View 1 2 3 4 5 3 chunks +4 lines, -3 lines 0 comments Download

Messages

Total messages: 75 (52 generated)
Matt Giuca
Hi all: this follows up on a previous CL where I added a StringPiece overload ...
3 years, 9 months ago (2017-03-15 06:25:19 UTC) #18
Matt Giuca
Oops; +garykac for remoting/*. Please see above message.
3 years, 9 months ago (2017-03-15 06:26:09 UTC) #20
Matt Giuca
+torne for android_webview. Thanks! (See detailed message above.)
3 years, 9 months ago (2017-03-15 07:59:34 UTC) #26
vabr (Chromium)
components/autofill and components/password_manager LGTM. I checked that the StringPieces are only used before the strings ...
3 years, 9 months ago (2017-03-15 08:25:11 UTC) #33
Torne
android_webview LGTM, thanks
3 years, 9 months ago (2017-03-15 09:00:54 UTC) #34
dcheng
https://codereview.chromium.org/2695883003/diff/270001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2695883003/diff/270001/base/feature_list.cc#newcode342 base/feature_list.cc:342: for (auto value : SplitFeatureListString(feature_list)) { Hmmm... StringPiece are ...
3 years, 9 months ago (2017-03-15 10:02:36 UTC) #35
Roger Tawa OOO till Jul 10th
google_apis lgtm
3 years, 9 months ago (2017-03-15 13:39:56 UTC) #36
Nico
chrome/, ui/ lgtm https://codereview.chromium.org/2695883003/diff/270001/chrome/browser/extensions/chrome_content_verifier_delegate.cc File chrome/browser/extensions/chrome_content_verifier_delegate.cc (right): https://codereview.chromium.org/2695883003/diff/270001/chrome/browser/extensions/chrome_content_verifier_delegate.cc#newcode182 chrome/browser/extensions/chrome_content_verifier_delegate.cc:182: "&"), Is this really simpler than ...
3 years, 9 months ago (2017-03-15 13:52:11 UTC) #37
blundell
//components lgtm I gave it a high-level review. If there's any part that you feel ...
3 years, 9 months ago (2017-03-15 15:14:44 UTC) #38
piman
LGTM for content/ and gpu/
3 years, 9 months ago (2017-03-15 16:31:27 UTC) #39
stevenjb
chromeos/, c/b/chromeos/ lgtm
3 years, 9 months ago (2017-03-15 17:35:54 UTC) #40
Matt Giuca
https://codereview.chromium.org/2695883003/diff/270001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2695883003/diff/270001/base/feature_list.cc#newcode342 base/feature_list.cc:342: for (auto value : SplitFeatureListString(feature_list)) { On 2017/03/15 10:02:36, ...
3 years, 9 months ago (2017-03-17 00:16:27 UTC) #49
dcheng
https://codereview.chromium.org/2695883003/diff/270001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2695883003/diff/270001/base/feature_list.cc#newcode342 base/feature_list.cc:342: for (auto value : SplitFeatureListString(feature_list)) { On 2017/03/17 00:16:27, ...
3 years, 9 months ago (2017-03-17 05:28:34 UTC) #54
Matt Giuca
On 2017/03/17 05:28:34, dcheng wrote: > https://codereview.chromium.org/2695883003/diff/270001/base/feature_list.cc > File base/feature_list.cc (right): > > https://codereview.chromium.org/2695883003/diff/270001/base/feature_list.cc#newcode342 > ...
3 years, 9 months ago (2017-03-17 06:01:21 UTC) #55
Matt Giuca
https://codereview.chromium.org/2695883003/diff/270001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/2695883003/diff/270001/base/feature_list.cc#newcode342 base/feature_list.cc:342: for (auto value : SplitFeatureListString(feature_list)) { On 2017/03/17 05:28:34, ...
3 years, 9 months ago (2017-03-21 08:28:48 UTC) #56
Matt Giuca
Done, PTAL (dcheng). I hope we can land this soon (tomorrow?). The merge conflicts are ...
3 years, 9 months ago (2017-03-27 03:24:58 UTC) #57
dcheng
On 2017/03/27 03:24:58, Matt Giuca wrote: > Done, PTAL (dcheng). > > I hope we ...
3 years, 9 months ago (2017-03-27 04:35:58 UTC) #62
benwells
lgtm
3 years, 9 months ago (2017-03-27 06:22:56 UTC) #63
garykac
lgtm
3 years, 9 months ago (2017-03-27 17:28:46 UTC) #64
Matt Giuca
On 2017/03/27 04:35:58, dcheng wrote: > FWIW, for future CLs of this nature, it's probably ...
3 years, 9 months ago (2017-03-27 22:40:02 UTC) #65
commit-bot: I haz the power
This CL has an open dependency (Issue 2722413002 Patch 60001). Please resolve the dependency and ...
3 years, 9 months ago (2017-03-27 22:40:50 UTC) #69
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2695883003/430001
3 years, 9 months ago (2017-03-28 00:34:21 UTC) #72
commit-bot: I haz the power
3 years, 9 months ago (2017-03-28 02:08:11 UTC) #75
Message was sent while issue was closed.
Committed patchset #18 (id:430001) as
https://chromium.googlesource.com/chromium/src/+/30f7588201a275c75d0382ad4d63...

Powered by Google App Engine
This is Rietveld 408576698