Bug 113273

Summary: Refactoring: Replace Element::disabled and isEnabledFormControl with isDisabledFormControl
Product: WebKit Reporter: Matt Falkenhagen <falken>
Component: Layout and RenderingAssignee: Matt Falkenhagen <falken>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, allan.jensen, apinheiro, cfleizach, cmarcelo, darin, dmazzoni, eric, esprehn+autocc, japhet, jdiggs, macpherson, menard, mifenton, morrita, ojan.autocc, rwlbuis, tkent, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 84635    
Attachments:
Description Flags
Patch
none
add OVERRIDE
none
fix bad merging none

Matt Falkenhagen
Reported 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.
Attachments
Patch (63.81 KB, patch)
2013-03-25 21:51 PDT, Matt Falkenhagen 		no flags
DetailsFormatted DiffDiff
add OVERRIDE (61.92 KB, patch)
2013-03-27 16:16 PDT, Matt Falkenhagen 		no flags
DetailsFormatted DiffDiff
fix bad merging (60.91 KB, patch)
2013-03-27 18:09 PDT, Matt Falkenhagen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Matt Falkenhagen
Comment 1 2013-03-25 21:51:02 PDT
Created attachment 194998 [details] Patch
Hajime Morrita
Comment 2 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.
Kent Tamura
Comment 3 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.
Matt Falkenhagen
Comment 4 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.
Hajime Morrita
Comment 5 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.
Matt Falkenhagen
Comment 6 2013-03-27 16:16:42 PDT
Created attachment 195428 [details] add OVERRIDE
Kent Tamura
Comment 7 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.
Matt Falkenhagen
Comment 8 2013-03-27 18:09:35 PDT
Created attachment 195452 [details] fix bad merging
Kent Tamura
Comment 9 2013-03-27 18:24:16 PDT
Comment on attachment 195452 [details] fix bad merging looks ok
WebKit Review Bot
Comment 10 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>
WebKit Review Bot
Comment 11 2013-03-28 11:37:01 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.