RESOLVED FIXED 39639
[Chromium] Implement new PopupMenuClient methods for AutoFill. 
https://bugs.webkit.org/show_bug.cgi?id=39639
Summary [Chromium] Implement new PopupMenuClient methods for AutoFill.
James Hawkins
Reported 2010-05-24 19:12:18 PDT
Patch to be uploaded.
Attachments
Patch (11.57 KB, patch)
2010-05-24 19:13 PDT, James Hawkins 		no flags
DetailsFormatted DiffDiff
Patch (11.74 KB, patch)
2010-06-01 13:34 PDT, James Hawkins 		no flags
DetailsFormatted DiffDiff
Patch (11.66 KB, patch)
2010-06-01 13:52 PDT, James Hawkins 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
James Hawkins
Comment 1 2010-05-24 19:13:52 PDT
Created attachment 56959 [details] Patch
Darin Fisher (:fishd, Google)
Comment 2 2010-05-28 10:10:58 PDT
Comment on attachment 56959 [details] Patch WebCore/rendering/RenderMenuList.h:104 > + virtual void selectionChanged(unsigned, bool) {} nit: please give these parameters some names. the type names alone do not adequately describe their purpose. WebCore/rendering/RenderTextControlSingleLine.h:111 > + virtual void selectionChanged(unsigned, bool) {} nit: please give these parameters some names. the type names alone do not adequately describe their purpose. WebKit/chromium/src/AutoFillPopupMenuClient.cpp:102 + // FIXME: Refactor this method. nit: please be a bit more descriptive with this comment. if someone else is reading this, it'd be great to not have to guess what you had in mind. i'm not sure what you intend to refactor ;-) WebKit/chromium/src/SuggestionsPopupMenuClient.h:64 + virtual void selectionChanged(unsigned, bool) {} nit: please give these parameters some names... otherwise, LGTM.
James Hawkins
Comment 3 2010-06-01 13:26:38 PDT
(In reply to comment #2) > (From update of attachment 56959 [details]) > WebCore/rendering/RenderMenuList.h:104 > > + virtual void selectionChanged(unsigned, bool) {} > nit: please give these parameters some names. the type names alone > do not adequately describe their purpose. > Done. > WebCore/rendering/RenderTextControlSingleLine.h:111 > > + virtual void selectionChanged(unsigned, bool) {} > nit: please give these parameters some names. the type names alone > do not adequately describe their purpose. > Done. > WebKit/chromium/src/AutoFillPopupMenuClient.cpp:102 > + // FIXME: Refactor this method. > nit: please be a bit more descriptive with this comment. if someone else > is reading this, it'd be great to not have to guess what you had in mind. > i'm not sure what you intend to refactor ;-) > Done. > WebKit/chromium/src/SuggestionsPopupMenuClient.h:64 > + virtual void selectionChanged(unsigned, bool) {} > nit: please give these parameters some names... > > otherwise, LGTM. Done.
James Hawkins
Comment 4 2010-06-01 13:34:32 PDT
Created attachment 57588 [details] Patch
Eric Seidel (no email)
Comment 5 2010-06-01 13:41:33 PDT
Attachment 57588 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/2795065
James Hawkins
Comment 6 2010-06-01 13:52:50 PDT
Created attachment 57592 [details] Patch
James Hawkins
Comment 7 2010-06-01 13:53:29 PDT
Removed param names from the methods that left the params unused -> build fix.
James Hawkins
Comment 8 2010-06-01 16:30:12 PDT
Committed r60514: <http://trac.webkit.org/changeset/60514>
WebKit Review Bot
Comment 9 2010-06-01 16:47:06 PDT
http://trac.webkit.org/changeset/60514 might have broken Chromium Win Release
Kent Tamura
Comment 10 2010-06-12 19:46:07 PDT
Comment on attachment 57592 [details] Patch This was already committed, right? Clearing the review flag.
Note You need to log in before you can comment on or make changes to this bug.