[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 #21 from Zach Li <zacharyli323 at gmail.com> ---
(In reply to comment #20)
> Comment on attachment 269831 [details]
> Patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269831&action=review
> 
> Code is OK. Still one significant problem with it (no way to remove the
> auto-fill button), and a number of questionable points of style.
> 
> > Source/WebCore/html/HTMLTextFormControlElement.h:37
> > +enum class AutoFillButtonType : uint8_t { Uninitialized, Credentials, Contacts };
> 
> I think that the first value is None, not Uninitialized. It means no
> auto-fill button at all, not “uninitialized”.

I didn't think Uninitialized was a good word choice, but I couldn't think of something better. I will use None.

> 
> > Source/WebCore/html/TextFieldInputType.cpp:643
> > +    AtomicString pseudoClassName = autoFillButtonTypeToAutoFillButtonPseudoClassName(autoFillButtonType);
> > +    m_autoFillButton->setPseudo(pseudoClassName);
> 
> Would read better without a local variable as a single line.

Will combine these two lines.

> 
> > Source/WebCore/html/TextFieldInputType.cpp:654
> > +            createAutoFillButton(element().autoFillButtonType());
> 
> Missing code to remove the auto-fill button if the type changes from
> contacts or credentials to none.

If the type changes from contacts or credentials to none, the button will not be displayed with the code, which is our existing approach when we set show AutoFill button to false:

m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone, true);

This works and I don't think we have to actually remove the AutoFill button.

> 
> > Source/WebCore/html/TextFieldInputType.cpp:657
> > +        AtomicString pseudoContactsAutoFillClassName = AtomicString("-webkit-contacts-auto-fill-button", AtomicString::ConstructFromLiteral);
> > +        AtomicString pseudoCredentialsAutoFillClassName = AtomicString("-webkit-credentials-auto-fill-button", AtomicString::ConstructFromLiteral);
> 
> This is a unnecessarily slow way to write the code. You can just write
> attribute == "-webkit-contacts-auto-fill-button"; no need to do all the
> extra work to create an atomic string.

I see. I will just compare the string with the `attribute`.

> 
> > Source/WebCore/html/TextFieldInputType.cpp:658
> > +        AtomicString attribute = m_autoFillButton->getAttribute(pseudoAttr);
> 
> Wrong type here. It should be auto&, or const AtomicString& to avoid
> unnecessary work.

Will change to const AtomicString&.

> 
> Wrong function called here. It should be fastGetAttribute.

Will use fastGetAttribute.

> 
> > Source/WebCore/html/TextFieldInputType.cpp:659
> > +        bool shouldUpdateAutoFillButtonType = ((attribute == pseudoContactsAutoFillClassName && element().autoFillButtonType() != AutoFillButtonType::Contacts) || (attribute == pseudoCredentialsAutoFillClassName && element().autoFillButtonType() != AutoFillButtonType::Credentials));
> 
> This is really hard to read. I think a helper function would make it easier
> to read. But I also don’t understand why we wouldn’t just call setPseudo
> unconditionally. Are we trying to optimize something?

I don't want to call setPseudo when the button type was not changed, or is this an unnecessary optimization?

Thanks for your review!

-- 
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/20160127/d254804d/attachment.html>