Bug 83777 - Introduce MenuItemID to autofill popup
Summary: Introduce MenuItemID to autofill popup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks: 83742
  Show dependency treegraph
 
Reported: 2012-04-12 06:19 PDT by Keishi Hattori
Modified: 2012-04-16 14:29 PDT (History)
7 users (show)

See Also:


Attachments
Patch (16.72 KB, patch)
2012-04-12 06:46 PDT, Keishi Hattori
fishd: review-
Details | Formatted Diff | Diff
Added argument with default value so we don't break the build (19.59 KB, patch)
2012-04-13 01:03 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Fixed mistake (16.71 KB, patch)
2012-04-13 01:08 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (19.29 KB, patch)
2012-04-13 06:30 PDT, Keishi Hattori
tkent: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Rebased (19.50 KB, patch)
2012-04-15 19:29 PDT, Keishi Hattori
tkent: commit-queue-
Details | Formatted Diff | Diff
Fixed ChangeLog (19.24 KB, patch)
2012-04-15 21:22 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2012-04-12 06:19:00 PDT
Introduce MenuItemID so we can add multiple separators to a menu.
Comment 1 Keishi Hattori 2012-04-12 06:46:59 PDT
Created attachment 136895 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-12 06:53:03 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Darin Fisher (:fishd, Google) 2012-04-12 14:00:44 PDT
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?
Comment 4 Kent Tamura 2012-04-12 19:31:09 PDT
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?
Comment 5 Keishi Hattori 2012-04-13 01:03:20 PDT
Created attachment 137054 [details]
Added argument with default value so we don't break the build
Comment 6 WebKit Review Bot 2012-04-13 01:05:42 PDT
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.
Comment 7 Keishi Hattori 2012-04-13 01:08:55 PDT
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.
Comment 8 Kent Tamura 2012-04-13 01:30:24 PDT
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?
Comment 9 Keishi Hattori 2012-04-13 06:30:33 PDT
Created attachment 137077 [details]
Patch
Comment 10 Keishi Hattori 2012-04-13 06:33:31 PDT
(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.
Comment 11 Kent Tamura 2012-04-13 17:37:48 PDT
Comment on attachment 137077 [details]
Patch

ok, it looks reasonable.
Comment 12 WebKit Review Bot 2012-04-13 21:39:24 PDT
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
Comment 13 Keishi Hattori 2012-04-15 19:29:53 PDT
Created attachment 137266 [details]
Rebased
Comment 14 Kent Tamura 2012-04-15 21:13:27 PDT
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  <keishi@webkit.org>
> +2012-04-12  Keishi Hattori  <keishi@webkit.org>
>  

Do not change the existing ChangeLog entry.
Comment 15 Keishi Hattori 2012-04-15 21:22:51 PDT
Created attachment 137274 [details]
Fixed ChangeLog
Comment 16 WebKit Review Bot 2012-04-15 22:22:14 PDT
Comment on attachment 137274 [details]
Fixed ChangeLog

Clearing flags on attachment: 137274

Committed r114223: <http://trac.webkit.org/changeset/114223>
Comment 17 WebKit Review Bot 2012-04-15 22:22:21 PDT
All reviewed patches have been landed.  Closing bug.