[Webkit-unassigned] [Bug 45425] HTMLLinkElement.disabled does not forward value to the Stylesheet's disabled attribute on setting

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #5 from Darin Adler <darin at apple.com>  2010-10-13 17:15:43 PST ---
(From update of attachment 67721)
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.

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