[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 #31 from Zach Li <zacharyli323 at gmail.com> ---
(In reply to comment #30)
> (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.

This looks great and it is more consistent with our coding style to reduce nesting.

> > 
> > 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.

Sam made this change in http://trac.webkit.org/changeset/181408, probably he knows. Sam, what was the reason behind setting important to true?

> 
> (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/3f92548d/attachment-0001.html>