<html>
    <head>
      <base href="https://bugs.webkit.org/" />
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_RESOLVED  bz_closed"
   title="RESOLVED FIXED - Need ability to specify alternate image for AutoFill button in input fields"
   href="https://bugs.webkit.org/show_bug.cgi?id=153116#c27">Comment # 27</a>
              on <a class="bz_bug_link 
          bz_status_RESOLVED  bz_closed"
   title="RESOLVED FIXED - Need ability to specify alternate image for AutoFill button in input fields"
   href="https://bugs.webkit.org/show_bug.cgi?id=153116">bug 153116</a>
              from <span class="vcard"><a class="email" href="mailto:zacharyli323&#64;gmail.com" title="Zach Li &lt;zacharyli323&#64;gmail.com&gt;"> <span class="fn">Zach Li</span></a>
</span></b>
        <pre>(In reply to <a href="show_bug.cgi?id=153116#c25">comment #25</a>)
<span class="quote">&gt; Comment on <span class=""><a href="attachment.cgi?id=270014&amp;action=diff" name="attach_270014" title="Patch v4">attachment 270014</a> <a href="attachment.cgi?id=270014&amp;action=edit" title="Patch v4">[details]</a></span>
&gt; Patch v4
&gt; 
&gt; View in context:
&gt; <a href="https://bugs.webkit.org/attachment.cgi?id=270014&amp;action=review">https://bugs.webkit.org/attachment.cgi?id=270014&amp;action=review</a>
&gt; 
&gt; I think we should eventually return to this and refine a little further.
&gt; 
&gt; &gt; Source/WebCore/html/TextFieldInputType.cpp:429
&gt; &gt; +static bool isAutoFillButtonTypeChanged(const AtomicString&amp; attribute, AutoFillButtonType autoFillButtonType)
&gt; &gt; +{
&gt; &gt; +    if (attribute == &quot;-webkit-contacts-auto-fill-button&quot; &amp;&amp; autoFillButtonType != AutoFillButtonType::Contacts)
&gt; &gt; +        return true;
&gt; &gt; +
&gt; &gt; +    if (attribute == &quot;-webkit-credentials-auto-fill-button&quot; &amp;&amp; autoFillButtonType != AutoFillButtonType::Credentials)
&gt; &gt; +        return true;
&gt; &gt; +
&gt; &gt; +    return false;
&gt; &gt; +}
&gt; 
&gt; I would have written this:
&gt; 
&gt;     return attribute !=
&gt; autoFillButtonTypeToAutoFillButtonPseudoClassName(autoFillButtonType);
&gt; 
&gt; But as you will see in my comments below, we don’t need this function at all
&gt; and it should be deleted.
&gt; 
&gt; &gt; Source/WebCore/html/TextFieldInputType.cpp:645
&gt; &gt; +void TextFieldInputType::createAutoFillButton(AutoFillButtonType autoFillButtonType)
&gt; 
&gt; This function should be entirely deleted and the code should be moved into
&gt; updateAutoFillButton. There are a few small mistakes that are hidden by
&gt; having this code in a separate function.
&gt; 
&gt; &gt; Source/WebCore/html/TextFieldInputType.cpp:650
&gt; &gt; +    if (autoFillButtonType == AutoFillButtonType::None)
&gt; &gt; +        return;
&gt; 
&gt; This is dead code and it’s not needed. It will never be called in this case.
&gt; 
&gt; &gt; Source/WebCore/html/TextFieldInputType.cpp:653
&gt; &gt; +    m_autoFillButton-&gt;setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(autoFillButtonType));
&gt; 
&gt; This isn’t needed. The caller can take care of this after creating the
&gt; button.
&gt; 
&gt; &gt; Source/WebCore/html/TextFieldInputType.cpp:664
&gt; &gt; -            createAutoFillButton();
&gt; &gt; +            createAutoFillButton(element().autoFillButtonType());
&gt; 
&gt; This change wasn’t needed.
&gt; 
&gt; &gt; Source/WebCore/html/TextFieldInputType.cpp:669
&gt; &gt; +        const AtomicString&amp; attribute = m_autoFillButton-&gt;fastGetAttribute(pseudoAttr);
&gt; &gt; +        bool shouldUpdateAutoFillButtonType = isAutoFillButtonTypeChanged(attribute, element().autoFillButtonType());
&gt; &gt; +        if (shouldUpdateAutoFillButtonType)
&gt; &gt; +            m_autoFillButton-&gt;setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(element().autoFillButtonType()));
&gt; 
&gt; There’s no value to making this conditional. It should just be:
&gt; 
&gt;    
&gt; m_autoFillButton-
&gt; &gt;setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(element().
&gt; autoFillButtonType()));
&gt; 
&gt; The other checking, including the isAutoFillButtonTypeChanged, is
&gt; unnecessary and slightly wasteful.</span >

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-&gt;appendChild(*m_autoFillButton, IGNORE_EXCEPTION);
        }

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

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

Also, filed <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Refine code for updateAutoFillButton method in TextFieldInputType.cpp"
   href="show_bug.cgi?id=153628">https://bugs.webkit.org/show_bug.cgi?id=153628</a> to track the improvement.

<span class="quote">&gt; 
&gt; &gt; Source/WebCore/testing/Internals.idl:64
&gt; &gt; +enum AutoFillButtonType {
&gt; &gt; +    &quot;AutoFillButtonTypeNone&quot;,
&gt; &gt; +    &quot;AutoFillButtonTypeContacts&quot;,
&gt; &gt; +    &quot;AutoFillButtonTypeCredentials&quot;
&gt; &gt; +};
&gt; 
&gt; I would have just used the strings &quot;none&quot;, &quot;credentials&quot;, and &quot;contacts&quot;.
&gt; Not sure why the long strings with capital letters are better.</span >

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?</pre>
        </div>
      </p>
      <hr>
      <span>You are receiving this mail because:</span>
      
      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>