[webkit-reviews] review denied: [Bug 31567] event handler in <script...for> should not get executed : [Attachment 43328] patch v1 to fix this issue

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Johnny Ding <jnd at chromium.org>'s
request for review:
Bug 31567: event handler in <script...for> should not get executed
https://bugs.webkit.org/show_bug.cgi?id=31567

Attachment 43328: patch v1 to fix this issue
https://bugs.webkit.org/attachment.cgi?id=43328&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // No type or language is specified, so we assume the script to be
JavaScript.
> +    bool isJavaScript = true;

I don't think it makes sense to move this comment up to the top of the function
without changing it. Perhaps if you are moving it, it could say "// Assume the
script is JavaScript if no type or language is specified."

>      String type = m_scriptElement->typeAttributeValue();
>      if (!type.isEmpty())
> -	   return
MIMETypeRegistry::isSupportedJavaScriptMIMEType(type.stripWhiteSpace().lower())
;
> +	   isJavaScript =
MIMETypeRegistry::isSupportedJavaScriptMIMEType(type.stripWhiteSpace().lower())
;
>  
>      String language = m_scriptElement->languageAttributeValue();
>      if (!language.isEmpty())
> -	   return isSupportedJavaScriptLanguage(language);
> +	   isJavaScript = isSupportedJavaScriptLanguage(language);
>  
> -    // No type or language is specified, so we assume the script to be
JavaScript.

I think it would be good to use nested ifs and return early if the answer is
known to be false. You would not need a boolean then. If you really did want
the boolean, then maybe we could factor that check out into a separate function
for clarity. Here is my suggested nested if form:

    String type = m_scriptElement->typeAttributeValue();
    if (!type.isEmpty()) {
	if
(!MIMETypeRegistry::isSupportedJavaScriptMIMEType(type.stripWhiteSpace().lower(
)))
	    return false;
    } else {
	String language = m_scriptElement->languageAttributeValue();
	if (!language.isEmpty() && !isSupportedJavaScriptLanguage(language))
	    return false;
    }

Your change to the code changes the logic, causing the language to take
precedence over the type if both are present. The old code would check the type
first and return a result based only on the type. The new code checks the
language second, and overwrites the value based on the type. This should cause
at least one existing test case to fail. If it doesn't, then we need better
test cases!

> -    String forAttribute = m_scriptElement->forAttributeValue();
> -    return forAttribute.isEmpty();
> +    if (isJavaScript) {
> +	   String forAttribute = m_scriptElement->forAttributeValue();
> +	   return forAttribute.isEmpty();
> +    }
> +    return false;

We normally would do an early exit here.

    if (!isJavaScript)
	return false;

And leave the other code in place. If you take my suggestion about changing
things around then you won't need to change this part of the function at all,
though.

review- because this reverses the precedence between type and language by
accident.