Bindings: Add ScalarValueString support

Bindings: Add ScalarValueString support

Spec:
http://encoding.spec.whatwg.org/#type-scalarvaluestring

Followup to:
Add ByteString support to IDL bindings
https://codereview.chromium.org/309553002/

Encoding API: Handle null input string
https://codereview.chromium.org/313393005/

BUG=379009
TEST=fast/encoding/api/utf16-surrogates-encode.html
TEST=fast/js/webidl-type-mapping.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176449

[jsbell, 2014-06-04 21:05:41]

nbarth@ - this sits on top of https://codereview.chromium.org/309553002/

Not urgent.

Handling [Default=NullString] explicitly in TextEncoder.cpp looks like an
existing bug (but that feature's still behind a flag)

[jsbell, 2014-06-09 18:38:25]

On 2014/06/04 21:05:41, jsbell wrote:
> Handling [Default=NullString] explicitly in TextEncoder.cpp looks like an
> existing bug (but that feature's still behind a flag)

FYI, https://codereview.chromium.org/313393005 for the existing null string bug.

[Nils Barth (inactive), 2014-06-11 03:54:26]

LGTM with nits.

Mostly: "could you link to specs for all the unfamiliar technicalities"?
Also a few formatting notes, and one code factoring nit.

Looks great, thanks for another great CL!

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_a...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_a...
Source/bindings/scripts/v8_attributes.py:111: idl_type.is_integer_type or
idl_type.name in ('ByteString', 'ScalarValueString'),
Could you wrap this line?

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:122: if argument.idl_type.name in
('ByteString', 'ScalarValueString', 'SerializedScriptValue') or
Could you wrap this line?

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:289: if (argument.is_optional and
idl_type.name in ('String', 'ByteString', 'ScalarValueString') and
Break/wrap?

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:409: elif idl_type.is_integer_type or
idl_type.name in ('ByteString', 'ScalarValueString'):  # NormalConversion
ditto

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:457: elif idl_type.is_integer_type or
idl_type.name in ('ByteString', 'ScalarValueString'):
ditto

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
File Source/wtf/text/StringImpl.cpp (right):

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1896: PassRefPtr<StringImpl>
StringImpl::replaceUnpairedSurrogates()
Could you link to spec, and quote spec in comments?
http://heycam.github.io/webidl/#dfn-obtain-unicode

(Also aligning variable names with spec: S, n, U)

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1904: // Scan to see if there are any unmatched
surrogates.
Could you comment that this is a Blink-specific optimization to avoid copying if
no changes needed?

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1905: bool unmatched = false;
This block would be a bit simpler as a helper function, no?

Then we could just say:
if (!hasUnpairedSurrogates())
    return this;
...and just have 'if ... return true;' instead of lots of unmatched = true and
breaks.

Also, unmatched vs. unpaired (standard).

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1908: if (c < 0xD800 || c > 0xDFFF) {
Understand "want to match below", though style-wise I'd generally write
if ... continue;
...to avoid a sequence of else ifs and make it a bit flatter.

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1943: } else if (i == length - 1) {
Could you add a comment stating:
// 0xD800 <= c && c <= 0xDBFF (Lead surrogate)

Also, the term "surrogate pair" is confusing if unfamiliar; could you link to
the spec?
http://www.unicode.org/versions/Unicode6.2.0/ch03.pdf#G2630

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1950: data[i] = c;
Could you comment that we're not translating the surrogate pair to a Unicode
character here (and note where it happens instead)?

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1962: 
nit: one blank line, right?

[haraken, 2014-06-11 04:21:12]

LGTM, though I'm not an owner of wtf/.

https://codereview.chromium.org/313993002/diff/20001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/313993002/diff/20001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:524:
TONATIVE_DEFAULT_EXCEPTIONSTATE(v8::Local<v8::String>, stringObject,
value->ToString(), exceptionState, String());

Probably you can (or should?) use TOSTRING_DEFAULT.

[jsbell, 2014-06-12 17:45:56]

Feedback incorporated. I may have gone overly spec-comment-happy.

nbarth@ - can you take one more look before I kick this over to a wtf/text
owner?

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_a...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_a...
Source/bindings/scripts/v8_attributes.py:111: idl_type.is_integer_type or
idl_type.name in ('ByteString', 'ScalarValueString'),
On 2014/06/11 03:54:25, Nils Barth wrote:
> Could you wrap this line?

Done.

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:122: if argument.idl_type.name in
('ByteString', 'ScalarValueString', 'SerializedScriptValue') or
On 2014/06/11 03:54:26, Nils Barth wrote:
> Could you wrap this line?

Done.

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:289: if (argument.is_optional and
idl_type.name in ('String', 'ByteString', 'ScalarValueString') and
On 2014/06/11 03:54:26, Nils Barth wrote:
> Break/wrap?

Done.

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:409: elif idl_type.is_integer_type or
idl_type.name in ('ByteString', 'ScalarValueString'):  # NormalConversion
On 2014/06/11 03:54:26, Nils Barth wrote:
> ditto

Done.

https://codereview.chromium.org/313993002/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:457: elif idl_type.is_integer_type or
idl_type.name in ('ByteString', 'ScalarValueString'):
On 2014/06/11 03:54:26, Nils Barth wrote:
> ditto

Done.

https://codereview.chromium.org/313993002/diff/20001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/313993002/diff/20001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:524:
TONATIVE_DEFAULT_EXCEPTIONSTATE(v8::Local<v8::String>, stringObject,
value->ToString(), exceptionState, String());
On 2014/06/11 04:21:12, haraken wrote:
> 
> Probably you can (or should?) use TOSTRING_DEFAULT.

As discussed in the ByteString bug, I think we want to leave this as-is?

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
File Source/wtf/text/StringImpl.cpp (right):

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1896: PassRefPtr<StringImpl>
StringImpl::replaceUnpairedSurrogates()
On 2014/06/11 03:54:26, Nils Barth wrote:
> Could you link to spec, and quote spec in comments?
> http://heycam.github.io/webidl/#dfn-obtain-unicode
> 
> (Also aligning variable names with spec: S, n, U)

Done.

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1904: // Scan to see if there are any unmatched
surrogates.
On 2014/06/11 03:54:26, Nils Barth wrote:
> Could you comment that this is a Blink-specific optimization to avoid copying
if
> no changes needed?

Done.

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1905: bool unmatched = false;
On 2014/06/11 03:54:26, Nils Barth wrote:
> This block would be a bit simpler as a helper function, no?
> 
> Then we could just say:
> if (!hasUnpairedSurrogates())
>     return this;
> ...and just have 'if ... return true;' instead of lots of unmatched = true and
> breaks.
> 
> Also, unmatched vs. unpaired (standard).

Done.

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1908: if (c < 0xD800 || c > 0xDFFF) {
On 2014/06/11 03:54:26, Nils Barth wrote:
> Understand "want to match below", though style-wise I'd generally write
> if ... continue;
> ...to avoid a sequence of else ifs and make it a bit flatter.

Done.

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1943: } else if (i == length - 1) {
On 2014/06/11 03:54:26, Nils Barth wrote:
> Could you add a comment stating:
> // 0xD800 <= c && c <= 0xDBFF (Lead surrogate)

I added ASSERTs instead.

> Also, the term "surrogate pair" is confusing if unfamiliar; could you link to
> the spec?
> http://www.unicode.org/versions/Unicode6.2.0/ch03.pdf#G2630

Done.

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1950: data[i] = c;
On 2014/06/11 03:54:26, Nils Barth wrote:
> Could you comment that we're not translating the surrogate pair to a Unicode
> character here (and note where it happens instead)?

Done.

https://codereview.chromium.org/313993002/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1962: 
On 2014/06/11 03:54:26, Nils Barth wrote:
> nit: one blank line, right?

Done.

[jsbell, 2014-06-12 17:46:44]

Also, should I split out the TextEncoder change/tests into a separate CL?

[haraken, 2014-06-12 17:49:02]

https://codereview.chromium.org/313993002/diff/20001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/313993002/diff/20001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:524:
TONATIVE_DEFAULT_EXCEPTIONSTATE(v8::Local<v8::String>, stringObject,
value->ToString(), exceptionState, String());
On 2014/06/12 17:45:55, jsbell wrote:
> On 2014/06/11 04:21:12, haraken wrote:
> > 
> > Probably you can (or should?) use TOSTRING_DEFAULT.
> 
> As discussed in the ByteString bug, I think we want to leave this as-is?

Yes!

[Nils Barth (inactive), 2014-06-16 07:08:29]

Hi Kent, could you PTAL?

Bindings code is fine, but this adds some WTF functions,
so needs review by an OWNER.

Thanks!

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/StringIm...
File Source/wtf/text/StringImpl.cpp (right):

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1915: }
Want to add something like:
// Lead surrogate.
// 0xD800 <= c <= 0xDBFF
ASSERT(0xD800 <= c && c <= 0xDBFF):

[Nils Barth (inactive), 2014-06-16 07:09:46]

On 2014/06/12 17:46:44, jsbell wrote:
> Also, should I split out the TextEncoder change/tests into a separate CL?

I think it's fine as is: we generally add tests *with* code they test, right,
and otherwise ScalarValueString is untested.

[Nils Barth (inactive), 2014-06-16 07:10:49]

On 2014/06/12 17:45:56, jsbell wrote:
> Feedback incorporated. I may have gone overly spec-comment-happy.

It's pretty detailed, but very clear -- thanks!

> nbarth@ - can you take one more look before I kick this over to a wtf/text
> owner?

Just did, and asked Kent to take a look (as he's local);
please feel free to reassign.

[tkent, 2014-06-16 07:51:16]

https://codereview.chromium.org/313993002/diff/1/Source/wtf/text/WTFString.h
File Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/313993002/diff/1/Source/wtf/text/WTFString.h#...
Source/wtf/text/WTFString.h:386: String toScalarValueString() const;
String is not suitable for this function.
'scalar value' is a concept of the web platform.  WTF should be independent from
the web platform.

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/StringIm...
File Source/wtf/text/StringImpl.cpp (right):

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1908: if (c < 0xD800 || c > 0xDFFF) {
!U16_IS_SURROGATE(c)

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1912: if (0xDC00 <= c && c <= 0xDFFF) {
U16_IS_TRAIL(c)

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1921: if (0xDC00 <= d && d <= 0xDFFF) {
U16_IS_TRAIL(d)

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/StringIm...
File Source/wtf/text/StringImpl.h (right):

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.h:401: PassRefPtr<StringImpl>
replaceUnmatchedSurrogates();
Unclear what unmatched surrogates are replaced with.

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/WTFString.h
File Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/WTFStrin...
Source/wtf/text/WTFString.h:386: String toScalarValueString() const;
WTF should not have such function.  I think this should be in
Source/platform/text/ or Source/bindings/

'scalar value' is a concept of the web platform, and WTF should not depend on
the web platform.

[jsbell, 2014-06-17 21:39:50]

Thanks, tkent@; moved everything out of Source/wtf so you're off the hook.

haraken@, nbarth@ - can you take another look? I went for an absurdly long
function name based on the spec algorithm name, but happy to change.

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/StringIm...
File Source/wtf/text/StringImpl.cpp (right):

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:1908: if (c < 0xD800 || c > 0xDFFF) {
On 2014/06/16 07:51:16, tkent wrote:
> !U16_IS_SURROGATE(c)

Done - used U16_XXX macros throughout. Also, tightened the
hasUnmatchedSurrogates() function up since the macros are self-documenting.

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/WTFString.h
File Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/313993002/diff/40001/Source/wtf/text/WTFStrin...
Source/wtf/text/WTFString.h:386: String toScalarValueString() const;
On 2014/06/16 07:51:16, tkent wrote:
> WTF should not have such function.  I think this should be in
> Source/platform/text/ or Source/bindings/
> 
> 'scalar value' is a concept of the web platform, and WTF should not depend on
> the web platform.

Okie dokie - moved it directly into V8Bindings.cpp.

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:609: u.append(static_cast<UChar32>((1 << 16) +
(a << 10) + b));
Since this is using StringBuilder, it's now "per spec" rather than just
re-appending the UTF-16 code units. I can change it back.

[tkent, 2014-06-17 23:38:54]

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:609: u.append(static_cast<UChar32>((1 << 16) +
(a << 10) + b));
U16_GET_SUPPLEMENTARY(c, d) ?

[Nils Barth (inactive), 2014-06-18 03:25:27]

LGTM (v. minor nits); thanks Kent for v. useful comments!
haraken, could you PTAL?
Main question is what to call the function ;)

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:551: static String
convertDOMStringToSequenceOfUnicodeCharacters(const String& string)
I admire your adherence to spec, but you're right, it's really long.
haraken: any opinions?
obtainUnicode would match the spec anchor;
utf16ToUnicode or domStringToUnicode are brief but a bit cryptic;
resolveSurrogates seems clear enough.
WDYT?

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:554: // but the output is still a sequence of
16-bit code units, effectively
Is this still true, now that you're using StringBuilder?

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:609: u.append(static_cast<UChar32>((1 << 16) +
(a << 10) + b));
On 2014/06/17 23:38:54, tkent wrote:
> U16_GET_SUPPLEMENTARY(c, d) ?

+1!

[haraken, 2014-06-18 04:57:33]

LGTM for bindings/.

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:551: static String
convertDOMStringToSequenceOfUnicodeCharacters(const String& string)
On 2014/06/18 03:25:26, Nils Barth wrote:
> I admire your adherence to spec, but you're right, it's really long.
> haraken: any opinions?
> obtainUnicode would match the spec anchor;
> utf16ToUnicode or domStringToUnicode are brief but a bit cryptic;
> resolveSurrogates seems clear enough.
> WDYT?

I don't have a strong opinion here, since this method is just used internally in
V8Binding.cpp.

[jsbell, 2014-06-18 17:25:20]

Thanks all!

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:551: static String
convertDOMStringToSequenceOfUnicodeCharacters(const String& string)
On 2014/06/18 03:25:26, Nils Barth wrote:
> I admire your adherence to spec, but you're right, it's really long.
> haraken: any opinions?
> obtainUnicode would match the spec anchor;
> utf16ToUnicode or domStringToUnicode are brief but a bit cryptic;
> resolveSurrogates seems clear enough.
> WDYT?

I'll go back to 'replaceUnmatchedSurrogates' and add a comment that they're
replaced by the replacement character (U+FFFD)

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:554: // but the output is still a sequence of
16-bit code units, effectively
On 2014/06/18 03:25:26, Nils Barth wrote:
> Is this still true, now that you're using StringBuilder?

Yes. Blink strings are 16-bit internally, this just guarantees that it's valid
UTF-16.

The point of the ScalarValueString type in the spec is that the binding layer -
both spec and impl - enforces that it's a valid Unicode string, so that the rest
of the spec/impl can simply refer to characters (i.e. scalar values); in the
impl those characters may still be stored as multiple code units.

[Excuse me while I track down the person that first roped me into encoding
issues, and slap them with a trout.]

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:609: u.append(static_cast<UChar32>((1 << 16) +
(a << 10) + b));
On 2014/06/18 03:25:26, Nils Barth wrote:
> On 2014/06/17 23:38:54, tkent wrote:
> > U16_GET_SUPPLEMENTARY(c, d) ?
> 
> +1!

Will do!

[jsbell, 2014-06-18 18:41:31]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-06-18 18:41:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jsbell@chromium.org/313993002/100001

[commit-bot: I haz the power, 2014-06-18 20:01:36]

Change committed as 176449

[Nils Barth (inactive), 2014-06-20 00:57:54]

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/313993002/diff/80001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:554: // but the output is still a sequence of
16-bit code units, effectively
Thanks for explanation!

On 2014/06/18 17:25:19, jsbell wrote:
> [Excuse me while I track down the person that first roped me into encoding
> issues, and slap them with a trout.]

(You could swap with Kouhei: he's been roped into SVG ;)