Allow WebString.utf8() to take UTF8ConversionMode

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}
Committed: https://chromium.googlesource.com/chromium/src/+/d280d3b7b888a5f8964e916cdc2360ebc7858418

[kinuko, 2017-01-12 03:16:16]

Patchset #1 (id:1) has been deleted

[kinuko, 2017-01-12 03:16:27]

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

[commit-bot: I haz the power, 2017-01-12 03:16:47]

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, 2017-01-12 03:25:30]

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

[commit-bot: I haz the power, 2017-01-12 03:25:31]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[kinuko, 2017-01-12 03:38:39]

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

[commit-bot: I haz the power, 2017-01-12 03:38:58]

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, 2017-01-12 03:47:36]

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

[commit-bot: I haz the power, 2017-01-12 03:47:37]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[kinuko, 2017-01-13 04:51:04]

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

[commit-bot: I haz the power, 2017-01-13 04:51:24]

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, 2017-01-13 07:12:51]

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

[commit-bot: I haz the power, 2017-01-13 07:12:52]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-01-13 07:33:11]

Description was changed from

==========
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
==========

to

==========
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
==========

[kinuko, 2017-01-13 07:33:12]

kinuko@chromium.org changed reviewers:
+ jochen@chromium.org, jsbell@chromium.org

[kinuko, 2017-01-13 07:34:40]

For the people who tried similar (but in a opposite way) before: what do you
think about this change?  We probably don't need to always stick to lenient utf8
conversion?

(The patch itself is a bit rough for now)

[jochen (gone - plz use gerrit), 2017-01-13 08:42:31]

lgtm

https://codereview.chromium.org/2629573002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebString.h (right):

https://codereview.chromium.org/2629573002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebString.h:82: enum UTF8ConversionMode {
nit. why not an enum class? constants should start with k

[kinuko, 2017-01-16 07:08:57]

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

[commit-bot: I haz the power, 2017-01-16 07:09:09]

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

[kinuko, 2017-01-16 07:10:41]

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

[commit-bot: I haz the power, 2017-01-16 07:10:49]

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, 2017-01-16 07:41:04]

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

[commit-bot: I haz the power, 2017-01-16 07:41:05]

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_...)

[kinuko, 2017-01-16 15:32:15]

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

[commit-bot: I haz the power, 2017-01-16 15:32:30]

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, 2017-01-16 15:45:35]

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

[commit-bot: I haz the power, 2017-01-16 15:45:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kinuko, 2017-01-16 15:45:59]

Patchset #5 (id:100001) has been deleted

[kinuko, 2017-01-16 15:46:04]

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

[commit-bot: I haz the power, 2017-01-16 15:46:18]

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, 2017-01-16 17:25:11]

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

[commit-bot: I haz the power, 2017-01-16 17:25:11]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-01-17 07:15:49]

Thanks!  Addressed comments & added tests.  Will be landing (feel free to
uncheck CQ if you have more comments)

https://codereview.chromium.org/2629573002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebString.h (right):

https://codereview.chromium.org/2629573002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebString.h:82: enum UTF8ConversionMode {
On 2017/01/13 08:42:31, jochen wrote:
> nit. why not an enum class? constants should start with k

Done.

[kinuko, 2017-01-17 07:15:53]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2017-01-17 07:15:54]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2629573002/#ps140001
(title: "comment fix")

[commit-bot: I haz the power, 2017-01-17 07:16:05]

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, 2017-01-17 09:08:51]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484637353985790,
"parent_rev": "8f31fcdefc919f605f7b9e69c0f690b15b660faf", "commit_rev":
"d280d3b7b888a5f8964e916cdc2360ebc7858418"}

[commit-bot: I haz the power, 2017-01-17 09:09:24]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/d280d3b7b888a5f8964e916cdc23...
==========

[commit-bot: I haz the power, 2017-01-17 09:09:25]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/d280d3b7b888a5f8964e916cdc23...

[esprehn, 2017-01-17 10:43:25]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2017-01-17 10:43:26]

In the future let's add some usage of the new API at the same time (one usage is
fine). This adds a bunch of new API surface but it's not clear where it'll get
used.

[kinuko, 2017-01-17 14:43:02]

On 2017/01/17 10:43:26, esprehn wrote:
> In the future let's add some usage of the new API at the same time (one usage
is
> fine). This adds a bunch of new API surface but it's not clear where it'll get
> used.

Yep sure, will do so.  (I'm going to use the new utf8 mode in
https://codereview.chromium.org/2613003002/)