[webkit-reviews] review denied: [Bug 36762] Forms with autocomplete=off should not consume saved state : [Attachment 52132] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 36762: Forms with autocomplete=off should not consume saved state
https://bugs.webkit.org/show_bug.cgi?id=36762

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

------- Additional Comments from Darin Adler <darin at apple.com>
>  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