[Webkit-unassigned] [Bug 153116] Need ability to specify alternate image for AutoFill button in input fields

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=153116

--- Comment #30 from Zach Li <zacharyli323 at gmail.com> ---
(In reply to comment #29)
> Three other small suggestions. You can probably do two of these if you come
> back for cleanup.
> 
> (In reply to comment #27)
> > 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.

(In reply to comment #28)
> (In reply to comment #27)
> > 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!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160129/bcb258fd/attachment.html>