<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#c29">Comment # 29</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:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Three other small suggestions. You can probably do two of these if you come back for cleanup.
(In reply to <a href="show_bug.cgi?id=153116#c27">comment #27</a>)
<span class="quote">> 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);
> }</span >
Only thing I would do differently is that I would do the "should not draw" case first as the early exit case, reversing the logic.
if (!shouldDrawAutoFillButton()) {
if (m_autoFillButton)
m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone, true);
return;
}
That way less of the real logic of the function is nested inside an if statement.
Also, should not use IGNORE_EXCEPTION in the call to appendChild. We want the default behavior, ASSERT_NO_EXCEPTION.
I’m also not 100% sure why we have to set important to true in these style rules. It would be nice if someone double checked that we need to do that, possibly even making a test case, and if not, remove those unsightly ", true" in all the calls to setInlineStyleProperty.</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>