[webkit-reviews] review denied: [Bug 93544] Google search query text reverts to original search query after multiple searches : [Attachment 157328] Patch v1 - Fix + layouttest

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Brady Eidson <beidson at apple.com>'s
request for review:
Bug 93544: Google search query text reverts to original search query after
multiple searches
https://bugs.webkit.org/show_bug.cgi?id=93544

Attachment 157328: Patch v1 - Fix + layouttest
https://bugs.webkit.org/attachment.cgi?id=157328&action=review

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


> Source/WebCore/html/HTMLInputElement.cpp:816
> +    if (m_inputType->storesValueSeparateFromAttribute()) {
> +	   // We usually reset values to the empty string to clear out
sensitive fields when restoring from the page cache.
> +	   // But if the input type is textual with autocomplete=off and the
defaultValue is non-empty it doesn't make sense
> +	   // to fallback to the default value. So we'll relax the rule in
those instances.
> +	   if (!m_inputType->isTextType() || getAttribute(autocompleteAttr) !=
"off" || defaultValue().isEmpty())
> +	       setValue(String());
> +    }

This seems like too low a level for this check. I don’t see how this reset
function is supposed to know that it’s being called as part of clearing out a
sensitive field. It could be used for other purposes. I’d like to see the fix
done at another level. Even if there has to be some logic here, it should be
driven by what function is being called or by a function argument, not just a
guess.

The autocompleteAttr logic here also seems suspect. I’d expect it to be
fastGetAttribute rather than getAttribute and I would not expect the string
"off" to be hard coded. Does this match how other call sites check the
autocomplete attribute?