Bug 36762 - Forms with autocomplete=off should not consume saved state
Summary: Forms with autocomplete=off should not consume saved state
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks: 23346
  Show dependency treegraph
 
Reported: 2010-03-29 09:57 PDT by benjie
Modified: 2010-04-02 11:31 PDT (History)
3 users (show)

See Also:


Attachments
Patch (7.98 KB, patch)
2010-03-30 23:51 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.2) (9.97 KB, patch)
2010-04-01 23:28 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description benjie 2010-03-29 09:57:22 PDT
This was reported in bug 23346 first, and Kent had patched in and attached a patch in bug 23346. But since 23346 describes multiple bugs, I am creating this new bug to keep track of this particular item.

The bug: autocomplete forms currently consume saved state, when WebKit is re-filling forms with previous values (on user back button action). They did not produce saved states earlier.
Comment 1 Kent Tamura 2010-03-30 23:51:13 PDT
Created attachment 52132 [details]
Patch
Comment 2 Darin Adler 2010-03-31 12:53:47 PDT
Comment on attachment 52132 [details]
Patch

> -    // Assuming we're still in a Form, respect the Form's setting
> -    if (HTMLFormElement* form = this->form())
> -        return form->autoComplete();
> -    
> -    // The default is true
> -    return true;
> +    return HTMLFormControlElementWithState::autoComplete();

The change log does not mention this change.
Comment 3 Kent Tamura 2010-04-01 02:18:54 PDT
(In reply to comment #2)
> (From update of attachment 52132 [details])
> > -    // Assuming we're still in a Form, respect the Form's setting
> > -    if (HTMLFormElement* form = this->form())
> > -        return form->autoComplete();
> > -    
> > -    // The default is true
> > -    return true;
> > +    return HTMLFormControlElementWithState::autoComplete();
> 
> The change log does not mention this change.

I think ChangeLog mentions it.  The behavior of this function won't be changed.
>         * html/HTMLInputElement.cpp:
>        (WebCore::HTMLInputElement::autoComplete):
>          Use HTMLFormControlElementWithState::autoComplete().
Comment 4 Darin Adler 2010-04-01 16:50:59 PDT
Comment on attachment 52132 [details]
Patch

>  void HTMLFormControlElementWithState::finishParsingChildren()
>  {
>      HTMLFormControlElement::finishParsingChildren();
> +    if (!autoComplete())
> +        return;
>      Document* doc = document();
>      if (doc->hasStateForNewFormElements()) {
>          String state;

This code change needs a comment. It's not at all clear why autoComplete has anything to do with restoring state; people should not have to look at change history to find out why. The comment will be a "why" comment.

>  {
>      if (m_autocomplete != Uninitialized)
>          return m_autocomplete == On;
> -    
> -    // Assuming we're still in a Form, respect the Form's setting
> -    if (HTMLFormElement* form = this->form())
> -        return form->autoComplete();
> -    
> -    // The default is true
> -    return true;
> +    return HTMLFormControlElementWithState::autoComplete();
>  }

The normal idiom when calling through to the base class is to name the immediate base. So you should write this as "return HTMLTextFormControlElement::autoComplete()".

>  bool HTMLSelectElement::saveFormControlState(String& value) const
>  {
> +    if (!autoComplete())
> +        return false;
>      return SelectElement::saveFormControlState(m_data, this, value);
>  }

Needs a "why" comment. But also, I suggest refactoring so you can put this logic into HTMLFormControlElementWithState. We don't want the implementers of each type of form control to have to get this right. And we also don't want the rules about saving to be in a different class than the rules about restoring.

A good way to do that would be to change the type of the argument to registerFormElementWithState to HTMLFormControlElementWithState*. Then change that function to call a non-virtual HTMLFormControlElementWithState function that checks autoComplete and then turns around and calls saveFormControlState. And you could move the saveFormControlState function from Element down into HTMLFormControlElementWithState.

>  bool HTMLTextAreaElement::saveFormControlState(String& result) const
>  {
> +    if (!autoComplete())
> +        return false;
>      result = value();
>      return true;
>  }

Needs a "why" comment.

review- because of the comments, but I think the change with a bit of cleanup would be more foolproof. Lets put all the responsibility for deciding how to handle state into a single place
Comment 5 Kent Tamura 2010-04-01 23:28:21 PDT
Created attachment 52382 [details]
Proposed patch (rev.2)
Comment 6 Kent Tamura 2010-04-01 23:37:25 PDT
Thank you for reviewing.

(In reply to comment #4)
> (From update of attachment 52132 [details])
> >  void HTMLFormControlElementWithState::finishParsingChildren()
> >  {
> >      HTMLFormControlElement::finishParsingChildren();
> > +    if (!autoComplete())
> > +        return;
> 
> This code change needs a comment. It's not at all clear why autoComplete has
> anything to do with restoring state; people should not have to look at change
> history to find out why. The comment will be a "why" comment.

Added a comment.

> The normal idiom when calling through to the base class is to name the
> immediate base. So you should write this as "return
> HTMLTextFormControlElement::autoComplete()".

It was just a mistake.  Fixed.

> 
> >  bool HTMLSelectElement::saveFormControlState(String& value) const
> >  {
> > +    if (!autoComplete())
> > +        return false;
> >      return SelectElement::saveFormControlState(m_data, this, value);
> >  }
> 
> Needs a "why" comment. But also, I suggest refactoring so you can put this
> logic into HTMLFormControlElementWithState. We don't want the implementers of
> each type of form control to have to get this right. And we also don't want the
> rules about saving to be in a different class than the rules about restoring.
> 
> A good way to do that would be to change the type of the argument to
> registerFormElementWithState to HTMLFormControlElementWithState*. Then change
> that function to call a non-virtual HTMLFormControlElementWithState function
> that checks autoComplete and then turns around and calls saveFormControlState.
> And you could move the saveFormControlState function from Element down into
> HTMLFormControlElementWithState.

I understand your advice.  But we can't do it because WML Form also uses this save/restore infrastructure, and we don't have a common parent class of HTMLFormControlElement and WMLFormControlElement. If you think we should introduce such common parent, I'll make another patch.

So, I introduced Element::shouldSaveAndRestoreFormControlState().
Comment 7 WebKit Commit Bot 2010-04-02 11:31:31 PDT
Comment on attachment 52382 [details]
Proposed patch (rev.2)

Clearing flags on attachment: 52382

Committed r57012: <http://trac.webkit.org/changeset/57012>
Comment 8 WebKit Commit Bot 2010-04-02 11:31:36 PDT
All reviewed patches have been landed.  Closing bug.