[webkit-reviews] review denied: [Bug 45425] HTMLLinkElement.disabled does not forward value to the Stylesheet's disabled attribute on setting : [Attachment 67721] Updated Patch - per style bot mod request

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Bijan Amirzada
<bijana at codeaurora.org>'s request for review:
Bug 45425: HTMLLinkElement.disabled does not forward value to the Stylesheet's
disabled attribute on setting
https://bugs.webkit.org/show_bug.cgi?id=45425

Attachment 67721: Updated Patch - per style bot mod request
https://bugs.webkit.org/attachment.cgi?id=67721&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=67721&action=review

Looks good. Nice test case. Some small stylistic problems that we should fix.

> LayoutTests/fast/html/htmllink-disable-expected.txt:1
> +This test checks HTMLLinkElement disabled conforms to HTML5 spec for the bug
id #45425 .

A slightly better directory for this test would be:

    fast/dom/HTMLLinkElement

You’ll find other tests of HTMLLinkElement DOM API there.

> WebCore/html/HTMLLinkElement.cpp:397
> +    if (StyleSheet* styleSheet = this->sheet())
> +	   return styleSheet->disabled();
> +
> +    return false;

In WebKit we normally use the early return style rather than nesting code.
Also, the "this->" notation would only be needed if the local variable was
named the same as the data member. There’s also no reason to use a non-inline
function to get one of our own data members. This function should just say:

    return m_sheet && m_sheet->disabled();

> WebCore/html/HTMLLinkElement.cpp:408
> +    if (StyleSheet* styleSheet = this->sheet())
> +	   styleSheet->setDisabled(setDisabled);
> +}
> +
> +
> +
>  }

It doesn’t make sense to name the argument “set disabled”. You could name it
“disabled” or “flag” or something like that.

This function should just be:

    if (!m_sheet)
	return;
    m_sheet->setDisabled(disabled);

Just one blank line after the function, please, not three.

> WebCore/html/HTMLLinkElement.h:76
> +    bool disabled();

This could and should be a const member function.