Bug 66756 – Implement an Event constructor in V8

• Component: DOM
• Status: RESOLVED FIXED

[Kentaro Hara]

Event should have a constructor: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#event

We are planning to implement constructors for all Events in V8. In order to avoid a regression, we are going to commit the patch for each Event one by one with unit tests. This is the first patch and trying to implement the constructor for Event. 

We need to implement them using WebCore/bindings/generic/EventConstructors.h proposed in the bug 63878, so that we can finally merge the JavaScriptCore implementation in the bug 63878 after all the planned patches are landed.

[Kentaro Hara]

Created attachment 104812 [details]
Patch

[WebKit Review Bot]

Attachment 104812 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:83:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 104812 [details]
Patch

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

> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:52
> +template<typename EventType, typename EventInitType>
> +static v8::Handle<v8::Value> constructV8Event(const v8::Arguments& args, bool (*filler)(EventInitType&, const OptionsObject&), WrapperTypeInfo* info)

Why is there a V8 custom implementation but no custom JSC implementation?

[Kentaro Hara]

Comment on attachment 104812 [details]
Patch

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

>> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:52
>> +static v8::Handle<v8::Value> constructV8Event(const v8::Arguments& args, bool (*filler)(EventInitType&, const OptionsObject&), WrapperTypeInfo* info)
> 
> Why is there a V8 custom implementation but no custom JSC implementation?

Sam has been writing a patch for JSC in the bug 63878. My plan is to implement all Event constructors for V8 one by one using EventConstructors.h proposed in the bug 63878, and then merge the patch for JSC. In any case, we would like to split the patch for V8 and that for JSC in order to avoid hard regression.

[Adam Barth]

Comment on attachment 104812 [details]
Patch

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

>>> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:52
>>> +template<typename EventType, typename EventInitType>
>>> +static v8::Handle<v8::Value> constructV8Event(const v8::Arguments& args, bool (*filler)(EventInitType&, const OptionsObject&), WrapperTypeInfo* info)
>> 
>> Why is there a V8 custom implementation but no custom JSC implementation?
> 
> Sam has been writing a patch for JSC in the bug 63878. My plan is to implement all Event constructors for V8 one by one using EventConstructors.h proposed in the bug 63878, and then merge the patch for JSC. In any case, we would like to split the patch for V8 and that for JSC in order to avoid hard regression.

Make sense.  (This is good information to put in the ChangeLog entry, BTW.)

[Adam Barth]

Would you like me to review the patch, or did you have someone else lined up?

[Kentaro Hara]

> Would you like me to review the patch, or did you have someone else lined up?

Abarth, thank you very much! I would be really happy if you could review this patch.

[Adam Barth]

Comment on attachment 104812 [details]
Patch

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

This looks pretty solid, but I'd like to see one more iteration.  A bunch of minor comments below.

> LayoutTests/ChangeLog:15
> +        * fast/events/event-constructors-expected.txt: Added.
> +        * fast/events/event-constructors.html: Added.

Do we need to skip this test on JSC until Sam writes his half of the patch?

> LayoutTests/fast/events/event-constructors.html:16
> +try {
> +    new Event();
> +} catch (e) {
> +    debug("PASS new Event() throws Exception.");
> +}

Why not use the shouldThrow function?  That will record the type of exception as well.

> Source/WebCore/bindings/generic/EventConstructors.h:2
> + * Copyright (C) 2011 Apple Inc. All rights reserved.

Apple => Google, right?

> Source/WebCore/bindings/generic/EventConstructors.h:40
> +#define EVENTINIT_CONSTRUCTORS(DICTIONARY_START, DICTIONARY_END, FILL_PARENT_PROPERTIES, FILL_PROPERTY) \
> +    EVENTINIT_CONSTRUCTOR_FOR_EVENT(DICTIONARY_START, DICTIONARY_END, FILL_PARENT_PROPERTIES, FILL_PROPERTY)

I don't fully understand what EVENTINIT_CONSTRUCTORS buys you, but I'll keep reading.

> Source/WebCore/bindings/v8/OptionsObject.h:49
>      bool getKeyBool(const String& key, bool& value) const;
>      bool getKeyInt32(const String& key, int32_t& value) const;
> +    bool getKeyDouble(const String& key, double& value) const;
>      bool getKeyString(const String& key, String& value) const;

Normal WebKit style would not have the "get" here.  Maybe toDouble ?  I would keep this consistent for now and write a follow-up patch to change the names.

> Source/WebCore/bindings/v8/OptionsObject.h:60
> +    bool getKeyValue(const String& key, bool& value) const
> +    {
> +        return getKeyBool(key, value);
> +    }
> +    bool getKeyValue(const String& key, double& value) const
> +    {
> +        return getKeyDouble(key, value);
> +    }

Should we have these for all the types?

> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:57
> +    if (!args.IsConstructCall())
> +        return throwError("DOM object constructor cannot be called as a function.");

Which type of exception should we through in this case?

> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:63
> +    EventInitType eventInit;

eventInit => eventInitializer ?  I'm not sure what exactly eventInit means.  We usually try to use complete words in variable names.

> Source/WebCore/dom/Event.cpp:71
> +Event::Event(const AtomicString& eventType, const EventInit& eventInit)

I see.  This is like EventConfiguration or something.

> Source/WebCore/dom/Event.h:191
> -        Event(const AtomicString& type, bool canBubble, bool cancelable);
> +        Event(const AtomicString&, bool, bool);

I would keep these parameter names.  It's not clear that the first bool is canBubble and the second is cancelable.

[Kentaro Hara]

Comment on attachment 104812 [details]
Patch

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

>> LayoutTests/ChangeLog:15
>> +        * fast/events/event-constructors.html: Added.
> 
> Do we need to skip this test on JSC until Sam writes his half of the patch?

Done. Added event-constructors.html to Skipped of mac, win, qt, wk2 and gtk.

>> LayoutTests/fast/events/event-constructors.html:16
>> +}
> 
> Why not use the shouldThrow function?  That will record the type of exception as well.

Done.

>> Source/WebCore/bindings/generic/EventConstructors.h:2
>> + * Copyright (C) 2011 Apple Inc. All rights reserved.
> 
> Apple => Google, right?

I think that this should be Apple. Actually, we are almost copying EventConstructors.h written by Sam in the bug 63878 (, so that we can finally merge the JSC implementation in the bug 63878).

>> Source/WebCore/bindings/generic/EventConstructors.h:40
>> +    EVENTINIT_CONSTRUCTOR_FOR_EVENT(DICTIONARY_START, DICTIONARY_END, FILL_PARENT_PROPERTIES, FILL_PROPERTY)
> 
> I don't fully understand what EVENTINIT_CONSTRUCTORS buys you, but I'll keep reading.

Renamed to SETUP_EVENT_CONFIGURATION_FOR_ALL_EVENTS.

>> Source/WebCore/bindings/v8/OptionsObject.h:49
>>      bool getKeyString(const String& key, String& value) const;
> 
> Normal WebKit style would not have the "get" here.  Maybe toDouble ?  I would keep this consistent for now and write a follow-up patch to change the names.

I see. For now, I would keep it.

>> Source/WebCore/bindings/v8/OptionsObject.h:60
>> +    }
> 
> Should we have these for all the types?

Added getKeyValue() for Int32 and String (, although they are not used in this patch).

>> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:57
>> +        return throwError("DOM object constructor cannot be called as a function.");
> 
> Which type of exception should we through in this case?

I explicitly wrote V8Proxy::TypeError here. The spec is here (http://www.w3.org/TR/WebIDL/#es-interface-call).

>> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:63
>> +    EventInitType eventInit;
> 
> eventInit => eventInitializer ?  I'm not sure what exactly eventInit means.  We usually try to use complete words in variable names.

Renamed to eventConfiguration.

>> Source/WebCore/dom/Event.cpp:71
>> +Event::Event(const AtomicString& eventType, const EventInit& eventInit)
> 
> I see.  This is like EventConfiguration or something.

Done. Renamed EventInit => EventConfiguration.

>> Source/WebCore/dom/Event.h:191
>> +        Event(const AtomicString&, bool, bool);
> 
> I would keep these parameter names.  It's not clear that the first bool is canBubble and the second is cancelable.

OK. Reverted it back to keep the parameter names.

[Kentaro Hara]

Created attachment 104960 [details]
Patch

[WebKit Review Bot]

Attachment 104960 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:83:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 104960 [details]
Patch

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

> Source/WebCore/bindings/generic/EventConstructors.h:40
> +#define SETUP_EVENT_CONFIGURATION_FOR_ALL_EVENTS(DICTIONARY_START, DICTIONARY_END, FILL_PARENT_PROPERTIES, FILL_PROPERTY) \
> +    SETUP_EVENT_CONFIGURATION_FOR_EVENT(DICTIONARY_START, DICTIONARY_END, FILL_PARENT_PROPERTIES, FILL_PROPERTY)

I still don't fully understand why SETUP_EVENT_CONFIGURATION_FOR_ALL_EVENTS is any different from SETUP_EVENT_CONFIGURATION_FOR_EVENT.

[Kentaro Hara]

Comment on attachment 104960 [details]
Patch

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

>> Source/WebCore/bindings/generic/EventConstructors.h:40
>> +    SETUP_EVENT_CONFIGURATION_FOR_EVENT(DICTIONARY_START, DICTIONARY_END, FILL_PARENT_PROPERTIES, FILL_PROPERTY)
> 
> I still don't fully understand why SETUP_EVENT_CONFIGURATION_FOR_ALL_EVENTS is any different from SETUP_EVENT_CONFIGURATION_FOR_EVENT.

From the next patch, we are going to add SETUP_EVENT_CONFIGURATION_FOR_******_EVENT to SETUP_EVENT_CONFIGURATION_FOR_ALL_EVENTS. Please see EventConstructors.h in https://bugs.webkit.org/attachment.cgi?id=101345&action=review.

[Adam Barth]

Makes sense.

[WebKit Review Bot]

Comment on attachment 104960 [details]
Patch

Rejecting attachment 104960 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:
hing file Source/WebCore/bindings/v8/OptionsObject.h
Hunk #1 FAILED at 45.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/bindings/v8/OptionsObject.h.rej
patching file Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp
patching file Source/WebCore/dom/Event.cpp
patching file Source/WebCore/dom/Event.h
patching file Source/WebCore/dom/Event.idl

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9511231

[Hajime Morrita]

Committed r93762: <http://trac.webkit.org/changeset/93762>

[Hajime Morrita]

Reverted r93762 for reason:

IndexedDb tests crash

Committed r93764: <http://trac.webkit.org/changeset/93764>

[Sam Weinig]

> 
> > Source/WebCore/dom/Event.cpp:71
> > +Event::Event(const AtomicString& eventType, const EventInit& eventInit)
> 
> I see.  This is like EventConfiguration or something.
> 

I prefer EventInit to match the spec.  I will probably change this back when I land my part.

[Sam Weinig]

The spec doesn't include "defaultPrevented" or "timestamp" as far as I can tell.  Have you discussed this with the spec author?

[Sam Weinig]

I landed the initial structural and JSC support in <http://trac.webkit.org/changeset/93951>, I don't think it should conflict too bad.

[Pavel Podivilov]

"new Event()" doesn't throw exception in chromium anymore, adding custom expectation for dom-constructors.html until this bug is resolved (see r93958).

[Kentaro Hara]

> I landed the initial structural and JSC support in <http://trac.webkit.org/changeset/93951>, I don't think it should conflict too bad.

Sam: Thank you very much for landing your patch! I will try to land the patch for V8 based on your landed patch. By the way, what would your plan be about implementing Event constructors? I am going to implement them for V8 one by one, according to EventConstructors.h that you landed. However, if you are planning to implement them sometime soon, I would like to wait for your patches for JSC. 

CustomEvent and ProgressEvent have the spec for their constructors. (CustomEvent: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#interface-customevent, ProgressEvent: http://www.w3.org/TR/progress-events/#interface-progressevent). However, there seems to be no spec for constructors of other events. We may need to discuss the spec before landing patches. 


> The spec doesn't include "defaultPrevented" or "timestamp" as far as I can tell.  Have you discussed this with the spec author?

Sorry, I was misunderstanding the spec. You are right.

[Kentaro Hara]

Created attachment 105584 [details]
WIP patch

WIP patch

[WebKit Review Bot]

Attachment 105584 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:82:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

(In reply to comment #22)
> > I landed the initial structural and JSC support in <http://trac.webkit.org/changeset/93951>, I don't think it should conflict too bad.
> 
> Sam: Thank you very much for landing your patch! I will try to land the patch for V8 based on your landed patch. By the way, what would your plan be about implementing Event constructors? I am going to implement them for V8 one by one, according to EventConstructors.h that you landed. However, if you are planning to implement them sometime soon, I would like to wait for your patches for JSC. 
> 

I don't have any immediate plans to implement more (I may do a bit more just to work out the kinks of the system).  I am not sure one at a time is necessary, you could probably do it in chunks.

> CustomEvent and ProgressEvent have the spec for their constructors. (CustomEvent: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#interface-customevent, ProgressEvent: http://www.w3.org/TR/progress-events/#interface-progressevent). However, there seems to be no spec for constructors of other events. We may need to discuss the spec before landing patches. 

For now, I would just add items for each value that was in the initFoo function on the interface.

[Kentaro Hara]

Created attachment 105720 [details]
WIP patch

WIP patch

[Dominic Cooney]

Comment on attachment 105720 [details]
WIP patch

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

I love it.

> LayoutTests/ChangeLog:9
> +        since the Event consructor is now available in V8.

Spelling: constructor

You probably don’t need this paragraph anyway. Just say "unskip fast/events/constructors/event-constructors.html" in on the comment line for test_expectations.

> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:37
> +

Delete this blank line.

> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:58
> +    if (AllowAllocation::current())

Yes, this looks good.

[WebKit Review Bot]

Attachment 105720 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:85:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

Created attachment 105724 [details]
Patch

[Kentaro Hara]

Comment on attachment 105720 [details]
WIP patch

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

>> LayoutTests/ChangeLog:9

> 
> Spelling: constructor
> 
> You probably don’t need this paragraph anyway. Just say "unskip fast/events/constructors/event-constructors.html" in on the comment line for test_expectations.

Done.

>> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:37
>> +
> 
> Delete this blank line.

As far as I see other codes in WebCore/bindings/v8/custom/, the convention seems to add the blank line here, and describe WebCore headers above the blank line and V8-specific headers below the blank line.

[WebKit Review Bot]

Attachment 105724 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:85:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 105724 [details]
Patch

Attachment 105724 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9558805

[WebKit Review Bot]

Comment on attachment 105720 [details]
WIP patch

Attachment 105720 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9570505

[David Levin]

Note that you'll need to update the results checked in here http://trac.webkit.org/changeset/94151 as you implement this.

(In retrospec I probably should have just added a line to test_expectations.txt instead of adding these baselines, but I'm getting tired and making mistakes I guess.)

[Kentaro Hara]

Created attachment 105756 [details]
Patch

[Dominic Cooney]

Comment on attachment 105756 [details]
Patch

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

Only minor formatting comments remain.

> Source/WebCore/ChangeLog:19
> +        (WebCore::OptionsObject::getKeyValue): Returns a value corresnponding to a given key.

nit: corresponding

> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:37
> +

Why is this blank line still here? :P

> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:85
> +     \

Did Sam’s patch indent the equivalent line one space less? What does check-webkit-style say?

[Kentaro Hara]

Created attachment 105787 [details]
Patch

[Kentaro Hara]

Comment on attachment 105756 [details]
Patch

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

>> Source/WebCore/ChangeLog:19
>> +        (WebCore::OptionsObject::getKeyValue): Returns a value corresnponding to a given key.
> 
> nit: corresponding

Done.

>> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:37
>> +
> 
> Why is this blank line still here? :P

As I replied in the previous patch, as far as I looked at other files around this file, this blank line is a separator between WebCore headers and V8-specific headers.

>> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:85
>> +     \
> 
> Did Sam’s patch indent the equivalent line one space less? What does check-webkit-style say?

Removed the space. However, check-webkit-style still complains "Code inside a namespace should not be indented.  [whitespace/indent] [4]", even if I remove this blank line. To convince check-webkit-style, we need to remove the leading spaces in the next line ("v8::Handle<v8::Value> V8##EventType::constructorCallback(const v8::Arguments& args) \").

[Adam Barth]

Comment on attachment 105787 [details]
Patch

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

This looks great.

> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:59
> +    if (AllowAllocation::current())
> +        return args.Holder();

Crazy.  Is this a v8 function?  Why does it not require the v8 namespace?  Are we using namespace v8 somewhere?  (We shouldn't be.)

> Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:75
> +    return toV8(event.release(), args.Holder());

The holder is always the global object.  I guess that makes sense.

[Kentaro Hara]

Adam: Thank you for the review. 

> > Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp:59
> > +    if (AllowAllocation::current())
> > +        return args.Holder();
> 
> Crazy.  Is this a v8 function?  Why does it not require the v8 namespace?  Are we using namespace v8 somewhere?  (We shouldn't be.)

AllowAllocation::current() is in the WebCore namespace. (http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/bindings/v8/V8Binding.h)

[WebKit Review Bot]

Comment on attachment 105787 [details]
Patch

Rejecting attachment 105787 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ratorV8.pm
Hunk #1 FAILED at 2049.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/bindings/scripts/CodeGeneratorV8.pm.rej
patching file Source/WebCore/bindings/v8/OptionsObject.cpp
patching file Source/WebCore/bindings/v8/OptionsObject.h
patching file Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp
patching file Source/WebCore/dom/Event.idl

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9578016

[Dominic Cooney]

I will land this manually.

[Roland Steiner]

Committed r94424: <http://trac.webkit.org/changeset/94424>