<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Need ability to specify alternate image for AutoFill button in input fields"
href="https://bugs.webkit.org/show_bug.cgi?id=153116#c21">Comment # 21</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - 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@gmail.com" title="Zach Li <zacharyli323@gmail.com>"> <span class="fn">Zach Li</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=153116#c20">comment #20</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=269831&action=diff" name="attach_269831" title="Patch v3">attachment 269831</a> <a href="attachment.cgi?id=269831&action=edit" title="Patch v3">[details]</a></span>
> Patch v3
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=269831&action=review">https://bugs.webkit.org/attachment.cgi?id=269831&action=review</a>
>
> Code is OK. Still one significant problem with it (no way to remove the
> auto-fill button), and a number of questionable points of style.
>
> > Source/WebCore/html/HTMLTextFormControlElement.h:37
> > +enum class AutoFillButtonType : uint8_t { Uninitialized, Credentials, Contacts };
>
> I think that the first value is None, not Uninitialized. It means no
> auto-fill button at all, not “uninitialized”.</span >
I didn't think Uninitialized was a good word choice, but I couldn't think of something better. I will use None.
<span class="quote">>
> > Source/WebCore/html/TextFieldInputType.cpp:643
> > + AtomicString pseudoClassName = autoFillButtonTypeToAutoFillButtonPseudoClassName(autoFillButtonType);
> > + m_autoFillButton->setPseudo(pseudoClassName);
>
> Would read better without a local variable as a single line.</span >
Will combine these two lines.
<span class="quote">>
> > Source/WebCore/html/TextFieldInputType.cpp:654
> > + createAutoFillButton(element().autoFillButtonType());
>
> Missing code to remove the auto-fill button if the type changes from
> contacts or credentials to none.</span >
If the type changes from contacts or credentials to none, the button will not be displayed with the code, which is our existing approach when we set show AutoFill button to false:
m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone, true);
This works and I don't think we have to actually remove the AutoFill button.
<span class="quote">>
> > Source/WebCore/html/TextFieldInputType.cpp:657
> > + AtomicString pseudoContactsAutoFillClassName = AtomicString("-webkit-contacts-auto-fill-button", AtomicString::ConstructFromLiteral);
> > + AtomicString pseudoCredentialsAutoFillClassName = AtomicString("-webkit-credentials-auto-fill-button", AtomicString::ConstructFromLiteral);
>
> This is a unnecessarily slow way to write the code. You can just write
> attribute == "-webkit-contacts-auto-fill-button"; no need to do all the
> extra work to create an atomic string.</span >
I see. I will just compare the string with the `attribute`.
<span class="quote">>
> > Source/WebCore/html/TextFieldInputType.cpp:658
> > + AtomicString attribute = m_autoFillButton->getAttribute(pseudoAttr);
>
> Wrong type here. It should be auto&, or const AtomicString& to avoid
> unnecessary work.</span >
Will change to const AtomicString&.
<span class="quote">>
> Wrong function called here. It should be fastGetAttribute.</span >
Will use fastGetAttribute.
<span class="quote">>
> > Source/WebCore/html/TextFieldInputType.cpp:659
> > + bool shouldUpdateAutoFillButtonType = ((attribute == pseudoContactsAutoFillClassName && element().autoFillButtonType() != AutoFillButtonType::Contacts) || (attribute == pseudoCredentialsAutoFillClassName && element().autoFillButtonType() != AutoFillButtonType::Credentials));
>
> This is really hard to read. I think a helper function would make it easier
> to read. But I also don’t understand why we wouldn’t just call setPseudo
> unconditionally. Are we trying to optimize something?</span >
I don't want to call setPseudo when the button type was not changed, or is this an unnecessary optimization?
Thanks for your review!</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>