Summary: | HTMLInputElement has two similarly named methods: "checked" and "isChecked" | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Fisher (:fishd, Google) <fishd> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | dglazkov, dominicc, fishd, mustaf.here, sravan.ken, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Darin Fisher (:fishd, Google)
2011-02-15 10:23:57 PST
Do we still need this? If required, I can work on this. "need" is a strong word. I think there is an opportunity here to make the code less confusing to developers. Hi, In the course of understanding "isChecked" and "checked" functions, following was the analysis made. 1.In "isChecked", "m_inputType->isCheckable()" method seems to be redundant, since this will get executed only if "checked" is true. 2.And this is true only when a checkbox or radio button is selected. In which case both "isChecked" and "checked" are true since "m_inputType->isCheckable()" always returns true.(please correct me if i am wrong with an example use-case) So, based of this observation i think it is better to 1. Remove "m_inputType->isCheckable()" from "isChecked" method. 2. Return only "m_isChecked" from "isChecked" method. 3. Remove "checked" definition and declaration. 4. Replace all "checked" calls with "isChecked". Please let me know if i missed out some thing. Created attachment 99805 [details]
Patch v1
HTMLInputElement::isChecked() is called only for Radio button & Checkboxes. When a radiobutton or checkbox is set, checked() returns TRUE & m_inputType->isCheckable() also returns TRUE. Therefore to find out whether a radio button or checkbox is set, checking checked() is sufficient, no need to check for m_inputType->isCheckable().
Therefore, we have replaced all the instances of isChecked() with checked().
Also, we have tested the radio/checkbox related test cases in LayoutTest/fast/forms, so no need to add additional test cases.
Comment on attachment 99805 [details] Patch v1 Attachment 99805 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8988678 New failing tests: fast/dom/HTMLInputElement/checked-pseudo-selector.html Created attachment 99807 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 99805 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=99805&action=review I am not a reviewer, but I have some thoughts on how your patch could be improved. > Source/WebCore/ChangeLog:3 > + https://bugs.webkit.org/show_bug.cgi?id=54476 You should swap this line and the next one. > Source/WebCore/ChangeLog:8 > + Removed isChecked() from HTMLInputElement class This doesn’t add much that isn’t in the summary, so consider removing it. > Source/WebCore/ChangeLog:19 > + * html/HTMLInputElement.h: If you wanted to you could put commentary like what is losing what method briefly after the : here (same line) > Source/WebCore/css/CSSStyleSelector.cpp:-2922 > - if (inputElement && inputElement->isChecked() && !inputElement->isIndeterminate()) Have you considered cases like an input element that is a checked checkbox, which is then changed to something not checkable like a textbox? Is m_checked reset? Do rules using the ::checked pseudoclass still apply? They should not. It might almost be worth verifying your expectation with a layout test. > Source/WebCore/html/HTMLInputElement.h:129 > // isChecked is used by the rendering tree/CSS while checked() is used by JS to determine checked state This comment probably needs to be updated… Created attachment 100081 [details]
Updated Patch v2
Modified to fix test issues reported by Chromium EWS & incorporated review comments from Dominic
isChecked() is renamed to isInputTypeChecked() to check if the checked attribute (when set) is valid for a particular kind of input type element.
Comment on attachment 100081 [details] Updated Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=100081&action=review > Source/WebCore/html/HTMLInputElement.h:129 > + virtual bool isInputTypeChecked() const; this name doesn't seem to improve things. if i told you that an input element has two properties, checked and input-type-checked, would you have any idea what the difference is? it seems like there are really two properties here: a checked property and a can-render-as-checked property. the problem here is that checked() returns the first, and isChecked() returns the AND of both. one solution is to expose both methods, and then fix callsites to check both properties. another idea is to come up with a good name for the AND case. an uncreative approach: isCheckedAndCanRenderAsChecked maybe a shorter name would be: shouldRenderAsChecked or even renderAsChecked my vote is to go with renderAsChecked. Created attachment 100435 [details]
Patch v3
replaced isChecked() with renderAsChecked() as suggested by Darin Fisher.
Comment on attachment 100435 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=100435&action=review > Source/WebCore/dom/CheckedRadioButtons.cpp:82 > + ASSERT(inputElement->renderAsChecked()); This is OK, particularly because it is the status quo, but maybe checked() is better? Since the isCheckable half of renderAsChecked is redundant if this is also asserting isRadioButton. Hi Dominic, Yes it is redundant as you mentioned in isRadioButton case, and there are other cases as well. But the real use of it comes in to picture w.r.t pseudo-classes. As you already mentioned. So, we are going with just a name change. Comment on attachment 100435 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=100435&action=review Since the entire purpose of this patch is renaming, and I am suggesting a different name, review-. > Source/WebCore/html/HTMLInputElement.h:129 > + // renderAsChecked is used by the rendering tree/CSS while checked() is used by JS to determine checked state > + virtual bool renderAsChecked() const; I think it’s great to choose a name here that is distinct from the DOM function checked, but I am not a big fan of the term “render” here. I think there are clearer less-jargony terms we can use. The name I like best is shouldAppearChecked. Other possible names could be isVisiblyChecked or appearsChecked. The only reason this function or isIndeterminate was a virtual function was the WML support. This should not be virtual any more. If it was really a virtual function then we’d have to rename the other implementations, but there are none. No reason to spend the extra runtime overhead of a virtual function dispatch. > Source/WebCore/html/HTMLInputElement.h:130 > virtual bool isIndeterminate() const { return indeterminate(); } Not your responsibility in this patch, but this function needs to be removed. It only existed because of WML, and it’s now just a better-named, slow synonym for the DOM indeterminate function. Created attachment 100632 [details]
Updated Patch v4
Hi Darin, Thanks for the review comments. Made following changes
1. Replaced isChecked() with shouldAppearChecked()
2. Removed virtual declaration from shouldAppearChecked() function declaration.
Comment on attachment 100632 [details] Updated Patch v4 Clearing flags on attachment: 100632 Committed r91049: <http://trac.webkit.org/changeset/91049> All reviewed patches have been landed. Closing bug. |