Bug 45425

Summary: HTMLLinkElement.disabled does not forward value to the Stylesheet's disabled attribute on setting
Product: WebKit Reporter: Bijan Amirzada <bijana@codeaurora.org>
Component: CSSAssignee: Julien Chaffraix <jchaffraix@webkit.org>
Status: RESOLVED INVALID    
Severity: Trivial CC: ap@webkit.org, bijana@codeaurora.org, commit-queue@webkit.org, jamesr@chromium.org, jchaffraix@webkit.org, seasoup@gmail.com, tnainani@codeaurora.org, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 65140    
Attachments:
Description Flags
Patch
none
Updated Patch
none
Updated Patch - per style bot mod request
darin: review-, darin: commit‑queue-
with style fixes
none
Upated the patch on top of ToT - removed testing for boolean reflected attribute on link:disabled per HTML5 standard & other browsers' behavior none

Description From 2010-09-08 17:32:34 PST
Created an attachment (id=66970) [details]
Patch

+++ This bug was initially created as a clone of Bug #25287 +++

See testcase: attached

Per the html5 spec 4.2.7 - The disabled IDL attribute on link and style elements must return false and do nothing on setting, if the sheet attribute of their LinkStyle interface is null. Otherwise, it must return the value of the StyleSheet interface's disabled attribute on getting, and forward the new value to that same attribute on setting.

Currently, the HTMLLinkElement.disabled is not forwarding the new value to the Stylesheet's disabled attribute on setting.
------- Comment #1 From 2010-09-10 11:39:53 PST -------
Created an attachment (id=67210) [details]
Updated Patch
------- Comment #2 From 2010-09-15 10:41:53 PST -------
Attachment 67210 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/html/HTMLLinkElement.cpp:393:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #3 From 2010-09-15 14:58:51 PST -------
Created an attachment (id=67721) [details]
Updated Patch - per style bot mod request
------- Comment #4 From 2010-09-15 14:59:33 PST -------
(From update of attachment 67210 [details])
This patch is now obsolete.
------- Comment #5 From 2010-10-13 17:15:43 PST -------
(From update of attachment 67721 [details])
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.
------- Comment #6 From 2011-02-17 11:13:38 PST -------
Created an attachment (id=82832) [details]
with style fixes

I tried to update this patch for landing, but it actually breaks a regression test, fast/dom/boolean-attribute-reflection.html.
------- Comment #7 From 2011-04-08 17:20:17 PST -------
Created an attachment (id=88904) [details]
Upated the patch on top of ToT - removed testing for boolean reflected attribute on link:disabled per HTML5 standard & other browsers' behavior
------- Comment #8 From 2011-04-19 19:13:51 PST -------
(From update of attachment 88904 [details])
Clearing flags on attachment: 88904

Committed r84329: <http://trac.webkit.org/changeset/84329>
------- Comment #9 From 2011-04-19 19:13:55 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #10 From 2011-05-25 16:43:48 PST -------
This appears to have caused https://bugs.webkit.org/show_bug.cgi?id=61400
------- Comment #11 From 2011-11-10 09:05:40 PST -------
This bug is still RESOLVED/FIXED, but it looks like the changes have been reverted in r94369. Should it be reopened?
------- Comment #12 From 2011-11-10 09:07:34 PST -------
If we want to track this as bug 67436 now, maybe this bug should be changed to INVALID?
------- Comment #13 From 2011-11-10 21:23:16 PST -------
(In reply to comment #12)
> If we want to track this as bug 67436 now, maybe this bug should be changed to INVALID?

That's the idea. Sorry for putting the wrong state.