<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#c31">Comment # 31</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#c30">comment #30</a>)
<span class="quote">> (In reply to <a href="show_bug.cgi?id=153116#c29">comment #29</a>)
> > 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.</span >
This looks great and it is more consistent with our coding style to reduce nesting.
<span class="quote">> >
> > 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 >
Sam made this change in <a href="http://trac.webkit.org/changeset/181408">http://trac.webkit.org/changeset/181408</a>, probably he knows. Sam, what was the reason behind setting important to true?
<span class="quote">>
> (In reply to <a href="show_bug.cgi?id=153116#c28">comment #28</a>)
> > (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.
>
> This is really helpful!</span ></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>