[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 #27 from Zach Li <zacharyli323 at gmail.com> ---
(In reply to comment #25)
> Comment on attachment 270014 [details]
> 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.

Taking your above comments into consideration, we only need the updateAutoFillButton() method that looks like:

void TextFieldInputType::updateAutoFillButton()
{
    if (shouldDrawAutoFillButton()) {
        if (!m_container)
            createContainer();

        if (!m_autoFillButton) {
            m_autoFillButton = AutoFillButtonElement::create(element().document(), *this);
            m_container->appendChild(*m_autoFillButton, IGNORE_EXCEPTION);
        }

        m_autoFillButton->setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(element().autoFillButtonType()));
        m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueBlock, true);
        return;
    }

    if (m_autoFillButton)
        m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone, true);        
}

Also, filed https://bugs.webkit.org/show_bug.cgi?id=153628 to track the improvement.

> 
> > 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.

I also prefer shorter strings as long as they are clear. In Internals.idl, there are strings with capital letters and there are strings with lower case for enum, what is the recommended practice for this?

-- 
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/20160129/6f1b8ad7/attachment-0001.html>