[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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #76493|review?                     |review-
               Flag|                            |




--- Comment #4 from Darin Adler <darin at apple.com>  2010-12-29 18:23:47 PST ---
(From update of attachment 76493)
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.

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