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

Tue Jan 19 10:18:26 PST 2016

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

--- Comment #11 from Daniel Bates <dbates at webkit.org> ---
(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) 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;

-- 
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/20160119/9d284f34/attachment-0001.html>