[Webkit-unassigned] [Bug 66756] Implement an Event constructor in V8

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66756





--- Comment #9 from Kentaro Hara <haraken at google.com>  2011-08-23 21:34:07 PST ---
(From update of attachment 104812)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.