Bug 153116

Summary:

Need ability to specify alternate image for AutoFill button in input fields

Product:

WebKit

Reporter:

Zach Li <a.tion.surf>

Component:

WebKit2

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, a.tion.surf, buildbot, commit-queue, darin, dbates, rniwa, sam

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch v1	dbates: review-, buildbot: commit-queue-
Archive of layout-test-results from ews107 for mac-yosemite-wk2	none
Patch v2	darin: review-
Patch v3	darin: review+
Patch v4	none

Zach Li

Reported 2016-01-14 20:32:19 PST

Need ability to specify alternate image for AutoFill button in input fields.

Attachments
Patch v1 (86.17 KB, patch) 2016-01-15 11:25 PST, Zach Li	dbates: review- buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (932.54 KB, application/zip) 2016-01-15 12:23 PST, Build Bot	no flags	Details
Patch v2 (91.97 KB, patch) 2016-01-19 15:35 PST, Zach Li	darin: review-	Details Formatted Diff Diff
Patch v3 (96.13 KB, patch) 2016-01-25 18:06 PST, Zach Li	darin: review+	Details Formatted Diff Diff
Patch v4 (95.95 KB, patch) 2016-01-27 11:25 PST, Zach Li	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Zach Li

Comment 1 2016-01-15 11:25:47 PST

Created attachment 269073 [details] Patch v1

Build Bot

Comment 2 2016-01-15 12:23:47 PST

Comment on attachment 269073 [details] Patch v1 Attachment 269073 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/695234 New failing tests: http/tests/contentextensions/font-display-none-repeated-layout.html

Build Bot

Comment 3 2016-01-15 12:23:49 PST

Created attachment 269082 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Daniel Bates

Comment 4 2016-01-15 13:00:16 PST

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. > 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.

Daniel Bates

Comment 5 2016-01-15 13:10:54 PST

I also suggest that we rename -webkit-auto-fill-button to better describe its purpose. Maybe -webkit-password-auto-fill-button? or -webkit-credentials-auto-fill-button?

Daniel Bates

Comment 6 2016-01-15 13:19:58 PST

Comment on attachment 269073 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=269073&action=review > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNodeHandlePrivate.h:60 > +WK_EXPORT void WKBundleNodeHandleSetHTMLInputElementAutoFillButtonEnabled(WKBundleNodeHandleRef htmlInputElementHandle, bool enabled, WKAutoFillButtonType autoFillButtonType = WKAutoFillButtonTypeCredentials); This will break nightlies. Notice that the nightlies launch system Safari with the nightly build of WebKit.

Zach Li

Comment 7 2016-01-15 15:14:22 PST

(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?

Zach Li

Comment 8 2016-01-15 18:09:05 PST

(In reply to comment #5) > I also suggest that we rename -webkit-auto-fill-button to better describe > its purpose. Maybe -webkit-password-auto-fill-button? or > -webkit-credentials-auto-fill-button? My original concern was that some clients might have adopted this so I did not want to just rename it and break the functionality for other people. After discussing with Daniel and Andy, it should be fine to rename it.

Zach Li

Comment 9 2016-01-15 18:13:20 PST

(In reply to comment #6) > Comment on attachment 269073 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269073&action=review > > > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNodeHandlePrivate.h:60 > > +WK_EXPORT void WKBundleNodeHandleSetHTMLInputElementAutoFillButtonEnabled(WKBundleNodeHandleRef htmlInputElementHandle, bool enabled, WKAutoFillButtonType autoFillButtonType = WKAutoFillButtonTypeCredentials); > > This will break nightlies. Notice that the nightlies launch system Safari > with the nightly build of WebKit. Nice catch, C does not support default argument actually. My approach will be name the new method as WKBundleNodeHandleSetHTMLInputElementAutoFillButtonEnabledWithButtonType(WKBundleNodeHandleRef htmlInputElementHandle, bool enabled, WKAutoFillButtonType autoFillButtonType) and still keep the old method that takes two argument.

Zach Li

Comment 10 2016-01-15 18:25:28 PST

(In reply to comment #2) > Comment on attachment 269073 [details] > Patch v1 > > Attachment 269073 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.webkit.org/results/695234 > > New failing tests: > http/tests/contentextensions/font-display-none-repeated-layout.html I think these are false positive, the layout tests passed when I ran on my machine. Filed https://bugs.webkit.org/show_bug.cgi?id=153172.

Daniel Bates

Comment 11 2016-01-19 10:18:26 PST

(In reply to comment #7) > (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>. > I know that test added in the patch for bug #142564 did not include a doctype declaration. Although that test and the test included in your patch (attachment #269073 [details]) will render fine in quirks mode. I suggest that we reserve use of quirks mode (by omitting a doctype or using a doctype other than <!DOCTYPE html>) to those tests that make uses of such quirks. The reasoning behind this suggestion is that it avoids the chance, though unlikely, that a rendering bug may be masked in the test by a quirk. As aforementioned, this test is not affected by quirk mode. Regardless, I suggest that we get in the habit of declaring pages as standards compliant (by using <!DOCTYPE html>) unless the page depends on quirks mode. > [...] > > > > > 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 > }; > Yes, though please indent the contents of the enum class. > 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? > I am assuming you mean Internals.cpp. What is the compiler error that you got? > 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. > OK. > > > > 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? > > > OK, leave the definition in HTMLTextFormControlElement.h. > > [...] > > > > > 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? > Correct, we do not want to use an enum class here because this file is for C API and enum classes are a feature of C++11 and later. That is, C does not support enum classes. > > > > > 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 should forward declare AutoFillButtonType. Then you can remove the #include for header file HTMLTextFormControlElement.h. For clarity, forwarding declaring an enum class does not mean re-declaring it and its values. To forward declare AutoFillButtonType we would write: enum class AutoFillButtonType : uint32_t;

Zach Li

Comment 12 2016-01-19 11:00:35 PST

(In reply to comment #11) > (In reply to comment #7) > > (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>. > > > > I know that test added in the patch for bug #142564 did not include a > doctype declaration. Although that test and the test included in your patch > (attachment #269073 [details]) will render fine in quirks mode. I suggest > that we reserve use of quirks mode (by omitting a doctype or using a doctype > other than <!DOCTYPE html>) to those tests that make uses of such quirks. > The reasoning behind this suggestion is that it avoids the chance, though > unlikely, that a rendering bug may be masked in the test by a quirk. As > aforementioned, this test is not affected by quirk mode. Regardless, I > suggest that we get in the habit of declaring pages as standards compliant > (by using <!DOCTYPE html>) unless the page depends on quirks mode. > Yep, I added <!DOCTYPE html>. > > [...] > > > > > > > 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 > > }; > > > > Yes, though please indent the contents of the enum class. > > > 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? > > > > I am assuming you mean Internals.cpp. What is the compiler error that you > got? I do mean internals.idl. I am passing AutoFillButtonType as an extra parameter to -setShowAutoFillButton. The compiler error I got is: Next token should be {, but AutoFillButtonType at 34: enum class AutoFillButtonType { IDLParser.pm:774 at WebCore/bindings/scripts/IDLParser.pm line 143. in /Users/zhuoli/Source/Safari/OpenSource/Source/WebCore/testing/Internals.idl at WebCore/bindings/scripts/IDLParser.pm line 200. make: *** [JSInternals.h] Error 255 make: *** Waiting for unfinished jobs.... > > > 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. > > > > OK. > > > > > > > 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? > > > > > > > OK, leave the definition in HTMLTextFormControlElement.h. > > > > [...] > > > > > > > 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? > > > > Correct, we do not want to use an enum class here because this file is for C > API and enum classes are a feature of C++11 and later. That is, C does not > support enum classes. > > > > > > > > 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 should forward declare AutoFillButtonType. Then you can remove the > #include for header file HTMLTextFormControlElement.h. For clarity, > forwarding declaring an enum class does not mean re-declaring it and its > values. To forward declare AutoFillButtonType we would write: > > enum class AutoFillButtonType : uint32_t; Thanks for your feedback!

Zach Li

Comment 13 2016-01-19 15:35:41 PST

Created attachment 269302 [details] Patch v2

Daniel Bates

Comment 14 2016-01-20 10:01:47 PST

Comment on attachment 269302 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=269302&action=review Thank you Zhuo Li for updating the patch. It looks good to me though a WebKit2 owner will need to formally review it. I noticed some very minor nits. > Source/WebCore/html/TextFieldInputType.cpp:621 > + ASSERT(autoFillButtonType == AutoFillButtonType::Credentials || autoFillButtonType == AutoFillButtonType::AddressBook); > + if (autoFillButtonType != AutoFillButtonType::Credentials && autoFillButtonType != AutoFillButtonType::AddressBook) > + return; Please remove this code as it is unnecessary to runtime assert the button type because our use of the switch statement below and AutoFillButtonType being a enum class implies that a compile-time failure will occur if a enumerator value is added to-/renamed/removed from- AutoFillButtonType and the switch (below) is not updated. > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNodeHandle.cpp:49 > + ASSERT_NOT_REACHED(); > + return WebCore::AutoFillButtonType::Credentials; Are these lines necessary? I thought the supported versions of clang/gcc were smart enough to realize that a switch block for an enum/enum class handles all possible code paths and hence the proposed added code is unreachable.

Zach Li

Comment 15 2016-01-20 15:19:53 PST

Comment on attachment 269302 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=269302&action=review >> Source/WebCore/html/TextFieldInputType.cpp:621 >> + return; > > Please remove this code as it is unnecessary to runtime assert the button type because our use of the switch statement below and AutoFillButtonType being a enum class implies that a compile-time failure will occur if a enumerator value is added to-/renamed/removed from- AutoFillButtonType and the switch (below) is not updated. Okay. >> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNodeHandle.cpp:49 >> + return WebCore::AutoFillButtonType::Credentials; > > Are these lines necessary? I thought the supported versions of clang/gcc were smart enough to realize that a switch block for an enum/enum class handles all possible code paths and hence the proposed added code is unreachable. These lines are necessary. When I tried to remove them, I got "control may reach end of non-void function."

Darin Adler

Comment 16 2016-01-24 15:02:03 PST

Comment on attachment 269302 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=269302&action=review review- because it’s not OK to make every HTMLInputElement so much bigger; should be easy to fix, though > Source/WebCore/html/HTMLInputElement.h:436 > + AutoFillButtonType m_autoFillButtonType; This is only one bit. It’s not great that it’s going to make every HTMLInputElement 32 bits bigger (or more). Notice that all the fields around this are using bitfields to keep things cheap. Not OK to make every input element so much bigger. I think this should be: unsigned m_autoFillButtonType : 1; // AutoFillButtonType To match how we do things in the rest of the class. We could also consider combining this with > Source/WebCore/html/HTMLTextFormControlElement.h:37 > +enum class AutoFillButtonType : uint32_t { Credentials, AddressBook }; 32 bits just to hold one bit? How about 8 bits? > Source/WebCore/html/InputType.h:268 > + virtual void updateAutoFillButton(AutoFillButtonType); We should not add this argument to this function. The members functions in TextFieldInputType can get the type from the element directly; it doesn’t need to be passed in. > Source/WebCore/html/TextFieldInputType.cpp:644 > -void TextFieldInputType::updateAutoFillButton() > +void TextFieldInputType::updateAutoFillButton(AutoFillButtonType autoFillButtonType) > { > if (shouldDrawAutoFillButton()) { > if (!m_container) > createContainer(); > > if (!m_autoFillButton) > - createAutoFillButton(); > + createAutoFillButton(autoFillButtonType); This looks wrong. If the auto fill button type has changed, I don’t see code to cause the old one to be destroyed and a new one of the correct type to be created.

Darin Adler

Comment 17 2016-01-24 15:04:38 PST

Comment on attachment 269302 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=269302&action=review >> Source/WebCore/html/HTMLInputElement.h:436 >> + AutoFillButtonType m_autoFillButtonType; > > This is only one bit. It’s not great that it’s going to make every HTMLInputElement 32 bits bigger (or more). Notice that all the fields around this are using bitfields to keep things cheap. Not OK to make every input element so much bigger. I think this should be: > > unsigned m_autoFillButtonType : 1; // AutoFillButtonType > > To match how we do things in the rest of the class. > > We could also consider combining this with We could also consider combining this with the flag that says there is an auto-fill button. We don’t need to support setting the type when showAutoFillButton is false, and over time that could save us some bits. >> Source/WebCore/html/TextFieldInputType.cpp:644 >> + createAutoFillButton(autoFillButtonType); > > This looks wrong. If the auto fill button type has changed, I don’t see code to cause the old one to be destroyed and a new one of the correct type to be created. We should make a test case that shows the auto-fill button correctly changing even when the attribute is changed after the fact.

Zach Li

Comment 18 2016-01-25 17:02:38 PST

Comment on attachment 269302 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=269302&action=review >>> Source/WebCore/html/HTMLInputElement.h:436 >>> + AutoFillButtonType m_autoFillButtonType; >> >> This is only one bit. It’s not great that it’s going to make every HTMLInputElement 32 bits bigger (or more). Notice that all the fields around this are using bitfields to keep things cheap. Not OK to make every input element so much bigger. I think this should be: >> >> unsigned m_autoFillButtonType : 1; // AutoFillButtonType >> >> To match how we do things in the rest of the class. >> >> We could also consider combining this with > > We could also consider combining this with the flag that says there is an auto-fill button. We don’t need to support setting the type when showAutoFillButton is false, and over time that could save us some bits. Great suggestion! The new AutoFill button type will contain Contacts, Credentials, and Uninitialized. The Uninitialized is equivalent to the AutoFill button being not enabled. >> Source/WebCore/html/HTMLTextFormControlElement.h:37 >> +enum class AutoFillButtonType : uint32_t { Credentials, AddressBook }; > > 32 bits just to hold one bit? How about 8 bits? Will change to uint8_t. >> Source/WebCore/html/InputType.h:268 >> + virtual void updateAutoFillButton(AutoFillButtonType); > > We should not add this argument to this function. The members functions in TextFieldInputType can get the type from the element directly; it doesn’t need to be passed in. You are right, I can get the type from element().autoFillButtonType(). >>> Source/WebCore/html/TextFieldInputType.cpp:644 >>> + createAutoFillButton(autoFillButtonType); >> >> This looks wrong. If the auto fill button type has changed, I don’t see code to cause the old one to be destroyed and a new one of the correct type to be created. > > We should make a test case that shows the auto-fill button correctly changing even when the attribute is changed after the fact. Ideally, the case should not happen, but I will handle such case.

Zach Li

Comment 19 2016-01-25 18:06:39 PST

Created attachment 269831 [details] Patch v3 Addressed Daniel and Darin's comments.

Darin Adler

Comment 20 2016-01-26 15:09:34 PST

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”. > 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. > 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. > 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. > 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. Wrong function called here. It should be 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?

Zach Li

Comment 21 2016-01-27 00:46:09 PST

(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!

Zach Li

Comment 22 2016-01-27 11:25:01 PST

Created attachment 270014 [details] Patch v4 Reviewed by Darin, but I am not a committer, so I need someone to help me commit the change.

WebKit Commit Bot

Comment 23 2016-01-27 12:24:46 PST

Comment on attachment 270014 [details] Patch v4 Clearing flags on attachment: 270014 Committed r195685: <http://trac.webkit.org/changeset/195685>

WebKit Commit Bot

Comment 24 2016-01-27 12:24:53 PST

All reviewed patches have been landed. Closing bug.

Darin Adler

Comment 25 2016-01-28 09:19:23 PST

Comment on attachment 270014 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=270014&action=review I think we should eventually return to this and refine a little further. > Source/WebCore/html/TextFieldInputType.cpp:429 > +static bool isAutoFillButtonTypeChanged(const AtomicString& attribute, AutoFillButtonType autoFillButtonType) > +{ > + if (attribute == "-webkit-contacts-auto-fill-button" && autoFillButtonType != AutoFillButtonType::Contacts) > + return true; > + > + if (attribute == "-webkit-credentials-auto-fill-button" && autoFillButtonType != AutoFillButtonType::Credentials) > + return true; > + > + return false; > +} I would have written this: return attribute != autoFillButtonTypeToAutoFillButtonPseudoClassName(autoFillButtonType); But as you will see in my comments below, we don’t need this function at all and it should be deleted. > Source/WebCore/html/TextFieldInputType.cpp:645 > +void TextFieldInputType::createAutoFillButton(AutoFillButtonType autoFillButtonType) This function should be entirely deleted and the code should be moved into updateAutoFillButton. There are a few small mistakes that are hidden by having this code in a separate function. > Source/WebCore/html/TextFieldInputType.cpp:650 > + if (autoFillButtonType == AutoFillButtonType::None) > + return; This is dead code and it’s not needed. It will never be called in this case. > Source/WebCore/html/TextFieldInputType.cpp:653 > + m_autoFillButton->setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(autoFillButtonType)); This isn’t needed. The caller can take care of this after creating the button. > Source/WebCore/html/TextFieldInputType.cpp:664 > - createAutoFillButton(); > + createAutoFillButton(element().autoFillButtonType()); This change wasn’t needed. > Source/WebCore/html/TextFieldInputType.cpp:669 > + const AtomicString& attribute = m_autoFillButton->fastGetAttribute(pseudoAttr); > + bool shouldUpdateAutoFillButtonType = isAutoFillButtonTypeChanged(attribute, element().autoFillButtonType()); > + if (shouldUpdateAutoFillButtonType) > + m_autoFillButton->setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(element().autoFillButtonType())); There’s no value to making this conditional. It should just be: m_autoFillButton->setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(element().autoFillButtonType())); The other checking, including the isAutoFillButtonTypeChanged, is unnecessary and slightly wasteful. > Source/WebCore/testing/Internals.idl:64 > +enum AutoFillButtonType { > + "AutoFillButtonTypeNone", > + "AutoFillButtonTypeContacts", > + "AutoFillButtonTypeCredentials" > +}; I would have just used the strings "none", "credentials", and "contacts". Not sure why the long strings with capital letters are better.

Darin Adler

Comment 26 2016-01-28 16:54:33 PST

(In reply to comment #21) > I don't want to call setPseudo when the button type was not changed, or is > this an unnecessary optimization? Yes, I think it’s an unnecessary optimization.

Zach Li

Comment 27 2016-01-28 18:25:47 PST

(In reply to comment #25) > Comment on attachment 270014 [details] > Patch v4 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270014&action=review > > I think we should eventually return to this and refine a little further. > > > Source/WebCore/html/TextFieldInputType.cpp:429 > > +static bool isAutoFillButtonTypeChanged(const AtomicString& attribute, AutoFillButtonType autoFillButtonType) > > +{ > > + if (attribute == "-webkit-contacts-auto-fill-button" && autoFillButtonType != AutoFillButtonType::Contacts) > > + return true; > > + > > + if (attribute == "-webkit-credentials-auto-fill-button" && autoFillButtonType != AutoFillButtonType::Credentials) > > + return true; > > + > > + return false; > > +} > > I would have written this: > > return attribute != > autoFillButtonTypeToAutoFillButtonPseudoClassName(autoFillButtonType); > > But as you will see in my comments below, we don’t need this function at all > and it should be deleted. > > > Source/WebCore/html/TextFieldInputType.cpp:645 > > +void TextFieldInputType::createAutoFillButton(AutoFillButtonType autoFillButtonType) > > This function should be entirely deleted and the code should be moved into > updateAutoFillButton. There are a few small mistakes that are hidden by > having this code in a separate function. > > > Source/WebCore/html/TextFieldInputType.cpp:650 > > + if (autoFillButtonType == AutoFillButtonType::None) > > + return; > > This is dead code and it’s not needed. It will never be called in this case. > > > Source/WebCore/html/TextFieldInputType.cpp:653 > > + m_autoFillButton->setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(autoFillButtonType)); > > This isn’t needed. The caller can take care of this after creating the > button. > > > Source/WebCore/html/TextFieldInputType.cpp:664 > > - createAutoFillButton(); > > + createAutoFillButton(element().autoFillButtonType()); > > This change wasn’t needed. > > > Source/WebCore/html/TextFieldInputType.cpp:669 > > + const AtomicString& attribute = m_autoFillButton->fastGetAttribute(pseudoAttr); > > + bool shouldUpdateAutoFillButtonType = isAutoFillButtonTypeChanged(attribute, element().autoFillButtonType()); > > + if (shouldUpdateAutoFillButtonType) > > + m_autoFillButton->setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(element().autoFillButtonType())); > > There’s no value to making this conditional. It should just be: > > > m_autoFillButton- > >setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(element(). > autoFillButtonType())); > > The other checking, including the isAutoFillButtonTypeChanged, is > unnecessary and slightly wasteful. Taking your above comments into consideration, we only need the updateAutoFillButton() method that looks like: void TextFieldInputType::updateAutoFillButton() { if (shouldDrawAutoFillButton()) { if (!m_container) createContainer(); if (!m_autoFillButton) { m_autoFillButton = AutoFillButtonElement::create(element().document(), *this); m_container->appendChild(*m_autoFillButton, IGNORE_EXCEPTION); } m_autoFillButton->setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(element().autoFillButtonType())); m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueBlock, true); return; } if (m_autoFillButton) m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone, true); } Also, filed https://bugs.webkit.org/show_bug.cgi?id=153628 to track the improvement. > > > Source/WebCore/testing/Internals.idl:64 > > +enum AutoFillButtonType { > > + "AutoFillButtonTypeNone", > > + "AutoFillButtonTypeContacts", > > + "AutoFillButtonTypeCredentials" > > +}; > > I would have just used the strings "none", "credentials", and "contacts". > Not sure why the long strings with capital letters are better. I also prefer shorter strings as long as they are clear. In Internals.idl, there are strings with capital letters and there are strings with lower case for enum, what is the recommended practice for this?

Darin Adler

Comment 28 2016-01-29 09:26:20 PST

(In reply to comment #27) > Taking your above comments into consideration, we only need the > updateAutoFillButton() method that looks like: Yes, that looks right. > > > Source/WebCore/testing/Internals.idl:64 > > > +enum AutoFillButtonType { > > > + "AutoFillButtonTypeNone", > > > + "AutoFillButtonTypeContacts", > > > + "AutoFillButtonTypeCredentials" > > > +}; > > > > I would have just used the strings "none", "credentials", and "contacts". > > Not sure why the long strings with capital letters are better. > > I also prefer shorter strings as long as they are clear. In Internals.idl, > there are strings with capital letters and there are strings with lower case > for enum, what is the recommended practice for this? I think there’s some confusion between true enums in C++ code, where the values need to avoid conflicting with each other and look right in C++ code context, enums in DOM APIs and enums in our internals file. 1) For enums in C++ code, they need to not collide with each other so we either need to prefix values with the type, or use enum class. And we use capital letters in those by convention. 2) For enums in DOM APIs, we need to match the style of the DOM specifications, which means we use ALL_CAPITAL_LETTERS style for the values. There is also some issue of name conflict there. 3) For these enums for internals, what we are calling enums is actually just a list of string values. There is no need to make them unique, prefix them with a type name, or anything else like that. So for these, in category (3) I think we should use lower case strings. When there is more than one word, not sure if we should use spaces (like normal English), hyphens (like CSS identifiers), underscores, or capital letters (like local variables in C++ code). But really no need to use capital letters.

Darin Adler

Comment 29 2016-01-29 09:30:35 PST

Three other small suggestions. You can probably do two of these if you come back for cleanup. (In reply to comment #27) > void TextFieldInputType::updateAutoFillButton() > { > if (shouldDrawAutoFillButton()) { > if (!m_container) > createContainer(); > > if (!m_autoFillButton) { > m_autoFillButton = AutoFillButtonElement::create(element().document(), *this); > m_container->appendChild(*m_autoFillButton, IGNORE_EXCEPTION); > } > > m_autoFillButton->setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(element().autoFillButtonType())); > m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueBlock, true); > return; > } > > if (m_autoFillButton) > m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone, true); > } Only thing I would do differently is that I would do the "should not draw" case first as the early exit case, reversing the logic. if (!shouldDrawAutoFillButton()) { if (m_autoFillButton) m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone, true); return; } That way less of the real logic of the function is nested inside an if statement. Also, should not use IGNORE_EXCEPTION in the call to appendChild. We want the default behavior, ASSERT_NO_EXCEPTION. I’m also not 100% sure why we have to set important to true in these style rules. It would be nice if someone double checked that we need to do that, possibly even making a test case, and if not, remove those unsightly ", true" in all the calls to setInlineStyleProperty.

Zach Li

Comment 30 2016-01-29 10:58:15 PST

(In reply to comment #29) > Three other small suggestions. You can probably do two of these if you come > back for cleanup. > > (In reply to comment #27) > > void TextFieldInputType::updateAutoFillButton() > > { > > if (shouldDrawAutoFillButton()) { > > if (!m_container) > > createContainer(); > > > > if (!m_autoFillButton) { > > m_autoFillButton = AutoFillButtonElement::create(element().document(), *this); > > m_container->appendChild(*m_autoFillButton, IGNORE_EXCEPTION); > > } > > > > m_autoFillButton->setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(element().autoFillButtonType())); > > m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueBlock, true); > > return; > > } > > > > if (m_autoFillButton) > > m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone, true); > > } > > Only thing I would do differently is that I would do the "should not draw" > case first as the early exit case, reversing the logic. > > if (!shouldDrawAutoFillButton()) { > if (m_autoFillButton) > m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, > CSSValueNone, true); > return; > } > > That way less of the real logic of the function is nested inside an if > statement. > > Also, should not use IGNORE_EXCEPTION in the call to appendChild. We want > the default behavior, ASSERT_NO_EXCEPTION. > > I’m also not 100% sure why we have to set important to true in these style > rules. It would be nice if someone double checked that we need to do that, > possibly even making a test case, and if not, remove those unsightly ", > true" in all the calls to setInlineStyleProperty. (In reply to comment #28) > (In reply to comment #27) > > Taking your above comments into consideration, we only need the > > updateAutoFillButton() method that looks like: > > Yes, that looks right. > > > > > Source/WebCore/testing/Internals.idl:64 > > > > +enum AutoFillButtonType { > > > > + "AutoFillButtonTypeNone", > > > > + "AutoFillButtonTypeContacts", > > > > + "AutoFillButtonTypeCredentials" > > > > +}; > > > > > > I would have just used the strings "none", "credentials", and "contacts". > > > Not sure why the long strings with capital letters are better. > > > > I also prefer shorter strings as long as they are clear. In Internals.idl, > > there are strings with capital letters and there are strings with lower case > > for enum, what is the recommended practice for this? > > I think there’s some confusion between true enums in C++ code, where the > values need to avoid conflicting with each other and look right in C++ code > context, enums in DOM APIs and enums in our internals file. > > 1) For enums in C++ code, they need to not collide with each other so we > either need to prefix values with the type, or use enum class. And we use > capital letters in those by convention. > > 2) For enums in DOM APIs, we need to match the style of the DOM > specifications, which means we use ALL_CAPITAL_LETTERS style for the values. > There is also some issue of name conflict there. > > 3) For these enums for internals, what we are calling enums is actually just > a list of string values. There is no need to make them unique, prefix them > with a type name, or anything else like that. > > So for these, in category (3) I think we should use lower case strings. When > there is more than one word, not sure if we should use spaces (like normal > English), hyphens (like CSS identifiers), underscores, or capital letters > (like local variables in C++ code). But really no need to use capital > letters. This is really helpful!

Zach Li

Comment 31 2016-01-29 11:06:34 PST

(In reply to comment #30) > (In reply to comment #29) > > Three other small suggestions. You can probably do two of these if you come > > back for cleanup. > > > > (In reply to comment #27) > > > void TextFieldInputType::updateAutoFillButton() > > > { > > > if (shouldDrawAutoFillButton()) { > > > if (!m_container) > > > createContainer(); > > > > > > if (!m_autoFillButton) { > > > m_autoFillButton = AutoFillButtonElement::create(element().document(), *this); > > > m_container->appendChild(*m_autoFillButton, IGNORE_EXCEPTION); > > > } > > > > > > m_autoFillButton->setPseudo(autoFillButtonTypeToAutoFillButtonPseudoClassName(element().autoFillButtonType())); > > > m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueBlock, true); > > > return; > > > } > > > > > > if (m_autoFillButton) > > > m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone, true); > > > } > > > > Only thing I would do differently is that I would do the "should not draw" > > case first as the early exit case, reversing the logic. > > > > if (!shouldDrawAutoFillButton()) { > > if (m_autoFillButton) > > m_autoFillButton->setInlineStyleProperty(CSSPropertyDisplay, > > CSSValueNone, true); > > return; > > } > > > > That way less of the real logic of the function is nested inside an if > > statement. This looks great and it is more consistent with our coding style to reduce nesting. > > > > Also, should not use IGNORE_EXCEPTION in the call to appendChild. We want > > the default behavior, ASSERT_NO_EXCEPTION. > > > > I’m also not 100% sure why we have to set important to true in these style > > rules. It would be nice if someone double checked that we need to do that, > > possibly even making a test case, and if not, remove those unsightly ", > > true" in all the calls to setInlineStyleProperty. Sam made this change in http://trac.webkit.org/changeset/181408, probably he knows. Sam, what was the reason behind setting important to true? > > (In reply to comment #28) > > (In reply to comment #27) > > > Taking your above comments into consideration, we only need the > > > updateAutoFillButton() method that looks like: > > > > Yes, that looks right. > > > > > > > Source/WebCore/testing/Internals.idl:64 > > > > > +enum AutoFillButtonType { > > > > > + "AutoFillButtonTypeNone", > > > > > + "AutoFillButtonTypeContacts", > > > > > + "AutoFillButtonTypeCredentials" > > > > > +}; > > > > > > > > I would have just used the strings "none", "credentials", and "contacts". > > > > Not sure why the long strings with capital letters are better. > > > > > > I also prefer shorter strings as long as they are clear. In Internals.idl, > > > there are strings with capital letters and there are strings with lower case > > > for enum, what is the recommended practice for this? > > > > I think there’s some confusion between true enums in C++ code, where the > > values need to avoid conflicting with each other and look right in C++ code > > context, enums in DOM APIs and enums in our internals file. > > > > 1) For enums in C++ code, they need to not collide with each other so we > > either need to prefix values with the type, or use enum class. And we use > > capital letters in those by convention. > > > > 2) For enums in DOM APIs, we need to match the style of the DOM > > specifications, which means we use ALL_CAPITAL_LETTERS style for the values. > > There is also some issue of name conflict there. > > > > 3) For these enums for internals, what we are calling enums is actually just > > a list of string values. There is no need to make them unique, prefix them > > with a type name, or anything else like that. > > > > So for these, in category (3) I think we should use lower case strings. When > > there is more than one word, not sure if we should use spaces (like normal > > English), hyphens (like CSS identifiers), underscores, or capital letters > > (like local variables in C++ code). But really no need to use capital > > letters. > > This is really helpful!

Note You need to log in before you can comment on or make changes to this bug.