<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:dbates@webkit.org" title="Daniel Bates <dbates@webkit.org>"> <span class="fn">Daniel Bates</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Need ability to specify alternate image for AutoFill button in input fields"
href="https://bugs.webkit.org/show_bug.cgi?id=153116">bug 153116</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #269073 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Need ability to specify alternate image for AutoFill button in input fields"
href="https://bugs.webkit.org/show_bug.cgi?id=153116#c4">Comment # 4</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Need ability to specify alternate image for AutoFill button in input fields"
href="https://bugs.webkit.org/show_bug.cgi?id=153116">bug 153116</a>
from <span class="vcard"><a class="email" href="mailto:dbates@webkit.org" title="Daniel Bates <dbates@webkit.org>"> <span class="fn">Daniel Bates</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=269073&action=diff" name="attach_269073" title="Patch v1">attachment 269073</a> <a href="attachment.cgi?id=269073&action=edit" title="Patch v1">[details]</a></span>
Patch v1
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=269073&action=review">https://bugs.webkit.org/attachment.cgi?id=269073&action=review</a>
This patch A WebKit2 Owner will need to review this patch since it affects WebKit2.
<span class="quote">> 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></span >
Please add <!DOCTYPE html> to the top of this file.
<span class="quote">> 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></span >
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.
<span class="quote">> LayoutTests/fast/forms/auto-fill-button/input-address-book-auto-fill-button.html:8
> +</span >
Please remove this empty line as I do not feel it improves the readability of this file.
<span class="quote">> LayoutTests/fast/forms/auto-fill-button/input-address-book-auto-fill-button.html:16
> + document.querySelector("#container").appendChild(dynamicInput);</span >
This is OK as-is. I would have written this as:
document.getElementById("container").appendChild(dynamicInput);
<span class="quote">> LayoutTests/fast/forms/auto-fill-button/input-address-book-auto-fill-button.html:20
> + document.querySelector("#container").appendChild(dynamicInput2);</span >
This is OK as-is. I would have written this as:
document.getElementById("container").appendChild(dynamicInput2);
<span class="quote">> LayoutTests/fast/forms/auto-fill-button/input-address-book-auto-fill-button.html:26
> + dynamicInput3.setAttribute("name", "phone_number");</span >
This is OK as-is. I would have written this as:
document.getElementById("container").appendChild(dynamicInput3);
<span class="quote">> 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.</span >
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 <<a href="https://webkit.org/contributing-code/#changelog-files">https://webkit.org/contributing-code/#changelog-files</a>>.
<span class="quote">> Source/WebCore/ChangeLog:32
> + (WebCore::InputType::updateAutoFillButton): Add a new parameter to specify what the type</span >
Nit: I suggest removing the word "what" to make this sentence read well. Alternatively, add the word "is" to the end of this sentence.
<span class="quote">> Source/WebCore/ChangeLog:43
> + Add a new parameter to specify what the type of the AutoFill button.</span >
Ditto.
<span class="quote">> Source/WebCore/ChangeLog:47
> + (WebCore::stringToAutoFillButtonType): Convert the string the AutoFill button type enum.</span >
Nit: This sentence does not read well.
<span class="quote">> Source/WebCore/ChangeLog:48
> + (WebCore::Internals::setShowAutoFillButton): Add a new parameter to specify what the type of the AutoFill button.</span >
Nit: I suggest removing the word "what" to make this sentence read well. Alternatively, add the word "is" to the end of this sentence.
<span class="quote">> Source/WebCore/html/HTMLTextFormControlElement.h:37
> +enum AutoFillButtonType { AutoFillButtonTypeCredentials, AutoFillButtonTypeAddressBook };</span >
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?
<span class="quote">> 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));</span >
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);
<span class="quote">> Source/WebCore/testing/Internals.cpp:1286
> + if (autoFillButtonType == "AutoFillButtonTypeCredentials")
> + return AutoFillButtonTypeCredentials;
> + if (autoFillButtonType == "AutoFillButtonTypeAddressBook")
> + return AutoFillButtonTypeAddressBook;
> + ASSERT_NOT_REACHED();
> + return AutoFillButtonTypeCredentials;</span >
Similarly we should use a switch statement here without a default case. Then we can remove the ASSERT_NOT_REACHED().
<span class="quote">> Source/WebCore/testing/Internals.idl:62
> + "AutoFillButtonTypeCredentials",
> + "AutoFillButtonTypeAddressBook"</span >
Please switch the order of these enumerators such that they are sorted according to the UNIX sort command.
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNodeHandle.cpp:39
> +static WebCore::AutoFillButtonType convertToAutoFillButtonType(WKAutoFillButtonType autoFillButtonType)</span >
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.
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNodeHandle.cpp:41
> + ASSERT(autoFillButtonType == WKAutoFillButtonTypeAddressBook || autoFillButtonType == WKAutoFillButtonTypeCredentials);</span >
I do not see the value in having this ASSERT() given the presence of ASSERT_NOT_REACHED() below (line 47).
<span class="quote">> 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;</span >
I would write this as:
switch (wkAutoFillButtonType) {
case WKAutoFillButtonTypeAddressBook:
return WebCore::AutoFillButtonTypeAddressBook;
case WKAutoFillButtonTypeCredentials:
return WebCore::AutoFillButtonTypeCredentials;
}
ASSERT_NOT_REACHED();
return WebCore::AutoFillButtonTypeCredentials;
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNodeHandlePrivate.h:38
> +enum WKAutoFillButtonType { WKAutoFillButtonTypeCredentials, WKAutoFillButtonTypeAddressBook };</span >
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;
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/DOM/InjectedBundleNodeHandle.h:32
> +#include <WebCore/HTMLTextFormControlElement.h></span >
We should forward declare AutoFillButtonType here instead of including HTMLTextFormControlElement.h.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>