Closed Bug 1292293 Opened 3 years ago Closed 3 years ago

Update bindings generators to support default ByteString values in a dictionary

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: malisa.tsmith, Assigned: Nika)

References

Details

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36

Steps to reproduce:

(I initially encountered this issue when trying to implement the Response API for Servo [1])

In summary, the Response class [2] has a dictionary entry:
> ByteString statusText = "OK";

Firefox got rid of the default value of "OK" in the webidl [3], instead addressing it in Response.cpp [4]. This was justified because the Fetch spec used to say:
> There is no way to represent a constant ByteString value in IDL
However, it is now updated to say:
> There is no way to represent a constant ByteString value in IDL, although ByteString dictionary member and operation optional argument default values can be specified using a string literal. [5]

More about string literals here: [6]

In conclusion, the Firefox bindings generators should be updated to support default ByteString values in a dictionary.

[1] https://github.com/whatwg/fetch/issues/348
[2] https://fetch.spec.whatwg.org/#response-class
[3] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Response.webidl#35
[4] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Response.cpp#156
[5] http://heycam.github.io/webidl/#idl-ByteString
[6] http://heycam.github.io/webidl/#string-literal



Expected results:

The Firefox bindings generators should be updated to support default ByteString values in a dictionary.
Boris, do you know the history here?
Flags: needinfo?(bzbarsky)
Priority: -- → P2
The history is that this apparently got changed in the Web IDL spec 11 hours ago.  See <https://github.com/heycam/webidl/commit/7e2d743779eb47959ab3c5520937a4bb7948a6b8>.  Unfortunately, that commit didn't reference anything useful so it took me a while to figure out what actually happened.  Looks like http://heycam.github.io/webidl/#string-literal covers how to do the conversion, though that's not obvious.  I filed https://github.com/heycam/webidl/issues/141 on making the spec clearer.

This is probably not hard to implement if someone wants a simple-ish bindings project.  Especially if we're OK just forbidding non-ASCII constants initializing ByteString for now, so we don't have to worry about UTF8-encoding bits.  Let me know if you have someone who wants to take this; if not I'll just fix it.
Flags: needinfo?(bzbarsky)
Assignee: nobody → michael
Comment on attachment 8778467 [details]
Bug 1292293 - Part 1: Update the WebIDL Parser to allow default values for ByteString,

https://reviewboard.mozilla.org/r/69776/#review67282

::: dom/bindings/Codegen.py:5466
(Diff revision 1)
>              """,
>              nullable=nullable,
>              exceptionCode=exceptionCode)
>  
> -        # ByteString arguments cannot have a default value.
> -        assert defaultValue is None
> +        if defaultValue is not None:
> +            assert isinstance(defaultValue.value, str) # Check for byte string

It's not clear what this assert is really testing.  If there were a non-ASCII thing in the IDL, what would happen here, exactly?  Would the assert fail?  Would we just get garbage?

Have you tested this case?

::: dom/bindings/Codegen.py:5469
(Diff revision 1)
>  
> -        # ByteString arguments cannot have a default value.
> -        assert defaultValue is None
> +        if defaultValue is not None:
> +            assert isinstance(defaultValue.value, str) # Check for byte string
> +
> +            defaultCode = fill("""
> +                static const char data[] = { ${data} }; /* ${repr} */

This seems wrong.  What if the string includes `*/` in it?

::: dom/bindings/Codegen.py:5470
(Diff revision 1)
> -        # ByteString arguments cannot have a default value.
> -        assert defaultValue is None
> +        if defaultValue is not None:
> +            assert isinstance(defaultValue.value, str) # Check for byte string
> +
> +            defaultCode = fill("""
> +                static const char data[] = { ${data} }; /* ${repr} */
> +                $${declName}.Rebind(data, ArrayLength(data) - 1);

Have you tested this with unions containing ByteString?

In fact, this needs tests in general.  I see no changes to TestCodeGen.webidl in this patch...

::: dom/bindings/Codegen.py:5472
(Diff revision 1)
> +
> +            defaultCode = fill("""
> +                static const char data[] = { ${data} }; /* ${repr} */
> +                $${declName}.Rebind(data, ArrayLength(data) - 1);
> +                """,
> +                data=", ".join([str(ord(char)) for char in defaultValue.value] + ["0"]),

The DOMString case just uses `"'" + char + "'"`.  Might be clearer to do that here too.

In fact, there's a lot of code duplication with handleDefaultStringValue here....

::: dom/bindings/parser/WebIDL.py:3388
(Diff revision 1)
>              # treats USVString just like DOMString, but with an
>              # extra normalization step.
>              assert self.type.isDOMString()
>              return self
> +        elif self.type.isString() and type.isByteString():
> +            # Allow coersion from String literals to ByteString

You should assert `self.type.isDOMString()` here, like the USVString case does.
Attachment #8778467 - Flags: review?(bzbarsky) → review-
Comment on attachment 8778468 [details]
Bug 1292293 - Part 2: Remove custom default ByteString handling logic from ResponseInit,

https://reviewboard.mozilla.org/r/69778/#review67286

r=me
Attachment #8778468 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #5)
> The DOMString case just uses `"'" + char + "'"`.  Might be clearer to do
> that here too.

The reason I used str(ord(char)) instead of "'" + char + "'" was because I was worried about the code misbehaving if a special character, such as \n was to appear in the string, as it would break the output code. I'm changing the code to share the code path with other consumers of handleDefaultStringValue now.
Comment on attachment 8778467 [details]
Bug 1292293 - Part 1: Update the WebIDL Parser to allow default values for ByteString,

https://reviewboard.mozilla.org/r/69776/#review67326

I still don't see where we fail out for non-ASCII data.

You have a good point about random chars in `''` not being a good idea.  We should probably fail out for input like that....

::: dom/bindings/test/TestCodeGen.webidl:1012
(Diff revision 2)
>    DOMString empty = "";
>    TestEnum otherEnum = "b";
>    DOMString otherStr = "def";
>    DOMString? yetAnotherStr = null;
>    DOMString template;
> +  ByteString byteStr;

This still doesn't have tests for the union case, right?
Attachment #8778467 - Flags: review?(bzbarsky) → review-
I'm not sure whether this is an improvement or not with regards to the testing - I'm not sure what the best practices are for testing in this module, so I am probably still not testing enough. Sorry about that :(
Comment on attachment 8778467 [details]
Bug 1292293 - Part 1: Update the WebIDL Parser to allow default values for ByteString,

https://reviewboard.mozilla.org/r/69776/#review67752

::: dom/bindings/parser/WebIDL.py:3396
(Diff revision 3)
> +            # extra steps. We want to make sure that our string contains
> +            # only valid characters, so we check that here.
> +            valid_ascii_lit = " " + string.ascii_letters + string.digits + string.punctuation
> +            for c in self.value:
> +                if c not in valid_ascii_lit:
> +                    raise WebIDLError("Trying to coerce non-ascii string literal %s to ByteString"

This exception message should probably add that this is not supported yet.  And include some indication of what the failing char `c` was.

::: dom/bindings/test/TestCodeGen.webidl:1014
(Diff revision 3)
>    DOMString otherStr = "def";
>    DOMString? yetAnotherStr = null;
>    DOMString template;
> +  ByteString byteStr;
> +  ByteString emptyByteStr = "";
> +  ByteString otherByteStr = "def";

Thank you for adding the Web IDL parser tests; that's a good idea.

But what I was asking for was tests for the case of ByteString in a union.  The general state of testing for this module is that we aim to exercise all the codegen paths via TestCodeGen.webidl so we at least test that the code that gets generated compiles, and ideally examine it to make sure it makes sense.

From that point of view, you should do the following:

1. Add a passOptionalByteStringWithDefaultValue function right after passOptionalByteString in the TestInterface interface.  Also add it to TestExampleInterface in TestExampleGen.webidl and testJSImplInterface in TestJSImplGen.webidl.
2. After passUnionByteString, add a passOptionalUnionByteStringWithDefaultValue like so: `void passOptionalUnionByteStringWithDefaultValue(optional (ByteString or long) arg = "abc");`.  Again, add this to all three of the test webidl files I mention above.
Attachment #8778467 - Flags: review?(bzbarsky) → review-
Comment on attachment 8778467 [details]
Bug 1292293 - Part 1: Update the WebIDL Parser to allow default values for ByteString,

Gah.  I hate mozreview...

What I meant to say was "r+ assuming those tests compile and the generated code looks reasonable".  But it never asked me whether to mark r+ or not!
Attachment #8778467 - Flags: review- → review+
Comment on attachment 8778467 [details]
Bug 1292293 - Part 1: Update the WebIDL Parser to allow default values for ByteString,

https://reviewboard.mozilla.org/r/69776/#review69440

::: dom/bindings/parser/WebIDL.py:3395
(Diff revision 3)
> +            # No coercion is required as Codegen.py will handle the
> +            # extra steps. We want to make sure that our string contains
> +            # only valid characters, so we check that here.
> +            valid_ascii_lit = " " + string.ascii_letters + string.digits + string.punctuation
> +            for c in self.value:
> +                if c not in valid_ascii_lit:

Can this be `ord(c) <= 0x7f`?
(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #16)
> Comment on attachment 8778467 [details]
> Bug 1292293 - Part 1: Update the WebIDL Parser to allow default values for
> ByteString,
> 
> https://reviewboard.mozilla.org/r/69776/#review69440
> 
> ::: dom/bindings/parser/WebIDL.py:3395
> (Diff revision 3)
> > +            # No coercion is required as Codegen.py will handle the
> > +            # extra steps. We want to make sure that our string contains
> > +            # only valid characters, so we check that here.
> > +            valid_ascii_lit = " " + string.ascii_letters + string.digits + string.punctuation
> > +            for c in self.value:
> > +                if c not in valid_ascii_lit:
> 
> Can this be `ord(c) <= 0x7f`?

That would allow characters like the newline character and other control characters, which won't be handled correctly by this code path.
Comment on attachment 8778467 [details]
Bug 1292293 - Part 1: Update the WebIDL Parser to allow default values for ByteString,

https://reviewboard.mozilla.org/r/69776/#review72122

::: dom/bindings/Codegen.py:4243
(Diff revision 4)
> -    Returns a string which ends up calling 'method' with a (char16_t*, length)
> +    Returns a string which ends up calling 'method' with a (char_t*, length)
>      pair that sets this string default value.  This string is suitable for
>      passing as the second argument of handleDefault; in particular it does not
>      end with a ';'
>      """
> -    assert defaultValue.type.isDOMString()
> +    assert defaultValue.type.isDOMString() or defaultValue.type.isByteString()

How can defaultValue end up as isByteString()?  I thought we explicitly didn't do that in the IDL parser and left default value as domstring instead?

::: dom/bindings/Codegen.py:9618
(Diff revision 4)
>                      body=body % uninit))
>                  if self.ownsMembers:
>                      methods.append(vars["setter"])
>                      # Provide a SetStringData() method to support string defaults.
>                      # Exclude ByteString here because it does not support defaults
>                      # and only supports narrow nsCString.

This comment needs updating; for one thing ByteString supports defaults now.

::: dom/bindings/Codegen.py:9888
(Diff revision 4)
>                                             bodyInHeader=True,
>                                             body=body,
>                                             visibility="private"))
>                  # Provide a SetStringData() method to support string defaults.
>                  # Exclude ByteString here because it does not support defaults
>                  # and only supports narrow nsCString.

And this comment needs updating too.

::: dom/bindings/test/TestExampleGen.webidl:350
(Diff revision 4)
>    void passOptionalByteString(optional ByteString arg);
> +  void passOptionalByteStringWithDefaultValue(optional ByteString arg = "abc");
>    void passOptionalNullableByteString(optional ByteString? arg);
> +  void passOptionalNullableByteStringWithDefaultValue(optional ByteString? arg = null);
>    void passVariadicByteString(ByteString... arg);
>    void passUnionByteString((ByteString or long) arg);

Um... this isn't in TestCodeGen.webidl.  It should be!

So should the two new things you're adding.  Also, the capitalization in TestJSImplGen should be aligned with the capitalization here.  Yes, I know it's not your fault, but please fix it while you're here.

Basically, these three interfaces should look as identical as possible.

r=me with those nits fixed.
Attachment #8778467 - Flags: review?(bzbarsky) → review+
> How can defaultValue end up as isByteString()? 

OK, I know why that is: unions always coerce default values to their concrete type.

Given that, I think we should do the following:

1.  IDLValue.coerceToType should return IDLValue(self.location, type, self.value) when type.isByteString.  That way we will consistently have bytestring return values be bytestring-typed.

2.  Then handleDefaultStringValue does not need a char_t arg: it can just look at defaultValue.type to decide what char type to use.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: michael → bzbarsky
Status: NEW → ASSIGNED
Attachment #8784986 - Attachment is obsolete: true
Attachment #8784986 - Flags: review?(michael)
Oh, and this should have stayed assigned to Michael!  Maybe then he will check it in too.  ;)
Assignee: bzbarsky → michael
(In reply to Boris Zbarsky [:bz] from comment #25)
> Oh, and this should have stayed assigned to Michael!  Maybe then he will
> check it in too.  ;)

I KNEW I had forgotten to check something in...
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/autoland/rev/75766f418be3
Part 1: Update the WebIDL Parser to allow default values for ByteString, r=bz
https://hg.mozilla.org/integration/autoland/rev/88cc4d51a1ba
Part 2: Remove custom default ByteString handling logic from ResponseInit, r=bz
https://hg.mozilla.org/mozilla-central/rev/75766f418be3
https://hg.mozilla.org/mozilla-central/rev/88cc4d51a1ba
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.