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

Issue 1215933004: New new versions of Starts/EndsWith and SplitString in net (Closed)

Created:
5 years, 5 months ago by brettw
Modified:
5 years, 5 months ago
Reviewers:
Ryan Sleevi
CC:
chromium-reviews, cbentzel+watch_chromium.org, erikwright+watch_chromium.org, mmenke, jshin+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@starts_with
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

New new versions of Starts/EndsWith and SplitString in net. The new calls are longer but more explicit and also more general. I didn't want change the semantics. The old SplitString maps to TRIM_WHITESPACE, SPLIT_WANT_ALL so I generally kept that even if it's not clear if the calling code wants that particular behavior. In places that obviously didn't need copies, use the StringPiece variant to avoid copies. I didn't expend too much effort in this area, especially in unit test code. It's like that more copies could be deleted with more effort. Many places just used SplitString to fill a vector that is iterated over. In these places I updated it to use a range-based for loop over the result of the function call. Add StringPiece versions of TrimWhitesace to match the existing Trim functions. Update IPLiteralToNumber to take a string piece. This is commonly used in places that have string pieces so this saves a copy. In net/dns/dns_config_service_win.cc I replaced a UTF16TOASCII call with assign(). This is what UTF16ToASCII actually does and avoids a copy because the current UTF16ToASCII doesn't support StringPiece. Updating this function call or adding an override is a big project due to the way it's use with Blink's WebString. Removed parts of StringPieceUtils that duplicate base code. Made the remaining functions in that file not locale dependent (tolower is basically always wrong for non-ASCII). BUG=506920, 506255 Committed: https://crrev.com/3a2c6907279aca02c1ecfa971237068338ba49d8 Cr-Commit-Position: refs/heads/master@{#337445}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : only net #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Total comments: 15

Patch Set 8 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+285 lines, -257 lines) Patch
M base/strings/string_split.cc View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M base/strings/string_util.h View 1 2 3 4 1 chunk +10 lines, -6 lines 0 comments Download
M base/strings/string_util.cc View 1 chunk +9 lines, -0 lines 0 comments Download
M net/base/data_url.cc View 1 2 1 chunk +3 lines, -3 lines 0 comments Download
M net/base/host_mapping_rules.cc View 1 2 5 6 1 chunk +3 lines, -4 lines 0 comments Download
M net/base/host_port_pair.cc View 1 2 chunks +3 lines, -3 lines 0 comments Download
M net/base/ip_address_number.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M net/base/ip_address_number.cc View 1 2 chunks +7 lines, -5 lines 0 comments Download
M net/base/ip_pattern.cc View 1 2 2 chunks +19 lines, -20 lines 0 comments Download
M net/base/mime_util.cc View 1 2 3 4 5 6 7 4 chunks +17 lines, -16 lines 0 comments Download
M net/base/net_util_icu.cc View 1 2 2 chunks +6 lines, -5 lines 0 comments Download
M net/cert/x509_certificate_unittest.cc View 1 2 2 chunks +6 lines, -5 lines 0 comments Download
M net/cookies/cookie_monster_unittest.cc View 1 2 2 chunks +10 lines, -14 lines 0 comments Download
M net/dns/dns_config_service_unittest.cc View 1 1 chunk +5 lines, -8 lines 0 comments Download
M net/dns/dns_config_service_win.cc View 1 2 2 chunks +4 lines, -6 lines 0 comments Download
M net/dns/host_resolver.cc View 1 1 chunk +2 lines, -3 lines 0 comments Download
M net/dns/host_resolver_impl.cc View 1 chunk +4 lines, -2 lines 0 comments Download
M net/dns/mock_host_resolver.cc View 1 1 chunk +4 lines, -5 lines 0 comments Download
M net/filter/filter.cc View 1 chunk +2 lines, -1 line 0 comments Download
M net/ftp/ftp_directory_listing_parser_ls.cc View 1 2 3 chunks +9 lines, -6 lines 0 comments Download
M net/ftp/ftp_directory_listing_parser_netware.cc View 1 2 1 chunk +3 lines, -2 lines 0 comments Download
M net/ftp/ftp_directory_listing_parser_os2.cc View 1 2 2 chunks +4 lines, -2 lines 0 comments Download
M net/ftp/ftp_directory_listing_parser_vms.cc View 1 2 6 chunks +19 lines, -13 lines 0 comments Download
M net/ftp/ftp_directory_listing_parser_windows.cc View 1 2 2 chunks +4 lines, -2 lines 0 comments Download
M net/ftp/ftp_network_transaction.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M net/ftp/ftp_util.cc View 1 2 3 4 5 2 chunks +6 lines, -4 lines 0 comments Download
M net/http/http_auth_cache.cc View 1 chunk +2 lines, -1 line 0 comments Download
M net/http/http_response_headers.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M net/http/http_stream_factory.cc View 1 2 3 chunks +8 lines, -6 lines 0 comments Download
M net/http/http_util.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/proxy/proxy_bypass_rules.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -2 lines 0 comments Download
M net/proxy/proxy_config_service_linux.cc View 1 chunk +2 lines, -1 line 0 comments Download
M net/quic/quic_utils.cc View 1 2 1 chunk +3 lines, -3 lines 0 comments Download
M net/server/http_server_unittest.cc View 1 2 5 chunks +16 lines, -9 lines 0 comments Download
M net/spdy/spdy_test_util_common.cc View 1 1 chunk +4 lines, -5 lines 0 comments Download
M net/ssl/ssl_cipher_suite_names.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/test/embedded_test_server/embedded_test_server.cc View 1 chunk +1 line, -1 line 0 comments Download
M net/test/embedded_test_server/http_request.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M net/test/spawned_test_server/remote_test_server.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M net/tools/balsa/balsa_frame.cc View 1 4 chunks +5 lines, -4 lines 0 comments Download
M net/tools/balsa/balsa_headers_token_utils.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M net/tools/balsa/string_piece_utils.h View 1 2 3 4 5 7 chunks +21 lines, -33 lines 0 comments Download
M net/tools/disk_cache_memory_test/disk_cache_memory_test.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M net/tools/flip_server/flip_in_mem_edsm_server.cc View 1 3 chunks +6 lines, -6 lines 0 comments Download
M net/tools/flip_server/loadtime_measurement.h View 1 2 chunks +10 lines, -9 lines 0 comments Download
M net/tools/gdig/gdig.cc View 1 1 chunk +4 lines, -4 lines 0 comments Download
M net/tools/net_watcher/net_watcher.cc View 1 1 chunk +3 lines, -3 lines 0 comments Download
M net/tools/quic/quic_client_bin.cc View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M net/tools/quic/quic_in_memory_cache.cc View 1 chunk +4 lines, -2 lines 0 comments Download
M net/tools/quic/quic_simple_client_bin.cc View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M net/url_request/url_request_unittest.cc View 1 3 chunks +10 lines, -10 lines 0 comments Download
M pdf/pdfium/pdfium_engine.cc View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 27 (11 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215933004/40001
5 years, 5 months ago (2015-07-04 03:28:16 UTC) #2
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/27049) mac_chromium_rel_ng on ...
5 years, 5 months ago (2015-07-04 03:41:49 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215933004/80001
5 years, 5 months ago (2015-07-04 05:06:44 UTC) #6
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_chromeos_rel/builds/54157) (exceeded global ...
5 years, 5 months ago (2015-07-04 05:22:58 UTC) #8
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215933004/100001
5 years, 5 months ago (2015-07-05 06:14:57 UTC) #10
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/27068)
5 years, 5 months ago (2015-07-05 06:55:02 UTC) #12
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215933004/120001
5 years, 5 months ago (2015-07-05 15:59:00 UTC) #14
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 5 months ago (2015-07-05 17:21:42 UTC) #16
brettw
5 years, 5 months ago (2015-07-05 17:48:21 UTC) #18
Ryan Sleevi
LGTM I've left several nits in the form of either pedantry or consistency (with the ...
5 years, 5 months ago (2015-07-06 08:52:02 UTC) #19
brettw
https://codereview.chromium.org/1215933004/diff/120001/net/base/ip_address_number.cc File net/base/ip_address_number.cc (right): https://codereview.chromium.org/1215933004/diff/120001/net/base/ip_address_number.cc#newcode165 net/base/ip_address_number.cc:165: std::string host_brackets = "["; I find the suggested form ...
5 years, 5 months ago (2015-07-06 16:52:34 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215933004/140001
5 years, 5 months ago (2015-07-06 16:53:12 UTC) #23
Ryan Sleevi
https://codereview.chromium.org/1215933004/diff/120001/net/cookies/cookie_monster_unittest.cc File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/1215933004/diff/120001/net/cookies/cookie_monster_unittest.cc#newcode441 net/cookies/cookie_monster_unittest.cc:441: base::SPLIT_WANT_ALL)) { On 2015/07/06 16:52:34, brettw wrote: > On ...
5 years, 5 months ago (2015-07-06 16:58:12 UTC) #24
brettw
On 2015/07/06 16:58:12, Ryan Sleevi (slow through 7-15 wrote: > https://codereview.chromium.org/1215933004/diff/120001/net/cookies/cookie_monster_unittest.cc > File net/cookies/cookie_monster_unittest.cc (right): ...
5 years, 5 months ago (2015-07-06 17:02:37 UTC) #25
commit-bot: I haz the power
Committed patchset #8 (id:140001)
5 years, 5 months ago (2015-07-06 19:43:40 UTC) #26
commit-bot: I haz the power
5 years, 5 months ago (2015-07-06 19:44:41 UTC) #27
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/3a2c6907279aca02c1ecfa971237068338ba49d8
Cr-Commit-Position: refs/heads/master@{#337445}

Powered by Google App Engine
This is Rietveld 408576698