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

[bugzilla-daemon at webkit.org]

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


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #104812|review?                     |review-
               Flag|                            |




--- Comment #8 from Adam Barth <abarth at webkit.org>  2011-08-23 18:14:59 PST ---
(From update of attachment 104812)
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.

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