<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #269831 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<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#c20">Comment # 20</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:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>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.
<span class="quote">> Source/WebCore/html/HTMLTextFormControlElement.h:37
> +enum class AutoFillButtonType : uint8_t { Uninitialized, Credentials, Contacts };</span >
I think that the first value is None, not Uninitialized. It means no auto-fill button at all, not “uninitialized”.
<span class="quote">> Source/WebCore/html/TextFieldInputType.cpp:643
> + AtomicString pseudoClassName = autoFillButtonTypeToAutoFillButtonPseudoClassName(autoFillButtonType);
> + m_autoFillButton->setPseudo(pseudoClassName);</span >
Would read better without a local variable as a single line.
<span class="quote">> Source/WebCore/html/TextFieldInputType.cpp:654
> + createAutoFillButton(element().autoFillButtonType());</span >
Missing code to remove the auto-fill button if the type changes from contacts or credentials to none.
<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);</span >
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 class="quote">> Source/WebCore/html/TextFieldInputType.cpp:658
> + AtomicString attribute = m_autoFillButton->getAttribute(pseudoAttr);</span >
Wrong type here. It should be auto&, or const AtomicString& to avoid unnecessary work.
Wrong function called here. It should be fastGetAttribute.
<span class="quote">> Source/WebCore/html/TextFieldInputType.cpp:659
> + bool shouldUpdateAutoFillButtonType = ((attribute == pseudoContactsAutoFillClassName && element().autoFillButtonType() != AutoFillButtonType::Contacts) || (attribute == pseudoCredentialsAutoFillClassName && element().autoFillButtonType() != AutoFillButtonType::Credentials));</span >
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?</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>