[webkit-reviews] review denied: [Bug 32261] Add ability to specify suggested autocomplete value in HTMLInputElement : [Attachment 44581] Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Zelidrag Hornung
<zelidrag at chromium.org>'s request for review:
Bug 32261: Add ability to specify suggested autocomplete value in
HTMLInputElement
https://bugs.webkit.org/show_bug.cgi?id=32261

Attachment 44581: Added ability to separate autocomplete suggestion previewing
from accepting of the suggested value in HTMLInputElement /
RenderTextControlSingleLine
https://bugs.webkit.org/attachment.cgi?id=44581&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
How about using a suggested value of null to mean suggest and thus eliminating
the separate enterSuggestMode function and m_suggestMode?

I don't think you need a leaveSuggestMode function. The client can call
setValue(suggestedValue) and then setSuggestedValue(String()) or just
setSuggestedValue(String()). If you think setSuggestedValue(String()) is too
ugly you could make a convenience function member called clearSuggestedValue.

Ideally you'd factor things so more logic was shared with setValue. Since there
is no regression test for this that's particularly important. It's likely
people would forget to update setSuggestedValue as they change setValue.

The name suggestMode is not a good name for a function or data member that's a
boolean. A good name would be something that completes the phrase "element
<xxx>", and "element suggest mode" does not work for that. I think
hasSuggestion or hasSuggestedValue would be good. Or you could omit this
function and say !suggestedValue().isNull() at the call sites.

We frown on the use of booleans for function arguments if the caller will be
passing a constant to a function like leaveSuggestMode. Instead we use enums.
See ReferrerPolicy, for example. But I suggest just removing the function
entirely.

The PopupMenu changes should be in a separate patch. Even if for you they are
part of the same task, they are not directly related.