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

Tue Jan 19 11:00:35 PST 2016

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

--- Comment #12 from Zach Li <zacharyli323 at gmail.com> ---
(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!

-- 
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/a476e093/attachment-0001.html>