Improve handling of dictionary conversions.

Improve handling of dictionary conversions.

When generating bindings for the dictionaries that Event constructors
take, emit code that enforce the required conversion step as per
WebIDL. i.e., if the property fails to convert, emit the required
(TypeError) exception and fail.

Previously these were mapped to empty/zero/null instead.

R=
BUG=323036
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162859

[sof, 2013-11-25 07:10:42]

At your own leisure, please have a look. (If there are others even better placed
to review, please do let me know.)

The follow-on test updates from the _main_ changes in code_generator_v8.pm and
Dictionary are somewhat big. I do apologize for the CL footprint as a result.

[haraken, 2013-11-25 07:31:05]

What's the behavior of other browsers?

[sof, 2013-11-25 07:37:02]

On 2013/11/25 07:31:05, haraken wrote:
> What's the behavior of other browsers?

e.g. `new UIEvent("ev", {view:{}})` throws an internal invalid value exception
(not a TypeError) in Gecko.

(I can't see any event constructor support in IE11, but my testing has been
light there.)

[sof, 2013-11-25 08:21:32]

https://codereview.chromium.org/85263002/diff/1/Source/bindings/v8/Dictionary.h
File Source/bindings/v8/Dictionary.h (right):

https://codereview.chromium.org/85263002/diff/1/Source/bindings/v8/Dictionary...
Source/bindings/v8/Dictionary.h:102: bool convert(const String&, bool&,
ExceptionState&) const;
An alternate design that I played with was to have these convert() methods take
a generic context value describing the conversion details (nullable, desired
numeric conversion, typename etc.) It seemed a bit overkill to do so, hence I
opted for this.

If that's deemed cleaner (and more extensible), I can pursue that instead.

[Mike West, 2013-11-25 08:22:48]

Looks pretty reasonable, though larger than I'd have expected. Also, I hate
perl. :)

A few comments below:

https://codereview.chromium.org/85263002/diff/1/LayoutTests/fast/events/const...
File
LayoutTests/fast/events/constructors/composition-event-constructor-expected.txt
(right):

https://codereview.chromium.org/85263002/diff/1/LayoutTests/fast/events/const...
LayoutTests/fast/events/constructors/composition-event-constructor-expected.txt:17:
PASS new CompositionEvent('eventType', { view: testObject }).view threw
exception TypeError: The 'view' property does not have a Window type..
Ideally, these would throw using the 'ExceptionMessages::failedToConstruct'
message.

https://codereview.chromium.org/85263002/diff/1/Source/bindings/v8/Dictionary...
File Source/bindings/v8/Dictionary.cpp (right):

https://codereview.chromium.org/85263002/diff/1/Source/bindings/v8/Dictionary...
Source/bindings/v8/Dictionary.cpp:191:
exceptionState.throwTypeError(ExceptionMessages::illTypedProperty(key, "is not
of type 'double'."));
So, as I noted in an earlier comment, I'd like to see something like
`ExceptionMessages::failedToConstruct('xxx',
ExceptionMessages::illTypedProperty(key, ...));`

It's not clear to me when these conversion methods are called, however. If it's
difficult to distinguish between construction and other types of conversion,
then that nicety might not be worth it.

https://codereview.chromium.org/85263002/diff/1/Source/bindings/v8/ExceptionM...
File Source/bindings/v8/ExceptionMessages.h (right):

https://codereview.chromium.org/85263002/diff/1/Source/bindings/v8/ExceptionM...
Source/bindings/v8/ExceptionMessages.h:45: static String illTypedProperty(const
String& property, const String& detail);
How about "incorrectPropertyType" instead? "Ill-typed" is nicely poetic, but I
worry that it might not be as comprehensible. :)

https://codereview.chromium.org/85263002/diff/1/Source/core/events/MessageEve...
File Source/core/events/MessageEvent.cpp (right):

https://codereview.chromium.org/85263002/diff/1/Source/core/events/MessageEve...
Source/core/events/MessageEvent.cpp:140:
exceptionState.throwTypeError(ExceptionMessages::failedToConstruct("MessageEvent",
"The optional 'source' property is not a Window nor MessagePort."));
Nit: "neither ... nor"

[sof, 2013-11-25 11:57:22]

https://codereview.chromium.org/85263002/diff/1/Source/bindings/v8/Dictionary...
File Source/bindings/v8/Dictionary.cpp (right):

https://codereview.chromium.org/85263002/diff/1/Source/bindings/v8/Dictionary...
Source/bindings/v8/Dictionary.cpp:191:
exceptionState.throwTypeError(ExceptionMessages::illTypedProperty(key, "is not
of type 'double'."));
On 2013/11/25 08:22:48, Mike West wrote:
> So, as I noted in an earlier comment, I'd like to see something like
> `ExceptionMessages::failedToConstruct('xxx',
> ExceptionMessages::illTypedProperty(key, ...));`
> 
> It's not clear to me when these conversion methods are called, however. If
it's
> difficult to distinguish between construction and other types of conversion,
> then that nicety might not be worth it.

I like the suggestion, we should do it. I'm reasonably certain that these
conversions are currently only called upon for event constructors, but that
might well change.

I could extend the method signatures to pass in the context of use, and emit
error messages based on that, but this makes the case for a single constant
context parameter recording this & other settings, even more compelling. Will
give that a go.

[sof, 2013-11-25 23:24:04]

https://codereview.chromium.org/85263002/diff/1/Source/bindings/v8/Dictionary...
File Source/bindings/v8/Dictionary.cpp (right):

https://codereview.chromium.org/85263002/diff/1/Source/bindings/v8/Dictionary...
Source/bindings/v8/Dictionary.cpp:191:
exceptionState.throwTypeError(ExceptionMessages::illTypedProperty(key, "is not
of type 'double'."));
On 2013/11/25 11:57:23, sof wrote:
> On 2013/11/25 08:22:48, Mike West wrote:
> > So, as I noted in an earlier comment, I'd like to see something like
> > `ExceptionMessages::failedToConstruct('xxx',
> > ExceptionMessages::illTypedProperty(key, ...));`
> > 
> > It's not clear to me when these conversion methods are called, however. If
> it's
> > difficult to distinguish between construction and other types of
conversion,
> > then that nicety might not be worth it.
> 
> I like the suggestion, we should do it. I'm reasonably certain that these
> conversions are currently only called upon for event constructors, but that
> might well change.
> 
> I could extend the method signatures to pass in the context of use, and emit
> error messages based on that, but this makes the case for a single constant
> context parameter recording this & other settings, even more compelling. Will
> give that a go.

Done.

https://codereview.chromium.org/85263002/diff/1/Source/bindings/v8/ExceptionM...
File Source/bindings/v8/ExceptionMessages.h (right):

https://codereview.chromium.org/85263002/diff/1/Source/bindings/v8/ExceptionM...
Source/bindings/v8/ExceptionMessages.h:45: static String illTypedProperty(const
String& property, const String& detail);
On 2013/11/25 08:22:48, Mike West wrote:
> How about "incorrectPropertyType" instead? "Ill-typed" is nicely poetic, but
I
> worry that it might not be as comprehensible. :)

Done. Interesting, this is standard terminology in Haskell (my background and
language of preference) for something that doesn't "type."

https://codereview.chromium.org/85263002/diff/1/Source/core/events/MessageEve...
File Source/core/events/MessageEvent.cpp (right):

https://codereview.chromium.org/85263002/diff/1/Source/core/events/MessageEve...
Source/core/events/MessageEvent.cpp:140:
exceptionState.throwTypeError(ExceptionMessages::failedToConstruct("MessageEvent",
"The optional 'source' property is not a Window nor MessagePort."));
On 2013/11/25 08:22:48, Mike West wrote:
> Nit: "neither ... nor"

Done.

[sof, 2013-11-27 23:46:42]

https://codereview.chromium.org/85263002/diff/120001/Source/bindings/v8/Dicti...
File Source/bindings/v8/Dictionary.h (right):

https://codereview.chromium.org/85263002/diff/120001/Source/bindings/v8/Dicti...
Source/bindings/v8/Dictionary.h:105: : m_interfaceName(interfaceName)
I notice https://codereview.chromium.org/87963002, has part of this context been
added to ExceptionState?

[Mike West, 2013-11-28 08:21:39]

On 2013/11/27 23:46:42, sof wrote:
>
https://codereview.chromium.org/85263002/diff/120001/Source/bindings/v8/Dicti...
> File Source/bindings/v8/Dictionary.h (right):
> 
>
https://codereview.chromium.org/85263002/diff/120001/Source/bindings/v8/Dicti...
> Source/bindings/v8/Dictionary.h:105: : m_interfaceName(interfaceName)
> I notice https://codereview.chromium.org/87963002, has part of this context
been
> added to ExceptionState?

Not yet. I'm working through all the custom bindings that create ExceptionState
objects to add the context there, and remove it from the callees:
https://codereview.chromium.org/88063003/ for example. Next step will be to
change the generated bindings, and remove the context from everywhere else.
That'll be a big CL. :)

If you'd like to help, I'm happy to review patches!

[Mike West, 2013-11-28 09:58:22]

Sorry, I thought this already went though!

LGTM. Land it. :)

[sof, 2013-11-28 10:27:53]

On 2013/11/28 08:21:39, Mike West wrote:
> On 2013/11/27 23:46:42, sof wrote:
> >
>
https://codereview.chromium.org/85263002/diff/120001/Source/bindings/v8/Dicti...
> > File Source/bindings/v8/Dictionary.h (right):
> > 
> >
>
https://codereview.chromium.org/85263002/diff/120001/Source/bindings/v8/Dicti...
> > Source/bindings/v8/Dictionary.h:105: : m_interfaceName(interfaceName)
> > I notice https://codereview.chromium.org/87963002, has part of this context
> been
> > added to ExceptionState?
> 
> Not yet. I'm working through all the custom bindings that create
ExceptionState
> objects to add the context there, and remove it from the callees:
> https://codereview.chromium.org/88063003/ for example. Next step will be to
> change the generated bindings, and remove the context from everywhere else.
> That'll be a big CL. :)
> 
> If you'd like to help, I'm happy to review patches!

Good stuff, it looks like it will simplify and tidy up call/error-generating
sites. I'll watch out for reviews in the area, as well.

[commit-bot: I haz the power, 2013-11-28 10:28:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/85263002/120001

[commit-bot: I haz the power, 2013-11-28 10:46:14]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[commit-bot: I haz the power, 2013-11-28 11:13:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/85263002/120001

[sof, 2013-11-28 11:14:53]

@tommyw: when you have a moment, are the modules/mediastream/ changes
acceptable?

[commit-bot: I haz the power, 2013-11-28 11:31:45]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[Tommy Widenflycht, 2013-11-28 12:08:51]

lgtm

[commit-bot: I haz the power, 2013-11-28 14:41:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/85263002/120001

[commit-bot: I haz the power, 2013-11-28 15:49:48]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-11-28 15:51:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/85263002/120001

[commit-bot: I haz the power, 2013-11-28 16:53:21]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-11-28 21:39:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/85263002/120001

[commit-bot: I haz the power, 2013-11-28 23:39:23]

Change committed as 162859