<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<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#c7">Comment # 7</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:zacharyli323@gmail.com" title="Zach Li <zacharyli323@gmail.com>"> <span class="fn">Zach Li</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=153116#c4">comment #4</a>)
<span class="quote">> 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.
>
> > 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.</span >
I was using the change in <a class="bz_bug_link
bz_status_RESOLVED bz_closed"
title="RESOLVED FIXED - Allow adding a button in input elements for auto fill related functionality"
href="show_bug.cgi?id=142564">https://bugs.webkit.org/show_bug.cgi?id=142564</a> as a reference, but I could add <!DOCTYPE html>.
<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>
>
> 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 >
Ditto. I will stick with double quote and make it consistent in existing AutoFill tests.
<span class="quote">>
> > 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.</span >
Sure.
<span class="quote">>
> > 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);</span >
Then I leave as-is unless you feel strongly about it.
<span class="quote">>
> > 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);</span >
Ditto.
<span class="quote">>
> > 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);</span >
Ditto.
<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.
>
> 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 >
Will do.
<span class="quote">>
> > 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.</span >
Oops, I forgot the "is", but I will remove the "what".
<span class="quote">>
> > Source/WebCore/ChangeLog:43
> > + Add a new parameter to specify what the type of the AutoFill button.
>
> Ditto.</span >
Ditto.
<span class="quote">>
> > Source/WebCore/ChangeLog:47
> > + (WebCore::stringToAutoFillButtonType): Convert the string the AutoFill button type enum.
>
> Nit: This sentence does not read well.</span >
Changed to "Convert the string to AutoFill button type."
<span class="quote">>
> > 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.</span >
Oops, I forgot the "is", but I will remove the "what".
<span class="quote">>
> > 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.</span >
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.
<span class="quote">>
> On another note, can we move this definition to HTMLInputElement.h?</span >
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?
<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));
>
> 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 >
Will do.
<span class="quote">>
> > 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().</span >
Will do.
<span class="quote">>
> > 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.</span >
Will do.
<span class="quote">>
> > 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.</span >
Will do.
<span class="quote">>
> > 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).</span >
True.
<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;
>
> I would write this as:
>
> switch (wkAutoFillButtonType) {
> case WKAutoFillButtonTypeAddressBook:
> return WebCore::AutoFillButtonTypeAddressBook;
> case WKAutoFillButtonTypeCredentials:
> return WebCore::AutoFillButtonTypeCredentials;
> }
> ASSERT_NOT_REACHED();
> return WebCore::AutoFillButtonTypeCredentials;</span >
Looks better. Will change.
<span class="quote">>
> > 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;</span >
So here we are not using enum class anymore?
<span class="quote">>
> > Source/WebKit2/WebProcess/InjectedBundle/DOM/InjectedBundleNodeHandle.h:32
> > +#include <WebCore/HTMLTextFormControlElement.h>
>
> We should forward declare AutoFillButtonType here instead of including
> HTMLTextFormControlElement.h.</span >
Forward declaring AutoFillButtonType means I should declare enum AutoFillButtonType before the class declaration so that I don't need to include 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>