Bug 83777 – Introduce MenuItemID to autofill popup

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Keishi Hattori]

Introduce MenuItemID so we can add multiple separators to a menu.

[Keishi Hattori]

Created attachment 136895 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Darin Fisher (:fishd, Google)]

Comment on attachment 136895 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136895&action=review

> Source/WebKit/chromium/public/WebAutofillClient.h:43
> +    enum {

nit: we usually name enums, and then we use the enum name
as the typename elsewhere.

shouldn't didAcceptAutofillSuggestion use a named enum type
for the itemID parameter?

please also see:
http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums

> Source/WebKit/chromium/public/WebView.h:364
> +        const WebVector<int>& itemIDs) = 0;

Chrome doesn't yet call this function?  It is easy to land without breaking Chrome?

[Kent Tamura]

Comment on attachment 136895 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136895&action=review

>> Source/WebKit/chromium/public/WebAutofillClient.h:43
>> +    enum {
> 
> nit: we usually name enums, and then we use the enum name
> as the typename elsewhere.
> 
> shouldn't didAcceptAutofillSuggestion use a named enum type
> for the itemID parameter?
> 
> please also see:
> http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums

In my understanding, we usually use positive numbers for the ID and these symbols are special values.  Maybe we shouldn't use the enum name as the type?

[Keishi Hattori]

Created attachment 137054 [details]
Added argument with default value so we don't break the build

[WebKit Review Bot]

Attachment 137054 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1
Source/WebKit/chromium/public/WebInputElement.h:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keishi Hattori]

Created attachment 137056 [details]
Fixed mistake

> (From update of attachment 136895 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136895&action=review
> 
> > Source/WebKit/chromium/public/WebAutofillClient.h:43
> > +    enum {
> 
> nit: we usually name enums, and then we use the enum name
> as the typename elsewhere.
> 
> shouldn't didAcceptAutofillSuggestion use a named enum type
> for the itemID parameter?
> 
> please also see:
> http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums

Positive itemID are unique ids used to identify the autofill item. And the enum values are used for zero and negative itemIDs.

@fishd should I still use the enum name?

> > Source/WebKit/chromium/public/WebView.h:364
> > +        const WebVector<int>& itemIDs) = 0;
> 
> Chrome doesn't yet call this function?  It is easy to land without breaking Chrome?

I added a default value to the argument I want to remove, so we can transition without breaking the build.

[Kent Tamura]

Comment on attachment 137056 [details]
Fixed mistake

View in context: https://bugs.webkit.org/attachment.cgi?id=137056&action=review

> Source/WebKit/chromium/public/WebAutofillClient.h:45
> +        AutocompleteEntryMenuItemID = 0,
> +        WarningMessageMenuItemID = -1,

Anyway, we should name them like MenuItemIDAutocompleEntry, MenuItemIDWarningMessage, ....

> Source/WebKit/chromium/public/WebView.h:365
>          const WebVector<WebString>& names,
>          const WebVector<WebString>& labels,
>          const WebVector<WebString>& icons,
> -        const WebVector<int>& uniqueIDs,
> -        int separatorIndex) = 0;
> +        const WebVector<int>& itemIDs,
> +        int separatorIndex = -1) = 0;

You need to update the comment of this function.

The build issue can be avoided by the default argument, but I'm afraid this change will make problems because the expected content of the uniqueIDs/itemIDs is changed.
The current callsites of this function doesn't put SeparatorMenuItemID in uniqueIDs, right?

[Keishi Hattori]

Created attachment 137077 [details]
Patch

[Keishi Hattori]

(In reply to comment #8)
> (From update of attachment 137056 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137056&action=review
> 
> > Source/WebKit/chromium/public/WebAutofillClient.h:45
> > +        AutocompleteEntryMenuItemID = 0,
> > +        WarningMessageMenuItemID = -1,
> 
> Anyway, we should name them like MenuItemIDAutocompleEntry, MenuItemIDWarningMessage, ....
Done.

> > Source/WebKit/chromium/public/WebView.h:365
> >          const WebVector<WebString>& names,
> >          const WebVector<WebString>& labels,
> >          const WebVector<WebString>& icons,
> > -        const WebVector<int>& uniqueIDs,
> > -        int separatorIndex) = 0;
> > +        const WebVector<int>& itemIDs,
> > +        int separatorIndex = -1) = 0;
> 
> You need to update the comment of this function.
> 
> The build issue can be avoided by the default argument, but I'm afraid this change will make problems because the expected content of the uniqueIDs/itemIDs is changed.
> The current callsites of this function doesn't put SeparatorMenuItemID in uniqueIDs, right?

Yes it won't break the build but it will break autofill. So, for this new patch I made it so it will switch between the old behavior and the new one depending on the separatorIndex passed from the chromium side. Autofill will work fine before and after we land the chromium side of the patch.

[Kent Tamura]

Comment on attachment 137077 [details]
Patch

ok, it looks reasonable.

[WebKit Review Bot]

Comment on attachment 137077 [details]
Patch

Rejecting attachment 137077 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: ChangeLog entry in Source/WebKit/chromium/ChangeLog is not at the top of the file.

Full output: http://queues.webkit.org/results/12408066

[Keishi Hattori]

Created attachment 137266 [details]
Rebased

[Kent Tamura]

Comment on attachment 137266 [details]
Rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=137266&action=review

> Source/WebKit/chromium/ChangeLog:213
>  
> -2012-04-13  Keishi Hattori  <[e-mail]>
> [phone]  Keishi Hattori  <[e-mail]>
>  

Do not change the existing ChangeLog entry.

[Keishi Hattori]

Created attachment 137274 [details]
Fixed ChangeLog

[WebKit Review Bot]

Comment on attachment 137274 [details]
Fixed ChangeLog

Clearing flags on attachment: 137274

Committed r114223: <http://trac.webkit.org/changeset/114223>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.