[webkit-reviews] review denied: [Bug 66756] Implement an Event constructor in V8 : [Attachment 104812] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Kentaro Hara <haraken at google.com>'s
request for review:
Bug 66756: Implement an Event constructor in V8
https://bugs.webkit.org/show_bug.cgi?id=66756

Attachment 104812: Patch
https://bugs.webkit.org/attachment.cgi?id=104812&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.