[webkit-reviews] review denied: [Bug 50442] Radio button group state is not restored correctly : [Attachment 76493] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 50442: Radio button group state is not restored correctly
https://bugs.webkit.org/show_bug.cgi?id=50442

Attachment 76493: Patch
https://bugs.webkit.org/attachment.cgi?id=76493&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=76493&action=review

Looks great; a few comments.

> LayoutTests/fast/forms/state-restore-radio-group.html:31
> +	   // Submit form in a timeout to make sure that we create a new
back/forward list item. 	   

Disappointing that it has to work like this. It really would be better to have
another way to make this kind of test work.

> 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.

> WebCore/html/HTMLInputElement.cpp:714
> +	   // If m_createdByParser, delay the setChecked() call in order to
avoid
> +	   // 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.

> 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.

> WebCore/html/HTMLInputElement.cpp:803
> +    if (!stateRestored()) {
> +	   setChecked(m_defaultChecked);
> +	   m_useDefaultChecked = true;
> +    }

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.

> 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?

> WebCore/html/HTMLInputElement.h:214
> -    HTMLInputElement(const QualifiedName&, Document*, HTMLFormElement* = 0);

> +    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.