Summary: | Refactoring: Replace Element::disabled and isEnabledFormControl with isDisabledFormControl | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Falkenhagen <falken> | ||||||||
Component: | Layout and Rendering | Assignee: | 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
Matt Falkenhagen
2013-03-25 21:43:51 PDT
Created attachment 194998 [details]
Patch
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 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. (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. (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. Created attachment 195428 [details]
add OVERRIDE
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. Created attachment 195452 [details]
fix bad merging
Comment on attachment 195452 [details]
fix bad merging
looks ok
Comment on attachment 195452 [details] fix bad merging Clearing flags on attachment: 195452 Committed r147135: <http://trac.webkit.org/changeset/147135> All reviewed patches have been landed. Closing bug. |