1025183 - support ScalarValueString type in webidl

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

ScalarValueString is defined in the encoding spec[1] as:

"ScalarValueString is identical to DOMString except that convert a DOMString to a sequence of Unicode characters is used subsequently when converting to an IDL value. Ergo, the IDL value is a sequence of scalar values."

As a temporary work-around webidl files are just using a DOMString typedef.

This bug is to implement ScalarValueString in our webidl compiler as a type that can be referenced directly and automatically performs the necessary conversion before calling the binding class.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I've been told in IRC this will be similar to implementing ByteString.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ScalarValueString to webidl parser (v0)

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ScalarValueString to webidl Codegen.py (v0)

Interim patch that allows ScalarValueString to compile, although it basically just does exactly what DOMString does.  This compiles with the patches from bug 1027256.

I'll now go back and figure out the diffs to make ScalarValueString do the right thing.  I believe on IRC we decided to use a new ScalarValueString class for arguments and a DOMString for return values.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2: Add ScalarValueString to webidl Codegen.py (v1)

I believe this is mostly correct.  Still todo:

1) Clean up the patch a bit by moving a few changes into separate patches.
2) Write tests to confirm conversion to unicode characters is correct.

Thoughts on the implementation so far?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ScalarValueString to webidl parser (v1)

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ScalarValueString to webidl Codegen.py (v2)

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add tests for ScalarValueString.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add tests for ScalarValueString. (v1)

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

So, these patches have bit rotted. In particular, I made ScalarValueString mimic FakeDependentString.  When SetData() is called, then the unicode conversion is executed.

After bug 1032748, however, SetData() is gone.  Instead there is a Rebind() method and a BeginWriting() method.  I can perform the conversion in Rebind(), but not sure how to do it with BeginWriting().

Boris, what do you think?

Comment 10

[Peter Van der Beken [:peterv]]

(In reply to Ben Kelly (PTO / back July 21) [:bkelly] from comment #9)
> After bug 1032748, however, SetData() is gone.  Instead there is a Rebind()
> method and a BeginWriting() method.  I can perform the conversion in
> Rebind(), but not sure how to do it with BeginWriting().

SetData was renamed to Rebind, but it's just a naming change. Doing s/SetData/Rebind/ in your patches should be trivial.

Note that BeginWriting was added by bug 1032726, not bug 1032748.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I think you have two options.

Option 1 is to do the conversion in SetLength, which is the last thing AssignJSString will call on your string class.

Option 2 is to do the conversion in codegenned code, immediately after doing the ConvertJSValueToString.  Now that ConvertJSValueToString always makes a copy, there is no issue with just making the modification after the fact.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Thanks.  I think option 2 sounds good.  I could use my ScalarVauleString class at that point or just have a helper method.  I'd like to avoid making another copy if I can help it.  Not sire.if I can replace inline.

Comment 13

[Boris Zbarsky [:bzbarsky]]

You can replace inline, since the replacement is of unpaired surrogates (which are by definition a single 16-bit code unit) with U+FFFD, which is also a single 16-bit code unit.  So no extra copy needed.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ScalarValueString to webidl parser (v2)

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8465117 [details] [diff] [review]
P1 Add ScalarValueString to webidl parser (v2)

Sorry for the flag spam.  Realized I need to tweak a few more things in the following patches.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ScalarValueString to webidl Codegen.py (v3)

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add tests for ScalarValueString. (v2)

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ScalarValueString to webidl Codegen.py (v4)

Remove some stale code I accidentally left commented in.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Try build:

  https://tbpl.mozilla.org/?tree=Try&rev=5254401e0d52

Also, I believe P3 will need to be rebased after bug 1036214, but it appears to be pretty minor.  No major conflicts that I can see.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465117 [details] [diff] [review]
P1 Add ScalarValueString to webidl parser (v2)

>@@ -2594,16 +2622,18 @@ class IDLValue(IDLObject):
>+        elif self.type.isString() and type.isString():
>+            return self

Should we not return a new IDLValue of type "type" instead?  Please document why "self" is the right thing to return here.  In practice, is it just that self.type is DOMString if we're taking this return and that's what callers expect to see for a string default value?  If so, worth asserting that self.type.isDOMString(), or just checking for self.type.isDOMString().

This will allow things like void method(optional ByteString arg = "foo"), right?  Is that purposeful?

>+    harness.ok(True, "TestScalarValueString interface parsed without error.")

No need, since if an exception got thrown that will fail the test automatically.

>+    harness.check(len(members), 1, "Should be one production")

"one member"?

Please add the new type to dom/bindings/parser/tests/test_distinguishability.py

r=me with those nits picked.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465128 [details] [diff] [review]
P3 Add tests for ScalarValueString. (v2)

Oh, here's where test_distinguishability.py gets changed.  OK, great.

Don't you nee to:

  setDistinguishable("ScalarValueString", nonStrings)

?  How did the test pass without that?

>+  void passUnionWithByteString((ByteString or long) arg);

Could you please put this in the same place in the list in all three test interfaces?

r=me

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465131 [details] [diff] [review]
P2 Add ScalarValueString to webidl Codegen.py (v4)

I'm not sure I follow NormalizeScalarValueStringInternal. Why dose it ever need to change lengths?  I didn't think ScalarValueString ever changed the length in 16-bit units of the input.

Seems to me like a fairly faithful implementation of http://heycam.github.io/webidl/#dfn-obtain-unicode (well, but using NS_IS_LOW_SURROGATE/NS_IS_HIGH_SURROGATE) would just work in-place and not even require using UTF16CharEnumerator.  Am I just missing something?

The rest looks good, but I'd like to understand this part.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #22)
> I'm not sure I follow NormalizeScalarValueStringInternal. Why dose it ever
> need to change lengths?  I didn't think ScalarValueString ever changed the
> length in 16-bit units of the input.

Yea, I realized this was the case after I logged off last night.  My original understanding of the algorithm was a bit off, but I came to this conclusion as I worked through the test cases.

I'll go back and make it work completely in-place.

> Seems to me like a fairly faithful implementation of
> http://heycam.github.io/webidl/#dfn-obtain-unicode (well, but using
> NS_IS_LOW_SURROGATE/NS_IS_HIGH_SURROGATE) would just work in-place and not
> even require using UTF16CharEnumerator.  Am I just missing something?

I was trying to use UTF16CharEnumerator to avoid duplicating code within the tree.  It seemed to be implementing the same algorithm to me, but perhaps I missed something subtle.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #21)
> Oh, here's where test_distinguishability.py gets changed.  OK, great.
> 
> Don't you nee to:
> 
>   setDistinguishable("ScalarValueString", nonStrings)
> 
> ?  How did the test pass without that?

This might explain the huge amount of orange during make check in the try build. :-)  I guess I never re-ran these tests after doing the P1 patch.  I was only testing mochitest.  Thanks for pointing out the error!

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ScalarValueString to webidl parser (v3)

I moved the test_distinguishability.py changes to this patch.

Also, I restricted the default type coercion to only ScalarValueString as including ByteString was incorrect.  I left the return as self since Codegen.py expects string default values to be exactly DOMString.  Added a comment and assert to explain.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add tests for ScalarValueString. (v3)

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Document how UTF16CharEnumerator treats buffer position on error. (v0)

This is a short patch the documents how UTF16CharEnumerator treats the buffer position when it returns the UCS2_REPLACEMENT_CHAR.  I also clarified that the final error condition is actually unreachable.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add ScalarValueString to webidl Codegen.py (v5)

Greatly simplify the ScalarValueString code now that its clear to me that UTF16CharEnumerator simply replaces orphaned surrogates with the replacement character and does not change the length of the string.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add tests for ScalarValueString. (v4)

Renumber since I inserted a new patch.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465520 [details] [diff] [review]
P2 Document how UTF16CharEnumerator treats buffer position on error. (v0)

r=me; I verified that this is in fact what happens.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465523 [details] [diff] [review]
P3 Add ScalarValueString to webidl Codegen.py (v5)

>+  const nsString::char_type* nextChar = aString.BeginWriting();

s/nsString::char_type/char16_t/, please.  And document that it's const because you can't pass a foo** as a const foo**?

>+      MOZ_ASSERT(lastChar == (nextChar - 1));

Given this, could also just stash a non-const version of BeginWriting() in writableChars at the very beginning, nix lastChar, and here do:

  writableChars[nextChar - 1 - writableChars] = static_cast<uint16_t>(enumerated);

or some such...  Does lose the assert, though.

r=me either way

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Document how UTF16CharEnumerator treats buffer position on error. (v1)

Had to include mozilla/Assertions.h from nsUTF8Utils.h since the try build was failing (on windows only for some reason).

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add ScalarValueString to webidl Codegen.py (v6)

Updated to use char16_t and try to avoid const_cast.  I still use a separate lastCharIndex variable, though, as I think it makes the code more readable.  Also added a comment about the strangeness of not being able to pass char** for const char**.

Last try build:

https://tbpl.mozilla.org/?tree=Try&rev=c0ccbfe9bedd

Since I've had a few red tries, I'll wait until this is fairly green to push to inbound.

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Try was looking good, so pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1be9a6d20566
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/36fb56f9582f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/01c51cb8d8bd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d8052625fa

Comment 35

[Ed Morley [:emorley]]

Backed out for failures in test_scalarvaluestring.html on B2G emulator debug & Android 4.0 debug:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d8d8052625fa
https://tbpl.mozilla.org/php/getParsedLog.php?id=45014793&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=45014746&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c22972b0512
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/70e2b73d053b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f701253c5ba9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/45424f6d183f

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

The b2g and android package Makefiles don't set MOZ_DEBUG properly.  This means TestInterfaceJS.manifest is not properly included in these builds.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Define MOZ_DEBUG for b2g and android packagers so that TestInterfaceJS is included properly. (v0)

This should fix the b2g and android package Makefiles so that TestInterfaceJS is properly included.

https://tbpl.mozilla.org/?tree=Try&rev=3ad12594451d

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add tests for ScalarValueString. (v5)

Renumber.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Does that mean you can re-enable test_getWebIDLCaller.html on android?  Seems like you should be able to...

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #39)
> Does that mean you can re-enable test_getWebIDLCaller.html on android? 
> Seems like you should be able to...

Looks like it.  Follow-up in bug 1047514.

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Try was fully green, so with mshal's r+:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/69373df15281
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dd6d84edf08e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/728a5d18bcd9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/71babe0bc84b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c60f9054acf6

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add ScalarValueString to webidl Codegen.py (v7)

Catch up with bug 1035654 which landed in mozilla-inbound last night.  This creates another spot within Codegen.py where we need to treat ScalarValueString the same as DOMString.

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/08d487163e65
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3b63f275ab
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3000c183bec7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1be6390b8931
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4630d3e352ed

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08d487163e65
https://hg.mozilla.org/mozilla-central/rev/ce3b63f275ab
https://hg.mozilla.org/mozilla-central/rev/3000c183bec7
https://hg.mozilla.org/mozilla-central/rev/1be6390b8931
https://hg.mozilla.org/mozilla-central/rev/4630d3e352ed