<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#c30">Comment # 30</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@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#c29">comment #29</a>)
<span class="quote">> 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>)
> > 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);
> > }
>
> 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.</span >
(In reply to <a href="show_bug.cgi?id=153116#c28">comment #28</a>)
<span class="quote">> (In reply to <a href="show_bug.cgi?id=153116#c27">comment #27</a>)
> > Taking your above comments into consideration, we only need the
> > updateAutoFillButton() method that looks like:
>
> Yes, that looks right.
>
> > > > 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?
>
> I think there’s some confusion between true enums in C++ code, where the
> values need to avoid conflicting with each other and look right in C++ code
> context, enums in DOM APIs and enums in our internals file.
>
> 1) For enums in C++ code, they need to not collide with each other so we
> either need to prefix values with the type, or use enum class. And we use
> capital letters in those by convention.
>
> 2) For enums in DOM APIs, we need to match the style of the DOM
> specifications, which means we use ALL_CAPITAL_LETTERS style for the values.
> There is also some issue of name conflict there.
>
> 3) For these enums for internals, what we are calling enums is actually just
> a list of string values. There is no need to make them unique, prefix them
> with a type name, or anything else like that.
>
> So for these, in category (3) I think we should use lower case strings. When
> there is more than one word, not sure if we should use spaces (like normal
> English), hyphens (like CSS identifiers), underscores, or capital letters
> (like local variables in C++ code). But really no need to use capital
> letters.</span >
This is really helpful!</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>