Bug 70816 – Get rid of optional parameters in the middle in IDLs.

• Component: DOM
• Status: RESOLVED FIXED

[Pavel Podivilov]

Get rid of optional parameters in the middle in IDLs.

Optional parameters in the middle are prohibited by WebIDL spec.

There are two places where we use them:
1) DOMWindow.idl postMessage(in SerializedScriptValue message, in [Optional] Array messagePorts, in DOMString targetOrigin)
which actually means
postMessage(in SerializedScriptValue message, in DOMString targetOrigin)
postMessage(in SerializedScriptValue message, in Array messagePorts, in DOMString targetOrigin)

2) CanvasRenderingContext2D.idl putImageData(in ImageData imagedata, in float dx, in float dy, in [Optional] float dirtyX, in float dirtyY, in float dirtyWidth, in float dirtyHeight)
which actually means
putImageData(in ImageData imagedata, in float dx, in float dy)
putImageData(in ImageData imagedata, in float dx, in float dy, in float dirtyX, in float dirtyY, in float dirtyWidth, in float dirtyHeight)

[Pavel Podivilov]

Created attachment 112332 [details]
Patch

[Adam Barth]

Comment on attachment 112332 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112332&action=review

Did you run ./Tool/Scripts/run-bindings-tests ?  You might need to update the expected results.

> Source/WebCore/page/DOMWindow.idl:218
> -        [DoNotCheckDomainSecurity, Custom] void postMessage(in SerializedScriptValue message, in [Optional] Array messagePorts, in DOMString targetOrigin)
> +        [DoNotCheckDomainSecurity, Custom] void postMessage(in SerializedScriptValue message, in DOMString targetOrigin)
>              raises(DOMException);
> -        [DoNotCheckDomainSecurity, Custom] void webkitPostMessage(in SerializedScriptValue message, in [Optional] Array transferList, in DOMString targetOrigin)
> +        [DoNotCheckDomainSecurity, Custom] void postMessage(in SerializedScriptValue message, in Array messagePorts, in DOMString targetOrigin)
> +            raises(DOMException);

This is being discussed in the working group, but I see that you're not changing behavior.

> LayoutTests/fast/canvas/canvas-putImageData-expected.txt:147
> -PASS context.putImageData({}, 0, 0) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.
> +PASS context.putImageData({}, 0, 0) threw exception TypeError: Type error.

Interesting.  I didn't know these were different.  This looks like an OK behavior change.

[Adam Barth]

+levin because this touches the postMessage parameter tangle.

[David Levin]

Actually, I asked Alexey about this optional parameter not long ago.

Here is what he said
"The goal of IDL is to document actual behavior here - it's the middle argument that is optional indeed. Technically, it does not matter what the arguments are, since the function is [Custom] anyway.

See also: https://bugs.webkit.org/show_bug.cgi?id=63141

There is a long story behind it that I forgot, something about gradually adding arguments and bringing Window and MessagePort versions of postMessage in sync. And once we implemented the mad behavior HTML5 had at that point, the spec got changed again."

I guess https://bugs.webkit.org/show_bug.cgi?id=63141#c11 says that optional shouldn't be there, so this is following that advice.

[Adam Barth]

That's fine, but we have a long-term goal of converging WebKitIDL to WebIDL and internal optional parameters aren't allowed in WebIDL.  The WebIDL way to express this behavior is using overloading, which is what this patch does.

[David Levin]

(In reply to comment #5)
> That's fine, but we have a long-term goal of converging WebKitIDL to WebIDL and internal optional parameters aren't allowed in WebIDL.  The WebIDL way to express this behavior is using overloading, which is what this patch does.

Thanks!

[Alexey Proskuryakov]

Comment on attachment 112332 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112332&action=review

Specific consequences of this change look like they are both for the worse.

I don't feel very strong about this, and postMessage specifically seems to be in need of change, but this needs to be more clearly an improvement. Specifically, postMessage documentation changes should go together actual implementation changes (in a dedicated patch).

>> Source/WebCore/page/DOMWindow.idl:218
>> +            raises(DOMException);
> 
> This is being discussed in the working group, but I see that you're not changing behavior.

But is this change an improvement? [Optional] is a very useful bit of documentation here. Documentation is why there are arguments specified for [Custom] functions - they are ignored otherwise.

What this change does is making documentation not match reality.

>> LayoutTests/fast/canvas/canvas-putImageData-expected.txt:147
>> +PASS context.putImageData({}, 0, 0) threw exception TypeError: Type error.
> 
> Interesting.  I didn't know these were different.  This looks like an OK behavior change.

Is it specified what should happen? This looks like a change for worse - incorrect arguments for a built-in function should raise a DOM exception, not a JS one.

[Adam Barth]

> I don't feel very strong about this, and postMessage specifically seems to be in need of change, but this needs to be more clearly an improvement. Specifically, postMessage documentation changes should go together actual implementation changes (in a dedicated patch).

Respectfully, I disagree.  This patch doesn't change behavior.  It just better aligns the syntax of our IDL files with WebIDL.  We can consider changing behavior in a future patch.  There's no reason to block syntax convergence with WebIDL on behavioral changes.

> Is it specified what should happen? This looks like a change for worse - incorrect arguments for a built-in function should raise a DOM exception, not a JS one.

That's not how we handle this situation throughout the engine.

[Adam Barth]

Comment on attachment 112332 [details]
Patch

Thanks for your feedback Alexey, but, as far as I can tell, you aren't an active contributor to this area of the code anymore and your comments are based on an outdated understanding of how this code works and the direction we'd like this code to take.

[Alexey Proskuryakov]

To be clear, I certainly support getting rid of optional arguments in the middle. I also think that changing postMessage to match spec is good, subject to compatibility testing.

> Respectfully, I disagree.  This patch doesn't change behavior.

The issue that I have with this patch is that it changes documentation (IDL) to not match implementation. That's clearly a step in the wrong direction, and the fact that the patch doesn't change behavior is actually the problem.

The whole reason to have argument lists in IDL for [Custom] functions is for documentation. Is the  change for DOMWindow.idl because CodeGenerator parses argument lists even for [Custom] functions? In that case, an expedient way to resolve this could be to stop parsing them.

> That's not how we handle this situation throughout the engine.

Could you explain this in a little more detail? We throw TYPE_MISMATCH_ERR a lot, both from generated bindings code and from DOM code. What are the situations similar to this one where we throw a JS exception?

[Adam Barth]

Simplifying the diff a bit, here's the relevant portion of the change:

- void postMessage(in SerializedScriptValue message, in [Optional] Array messagePorts, in DOMString targetOrigin)
+ void postMessage(in SerializedScriptValue message, in DOMString targetOrigin)
+ void postMessage(in SerializedScriptValue message, in Array messagePorts, in DOMString targetOrigin)

Aside from the fact that the first declaration is technically meaningless, the two forms of declaring postMessage are equivalent.  The only difference is that that the new syntax is closer to valid WebIDL.

> > That's not how we handle this situation throughout the engine.
> 
> Could you explain this in a little more detail? We throw TYPE_MISMATCH_ERR a lot, both from generated bindings code and from DOM code. What are the situations similar to this one where we throw a JS exception?

Conceptually, it depends on which layer detects the error.  In the course of calling a DOM API, at some point you leave the "JavaScript" world and enter in the "DOM" world.  The JavaScript world is responsible for figuring out which API you're calling and for converting the arguments from the JavaScript world to the DOM world (e.g., calling toString on DOMString arguments).  If the error is detected before we leave the JavaScript world, we throw JavaScript errors (conceptually because this code is JavaScript code, not DOM).  If the error is detected after we enter the DOM world, we through DOM errors.

If you read the WebIDL specs, this is related to how the spec is factored between language neutral processing of WebIDL and JavaScript-specific processing.

[Alexey Proskuryakov]

> - void postMessage(in SerializedScriptValue message, in [Optional] Array messagePorts, in DOMString targetOrigin)
> + void postMessage(in SerializedScriptValue message, in DOMString targetOrigin)
> + void postMessage(in SerializedScriptValue message, in Array messagePorts, in DOMString targetOrigin)

I missed that there are two overrides added, as they were interspersed with webkitPostMessage. I don't have any objection against this part any more.

> > > That's not how we handle this situation throughout the engine.
> > 
> > Could you explain this in a little more detail? We throw TYPE_MISMATCH_ERR a lot, both from generated bindings code and from DOM code. What are the situations similar to this one where we throw a JS exception?
> 
> Conceptually, it depends on which layer detects the error.

I know the theory. You claimed that this is how we handle this situation throughout the engine. Is it difficult to give an example that's similar to this case?

[Adam Barth]

Comment on attachment 112332 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112332&action=review

>>> LayoutTests/fast/canvas/canvas-putImageData-expected.txt:147
>>> +PASS context.putImageData({}, 0, 0) threw exception TypeError: Type error.
>> 
>> Interesting.  I didn't know these were different.  This looks like an OK behavior change.
> 
> Is it specified what should happen? This looks like a change for worse - incorrect arguments for a built-in function should raise a DOM exception, not a JS one.

You can run this command to see a number of modern examples:

LayoutTests/fast/canvas/webgl$ grep -r "threw exception TypeError" .

[Ian 'Hixie' Hickson]

If you're going to be poking at the postMessage() part of the codebase, please just fix the order of the arguments to match the spec... the spec was changed specifically to avoid having optional arguments in the middle of the argument list.

See bug 63141.

[Adam Barth]

Yes, that's what I mentioned earlier.  This patch doesn't actually change any behavior.  It just changes the syntax we use to express the current behavior.  I suspect we should change our behavior, but that's a question for another bug.

[Alexey Proskuryakov]

Adam, seeing dozens of unrelated TypeError exceptions certainly opens my eyes as to why a patch that removes [Optional] from fourth argument of putImageData results in different exception being raised when {} is passed as its first argument.

I'm puzzled that you have such attitude mere weeks after asking me about some basic facts of how bindings work on IRC.

[Adam Barth]

Committed r98379: <http://trac.webkit.org/changeset/98379>

[Adam Barth]

> I'm puzzled that you have such attitude mere weeks after asking me about some basic facts of how bindings work on IRC.

I'm not sure what you're trying to imply with this statement.

[Ryosuke Niwa]

The test added by this patch is failing on SL, GTK, & Qt:
http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r98427%20(2715)/fast/canvas/canvas-putImageData-pretty-diff.html

It's probably a V8/JSC difference.

[Adam Barth]

> It's probably a V8/JSC difference.

Thanks.  Sounds like we need to create new baselines.

[Csaba Osztrogonác]

(In reply to comment #20)
> > It's probably a V8/JSC difference.
> 
> Thanks.  Sounds like we need to create new baselines.

I don't think if "FAIL" is correct ... It sounds like a bug. Reopen the bug to fix it.

--- /ramdisk/qt-linux-release/build/layout-test-results/fast/canvas/canvas-putImageData-expected.txt 
+++ /ramdisk/qt-linux-release/build/layout-test-results/fast/canvas/canvas-putImageData-actual.txt 
@@ -144,7 +144,7 @@
 PASS getPixel(9,9) is [0,128,0,255]
 PASS getPixel(1,1) is [0,128,0,255]
 PASS getPixel(9,9) is [0,128,0,255]
-PASS context.putImageData({}, 0, 0) threw exception TypeError: Type error.
+FAIL context.putImageData({}, 0, 0) should throw TypeError: Type error. Threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.
 PASS context.putImageData(buffer, NaN, 0, 0, 0, 0, 0) threw exception Error: NOT_SUPPORTED_ERR: DOM Exception 9.
 PASS context.putImageData(buffer, 0, NaN, 0, 0, 0, 0) threw exception Error: NOT_SUPPORTED_ERR: DOM Exception 9.
 PASS context.putImageData(buffer, 0, 0, NaN, 0, 0, 0) threw exception Error: NOT_SUPPORTED_ERR: DOM Exception 9.

[Adam Barth]

@Pavel: I've rolled out your patch.  It looks like we'll need to iterate some more to avoid breaking tests on ports that use JavaScriptCore.

[Pavel Podivilov]

Created attachment 112481 [details]
Patch

[Pavel Podivilov]

Overloaded methods throw different exceptions in V8 and JSC. Updated the test to call "context.putImageData(null, 0, 0)" just like canvas-createPattern and canvas-createImageData tests do (those won't pass with {} as well).

[Adam Barth]

Comment on attachment 112481 [details]
Patch

This looks like we're losing test coverage.  Can we make the two bindings throw the same exception?

[Darin Adler]

Comment on attachment 112481 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112481&action=review

> LayoutTests/ChangeLog:11
> +        Optional parameters in the middle are prohibited by WebIDL spec.
> +
> +        * fast/canvas/canvas-putImageData-expected.txt:
> +        * fast/canvas/canvas-putImageData.js:

What is the change to this test? Why change what’s being tested, rather than just adding a new test case?

[Pavel Podivilov]

Created attachment 112562 [details]
Patch

[Pavel Podivilov]

Generated overloaded methods in V8 throw Type Error.
putImageData has to be custom to throw DOM Error.

[Pavel Podivilov]

(In reply to comment #26)
> (From update of attachment 112481 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112481&action=review
> 
> > LayoutTests/ChangeLog:11
> > +        Optional parameters in the middle are prohibited by WebIDL spec.
> > +
> > +        * fast/canvas/canvas-putImageData-expected.txt:
> > +        * fast/canvas/canvas-putImageData.js:
> 
> What is the change to this test? Why change what’s being tested, rather than just adding a new test case?

putImageData was the only canvas method that throws DOM Exception when called with "{}" as first argument. All other methods throw Type Error in exact same situation.

New patch preserves existing behavior of putImageData.

[Adam Barth]

Comment on attachment 112562 [details]
Patch

We shouldn't add a custom putImageDataCallback just to throw a different exception.  We should fix the code generators to both throw the same exception.  (Either exception is fine, although I'd prefer the JS exception to the DOM exception, as discussed above.)

[Pavel Podivilov]

Created attachment 112659 [details]
Patch

[Pavel Podivilov]

(In reply to comment #30)
> (From update of attachment 112562 [details])
> We shouldn't add a custom putImageDataCallback just to throw a different exception.  We should fix the code generators to both throw the same exception.  (Either exception is fine, although I'd prefer the JS exception to the DOM exception, as discussed above.)

You are right, context.putImageData({}, 0, 0) should throw JS exception. I was tricked by wrong test expectation.

It threw DOM Exception due to wrong custom implementation in JSC and due to a bug in V8 generated version.

[Adam Barth]

Comment on attachment 112659 [details]
Patch

Yay for deleting buggy custom bindings.  Thanks.

[WebKit Review Bot]

Comment on attachment 112659 [details]
Patch

Clearing flags on attachment: 112659

Committed r98617: <http://trac.webkit.org/changeset/98617>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch may have caused canvas/philip/tests/2d.imageData.put.wrongtype.html and canvas/philip/tests/2d.missingargs.html to fail on Snow Leopard bots:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98676%20(34249)/results.html

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=canvas%2Fphilip%2Ftests%2F2d.missingargs.html%2Ccanvas%2Fphilip%2Ftests%2F2d.imageData.put.wrongtype.html&group=%40ToT%20-%20webkit.org

[Pavel Podivilov]

(In reply to comment #36)
> This patch may have caused canvas/philip/tests/2d.imageData.put.wrongtype.html and canvas/philip/tests/2d.missingargs.html to fail on Snow Leopard bots:
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98676%20(34249)/results.html
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=canvas%2Fphilip%2Ftests%2F2d.missingargs.html%2Ccanvas%2Fphilip%2Ftests%2F2d.imageData.put.wrongtype.html&group=%40ToT%20-%20webkit.org

These tests need new expectations, see https://bugs.webkit.org/show_bug.cgi?id=71104.