[Webkit-unassigned] [Bug 31567] event handler in <script...for> should not get executed

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #2 from Darin Adler <darin at apple.com>  2009-11-16 15:31:09 PST ---
(From update of attachment 43328)
> +    // 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.

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