[Webkit-unassigned] [Bug 25287] HTMLStyleElement.disabled doesn't work (affects jQuery)

[bugzilla-daemon at webkit.org]

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


David Kilzer (ddkilzer) <ddkilzer at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #67718|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #11 from David Kilzer (ddkilzer) <ddkilzer at webkit.org>  2010-10-24 07:50:57 PST ---
(From update of attachment 67718)
View in context: https://bugs.webkit.org/attachment.cgi?id=67718&action=review

Overall, this patch is heading in the right direction, but r- to add a test case and to fix the disabled() method.

It may be worth writing an XHTML test case as well.  I seem to recall that stylesheets are handled somewhat differently in XHTML, but I don't recall the details.

> LayoutTests/fast/html/script-tests/htmlstyle-disable.js:7
> +console.appendChild(testParent);

This test should also cover a detached element (see Comment #5 and Comment #6).  The attached element case looks good!

> LayoutTests/fast/html/script-tests/htmlstyle-disable.js:27
> +
> +
> +successfullyParsed = true;
> +
> +
> +

Nit:  There's excessive whitespace in this JavaScript file.  Usually one blank line is sufficient.

> WebCore/html/HTMLStyleElement.cpp:32
> -#include "ScriptableDocumentParser.h"
>  #include "ScriptEventListener.h"
> +#include "ScriptableDocumentParser.h"
> +

The headers were in the correct order before.  Also, please don't add unnecessary whitespace.

> WebCore/html/HTMLStyleElement.cpp:112
> +    StyleSheet* styleSheet = this->sheet();
> +    ASSERT(styleSheet);
> +    // HTML5 specified that we should be able to handle a null styleSheet but this never happens.
> +    return styleSheet->disabled();

Instead of an ASSERT() here, you need to check that the styleSheet is not NULL.  I don't see where StyleSheet::createSheet() guarantees that a stylesheet will be created.

    if (StyleSheet* styleSheet = this->sheet()) {
        // HTML5 specified that we should be able to handle a null styleSheet but this never happens.
        return styleSheet->disabled();
    }
    return false;

> +    // HTML5 specified that we should be able to handle a null styleSheet but this never happens.

I think this comment is wrong.  The StyleSheet::createSheet() method does not guarantee that a sheet object is returned.

Note that you may want to check the spec to see what disabled should return if there is no stylesheet.  I'm just assuming that false is the correct thing to do here.

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