Bug 70816 - Get rid of optional parameters in the middle in IDLs.
Summary: Get rid of optional parameters in the middle in IDLs.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pavel Podivilov
URL:
Keywords:
Depends on: 70875
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-25 07:01 PDT by Pavel Podivilov
Modified: 2011-10-28 02:42 PDT (History)
12 users (show)

See Also:


Attachments
Patch (13.29 KB, patch)
2011-10-25 07:02 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Patch (17.52 KB, patch)
2011-10-26 04:52 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Patch (17.10 KB, patch)
2011-10-26 10:35 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Patch (20.08 KB, patch)
2011-10-27 03:19 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Podivilov 2011-10-25 07:01:05 PDT
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)
Comment 1 Pavel Podivilov 2011-10-25 07:02:32 PDT
Created attachment 112332 [details]
Patch
Comment 2 Adam Barth 2011-10-25 10:49:23 PDT
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.
Comment 3 Adam Barth 2011-10-25 10:55:58 PDT
+levin because this touches the postMessage parameter tangle.
Comment 4 David Levin 2011-10-25 11:07:12 PDT
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.
Comment 5 Adam Barth 2011-10-25 11:10:44 PDT
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.
Comment 6 David Levin 2011-10-25 11:14:16 PDT
(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!
Comment 7 Alexey Proskuryakov 2011-10-25 11:15:00 PDT
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.
Comment 8 Adam Barth 2011-10-25 11:20:02 PDT
> 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.
Comment 9 Adam Barth 2011-10-25 11:22:42 PDT
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.
Comment 10 Alexey Proskuryakov 2011-10-25 11:28:56 PDT
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?
Comment 11 Adam Barth 2011-10-25 11:40:07 PDT
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.
Comment 12 Alexey Proskuryakov 2011-10-25 11:55:01 PDT
> - 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?
Comment 13 Adam Barth 2011-10-25 11:58:39 PDT
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" .
Comment 14 Ian 'Hixie' Hickson 2011-10-25 12:35:02 PDT
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.
Comment 15 Adam Barth 2011-10-25 12:46:18 PDT
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.
Comment 16 Alexey Proskuryakov 2011-10-25 12:47:15 PDT
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.
Comment 17 Adam Barth 2011-10-25 12:47:31 PDT
Committed r98379: <http://trac.webkit.org/changeset/98379>
Comment 18 Adam Barth 2011-10-25 12:49:25 PDT
> 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.
Comment 19 Ryosuke Niwa 2011-10-25 23:08:37 PDT
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.
Comment 20 Adam Barth 2011-10-25 23:11:28 PDT
> It's probably a V8/JSC difference.

Thanks.  Sounds like we need to create new baselines.
Comment 21 Csaba Osztrogonác 2011-10-25 23:17:00 PDT
(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.
Comment 22 Adam Barth 2011-10-25 23:46:11 PDT
@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.
Comment 23 Pavel Podivilov 2011-10-26 04:52:13 PDT
Created attachment 112481 [details]
Patch
Comment 24 Pavel Podivilov 2011-10-26 04:59:57 PDT
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).
Comment 25 Adam Barth 2011-10-26 09:32:24 PDT
Comment on attachment 112481 [details]
Patch

This looks like we're losing test coverage.  Can we make the two bindings throw the same exception?
Comment 26 Darin Adler 2011-10-26 10:03:10 PDT
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?
Comment 27 Pavel Podivilov 2011-10-26 10:35:25 PDT
Created attachment 112562 [details]
Patch
Comment 28 Pavel Podivilov 2011-10-26 10:36:27 PDT
Generated overloaded methods in V8 throw Type Error.
putImageData has to be custom to throw DOM Error.
Comment 29 Pavel Podivilov 2011-10-26 10:41:36 PDT
(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.
Comment 30 Adam Barth 2011-10-26 10:57:13 PDT
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.)
Comment 31 Pavel Podivilov 2011-10-27 03:19:36 PDT
Created attachment 112659 [details]
Patch
Comment 32 Pavel Podivilov 2011-10-27 03:22:04 PDT
(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.
Comment 33 Adam Barth 2011-10-27 11:13:46 PDT
Comment on attachment 112659 [details]
Patch

Yay for deleting buggy custom bindings.  Thanks.
Comment 34 WebKit Review Bot 2011-10-27 12:22:48 PDT
Comment on attachment 112659 [details]
Patch

Clearing flags on attachment: 112659

Committed r98617: <http://trac.webkit.org/changeset/98617>
Comment 35 WebKit Review Bot 2011-10-27 12:22:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Pavel Podivilov 2011-10-28 02:42:59 PDT
(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.