New issue
Advanced search Search tips

Issue 667131 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

WebString is easy to use in inefficient ways

Project Member Reported by dcheng@chromium.org, Nov 20 2016

Issue description

It's really easy to generate temporaries that shouldn't be needed, due to WebString's implicit conversion operators. Several examples I found while looking around:

Inefficient conversions:
  base::UTF16ToUTF8(base::StringPiece16(web_string))
should be:
  web_string.utf8()
Since the latter is more efficient if the string is already an 8-bit string.

Unnecessary conversions:
  bool is_post = base::EqualsASCII(
      base::StringPiece16(failed_request.httpMethod()), "POST");
creates a new string16 just to do a comparison.

More unnecessary conversions:
  if (!base::IsStringASCII(capability.mimeType) ||
creates a new string16 just to check if the string only contains ASCII chars.

More unnecessary conversions (part 2, in tests):
  provider_.RequestTextChecking(blink::WebString(WideToUTF16(kRussianWord)),
                                &completion,
                                std::vector<SpellCheckMarker>());
The first parameter of RequestTextChecking is a string16, so we convert a string literal to a string16, then turn it into a WebString, and promptly turn it back into a string16.

I'm not sure if there are other patterns: I was looking at the calls to https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebString.h?l=143&gs=cpp%253Ablink%253A%253Aclass-WebString%253A%253Aoperatorbasic_string()-const%2540chromium%252F..%252F..%252Fthird_party%252FWebKit%252Fpublic%252Fplatform%252FWebString.h%257Cdef&gsn=operator&ct=xref_usages

But there are similar issues with WebString's constructors as well.

We should consider:
- Making conversions explicit
- Change things like IsStringASCII to use containsOnlyASCII()
- Perhaps add equivalents for comparing against ASCII strings
- ???
 
Great find!!

While we're at it it would probably make sense to improve WebString documentation. I'm sure many people see "A UTF-16 string container" in the header and assume the underlying representation will always be 16 bit.
Owner: esprehn@chromium.org

Comment 3 by kinuko@chromium.org, Nov 21 2016

Thanks for filing this.  I think we should probably make the conversions explicit, more and more I look at this the currenot code is probably too easy to write inefficient code.

For additional contexts, there're a few patches I've been making to skip unnecessary string16 but as dcheng writes there could be a lot more: e.x. conversion: https://codereview.chromium.org/2469353003/ https://codereview.chromium.org/1686263008/
+1 to making the conversion explicit.

Comment 5 by kinuko@chromium.org, Dec 12 2016

Owner: kinuko@chromium.org
Status: Assigned (was: Available)
Would like to take this as this has been bothering me a bit.

Comment 6 by kinuko@chromium.org, Dec 14 2016

Per comment from fs blindly replacing UTF16ToUTF8(StringPiece16(myWebString)) to myWebString.utf8() might not be always right:

On 2016/12/13 09:37:41, fs wrote:
> base::UTF16ToUTF8(base::StringPiece16(params.language()));
> base::UTF16ToUTF8(base::StringPiece16(myWebString)) is _not_ semantically
> equivalent to WebString::utf8(). In terms of UTF8ConversionMode (from
> WTFString.h), I believe the former is
> StrictUTF8ConversionReplacingUnpairedSurrogatesWithFFFD, while the latter is
> LenientUTF8Conversion (default argument to WTF::String::utf8.) This is likely
> not a problem in most cases, but it's pretty difficult to just look at a
> callsite and determine if that is the case (even if you wrote and own the code
> in question I'd think...)
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 15 2016

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

commit d3054687cf8c2b90c25914065acaee0eb0a5ca28
Author: kinuko <kinuko@chromium.org>
Date: Thu Dec 15 10:48:15 2016

Add explicit conversions methods for WebString <-> string16

Initial patch to remove implicit conversions.

This CL adds following utf16 conversion methods and use
them in blink_platform_impl.cc to show how it'd look like.

* static WebString WebString::fromUTF16(const string16&);
* static WebString WebString::fromUTF16(const NullableString16&);
* string16 WebString::utf16();
* static NullableString16 WebString::toNullableString16();

BUG= 667131 

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

[modify] https://crrev.com/d3054687cf8c2b90c25914065acaee0eb0a5ca28/content/child/blink_platform_impl.cc
[modify] https://crrev.com/d3054687cf8c2b90c25914065acaee0eb0a5ca28/third_party/WebKit/Source/platform/exported/WebString.cpp
[modify] https://crrev.com/d3054687cf8c2b90c25914065acaee0eb0a5ca28/third_party/WebKit/public/platform/DEPS
[modify] https://crrev.com/d3054687cf8c2b90c25914065acaee0eb0a5ca28/third_party/WebKit/public/platform/WebString.h

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 17 2016

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

commit 6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b
Author: kinuko <kinuko@chromium.org>
Date: Sat Dec 17 02:35:59 2016

Use explicit WebString <-> string16 conversion methods in storage API files

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

Also adds FilePathToWebString() utility method that does reverse of
WebStringToFilePath.

BUG= 667131 

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

[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/content/child/database_util.cc
[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/content/child/db_message_filter.cc
[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/content/child/fileapi/webfilesystem_impl.cc
[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/content/child/indexed_db/indexed_db_callbacks_impl.cc
[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/content/child/indexed_db/indexed_db_database_callbacks_impl.cc
[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/content/child/indexed_db/indexed_db_key_builders.cc
[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/content/child/indexed_db/webidbdatabase_impl.cc
[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/content/child/indexed_db/webidbfactory_impl.cc
[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/content/child/web_database_observer_impl.cc
[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/content/child/webfileutilities_impl.cc
[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/third_party/WebKit/Source/platform/exported/FilePathConversion.cpp
[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/third_party/WebKit/Source/platform/exported/FilePathConversionTest.cpp
[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/third_party/WebKit/public/platform/FilePathConversion.h
[modify] https://crrev.com/6a43aaa91b9ce5fbbb1b4a499a15f0fba43cd79b/third_party/WebKit/public/platform/WebString.h

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 20 2016

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

commit 4a18cc0132fce0c9668989f4153851c5389657fa
Author: kinuko <kinuko@chromium.org>
Date: Tue Dec 20 07:47:33 2016

Use explicit WebString <-> string16 conversion methods in media files

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

BUG= 667131 

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

[modify] https://crrev.com/4a18cc0132fce0c9668989f4153851c5389657fa/content/renderer/media/canvas_capture_handler.cc
[modify] https://crrev.com/4a18cc0132fce0c9668989f4153851c5389657fa/content/renderer/media/media_stream_renderer_factory_impl.cc
[modify] https://crrev.com/4a18cc0132fce0c9668989f4153851c5389657fa/content/renderer/media/midi_message_filter.cc
[modify] https://crrev.com/4a18cc0132fce0c9668989f4153851c5389657fa/content/renderer/media/mock_web_rtc_peer_connection_handler_client.cc
[modify] https://crrev.com/4a18cc0132fce0c9668989f4153851c5389657fa/content/renderer/media/peer_connection_tracker.cc
[modify] https://crrev.com/4a18cc0132fce0c9668989f4153851c5389657fa/content/renderer/media/pepper_to_video_track_adapter.cc
[modify] https://crrev.com/4a18cc0132fce0c9668989f4153851c5389657fa/content/renderer/media/remote_media_stream_impl.cc
[modify] https://crrev.com/4a18cc0132fce0c9668989f4153851c5389657fa/content/renderer/media/rtc_data_channel_handler.cc
[modify] https://crrev.com/4a18cc0132fce0c9668989f4153851c5389657fa/content/renderer/media/rtc_dtmf_sender_handler.cc
[modify] https://crrev.com/4a18cc0132fce0c9668989f4153851c5389657fa/content/renderer/media/rtc_peer_connection_handler.cc
[modify] https://crrev.com/4a18cc0132fce0c9668989f4153851c5389657fa/content/renderer/media/user_media_client_impl.cc
[modify] https://crrev.com/4a18cc0132fce0c9668989f4153851c5389657fa/media/blink/key_system_config_selector.cc
[modify] https://crrev.com/4a18cc0132fce0c9668989f4153851c5389657fa/media/blink/webencryptedmediaclient_impl.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Dec 26 2016

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

commit 6f58cf2958619996b056b14622bb43b16705248f
Author: kinuko <kinuko@chromium.org>
Date: Mon Dec 26 08:55:33 2016

Use explicit WebString <-> string conversion methods for workers

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

This patch also replaces WebString::fromUTF8 with WebString::fromASCII
for some of the strings are guaranteed to be ASCII-only.

BUG= 667131 

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

[modify] https://crrev.com/6f58cf2958619996b056b14622bb43b16705248f/content/child/service_worker/service_worker_dispatcher.cc
[modify] https://crrev.com/6f58cf2958619996b056b14622bb43b16705248f/content/child/service_worker/web_service_worker_impl.cc
[modify] https://crrev.com/6f58cf2958619996b056b14622bb43b16705248f/content/renderer/service_worker/embedded_worker_dispatcher.cc
[modify] https://crrev.com/6f58cf2958619996b056b14622bb43b16705248f/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/6f58cf2958619996b056b14622bb43b16705248f/content/renderer/shared_worker/embedded_shared_worker_content_settings_client_proxy.cc
[modify] https://crrev.com/6f58cf2958619996b056b14622bb43b16705248f/content/renderer/shared_worker/embedded_shared_worker_stub.cc
[modify] https://crrev.com/6f58cf2958619996b056b14622bb43b16705248f/content/renderer/shared_worker_repository.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Dec 26 2016

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

commit 5204fb6a7ff224c2f1f4f7f8f086b215d50b10f7
Author: kinuko <kinuko@chromium.org>
Date: Mon Dec 26 13:57:31 2016

Use explicit WebString string conversion methods for WebURLLoader files

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

Also for some of the strings are guaranteed to be ASCII-only
replace WebString::fromUTF8 with WebString::fromASCII

BUG= 667131 

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

[modify] https://crrev.com/5204fb6a7ff224c2f1f4f7f8f086b215d50b10f7/content/child/web_url_loader_impl.cc
[modify] https://crrev.com/5204fb6a7ff224c2f1f4f7f8f086b215d50b10f7/content/child/web_url_request_util.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Dec 27 2016

Project Member

Comment 14 by bugdroid1@chromium.org, Jan 17 2017

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

commit d280d3b7b888a5f8964e916cdc2360ebc7858418
Author: kinuko <kinuko@chromium.org>
Date: Tue Jan 17 09:08:47 2017

Allow WebString.utf8() to take UTF8ConversionMode

Currently WebString.utf8() always converts the internal string to UTF-8
with LenientUTF8Conversion mode (if the string is not ascii), but
WebString::fromUTF8() only takes valid, strict UTF-8 string.

therefore
the same string may not be able to be converted back to WebString.

This has been causing a few issues:
- UTF16 (WTFString) -> UTF8 -> UTF16 (WTFString) round trip doesn't work
- Works differently from base::UTF16ToUTF8, and some code explicitly
  calls base version via .utf16() to avoid any problems while it could
  be more costly than .utf8() if the internal string is ascii/latin.

Similar but opposite attempt has been made in the past in
https://codereview.chromium.org/1768063002/
but hadn't landed as leniently converted UTF8 is not valid and probably
should not be accepted.

BUG= 667131 

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

[modify] https://crrev.com/d280d3b7b888a5f8964e916cdc2360ebc7858418/third_party/WebKit/Source/platform/BUILD.gn
[modify] https://crrev.com/d280d3b7b888a5f8964e916cdc2360ebc7858418/third_party/WebKit/Source/platform/exported/WebString.cpp
[add] https://crrev.com/d280d3b7b888a5f8964e916cdc2360ebc7858418/third_party/WebKit/Source/platform/exported/WebStringTest.cpp
[modify] https://crrev.com/d280d3b7b888a5f8964e916cdc2360ebc7858418/third_party/WebKit/Source/wtf/text/StringUTF8Adaptor.h
[modify] https://crrev.com/d280d3b7b888a5f8964e916cdc2360ebc7858418/third_party/WebKit/public/platform/WebString.h

Project Member

Comment 15 by bugdroid1@chromium.org, Jan 18 2017

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

commit c7f1bf8730353fab4a13f5173b282315ebbe11ba
Author: kinuko <kinuko@chromium.org>
Date: Wed Jan 18 04:30:20 2017

Use explicit WebString <-> string conversion methods for notifications

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

BUG= 667131 

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

[modify] https://crrev.com/c7f1bf8730353fab4a13f5173b282315ebbe11ba/content/child/notifications/notification_data_conversions.cc
[modify] https://crrev.com/c7f1bf8730353fab4a13f5173b282315ebbe11ba/content/child/notifications/notification_manager.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Jan 20 2017

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

commit a7687681877feabf6a4266e8003252290a461132
Author: kinuko <kinuko@chromium.org>
Date: Fri Jan 20 06:10:24 2017

Use explicit WebString conversion methods in content/renderer/media (unittests)

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

BUG= 667131 

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

[modify] https://crrev.com/a7687681877feabf6a4266e8003252290a461132/content/renderer/media/media_recorder_handler_unittest.cc
[modify] https://crrev.com/a7687681877feabf6a4266e8003252290a461132/content/renderer/media/media_stream_video_capturer_source_unittest.cc
[modify] https://crrev.com/a7687681877feabf6a4266e8003252290a461132/content/renderer/media/media_stream_video_renderer_sink_unittest.cc
[modify] https://crrev.com/a7687681877feabf6a4266e8003252290a461132/content/renderer/media/media_stream_video_source_unittest.cc
[modify] https://crrev.com/a7687681877feabf6a4266e8003252290a461132/content/renderer/media/media_stream_video_track_unittest.cc
[modify] https://crrev.com/a7687681877feabf6a4266e8003252290a461132/content/renderer/media/mock_media_stream_registry.cc
[modify] https://crrev.com/a7687681877feabf6a4266e8003252290a461132/content/renderer/media/rtc_peer_connection_handler_unittest.cc
[modify] https://crrev.com/a7687681877feabf6a4266e8003252290a461132/content/renderer/media/video_track_recorder_unittest.cc
[modify] https://crrev.com/a7687681877feabf6a4266e8003252290a461132/content/renderer/media/webrtc/media_stream_remote_video_source_unittest.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Jan 23 2017

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

commit 6addc13d7cb14eed79c7f3f742cc1724d15a41d3
Author: kinuko <kinuko@chromium.org>
Date: Mon Jan 23 02:07:34 2017

Use explicit WebString conversions in LayoutTest related files

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

BUG= 667131 

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

[modify] https://crrev.com/6addc13d7cb14eed79c7f3f742cc1724d15a41d3/components/test_runner/mock_grammar_check.cc
[modify] https://crrev.com/6addc13d7cb14eed79c7f3f742cc1724d15a41d3/components/test_runner/mock_spell_check.cc
[modify] https://crrev.com/6addc13d7cb14eed79c7f3f742cc1724d15a41d3/components/test_runner/spell_check_client.cc
[modify] https://crrev.com/6addc13d7cb14eed79c7f3f742cc1724d15a41d3/components/test_runner/web_view_test_client.cc
[modify] https://crrev.com/6addc13d7cb14eed79c7f3f742cc1724d15a41d3/content/public/test/render_view_test.cc
[modify] https://crrev.com/6addc13d7cb14eed79c7f3f742cc1724d15a41d3/content/shell/renderer/layout_test/blink_test_runner.cc
[modify] https://crrev.com/6addc13d7cb14eed79c7f3f742cc1724d15a41d3/content/test/layouttest_support.cc
[modify] https://crrev.com/6addc13d7cb14eed79c7f3f742cc1724d15a41d3/content/test/mock_webclipboard_impl.cc
[modify] https://crrev.com/6addc13d7cb14eed79c7f3f742cc1724d15a41d3/content/test/test_blink_web_unit_test_support.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Jan 24 2017

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

commit 0073e7c8e416685bfa479638a2ae05f8b217d434
Author: kinuko <kinuko@chromium.org>
Date: Tue Jan 24 00:42:40 2017

Use explicit WebString conversions in autofill

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

Also in some files WebString implicit conversions seem to be used
just to get base::string16, this CL also replaces them with base's
string16 conversion methods.

BUG= 667131 

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

[modify] https://crrev.com/0073e7c8e416685bfa479638a2ae05f8b217d434/chrome/renderer/autofill/form_autocomplete_browsertest.cc
[modify] https://crrev.com/0073e7c8e416685bfa479638a2ae05f8b217d434/chrome/renderer/autofill/form_autofill_browsertest.cc
[modify] https://crrev.com/0073e7c8e416685bfa479638a2ae05f8b217d434/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
[modify] https://crrev.com/0073e7c8e416685bfa479638a2ae05f8b217d434/chrome/renderer/autofill/password_generation_agent_browsertest.cc
[modify] https://crrev.com/0073e7c8e416685bfa479638a2ae05f8b217d434/components/autofill/content/renderer/autofill_agent.cc
[modify] https://crrev.com/0073e7c8e416685bfa479638a2ae05f8b217d434/components/autofill/content/renderer/form_autofill_util.cc
[modify] https://crrev.com/0073e7c8e416685bfa479638a2ae05f8b217d434/components/autofill/content/renderer/form_cache.cc
[modify] https://crrev.com/0073e7c8e416685bfa479638a2ae05f8b217d434/components/autofill/content/renderer/form_classifier.cc
[modify] https://crrev.com/0073e7c8e416685bfa479638a2ae05f8b217d434/components/autofill/content/renderer/password_autofill_agent.cc
[modify] https://crrev.com/0073e7c8e416685bfa479638a2ae05f8b217d434/components/autofill/content/renderer/password_form_conversion_utils.cc
[modify] https://crrev.com/0073e7c8e416685bfa479638a2ae05f8b217d434/components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
[modify] https://crrev.com/0073e7c8e416685bfa479638a2ae05f8b217d434/components/autofill/content/renderer/password_generation_agent.cc
[modify] https://crrev.com/0073e7c8e416685bfa479638a2ae05f8b217d434/components/autofill/content/renderer/renderer_save_password_progress_logger.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Jan 24 2017

kinuko: WDYT of adding operator== with StringPiece to WebString instead of char*? I think that could make a lot of the comparisons with char* faster.
Project Member

Comment 21 by bugdroid1@chromium.org, Jan 25 2017

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

commit 671a5e16b18fd2229be6d30fb66a9b4e05640a5b
Author: kinuko <kinuko@chromium.org>
Date: Wed Jan 25 01:10:43 2017

Use explicit WebString conversions in renderer/net/net_error_helper.cc

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

BUG= 667131 

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

[modify] https://crrev.com/671a5e16b18fd2229be6d30fb66a9b4e05640a5b/chrome/renderer/net/net_error_helper.cc

#20- Sounds good, and I made a quick shot-- but looks like it ends up with an ambiguous overloading error as a literal char can be implicitly converted into either WebString or StringPiece.  Do you have better idea about how to make it work?
Project Member

Comment 24 by bugdroid1@chromium.org, Jan 25 2017

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

commit 1735825385948a912baf6683ea186813dd88bbad
Author: kinuko <kinuko@chromium.org>
Date: Wed Jan 25 03:30:34 2017

Use explicit WebString conversions in chrome/renderer/safe_browsing

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

BUG= 667131 

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

[modify] https://crrev.com/1735825385948a912baf6683ea186813dd88bbad/chrome/renderer/safe_browsing/phishing_classifier.cc
[modify] https://crrev.com/1735825385948a912baf6683ea186813dd88bbad/chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc

I think you want to instead make a version of WebString::equals() that takes a length, and make operator==(WebString, const char*) call strlen inline in the header.
Project Member

Comment 26 by bugdroid1@chromium.org, Jan 25 2017

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

commit 6e262965967f5cd06e86045a51bae65c6d818bd4
Author: kinuko <kinuko@chromium.org>
Date: Wed Jan 25 15:22:34 2017

Use explicit WebString conversions in remaining chrome/renderer files

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

BUG= 667131 

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

[modify] https://crrev.com/6e262965967f5cd06e86045a51bae65c6d818bd4/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/6e262965967f5cd06e86045a51bae65c6d818bd4/chrome/renderer/chrome_render_frame_observer.cc
[modify] https://crrev.com/6e262965967f5cd06e86045a51bae65c6d818bd4/chrome/renderer/chrome_render_thread_observer.cc
[modify] https://crrev.com/6e262965967f5cd06e86045a51bae65c6d818bd4/chrome/renderer/content_settings_observer.cc
[modify] https://crrev.com/6e262965967f5cd06e86045a51bae65c6d818bd4/chrome/renderer/extensions/page_capture_custom_bindings.cc
[modify] https://crrev.com/6e262965967f5cd06e86045a51bae65c6d818bd4/chrome/renderer/searchbox/searchbox_extension.cc
[modify] https://crrev.com/6e262965967f5cd06e86045a51bae65c6d818bd4/chrome/renderer/web_apps.cc
[modify] https://crrev.com/6e262965967f5cd06e86045a51bae65c6d818bd4/chrome/renderer/worker_content_settings_client_proxy.cc

Project Member

Comment 27 by bugdroid1@chromium.org, Jan 25 2017

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

commit ca6ed36839c83303864e3383fb3fd16bda4ffd4c
Author: kinuko <kinuko@chromium.org>
Date: Wed Jan 25 19:25:30 2017

Use explicit WebString conversions in remaining components files

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

BUG= 667131 
R=blundell@chromium.org

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

[modify] https://crrev.com/ca6ed36839c83303864e3383fb3fd16bda4ffd4c/components/password_manager/content/renderer/credential_manager_client.cc
[modify] https://crrev.com/ca6ed36839c83303864e3383fb3fd16bda4ffd4c/components/printing/renderer/print_web_view_helper.cc

Project Member

Comment 28 by bugdroid1@chromium.org, Jan 26 2017

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

commit 2ae40e827e9a0164e0cf399f5ad4112891de773e
Author: kinuko <kinuko@chromium.org>
Date: Thu Jan 26 00:57:06 2017

Use explicit WebString conversions in plugins code

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

BUG= 667131 
R=tommycli@chromium.org

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

[modify] https://crrev.com/2ae40e827e9a0164e0cf399f5ad4112891de773e/chrome/renderer/plugins/chrome_plugin_placeholder.cc
[modify] https://crrev.com/2ae40e827e9a0164e0cf399f5ad4112891de773e/components/plugins/renderer/loadable_plugin_placeholder.cc

Project Member

Comment 29 by bugdroid1@chromium.org, Jan 26 2017

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

commit 365b58f79079581b244a12f4bf5aaa6c0c54b305
Author: kinuko <kinuko@chromium.org>
Date: Thu Jan 26 03:15:36 2017

Use explicit WebString conversions in clipboard and drag-and-drop code

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

BUG= 667131 
R=dcheng@chromium.org

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

[modify] https://crrev.com/365b58f79079581b244a12f4bf5aaa6c0c54b305/content/renderer/clipboard_utils.cc
[modify] https://crrev.com/365b58f79079581b244a12f4bf5aaa6c0c54b305/content/renderer/drop_data_builder.cc
[modify] https://crrev.com/365b58f79079581b244a12f4bf5aaa6c0c54b305/content/renderer/webclipboard_impl.cc

#25- cool, yeah looks like it works :D  I'll be sending a patch.
Project Member

Comment 31 by bugdroid1@chromium.org, Jan 26 2017

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

commit 7bab3265dc08756009c0374803bbbdd11fa6a1e8
Author: kinuko <kinuko@chromium.org>
Date: Thu Jan 26 09:38:02 2017

Use explicit WebString conversions in plugins code

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

BUG= 667131 
R=toyoshim@chromium.org

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

[modify] https://crrev.com/7bab3265dc08756009c0374803bbbdd11fa6a1e8/chrome/renderer/translate/translate_script_browsertest.cc
[modify] https://crrev.com/7bab3265dc08756009c0374803bbbdd11fa6a1e8/components/translate/content/renderer/translate_helper.cc

Project Member

Comment 32 by bugdroid1@chromium.org, Jan 26 2017

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

commit 56bb6b406b0a1cbfbc9b21fcbcaac6bd16e7e9a9
Author: kinuko <kinuko@chromium.org>
Date: Thu Jan 26 14:36:11 2017

Make WebString::equal take the length for const char* too

This makes strlen calls optimized away for literal strings.

(Verified a few callsites to see the strlen actually gets replaced with
immediate values for literal strings)

BUG= 667131 

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

[modify] https://crrev.com/56bb6b406b0a1cbfbc9b21fcbcaac6bd16e7e9a9/third_party/WebKit/Source/platform/exported/WebString.cpp
[modify] https://crrev.com/56bb6b406b0a1cbfbc9b21fcbcaac6bd16e7e9a9/third_party/WebKit/public/platform/WebString.h

Nice, I saw that people like comparing WebSecurityOrigin protocols (which are WebStrings) to literals, so I'm going to audit those and make sure they are used properly.
Hm using the strlen optimization is great for literals, but we should have a pattern for comparing WebString to std::string.

Right now, callers are using utf8() or similar which is not great. I wonder if we should add a public method on WebString equals(const char* characters, size_t length). Ideally we could also have comparators for std::string outside of blink.
#34- equals methods are actually exposed at public, but I found that some callsites are using them in a bit suboptimal way (and my change also made some cases behave worse).  I'll create a follow-up patch to address some of these...
Project Member

Comment 36 by bugdroid1@chromium.org, Jan 27 2017

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

commit a429302c3d58006f7520aa89e1ae0d3ba1cadc30
Author: kinuko <kinuko@chromium.org>
Date: Fri Jan 27 06:43:25 2017

Use explicit WebString conversions in remaining content files

As we're deprecating implicit conversions for WebString <-> string16
replace them with explicit methods. (See  crbug.com/667131  for more
details)

BUG= 667131 

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

[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/child/indexed_db/webidbdatabase_impl_unittest.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/child/webmessageportchannel_impl.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/public/common/color_suggestion.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/public/common/window_container_type.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/accessibility/render_accessibility_impl.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/browser_plugin/browser_plugin.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/context_menu_params_builder.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/devtools/devtools_agent.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/dom_serializer_browsertest.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/history_serialization.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/manifest/manifest_parser.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/menu_item_builder.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/render_view_impl.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/render_widget.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/renderer_blink_platform_impl.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/renderer_webcookiejar_impl.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/savable_resources.cc
[modify] https://crrev.com/a429302c3d58006f7520aa89e1ae0d3ba1cadc30/content/renderer/speech_recognition_dispatcher.cc

Project Member

Comment 37 by bugdroid1@chromium.org, Jan 27 2017

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

commit 1a681e3ecb0e8885fff42917be3c942fd05e0984
Author: kinuko <kinuko@chromium.org>
Date: Fri Jan 27 12:38:42 2017

Re-add WebString::equals that takes const char* only

I replaced this with the one that takes length, but it forced
other callsites that were manually calling equals() with literals
to call strlen().  This fixes it to readd the version that only
takes const char*.

BUG= 667131 
R=esprehn@chromium.org, csharrison@chromium.org

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

[modify] https://crrev.com/1a681e3ecb0e8885fff42917be3c942fd05e0984/third_party/WebKit/Source/platform/exported/WebString.cpp
[modify] https://crrev.com/1a681e3ecb0e8885fff42917be3c942fd05e0984/third_party/WebKit/public/platform/WebString.h

Project Member

Comment 38 by bugdroid1@chromium.org, Feb 7 2017

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

commit 3db94dc4cd5cc78b799faac1040d843019a9fb9a
Author: ricea <ricea@chromium.org>
Date: Tue Feb 07 00:45:59 2017

Remove obsolete WebString comment from string_util

string_util.h contained the following comment:

  // A convenience adaptor for WebStrings, as they don't convert into
  // StringPieces directly.

prior to the string16 overload of IsStringASCII().

Since r447320 the implicit conversion from WebString to string16 has
been removed so this doesn't work. WebString::containsOnlyASCII() should
be used instead.

BUG= 667131 

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

[modify] https://crrev.com/3db94dc4cd5cc78b799faac1040d843019a9fb9a/base/strings/string_util.h

Status: Fixed (was: Assigned)
There may be more potential fixes that can be done, but let me close this one.

Sign in to add a comment