<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#c11">Comment # 11</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>(In reply to <a href="show_bug.cgi?id=153116#c7">comment #7</a>)
<span class="quote">> (In reply to <a href="show_bug.cgi?id=153116#c4">comment #4</a>)
> > 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.
>
> 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 >
I know that test added in the patch for <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">bug #142564</a> did not include a doctype declaration. Although that test and the test included in your patch (<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>) 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.
<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.
>
> Do you mean I should do something like:
>
> enum class AutoFillButtonType : uint32_t {
> Credentials,
> AddressBook
> };
> </span >
Yes, though please indent the contents of the enum class.
<span class="quote">> 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?
> </span >
I am assuming you mean Internals.cpp. What is the compiler error that you got?
<span class="quote">> 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 >
OK.
<span class="quote">> >
> > 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?
>
> ></span >
OK, leave the definition in HTMLTextFormControlElement.h.
<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;
>
> So here we are not using enum class anymore?
> </span >
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.
<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.
>
> Forward declaring AutoFillButtonType means I should declare enum
> AutoFillButtonType before the class declaration so that I don't need to
> include HTMLTextFormControlElement.h?</span >
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;</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>