Bug 66756 - Implement an Event constructor in V8
Summary: Implement an Event constructor in V8
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 67824
  Show dependency treegraph
 
Reported: 2011-08-23 03:56 PDT by Kentaro Hara
Modified: 2011-09-08 18:05 PDT (History)
10 users (show)

See Also:


Attachments
Patch (19.37 KB, patch)
2011-08-23 04:07 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (23.90 KB, patch)
2011-08-23 21:34 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
WIP patch (26.48 KB, patch)
2011-08-29 23:16 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
WIP patch (11.13 KB, patch)
2011-08-30 17:02 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (11.72 KB, patch)
2011-08-30 17:36 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (11.70 KB, patch)
2011-08-31 01:09 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (11.70 KB, patch)
2011-08-31 10:13 PDT, Kentaro Hara
abarth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2011-08-23 03:56:25 PDT
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.
Comment 1 Kentaro Hara 2011-08-23 04:07:27 PDT
Created attachment 104812 [details]
Patch
Comment 2 WebKit Review Bot 2011-08-23 04:10:22 PDT
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.
Comment 3 Adam Barth 2011-08-23 10:09:47 PDT
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?
Comment 4 Kentaro Hara 2011-08-23 17:01:09 PDT
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.
Comment 5 Adam Barth 2011-08-23 17:24:41 PDT
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.)
Comment 6 Adam Barth 2011-08-23 17:25:07 PDT
Would you like me to review the patch, or did you have someone else lined up?
Comment 7 Kentaro Hara 2011-08-23 17:27:10 PDT
> 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.
Comment 8 Adam Barth 2011-08-23 18:14:59 PDT
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.
Comment 9 Kentaro Hara 2011-08-23 21:34:07 PDT
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.
Comment 10 Kentaro Hara 2011-08-23 21:34:58 PDT
Created attachment 104960 [details]
Patch
Comment 11 WebKit Review Bot 2011-08-23 21:37:39 PDT
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.
Comment 12 Adam Barth 2011-08-23 21:42:48 PDT
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.
Comment 13 Kentaro Hara 2011-08-23 21:47:04 PDT
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.
Comment 14 Adam Barth 2011-08-23 21:51:24 PDT
Makes sense.
Comment 15 WebKit Review Bot 2011-08-24 23:00:14 PDT
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
Comment 16 Hajime Morrita 2011-08-24 23:58:53 PDT
Committed r93762: <http://trac.webkit.org/changeset/93762>
Comment 17 Hajime Morrita 2011-08-25 01:13:48 PDT
Reverted r93762 for reason:

IndexedDb tests crash

Committed r93764: <http://trac.webkit.org/changeset/93764>
Comment 18 Sam Weinig 2011-08-28 15:48:54 PDT
> 
> > 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.
Comment 19 Sam Weinig 2011-08-28 16:34:55 PDT
The spec doesn't include "defaultPrevented" or "timestamp" as far as I can tell.  Have you discussed this with the spec author?
Comment 20 Sam Weinig 2011-08-28 17:25:02 PDT
I landed the initial structural and JSC support in <http://trac.webkit.org/changeset/93951>, I don't think it should conflict too bad.
Comment 21 Pavel Podivilov 2011-08-29 00:44:26 PDT
"new Event()" doesn't throw exception in chromium anymore, adding custom expectation for dom-constructors.html until this bug is resolved (see r93958).
Comment 22 Kentaro Hara 2011-08-29 02:02:30 PDT
> 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.
Comment 23 Kentaro Hara 2011-08-29 23:16:24 PDT
Created attachment 105584 [details]
WIP patch

WIP patch
Comment 24 WebKit Review Bot 2011-08-30 08:42:32 PDT
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.
Comment 25 Sam Weinig 2011-08-30 16:27:56 PDT
(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.
Comment 26 Kentaro Hara 2011-08-30 17:02:10 PDT
Created attachment 105720 [details]
WIP patch

WIP patch
Comment 27 Dominic Cooney 2011-08-30 17:23:10 PDT
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.
Comment 28 WebKit Review Bot 2011-08-30 17:34:10 PDT
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.
Comment 29 Kentaro Hara 2011-08-30 17:36:58 PDT
Created attachment 105724 [details]
Patch
Comment 30 Kentaro Hara 2011-08-30 17:37:47 PDT
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.
Comment 31 WebKit Review Bot 2011-08-30 17:40:37 PDT
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.
Comment 32 WebKit Review Bot 2011-08-30 21:26:15 PDT
Comment on attachment 105724 [details]
Patch

Attachment 105724 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9558805
Comment 33 WebKit Review Bot 2011-08-30 22:02:13 PDT
Comment on attachment 105720 [details]
WIP patch

Attachment 105720 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9570505
Comment 34 David Levin 2011-08-30 23:06:43 PDT
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.)
Comment 35 Kentaro Hara 2011-08-31 01:09:18 PDT
Created attachment 105756 [details]
Patch
Comment 36 Dominic Cooney 2011-08-31 09:45:05 PDT
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?
Comment 37 Kentaro Hara 2011-08-31 10:13:12 PDT
Created attachment 105787 [details]
Patch
Comment 38 Kentaro Hara 2011-08-31 10:13:44 PDT
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) \").
Comment 39 Adam Barth 2011-08-31 11:01:17 PDT
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.
Comment 40 Kentaro Hara 2011-08-31 11:18:02 PDT
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)
Comment 41 WebKit Review Bot 2011-08-31 12:47:34 PDT
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
Comment 42 Dominic Cooney 2011-08-31 13:10:58 PDT
I will land this manually.
Comment 43 Roland Steiner 2011-09-02 09:58:57 PDT
Committed r94424: <http://trac.webkit.org/changeset/94424>