Bug 39639 - [Chromium] Implement new PopupMenuClient methods for AutoFill.
Summary: [Chromium] Implement new PopupMenuClient methods for AutoFill.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-24 19:12 PDT by James Hawkins
Modified: 2010-06-12 19:46 PDT (History)
4 users (show)

See Also:


Attachments
Patch (11.57 KB, patch)
2010-05-24 19:13 PDT, James Hawkins
no flags Details | Formatted Diff | Diff
Patch (11.74 KB, patch)
2010-06-01 13:34 PDT, James Hawkins
no flags Details | Formatted Diff | Diff
Patch (11.66 KB, patch)
2010-06-01 13:52 PDT, James Hawkins
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Hawkins 2010-05-24 19:12:18 PDT
Patch to be uploaded.
Comment 1 James Hawkins 2010-05-24 19:13:52 PDT
Created attachment 56959 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 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.
Comment 3 James Hawkins 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.
Comment 4 James Hawkins 2010-06-01 13:34:32 PDT
Created attachment 57588 [details]
Patch
Comment 5 Eric Seidel (no email) 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
Comment 6 James Hawkins 2010-06-01 13:52:50 PDT
Created attachment 57592 [details]
Patch
Comment 7 James Hawkins 2010-06-01 13:53:29 PDT
Removed param names from the methods that left the params unused -> build fix.
Comment 8 James Hawkins 2010-06-01 16:30:12 PDT
Committed r60514: <http://trac.webkit.org/changeset/60514>
Comment 9 WebKit Review Bot 2010-06-01 16:47:06 PDT
http://trac.webkit.org/changeset/60514 might have broken Chromium Win Release
Comment 10 Kent Tamura 2010-06-12 19:46:07 PDT
Comment on attachment 57592 [details]
Patch

This was already committed, right?  Clearing the review flag.