WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
70816
Get rid of optional parameters in the middle in IDLs.
https://bugs.webkit.org/show_bug.cgi?id=70816
Summary
Get rid of optional parameters in the middle in IDLs.
Pavel Podivilov
Reported
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)
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Pavel Podivilov
Comment 1
2011-10-25 07:02:32 PDT
Created
attachment 112332
[details]
Patch
Adam Barth
Comment 2
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.
Adam Barth
Comment 3
2011-10-25 10:55:58 PDT
+levin because this touches the postMessage parameter tangle.
David Levin
Comment 4
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.
Adam Barth
Comment 5
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.
David Levin
Comment 6
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!
Alexey Proskuryakov
Comment 7
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.
Adam Barth
Comment 8
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.
Adam Barth
Comment 9
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.
Alexey Proskuryakov
Comment 10
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?
Adam Barth
Comment 11
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.
Alexey Proskuryakov
Comment 12
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?
Adam Barth
Comment 13
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" .
Ian 'Hixie' Hickson
Comment 14
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
.
Adam Barth
Comment 15
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.
Alexey Proskuryakov
Comment 16
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.
Adam Barth
Comment 17
2011-10-25 12:47:31 PDT
Committed
r98379
: <
http://trac.webkit.org/changeset/98379
>
Adam Barth
Comment 18
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.
Ryosuke Niwa
Comment 19
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.
Adam Barth
Comment 20
2011-10-25 23:11:28 PDT
> It's probably a V8/JSC difference.
Thanks. Sounds like we need to create new baselines.
Csaba Osztrogonác
Comment 21
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.
Adam Barth
Comment 22
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.
Pavel Podivilov
Comment 23
2011-10-26 04:52:13 PDT
Created
attachment 112481
[details]
Patch
Pavel Podivilov
Comment 24
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).
Adam Barth
Comment 25
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?
Darin Adler
Comment 26
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?
Pavel Podivilov
Comment 27
2011-10-26 10:35:25 PDT
Created
attachment 112562
[details]
Patch
Pavel Podivilov
Comment 28
2011-10-26 10:36:27 PDT
Generated overloaded methods in V8 throw Type Error. putImageData has to be custom to throw DOM Error.
Pavel Podivilov
Comment 29
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.
Adam Barth
Comment 30
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.)
Pavel Podivilov
Comment 31
2011-10-27 03:19:36 PDT
Created
attachment 112659
[details]
Patch
Pavel Podivilov
Comment 32
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.
Adam Barth
Comment 33
2011-10-27 11:13:46 PDT
Comment on
attachment 112659
[details]
Patch Yay for deleting buggy custom bindings. Thanks.
WebKit Review Bot
Comment 34
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
>
WebKit Review Bot
Comment 35
2011-10-27 12:22:56 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 36
2011-10-27 21:34:45 PDT
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
Comment 37
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
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug