[Webkit-unassigned] [Bug 50442] Radio button group state is not restored correctly

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50442





--- Comment #6 from Kent Tamura <tkent at chromium.org>  2011-01-05 22:45:40 PST ---
(From update of attachment 76493)
View in context: https://bugs.webkit.org/attachment.cgi?id=76493&action=review

Thank you for reviewing!

>> WebCore/html/HTMLFormControlElement.h:175
>> +    bool stateRestored() const { return m_stateRestored; }
> 
> Given that this function and data member are only used in HTMLInputElement, it is unpleasant we had to add them to HTMLFormControlElement. Since the restoreFormControlState function is virtual, I see no reason this couldn’t have been kept entirely within the HTMLInputElement class; I suggest you move it there.

ok, I moved the flag to HTMLInputElement.

>> WebCore/html/HTMLInputElement.cpp:714
>> +        // breaking resotred state of other radio buttons in the group.
> 
> This comment is a bit too mechanical; saying “if createdByParser” is unclear. It says that delaying the call will “avoid breaking” restored state of other radio buttons. But delay until then.
> 
> We do not want to call setChecked until we have restored saved form state, right. I think that’s the real issue. So the comment needs to explain why this logic helps that happen.

I improved the comment.

>> WebCore/html/HTMLInputElement.cpp:798
>> +    m_createdByParser = false;
> 
> Since we are resetting this to false once we are done parsing, it’s not a “created by parser” flag at all. It’s a “parsing in progress” flag, and so needs a name that means that. I’d also like to know if we can keep the number of new boolean data members down to one rather than needing a flag for this and another for “state was restored”. Maybe that’s not possible; probably not worth wasting a lot of time on that.

Renamed it to m_parsingInProgress.

>> WebCore/html/HTMLInputElement.cpp:803
>> +    }
> 
> It seems a little strange to do this unconditionally for all input elements even if they don’t have a checked attribute. Not great for performance either. Maybe we should do this only if m_defaultChecked?
> 
> As a side note, the m_defaultChecked data member is not helpful at all! It’s just a cached copy of hasAttribute(checkedAttr). We should get rid of it and just call hasAttribute(checkedAttr) as needed.

Done.

>> WebCore/html/HTMLInputElement.cpp:926
>> +    setNeedsValidityCheck();
> 
> The change log has no explanation of this fix and no mention other than listing this function name. Is this a separate bug fix? Is it covered by the test? Does it need a separate test?

This was moved from parseMappedAttribute().
We need to call setNeedsValidityCheck() whenever the value is changed. So we had to add it in HTMLInputElement::finishParsingChildren().  Then, I found adding setNeedsValidityCheck() to setChecked() was better than calling setNeedsValidityCheck() after every setChecked().
If we don't add setNeedsValidityCheck(), an existing test crashes by an assertion failure.

>> WebCore/html/HTMLInputElement.h:214
>> +    HTMLInputElement(const QualifiedName&, Document*, HTMLFormElement* = 0, bool createdByParser = false);
> 
> This seems wrong. Classes derived from HTMLInputElement are going to set createdByParser to false, even in cases where they are created by the parser. Having the wrong value for createdByParser might be harmless now, but it’s not a good situation.

ok.
I used the default parameter because we can't add new parameter after HTMLFormElement*=0 without the default parameter. I removed the default parameter of HTMLFormElement*.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.