Use explicit WebString <-> string conversion methods for workers

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
Committed: https://crrev.com/6f58cf2958619996b056b14622bb43b16705248f
Cr-Commit-Position: refs/heads/master@{#440701}

[kinuko, 2016-12-22 08:23:23]

Description was changed from

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

to

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

[kinuko, 2016-12-22 08:23:23]

kinuko@chromium.org changed reviewers:
+ falken@chromium.org

[kinuko, 2016-12-22 08:23:44]

Mostly mechanical only.  PTL

https://codereview.chromium.org/2596173002/diff/1/content/renderer/service_wo...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2596173002/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:458:
id.utf8()));
These ids should be ascii-only, but since it's given from scripts we can't be
sure

[kinuko, 2016-12-22 08:23:54]

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

[commit-bot: I haz the power, 2016-12-22 08:24:10]

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, 2016-12-22 09:41:35]

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

[commit-bot: I haz the power, 2016-12-22 09:41:36]

Dry run: This issue passed the CQ dry run.

[falken, 2016-12-26 01:26:20]

Sorry for the basic questions. Actually this confused me recently. I don't
really understand when we use ascii vs utf8 vs utf16. I was looking to change
most service worker messages from string16 to std::string (esp for things like
the error messages), as per guidance from this code review:
https://codereview.chromium.org/2433273003/#msg14

Do you know why service worker messages are using string16 to begin with? The
earliest commit I found was https://chromiumcodereview.appspot.com/23615009
which uses string16.

https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
content/child/service_worker/service_worker_dispatcher.cc:134:
blink::WebString::fromASCII(error_message)));
Just to be sure what happens if you pass something that's not ascii-only to
fromASCII? Does it just become a null string or is there some crash/security
risk? I almost think we should just use UTF8 everywhere for simplicity and avoid
crashes.

https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
File content/child/service_worker/web_service_worker_impl.cc (right):

https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
content/child/service_worker/web_service_worker_impl.cc:122: message.utf16(),
url::Origin(source_origin),
Do you know why this IPC should be a string16 and not just std::string?

[kinuko, 2016-12-26 04:21:38]

On 2016/12/26 01:26:20, falken wrote:
> Sorry for the basic questions. Actually this confused me recently. I don't
> really understand when we use ascii vs utf8 vs utf16. I was looking to change
> most service worker messages from string16 to std::string (esp for things like
> the error messages), as per guidance from this code review:
> https://codereview.chromium.org/2433273003/#msg14
> 
> Do you know why service worker messages are using string16 to begin with? The
> earliest commit I found was https://chromiumcodereview.appspot.com/23615009
> which uses string16.

Yeah, good questions.
If the string is non-ASCII / non-Latin1 using UTF-16 is more efficient because
Blink's string internally uses it (unless it's latin1/ascii), so in most cases
we used to default to string16.  Also the fact that WebString had implicit
conversions with string16 made people think that using utf-16 would be the best
choice, while it may not really be always true.

>
https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
> File content/child/service_worker/service_worker_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
> content/child/service_worker/service_worker_dispatcher.cc:134:
> blink::WebString::fromASCII(error_message)));
> Just to be sure what happens if you pass something that's not ascii-only to
> fromASCII? Does it just become a null string or is there some crash/security
> risk? I almost think we should just use UTF8 everywhere for simplicity and
avoid
> crashes.

The string is handled as if it's latin-1 in release build, but crashes in debug
build.  Yeah using UTF8 everywhere makes sense too for the code that isn't super
perf critical.  We're also trying to add ascii bit in blink's string so that we
can save repeated internal ascii-checks in blink code.

>
https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
> File content/child/service_worker/web_service_worker_impl.cc (right):
> 
>
https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
> content/child/service_worker/web_service_worker_impl.cc:122: message.utf16(),
> url::Origin(source_origin),
> Do you know why this IPC should be a string16 and not just std::string?

(Please see my answer above, it was a default choice)

[falken, 2016-12-26 05:16:25]

Thanks for the background. lgtm

On 2016/12/26 04:21:38, kinuko wrote:
> On 2016/12/26 01:26:20, falken wrote:
> > Sorry for the basic questions. Actually this confused me recently. I don't
> > really understand when we use ascii vs utf8 vs utf16. I was looking to
change
> > most service worker messages from string16 to std::string (esp for things
like
> > the error messages), as per guidance from this code review:
> > https://codereview.chromium.org/2433273003/#msg14
> > 
> > Do you know why service worker messages are using string16 to begin with?
The
> > earliest commit I found was https://chromiumcodereview.appspot.com/23615009
> > which uses string16.
> 
> Yeah, good questions.
> If the string is non-ASCII / non-Latin1 using UTF-16 is more efficient because
> Blink's string internally uses it (unless it's latin1/ascii), so in most cases
> we used to default to string16.  Also the fact that WebString had implicit
> conversions with string16 made people think that using utf-16 would be the
best
> choice, while it may not really be always true.


Interesting, so string16 should be more efficient than std::string when passing
a possibly non-ascii string to Blink. What's your take on the memory concern
raised in the linked code review? Would it still be sensible to change SW
messages to pass std::string to save memory?

Also discovered:
https://docs.google.com/document/d/1kOCUlJdh2WJMJGDf-WoEQhmnjKLaOYRbiHz5TiGJl...

> 
> >
>
https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
> > File content/child/service_worker/service_worker_dispatcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
> > content/child/service_worker/service_worker_dispatcher.cc:134:
> > blink::WebString::fromASCII(error_message)));
> > Just to be sure what happens if you pass something that's not ascii-only to
> > fromASCII? Does it just become a null string or is there some crash/security
> > risk? I almost think we should just use UTF8 everywhere for simplicity and
> avoid
> > crashes.
> 
> The string is handled as if it's latin-1 in release build, but crashes in
debug
> build.  Yeah using UTF8 everywhere makes sense too for the code that isn't
super
> perf critical.  We're also trying to add ascii bit in blink's string so that
we
> can save repeated internal ascii-checks in blink code.
> 
> >
>
https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
> > File content/child/service_worker/web_service_worker_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
> > content/child/service_worker/web_service_worker_impl.cc:122:
message.utf16(),
> > url::Origin(source_origin),
> > Do you know why this IPC should be a string16 and not just std::string?
> 
> (Please see my answer above, it was a default choice)

[kinuko, 2016-12-26 07:26:21]

On 2016/12/26 05:16:25, falken wrote:
> Thanks for the background. lgtm
> 
> On 2016/12/26 04:21:38, kinuko wrote:
> > On 2016/12/26 01:26:20, falken wrote:
> > > Sorry for the basic questions. Actually this confused me recently. I don't
> > > really understand when we use ascii vs utf8 vs utf16. I was looking to
> change
> > > most service worker messages from string16 to std::string (esp for things
> like
> > > the error messages), as per guidance from this code review:
> > > https://codereview.chromium.org/2433273003/#msg14
> > > 
> > > Do you know why service worker messages are using string16 to begin with?
> The
> > > earliest commit I found was
https://chromiumcodereview.appspot.com/23615009
> > > which uses string16.
> > 
> > Yeah, good questions.
> > If the string is non-ASCII / non-Latin1 using UTF-16 is more efficient
because
> > Blink's string internally uses it (unless it's latin1/ascii), so in most
cases
> > we used to default to string16.  Also the fact that WebString had implicit
> > conversions with string16 made people think that using utf-16 would be the
> best
> > choice, while it may not really be always true.
> 
> 
> Interesting, so string16 should be more efficient than std::string when
passing
> a possibly non-ascii string to Blink. What's your take on the memory concern
> raised in the linked code review? Would it still be sensible to change SW
> messages to pass std::string to save memory?

I don't have strong preference here, I'm not super worried about the CPU cost
for conversion as they're for error cases anyways, while if memory concern is
higher like nasko@ pointed it might make a better sense to convert them to
std::string.

> Also discovered:
>
https://docs.google.com/document/d/1kOCUlJdh2WJMJGDf-WoEQhmnjKLaOYRbiHz5TiGJl...
> 
> > 
> > >
> >
>
https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
> > > File content/child/service_worker/service_worker_dispatcher.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
> > > content/child/service_worker/service_worker_dispatcher.cc:134:
> > > blink::WebString::fromASCII(error_message)));
> > > Just to be sure what happens if you pass something that's not ascii-only
to
> > > fromASCII? Does it just become a null string or is there some
crash/security
> > > risk? I almost think we should just use UTF8 everywhere for simplicity and
> > avoid
> > > crashes.
> > 
> > The string is handled as if it's latin-1 in release build, but crashes in
> debug
> > build.  Yeah using UTF8 everywhere makes sense too for the code that isn't
> super
> > perf critical.  We're also trying to add ascii bit in blink's string so that
> we
> > can save repeated internal ascii-checks in blink code.
> > 
> > >
> >
>
https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
> > > File content/child/service_worker/web_service_worker_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2596173002/diff/1/content/child/service_worke...
> > > content/child/service_worker/web_service_worker_impl.cc:122:
> message.utf16(),
> > > url::Origin(source_origin),
> > > Do you know why this IPC should be a string16 and not just std::string?
> > 
> > (Please see my answer above, it was a default choice)

[kinuko, 2016-12-26 07:26:31]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2016-12-26 07:26:39]

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, 2016-12-26 08:55:38]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1482737191553130, "parent_rev":
"2872896e6546ba0f872e5db8d1670cad13c1ea60", "commit_rev":
"fecc4ca0a08010e1dcc6d84ba34e4db0ad6208be"}

[commit-bot: I haz the power, 2016-12-26 08:56:08]

Description was changed from

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

to

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

[commit-bot: I haz the power, 2016-12-26 08:56:09]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-26 08:58:09]

Description was changed from

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

to

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

Committed: https://crrev.com/6f58cf2958619996b056b14622bb43b16705248f
Cr-Commit-Position: refs/heads/master@{#440701}
==========

[commit-bot: I haz the power, 2016-12-26 08:58:10]

Patchset 1 (id:??) landed as
https://crrev.com/6f58cf2958619996b056b14622bb43b16705248f
Cr-Commit-Position: refs/heads/master@{#440701}