[Webkit-unassigned] [Bug 153116] Need ability to specify alternate image for AutoFill button in input fields

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=153116

--- Comment #25 from Darin Adler <darin at apple.com> ---
Comment on attachment 270014
  --> https://bugs.webkit.org/attachment.cgi?id=270014
Patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=270014&action=review

I think we should eventually return to this and refine a little further.

> Source/WebCore/html/TextFieldInputType.cpp:429
> +static bool isAutoFillButtonTypeChanged(const AtomicString& attribute, AutoFillButtonType autoFillButtonType)
> +{
> +    if (attribute == "-webkit-contacts-auto-fill-button" && autoFillButtonType != AutoFillButtonType::Contacts)
> +        return true;
> +
> +    if (attribute == "-webkit-credentials-auto-fill-button" && autoFillButtonType != AutoFillButtonType::Credentials)
> +        return true;
> +
> +    return false;
> +}

I would have written this:

    return attribute != autoFillButtonTypeToAutoFillButtonPseudoClassName(autoFillButtonType);

But as you will see in my comments below, we don’t need this function at all and it should be deleted.

> Source/WebCore/html/TextFieldInputType.cpp:645
> +void TextFieldInputType::createAutoFillButton(AutoFillButtonType autoFillButtonType)

This function should be entirely deleted and the code should be moved into updateAutoFillButton. There are a few small mistakes that are hidden by having this code in a separate function.

> Source/WebCore/html/TextFieldInputType.cpp:650
> +    if (autoFillButtonType == AutoFillButtonType::None)
> +        return;

This is dead code and it’s not needed. It will never be called in this case.

> Source/WebCore/html/TextFieldInputType.cpp:653
> +    m_autoFillButton->setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(autoFillButtonType));

This isn’t needed. The caller can take care of this after creating the button.

> Source/WebCore/html/TextFieldInputType.cpp:664
> -            createAutoFillButton();
> +            createAutoFillButton(element().autoFillButtonType());

This change wasn’t needed.

> Source/WebCore/html/TextFieldInputType.cpp:669
> +        const AtomicString& attribute = m_autoFillButton->fastGetAttribute(pseudoAttr);
> +        bool shouldUpdateAutoFillButtonType = isAutoFillButtonTypeChanged(attribute, element().autoFillButtonType());
> +        if (shouldUpdateAutoFillButtonType)
> +            m_autoFillButton->setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(element().autoFillButtonType()));

There’s no value to making this conditional. It should just be:

    m_autoFillButton->setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(element().autoFillButtonType()));

The other checking, including the isAutoFillButtonTypeChanged, is unnecessary and slightly wasteful.

> Source/WebCore/testing/Internals.idl:64
> +enum AutoFillButtonType {
> +    "AutoFillButtonTypeNone",
> +    "AutoFillButtonTypeContacts",
> +    "AutoFillButtonTypeCredentials"
> +};

I would have just used the strings "none", "credentials", and "contacts". Not sure why the long strings with capital letters are better.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160128/d84c5a6e/attachment-0001.html>