Bug 54476 – HTMLInputElement has two similarly named methods: "checked" and "isChecked"

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

HTMLInputElement has two similarly named methods: "checked" and "isChecked"

We should consider revising one of those as their distinction is not at all
clear from their names.

See https://bugs.webkit.org/show_bug.cgi?id=54333#c13 for more background.

[SravanKumar]

Do we still need this? If required, I can work on this.

[Darin Fisher (:fishd, Google)]

"need" is a strong word.  I think there is an opportunity here to make the code less confusing to developers.

[SravanKumar]

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.

[SravanKumar]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Dominic Cooney]

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…

[SravanKumar]

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.

[Darin Fisher (:fishd, Google)]

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.

[SravanKumar]

Created attachment 100435 [details]
Patch v3

replaced isChecked() with renderAsChecked() as suggested by Darin Fisher.

[Dominic Cooney]

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.

[SravanKumar]

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.

[Darin Adler]

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.

[SravanKumar]

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.

[WebKit Review Bot]

Comment on attachment 100632 [details]
Updated Patch v4

Clearing flags on attachment: 100632

Committed r91049: <http://trac.webkit.org/changeset/91049>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.