Bug 113273 - Refactoring: Replace Element::disabled and isEnabledFormControl with isDisabledFormControl
Summary: Refactoring: Replace Element::disabled and isEnabledFormControl with isDisabl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Falkenhagen
URL:
Keywords:
Depends on:
Blocks: 84635
  Show dependency treegraph
 
Reported: 2013-03-25 21:43 PDT by Matt Falkenhagen
Modified: 2013-03-28 11:37 PDT (History)
20 users (show)

See Also:


Attachments
Patch (63.81 KB, patch)
2013-03-25 21:51 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
add OVERRIDE (61.92 KB, patch)
2013-03-27 16:16 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
fix bad merging (60.91 KB, patch)
2013-03-27 18:09 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Falkenhagen 2013-03-25 21:43:51 PDT
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.
Comment 1 Matt Falkenhagen 2013-03-25 21:51:02 PDT
Created attachment 194998 [details]
Patch
Comment 2 Hajime Morrita 2013-03-25 22:06:49 PDT
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.
Comment 3 Kent Tamura 2013-03-25 22:34:11 PDT
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.
Comment 4 Matt Falkenhagen 2013-03-25 22:43:51 PDT
(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.
Comment 5 Hajime Morrita 2013-03-25 22:45:12 PDT
(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.
Comment 6 Matt Falkenhagen 2013-03-27 16:16:42 PDT
Created attachment 195428 [details]
add OVERRIDE
Comment 7 Kent Tamura 2013-03-27 17:20:10 PDT
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.
Comment 8 Matt Falkenhagen 2013-03-27 18:09:35 PDT
Created attachment 195452 [details]
fix bad merging
Comment 9 Kent Tamura 2013-03-27 18:24:16 PDT
Comment on attachment 195452 [details]
fix bad merging

looks ok
Comment 10 WebKit Review Bot 2013-03-28 11:36:55 PDT
Comment on attachment 195452 [details]
fix bad merging

Clearing flags on attachment: 195452

Committed r147135: <http://trac.webkit.org/changeset/147135>
Comment 11 WebKit Review Bot 2013-03-28 11:37:01 PDT
All reviewed patches have been landed.  Closing bug.