Bug 113273 – Refactoring: Replace Element::disabled and isEnabledFormControl with isDisabledFormControl

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Matt Falkenhagen]

Continuing from bug 112993 and bug 112085. Element::disabled is for form controls, and Element::isEnabledFormControl is redundant with Element::disabled, so replace them with a single function Element::isDisabledFormControl.

[Matt Falkenhagen]

Created attachment 194998 [details]
Patch

[Hajime Morrita]

Comment on attachment 194998 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194998&action=review

> Source/WebCore/html/HTMLFormControlElement.h:67
> +    virtual bool isDisabledFormControl() const;

It's time to add OVERRIDE.

> Source/WebCore/html/HTMLOptGroupElement.h:-48
> -    virtual bool isEnabledFormControl() const OVERRIDE { return !disabled(); }

I feel it worth having isEnabledFormControl() as a inlined shortcut  of !isDisabledFormControl().
Doing !isDisabledFormControl() feels like a confusing double-negative.

[Kent Tamura]

Comment on attachment 194998 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194998&action=review

> Source/WebCore/dom/Element.h:561
> +    virtual bool isDisabledFormControl() const;

nit: If you follow the terminology in the standard, this should be called "isActuallyDisabled."  http://www.whatwg.org/specs/web-apps/current-work/multipage/common-idioms.html#concept-element-disabled
"actually disabled" is applied to non-form controls too like <menuitem>.

You can ignore this comment.

[Matt Falkenhagen]

(In reply to comment #2)
> > Source/WebCore/html/HTMLOptGroupElement.h:-48
> > -    virtual bool isEnabledFormControl() const OVERRIDE { return !disabled(); }
> 
> I feel it worth having isEnabledFormControl() as a inlined shortcut  of !isDisabledFormControl().
> Doing !isDisabledFormControl() feels like a confusing double-negative.

I think isEnabledFormControl() feels confusing since it returns true for elements that are not form controls.

[Hajime Morrita]

(In reply to comment #4)
> (In reply to comment #2)
> > > Source/WebCore/html/HTMLOptGroupElement.h:-48
> > > -    virtual bool isEnabledFormControl() const OVERRIDE { return !disabled(); }
> > 
> > I feel it worth having isEnabledFormControl() as a inlined shortcut  of !isDisabledFormControl().
> > Doing !isDisabledFormControl() feels like a confusing double-negative.
> 
> I think isEnabledFormControl() feels confusing since it returns true for elements that are not form controls.
Makes sense.

[Matt Falkenhagen]

Created attachment 195428 [details]
add OVERRIDE

[Kent Tamura]

Comment on attachment 195428 [details]
add OVERRIDE

View in context: https://bugs.webkit.org/attachment.cgi?id=195428&action=review

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:389
>      RefPtr<HTMLInputElement> input(static_cast<HTMLInputElement*>(shadowHost()));
> -    if (!input || input->isDisabledOrReadOnly())
>          return;

Looks very wrong.

[Matt Falkenhagen]

Created attachment 195452 [details]
fix bad merging

[Kent Tamura]

Comment on attachment 195452 [details]
fix bad merging

looks ok

[WebKit Review Bot]

Comment on attachment 195452 [details]
fix bad merging

Clearing flags on attachment: 195452

Committed r147135: <http://trac.webkit.org/changeset/147135>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.