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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 15 15:14:22 PST 2016


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

--- Comment #7 from Zach Li <zacharyli323 at gmail.com> ---
(In reply to comment #4)
> Comment on attachment 269073 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269073&action=review
> 
> This patch A WebKit2 Owner will need to review this patch since it affects
> WebKit2.
> 
> > LayoutTests/fast/forms/auto-fill-button/input-address-book-auto-fill-button.html:1
> > +<p>This tests that the Address Book AutoFill button renders. It can only be tested in the test tool.</p>
> 
> Please add <!DOCTYPE html> to the top of this file.

I was using the change in https://bugs.webkit.org/show_bug.cgi?id=142564 as a reference, but I could add <!DOCTYPE html>.

> 
> > LayoutTests/fast/forms/auto-fill-button/input-address-book-auto-fill-button.html:6
> > +<div id='container'>
> > +    <input type='text'>
> > +    <input type='text' name='first_name'>
> > +    <input type='text' name='last_name'>
> > +</div>
> 
> We seem to use single quoted literals throughout this markup and double
> quoted literals below. I suggest we use double quoted literals. Regardless,
> we should pick one quoting style and stick with it throughout this file.

Ditto. I will stick with double quote and make it consistent in existing AutoFill tests.

> 
> > LayoutTests/fast/forms/auto-fill-button/input-address-book-auto-fill-button.html:8
> > +
> 
> Please remove this empty line as I do not feel it improves the readability
> of this file.

Sure.

> 
> > LayoutTests/fast/forms/auto-fill-button/input-address-book-auto-fill-button.html:16
> > +    document.querySelector("#container").appendChild(dynamicInput);
> 
> This is OK as-is. I would have written this as:
> 
> document.getElementById("container").appendChild(dynamicInput);

Then I leave as-is unless you feel strongly about it.

> 
> > LayoutTests/fast/forms/auto-fill-button/input-address-book-auto-fill-button.html:20
> > +    document.querySelector("#container").appendChild(dynamicInput2);
> 
> This is OK as-is. I would have written this as:
> 
> document.getElementById("container").appendChild(dynamicInput2);

Ditto.

> 
> > LayoutTests/fast/forms/auto-fill-button/input-address-book-auto-fill-button.html:26
> > +    dynamicInput3.setAttribute("name", "phone_number");
> 
> This is OK as-is. I would have written this as:
> 
> document.getElementById("container").appendChild(dynamicInput3);

Ditto.

> 
> > Source/WebCore/ChangeLog:11
> > +        Test: fast/forms/auto-fill-button/input-address-book-auto-fill-button.html
> > +
> > +        Add a new AutoFill button that can be shown in <input> elements.
> 
> Swap the order of these such that the description (line 11) is before the
> line that mentions the added test (line 9) to conform to the format of a
> ChangeLog entry. An example of this formatting can be seen on page
> <https://webkit.org/contributing-code/#changelog-files>.

Will do.

> 
> > Source/WebCore/ChangeLog:32
> > +        (WebCore::InputType::updateAutoFillButton): Add a new parameter to specify what the type
> 
> Nit: I suggest removing the word "what" to make this sentence read well.
> Alternatively, add the word "is" to the end of this sentence.

Oops, I forgot the "is", but I will remove the "what".

> 
> > Source/WebCore/ChangeLog:43
> > +        Add a new parameter to specify what the type of the AutoFill button.
> 
> Ditto.

Ditto.

> 
> > Source/WebCore/ChangeLog:47
> > +        (WebCore::stringToAutoFillButtonType): Convert the string the AutoFill button type enum.
> 
> Nit: This sentence does not read well.

Changed to "Convert the string to AutoFill button type."

> 
> > Source/WebCore/ChangeLog:48
> > +        (WebCore::Internals::setShowAutoFillButton): Add a new parameter to specify what the type of the AutoFill button.
> 
> Nit: I suggest removing the word "what" to make this sentence read well.
> Alternatively, add the word "is" to the end of this sentence.

Oops, I forgot the "is", but I will remove the "what".

> 
> > Source/WebCore/html/HTMLTextFormControlElement.h:37
> > +enum AutoFillButtonType { AutoFillButtonTypeCredentials, AutoFillButtonTypeAddressBook };
> 
> Please make this an enum class whose width is uint32_t (to match the width
> of the enum WKAutoFillButtonType defined in WKBundleNodeHandlePrivate.h).
> Among the benefits of using an enum class we can remove the prefix
> AutoFillButtonType from the names of the enumerators since they are scoped.
> So, we can rename the enumerators to Credentials (maybe a better name would
> be Password - singular) and AddressBook, respectively.

Do you mean I should do something like:

enum class AutoFillButtonType : uint32_t {
Credentials,
AddressBook
};

The reason why I did not use enum class is that I got compiler error when I used enum class in internals.idl, should I just use enum in internals.idl and use enum class else where?

The word Credentials was picked because it described the AutoFill type more accurately, since not only was password field AutoFilled, but username field as well.

> 
> On another note, can we move this definition to HTMLInputElement.h?

The reason why I put the enum in HTMLTextFormControlElement.h is that both HTMLInputElement.h and InputType.h already includes HTMLTextFormControlElement.h, but not one another. If I declare the enum in HTMLInputElement.h, I would need to include HTMLInputElement.h in InputType.h from InputType.cpp, do you think this is preferred approach?

> 
> > Source/WebCore/html/TextFieldInputType.cpp:627
> > +    ASSERT(autoFillButtonType == AutoFillButtonTypeCredentials || autoFillButtonType == AutoFillButtonTypeAddressBook);
> > +    if (autoFillButtonType != AutoFillButtonTypeCredentials && autoFillButtonType != AutoFillButtonTypeAddressBook)
> > +        return;
> >  
> >      m_autoFillButton = AutoFillButtonElement::create(element().document(), *this);
> > -    m_autoFillButton->setPseudo(AtomicString("-webkit-auto-fill-button", AtomicString::ConstructFromLiteral));
> > +    if (autoFillButtonType == AutoFillButtonTypeCredentials)
> > +        m_autoFillButton->setPseudo(AtomicString("-webkit-auto-fill-button", AtomicString::ConstructFromLiteral));
> > +    else if (autoFillButtonType == AutoFillButtonTypeAddressBook)
> > +        m_autoFillButton->setPseudo(AtomicString("-webkit-address-book-auto-fill-button", AtomicString::ConstructFromLiteral));
> 
> We should take advantage of AutoFillButtonType being a enum/enum class and
> use a switch statement without a default case to cause a compile time error
> (instead of using a runtime assertion) when a new button type is added
> without updating this code:
> 
> m_autoFillButton = AutoFillButtonElement::create(element().document(),
> *this);
> AtomicString pseudoClassName;
> switch (autoFillButtonType) {
> case AddressBook:
>     pseudoClassName = AtomicString("-webkit-address-book-auto-fill-button",
> AtomicString::ConstructFromLiteral));
>     break;
> case Password:
>     pseudoClassName  = AtomicString("-webkit-auto-fill-button",
> AtomicString::ConstructFromLiteral));
>     break;
> }
> m_autoFillButton->setPseudo(pseudoClassName);

Will do.

> 
> > Source/WebCore/testing/Internals.cpp:1286
> > +    if (autoFillButtonType == "AutoFillButtonTypeCredentials")
> > +        return AutoFillButtonTypeCredentials;
> > +    if (autoFillButtonType == "AutoFillButtonTypeAddressBook")
> > +        return AutoFillButtonTypeAddressBook;
> > +    ASSERT_NOT_REACHED();
> > +    return AutoFillButtonTypeCredentials;
> 
> Similarly we should use a switch statement here without a default case. Then
> we can remove the ASSERT_NOT_REACHED().

Will do.

> 
> > Source/WebCore/testing/Internals.idl:62
> > +    "AutoFillButtonTypeCredentials",
> > +    "AutoFillButtonTypeAddressBook"
> 
> Please switch the order of these enumerators such that they are sorted
> according to the UNIX sort command.

Will do.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNodeHandle.cpp:39
> > +static WebCore::AutoFillButtonType convertToAutoFillButtonType(WKAutoFillButtonType autoFillButtonType)
> 
> We tend to name such WebKit to WebCore conversion functions of the form toX
> where X is the type. So, I would name this function toAutoFillButtonType()
> and name the argument wkAutoFillButtonType.

Will do.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNodeHandle.cpp:41
> > +    ASSERT(autoFillButtonType == WKAutoFillButtonTypeAddressBook || autoFillButtonType == WKAutoFillButtonTypeCredentials);
> 
> I do not see the value in having this ASSERT() given the presence of
> ASSERT_NOT_REACHED() below (line 47).

True.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNodeHandle.cpp:48
> > +    if (autoFillButtonType == WKAutoFillButtonTypeAddressBook)
> > +        return WebCore::AutoFillButtonTypeAddressBook;
> > +    if (autoFillButtonType == WKAutoFillButtonTypeCredentials)
> > +        return WebCore::AutoFillButtonTypeCredentials;
> > +    ASSERT_NOT_REACHED();
> > +    return WebCore::AutoFillButtonTypeCredentials;
> 
> I would write this as:
> 
> switch (wkAutoFillButtonType) {
> case WKAutoFillButtonTypeAddressBook:
>     return WebCore::AutoFillButtonTypeAddressBook;
> case WKAutoFillButtonTypeCredentials:
>    return WebCore::AutoFillButtonTypeCredentials;
> }
> ASSERT_NOT_REACHED();
> return WebCore::AutoFillButtonTypeCredentials;

Looks better. Will change.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNodeHandlePrivate.h:38
> > +enum WKAutoFillButtonType { WKAutoFillButtonTypeCredentials, WKAutoFillButtonTypeAddressBook };
> 
> In the C API we seem to prefer using an anonymous enum, a typedef to
> represent an enumeration, and prefix enumerator values with "kWK". Renaming
> WKAutoFillButtonTypeCredentials and WKAutoFillButtonTypeAddressBook to
> kWKAutoFillButtonTypePassword and kWKAutoFillButtonTypeAddressBook, I would
> write this declaration as:
> 
> enum {
>    kWKAutoFillButtonCredentials,
>    kWKAutoFillButtonTypeAddressBook
> };
> typedef uint32_t WKAutoFillButtonType;

So here we are not using enum class anymore?

> 
> > Source/WebKit2/WebProcess/InjectedBundle/DOM/InjectedBundleNodeHandle.h:32
> > +#include <WebCore/HTMLTextFormControlElement.h>
> 
> We should forward declare AutoFillButtonType here instead of including
> HTMLTextFormControlElement.h.

Forward declaring AutoFillButtonType means I should declare enum AutoFillButtonType before the class declaration so that I don't need to include HTMLTextFormControlElement.h?

-- 
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/20160115/da9a9f3b/attachment-0001.html>


More information about the webkit-unassigned mailing list