[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 13:00:16 PST 2016
https://bugs.webkit.org/show_bug.cgi?id=153116
Daniel Bates <dbates at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #269073|review? |review-
Flags| |
--- Comment #4 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 269073
--> https://bugs.webkit.org/attachment.cgi?id=269073
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.
> 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.
> 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.
> 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);
> 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);
> 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);
> 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>.
> 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.
> Source/WebCore/ChangeLog:43
> + Add a new parameter to specify what the type of the AutoFill button.
Ditto.
> Source/WebCore/ChangeLog:47
> + (WebCore::stringToAutoFillButtonType): Convert the string the AutoFill button type enum.
Nit: This sentence does not read well.
> 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.
> 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.
On another note, can we move this definition to HTMLInputElement.h?
> 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);
> 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().
> 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.
> 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.
> 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).
> 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;
> 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;
> Source/WebKit2/WebProcess/InjectedBundle/DOM/InjectedBundleNodeHandle.h:32
> +#include <WebCore/HTMLTextFormControlElement.h>
We should forward declare AutoFillButtonType here instead of including 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/49ba803c/attachment-0001.html>
More information about the webkit-unassigned
mailing list