[webkit-reviews] review granted: [Bug 74909] Introduce RadioButtonGroup class to keep track of the group members and required state : [Attachment 122595] Patch 3
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Jan 21 20:50:09 PST 2012
Darin Adler <darin at apple.com> has granted Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 74909: Introduce RadioButtonGroup class to keep track of the group members
and required state
https://bugs.webkit.org/show_bug.cgi?id=74909
Attachment 122595: Patch 3
https://bugs.webkit.org/attachment.cgi?id=122595&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=122595&action=review
Seems OK, with room for improvement.
> Source/WebCore/dom/CheckedRadioButtons.cpp:34
> + HTMLInputElement* checkedButton() { return m_checkedButton; }
Could be const.
> Source/WebCore/dom/CheckedRadioButtons.cpp:65
> + if (m_members.contains(element))
> + return;
> +
> + m_members.add(element);
Calling contains followed by add does two hash table lookups. But add is
written so it already does nothing if the set already contains the new member
and it returns a boolean that indicates that happened.
The same thing can be done like this:
if (!m_members.add(element).second)
return;
I know that’s not terribly readable but it’s faster than contains/add. At some
point we’ll rename “second” to “newEntryAdded” or something like that.
> Source/WebCore/dom/CheckedRadioButtons.cpp:66
> + bool updateValidityStatusInGroup = false;
This is a verb phrase, and would be better if it was not. Maybe
mustUpdateValidityForAllMembers, or something like that.
> Source/WebCore/dom/CheckedRadioButtons.cpp:69
> + if (++m_requiredCount == 1)
> + updateValidityStatusInGroup = true;
Why? The fact that a group changing from not-required to required creates the
need to update the validity status of all radio buttons in a group is not
self-evident; it needs a comment. Unless the code can somehow be structured so
that it is self-evident.
For example, I would think that if the group has a checked button already,
making the group required does not require updating the validity status. I am
possibly wrong, but if so, reading this code wouldn’t clear things up for me.
> Source/WebCore/dom/CheckedRadioButtons.cpp:75
> + ASSERT(oldCheckedButton != element);
> + if (oldCheckedButton != element) {
There seems no chance this assertion will be false, so I think we should omit
the if statement. The function already checks if the set contains the new
element, and I believe m_checkedButton won’t ever be set to something not in
the set.
> Source/WebCore/dom/CheckedRadioButtons.cpp:80
> + else if (isRequired())
> + updateValidityStatusInGroup = true;
What this code does is not self-evident; it needs a comment. I gather that
what’s happening is that a group that has no checked button is now acquiring a
checked button, and that means the other buttons in the group are now valid.
But that required careful study to determine and was not obvious from the code
structure. Perhaps the code can somehow be structured so that it is
self-evident but otherwise we should add a suitable comment.
> Source/WebCore/dom/CheckedRadioButtons.cpp:86
> + else if (isRequired())
> + element->setNeedsValidityCheck();
Again, not clear why this is needed so needs a comment or different code
structure.
> Source/WebCore/dom/CheckedRadioButtons.cpp:89
> +void RadioButtonGroup::updateCheckedState(HTMLInputElement* element)
Why not name this button instead of element? Generally speaking if the function
can only be called with a radio button I think it’s good that we name the
function that way.
> Source/WebCore/dom/CheckedRadioButtons.cpp:98
> + if (element->checked()) {
> + HTMLInputElement* oldCheckedButton = m_checkedButton;
> + m_checkedButton = element;
> + if (oldCheckedButton)
> + oldCheckedButton->setChecked(false);
> + else if (isRequired())
> + updateButtonsValidity();
We need to find a way to share this logic with the “add already checked button”
code above in RadioButtonGroup::add. It’s identical, except for the coalescing
of the updateButtonsValidity call.
Should we also be asserting that element != m_checkedButton?
> Source/WebCore/dom/CheckedRadioButtons.cpp:105
> + if (m_checkedButton == element) {
> + m_checkedButton = 0;
> + if (isRequired())
> + updateButtonsValidity();
> + }
> + }
I’d like to see this share code with the remove function.
> Source/WebCore/dom/CheckedRadioButtons.cpp:113
> + if (element->required()) {
> + if (++m_requiredCount == 1)
> + updateButtonsValidity();
I’d like to see this share code with the add function.
> Source/WebCore/dom/CheckedRadioButtons.cpp:118
> + ASSERT(m_requiredCount);
> + if (!--m_requiredCount)
> + updateButtonsValidity();
> + }
I’d like to see this share code with the remove function.
> Source/WebCore/dom/CheckedRadioButtons.cpp:133
> + if (element->checked()) {
> + m_checkedButton = 0;
Why check checked() rather than checking == m_checkedButton?
> Source/WebCore/dom/CheckedRadioButtons.cpp:152
> +void RadioButtonGroup::updateButtonsValidity()
> +{
> + typedef HashSet<HTMLInputElement*>::const_iterator Iterator;
> + Iterator end = m_members.end();
> + for (Iterator it = m_members.begin(); it != end; ++it) {
> + HTMLInputElement* control = *it;
> + ASSERT(control->isRadioButton());
> + control->setNeedsValidityCheck();
> + }
> +}
I don’t like the difference in terminology here. This does not update the
validity of the radio buttons. It sets a flag saying they need a validity
check. That’s why the HTMLInputElement function is not named updateValidity.
> Source/WebCore/dom/CheckedRadioButtons.cpp:162
> +// Explicity define empty constructor and destructor in order to prevent the
> +// compiler from generating them as inlines and requiring RadioButtonGroup.
This comment should include the phrase “so we don’t need to define
RadioButtonGroup in the header”. I don’t think that “requiring
RadioButtonGroup” is clear enough.
> Source/WebCore/dom/CheckedRadioButtons.cpp:182
> + NameToGroupMap::iterator it =
m_nameToGroupMap->add(element->name().impl(),
PassOwnPtr<RadioButtonGroup>()).first;
> + if (!it->second)
> + it->second = RadioButtonGroup::create();
> + it->second->add(element);
I normally write these like this:
RefPtr<RadioButtonGroup>& group =
m_nameToGroupMap->add(element->name().impl(),
PassOwnPtr<RadioButtonGroup>()).first->second;
if (!group)
group = RadioButtonGroup::create();
group->add(element);
Your way has at least one advantage over mine; the iterator gets checked after
the call to RadioButtonGroup::create, so if something crazy happens we will
find out in debug builds. But I like mine better for readability.
> Source/WebCore/dom/CheckedRadioButtons.cpp:188
> + if (!shouldMakeRadioGroup(element))
> + return;
Is it more efficient to check shouldMakeRadioGroup, or to let the rest of the
code run and end up doing nothing since the element is not in the set in the
group? I would normally lean toward the latter.
> Source/WebCore/dom/CheckedRadioButtons.cpp:201
> + if (!shouldMakeRadioGroup(element))
> return;
Same comment as above.
> Source/WebCore/dom/CheckedRadioButtons.cpp:224
> + if (!shouldMakeRadioGroup(element))
> + return false;
Is all the logic in the shouldMakeRadioGroup function needed here? I worry that
this is more overhead than we need.
> Source/WebCore/dom/CheckedRadioButtons.cpp:235
> + if (!shouldMakeRadioGroup(element))
> return;
Does this improve performance? I’d suggest leaving it out unless we think it
does.
> Source/WebCore/dom/CheckedRadioButtons.cpp:248
> + it->second->remove(element);
> + if (it->second->isEmpty()) {
> + m_nameToGroupMap->remove(it);
> + if (m_nameToGroupMap->isEmpty())
> + m_nameToGroupMap.clear();
> + }
Deallocating these makes performance bad if we go from 0 to 1 and back and
forth. Not deallocating them makes memory use bad if we have transient groups
and leave behind empty data structures. I am not sure which is the right
tradeoff.
> Source/WebCore/dom/CheckedRadioButtons.h:33
> +// FIXME: Rename the class.
This would be a better FIXME if it stated explicitly what we think is wrong
with the current name.
> Source/WebCore/html/CheckboxInputType.cpp:54
> - return !element()->checked();
> + return element()->isRequiredFormControl() && !element()->checked();
It’s wasteful that isRequiredFormControl is a virtual function dispatch here.
It’s nothing new, though.
> Source/WebCore/html/FileInputType.cpp:132
> - return value.isEmpty();
> + return element()->isRequiredFormControl() && value.isEmpty();
Ditto.
> Source/WebCore/html/HTMLFormControlElement.cpp:136
> + // Updates for :required :optional classes.
> + setNeedsStyleRecalc();
This comment, which you moved but did not change, is confusing grammar-wise.
The phrase “updates for” could be a noun phrase. I would write:
// Style recalculation is needed because style selectors may include
:required and :optional pseudo-classes.
> Source/WebCore/html/HTMLInputElement.h:75
> + virtual bool isRequiredFormControl() const OVERRIDE;
It’s unfortunate that this public function is virtual. Any caller that has an
HTMLInputElement* could instead call a non-virtual version.
> Source/WebCore/html/RadioInputType.cpp:51
> - return
!element()->checkedRadioButtons().checkedButtonForGroup(element()->name());
> + return element()->checkedRadioButtons().isInRequiredGroup(element()) &&
!element()->checkedRadioButtons().checkedButtonForGroup(element()->name());
Can a single radio button, not in a group, still be required? Or is that
nonsense? If it is, do we have test cases for that?
More information about the webkit-reviews
mailing list