[webkit-reviews] review denied: [Bug 37205] Media listeners : [Attachment 66502] patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Luiz Agostini <luiz at webkit.org>'s
request for review:
Bug 37205: Media listeners
https://bugs.webkit.org/show_bug.cgi?id=37205

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=66502&action=prettypatch

I found this patch very mysterious.  The changes to CodeGeneratorJS don't seem
right.	There's too much of an assumption that these objects either return bool
or void.  Also, passing 1/0 to that help function isn't terribly enlightening.

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:39
> +namespace WebCore {
> +
> +
Please remove blank line.

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:40
> +static bool hasNoValue(const JSValue value)
const JSValue&	  ?

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:47
> +bool JSMediaQueryListListener::isEqual(MediaQueryListListener* other)
> +{
> +    JSMediaQueryListListener* ptr =
static_cast<JSMediaQueryListListener*>(other);
How do we know this case is valid?

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:52
> +    if (hasNoValue(m_data->callback()))
> +	   return hasNoValue(ptr->m_data->callback());
Abstracting hasNoValue into its own function just obfuscates what's going on
here.

> WebCore/bindings/js/JSMediaQueryListListenerCustom.cpp:55
> +    ExecState* exec = m_data->globalObject()->globalExec();
> +    return JSValueIsEqual(toRef(exec), toRef(exec, m_data->callback()),
toRef(exec, ptr->m_data->callback()), 0);
Generally, grabbing the exec state like this is a bad idea.  I don't understand
yet what this function is for.	Hopefully I'll learn later in the patch.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:2165
> +	       push(@headerContent, "	 virtual " .
GetNativeTypeForCallbacks($function->signature->type, 0) . " " .
$function->signature->name . "(");
Yuck.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:2261
> +	       if (GetNativeType($function->signature->type) eq "bool") {
> +		   push(@implContent, "        return true;\n\n");
> +	       } else {
> +		   push(@implContent, "        return;\n\n");
> +	       }
This seems wrong.  What if there are other types besides bool and void?

> WebCore/bindings/scripts/CodeGeneratorJS.pm:2269
> +	       foreach my $param (@params) {
> +		   if (!($param->type eq "boolean")) {
> +		       push(@implContent, "    ExecState* exec =
m_data->globalObject()->globalExec();\n");
> +		       last;
> +		   }
> +	       }
Why is grabbing the ExecState conditional on whether there's a boolean
parameter?  What if there's a string parameter?  Would we not need an ExecState
in that case?

> WebCore/bindings/scripts/CodeGeneratorV8.pm:2225
> -		   push(@args, GetNativeTypeForCallbacks($param->type) . " " .
$param->name);
> +		   push(@args, GetNativeTypeForCallbacks($param->type, 1) . " "
. $param->name);
Someone who reads this code later is going to have no idea what this 1 means.

> WebCore/css/MediaQueryList.cpp:39
> +PassRefPtr<MediaQueryList>
MediaQueryList::create(PassRefPtr<MediaQueryVector> vector,
PassRefPtr<MediaList> media, bool matches)
> +{
> +    return adoptRef(new MediaQueryList(vector, media, matches));
> +}
We usually put these in the header.

> WebCore/css/MediaQueryList.cpp:45
> +    , m_changeRound(m_evalRound - 1)
The -1 here is super mysterious.

> WebCore/css/MediaQueryListListener.idl:29
> +	   [Custom] boolean isEqual(in MediaQueryListListener other);
I see.	Why do we have this method?  Isn't JavaScript's notion of equality
sufficient?  Which spec is this from?

> WebCore/css/MediaQueryVector.cpp:103
> +    return new MediaQueryEvaluator(mediaType(), m_frame, rootStyle.get());
Naked new.  Please use adoptPtr.