[webkit-reviews] review denied: [Bug 25287] HTMLStyleElement.disabled doesn't work (affects jQuery) : [Attachment 67718] Updated bug fix for 25287 with fixing style

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Tarun Nainani
<tnainani at codeaurora.org>'s request for review:
Bug 25287: HTMLStyleElement.disabled doesn't work (affects jQuery)
https://bugs.webkit.org/show_bug.cgi?id=25287

Attachment 67718: Updated bug fix for 25287 with fixing style
https://bugs.webkit.org/attachment.cgi?id=67718&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
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.