Bug 83777

Created attachment 136895 [details] Patch

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 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 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?

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

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.

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 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?

Created attachment 137077 [details] Patch

(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 on attachment 137077 [details] Patch ok, it looks reasonable.

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

Created attachment 137266 [details] Rebased

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.

Created attachment 137274 [details] Fixed ChangeLog

Comment on attachment 137274 [details] Fixed ChangeLog Clearing flags on attachment: 137274 Committed r114223: <http://trac.webkit.org/changeset/114223>

All reviewed patches have been landed. Closing bug.