Adding Mouse Support for new GTK Autofill

Adding Mouse Support for new GTK Autofill

Adds the ability use the mouse to select an Autofill option and see the preview of an option.

BUG=51644
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121234

[csharp, 2012-01-26 22:19:06]

So I know this code isn't done yet, but I ran into more trouble getting it to
properly work then I had expected.

The basic issue I'm having is figuring out what messages I need to send to the
renderer to properly show the text. I've got something that "works" right now,
but I know there are problems with it.

I was hoping you could give me some pointers on how to clean up the IPC calls (I
think that is where all the trouble is coming from).

Thanks

https://chromiumcodereview.appspot.com/9235072/diff/1/chrome/renderer/autofil...
File chrome/renderer/autofill/autofill_agent.h (right):

https://chromiumcodereview.appspot.com/9235072/diff/1/chrome/renderer/autofil...
chrome/renderer/autofill/autofill_agent.h:108: void OnSetAutofillActionFill();
These functions all just use the currently set autofill_query_element_, is that
accepted or should we be explicitly telling this code which element to work on?
If we go that way do you know if it is possible to get the original web node
from the FormField? (I did a bit of searching and it looks like that isn't
possible right now)

[Ilya Sherman, 2012-01-30 22:11:09]

https://chromiumcodereview.appspot.com/9235072/diff/1/chrome/renderer/autofil...
File chrome/renderer/autofill/autofill_agent.h (right):

https://chromiumcodereview.appspot.com/9235072/diff/1/chrome/renderer/autofil...
chrome/renderer/autofill/autofill_agent.h:108: void OnSetAutofillActionFill();
On 2012/01/26 22:19:06, csharp wrote:
> These functions all just use the currently set autofill_query_element_, is
that
> accepted or should we be explicitly telling this code which element to work
on?

Yeah, that should be fine.  We've generally also used autofill_query_id_ to
ignore out of date responses, so the autofill_query_element_ should always be
the "right" element.

> If we go that way do you know if it is possible to get the original web node
> from the FormField? (I did a bit of searching and it looks like that isn't
> possible right now)

Nope, there's no way that I'm aware of to map from a FormField to the original
web node.  We could pass the raw node pointer to the browser process and pass it
back to the renderer; but that's a little unpleasant, because the raw pointer
does not point to anything meaningful within the browser process.

[Ilya Sherman, 2012-02-01 05:39:34]

From printing a trace while testing the Autofill functionality, it looks like
there are a couple of problems that lead to the weird behavior:
 (1) didAcceptAutofillSuggestions() is called when you click on an item in the
popup; but the form is left in a previewed state if you click off the popup. 
When the popup is hidden, if the suggestion has not already been accepted, we
should cancel the preview.
 (2) suggestions_options_index_ and suggestions_clear_index_ are not initialized
in the constructor of AutofillExternalDelegate, which can cause
AutofillExternalDelegate::SelectAutofillSuggestionAtIndex() to return early when
it oughtn't.

[csharp, 2012-02-03 22:15:09]

Ok, here is the first working draft of mouse support for the new GTK Autofill.

[Ilya Sherman, 2012-02-04 04:10:52]

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_external_delegate.cc:35:
suggestions_clear_index_ == list_index)
nit: This is probably fine as is, but I think we usually write conditionals like
this as "if (list_index == suggestions_options_index || ...)" (so that
list_index is on the left rather than on the right).

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_external_delegate.cc:166: }
Have you verified that this behaves correctly in the case that unique_id is
negative, i.e. if we're showing a warning message?  (It might be that this code
is unreachable in that case, which is fine, but would be even better with a
DCHECK to that effect.)

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
File chrome/browser/autofill/autofill_external_delegate.h (right):

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_external_delegate.h:65: void
didAcceptAutofillSuggestions(
nit: "didAccept..." -> "DidAccept"

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_external_delegate.h:66: string16 value,int
unique_id, unsigned index);
nit: Since this is a header file, the parameters should be listed on separate
lines.

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_external_delegate.h:69: // (i.e. there is now
Autofill selection now).
nit: "there is now" -> "there is no"

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_external_delegate.h:102: void virtual
HideAutofillPopupInternal() = 0;
nit: "void virtual" -> "virtual void"

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_external_delegate.h:106: // if the form is a
preview. If |is_preview| is true we are just showing a
nit: I find this first sentence a bit hard to make sense of.  Perhaps something
like "Fills the form with the Autofill data corresponding to |unique_id|" ?

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
File chrome/browser/autofill/autofill_external_delegate_unittest.cc (right):

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:5: #include
"chrome/browser/autofill/autofill_external_delegate_unittest.h"
Hmm, I don't see this file in the CL -- did you mean to add it or perhaps
#include a different header here?  If this is meant to be used by multiple
tests, it would probably be better to name it slightly differently -- perhaps
"autofill_external_delegate_test_helpers.h" or something along those lines.

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:26: class
AutofillManagerMock : public AutofillManager {
nit: Any reason not to move this into an anonymous namespace?

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:26: class
AutofillManagerMock : public AutofillManager {
nit: "AutofillManagerMock" -> "MockAutofillManager"

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:90:
EXPECT_CALL(external_delegate, HideAutofillPopupInternal());
nit: It's generally better to test the public interface rather than the private
interface -- any reason not to EXPECT_CALL(external_delegate, HideAutofillPopup)
instead?

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
File chrome/browser/autofill/autofill_popup_view.h (right):

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_popup_view.h:40: AutofillExternalDelegate*
external_delegate() { return external_delegate_; }
nit: Can this have protected visibility rather than public visibility?

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
File chrome/browser/autofill/autofill_popup_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:40: }
nit: Is it not ok to just call AutofillPopupView::SetSelectedLine() directly?

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:41: };
nit: Can this class be moved into an anonymous namespace?

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:48: virtual void
SetUpOnMainThread() OVERRIDE{
nit: "OVERRIDE{" -> "OVERRIDE {"

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:101:
std::vector<string16> autofill_value;
nit: Since this is a vector, let's call it |autofill_values|, even though it
only has one element.

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:109: // be updated so
so it is selected.
nit: "so so" -> "to show"?

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/autof...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:114: // Ensure that
the row isn't invalidated if it didn't change.
If you want the test to actually fail if this doesn't happen, you'll need to
call EXPECT_CALL(*autofill_popup_view_, InvalidateRow(_)).Times(0)

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/ui/gt...
File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/ui/gt...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:123: gboolean
AutofillPopupViewGtk::HandleButtonRelease(GtkWidget* widget,
nit: Is there a GTK-specific reason why this function needs to return a boolean?
 It seems to always return true -- is that just to indicate that the event was
handled?  If so... just to check... do we indeed want to indicate that we've
handled all clicks, rather than just left-clicks?

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/ui/gt...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:125: size_t line =
LineFromY(event->y);
nit: DCHECK_LT(line, autofill_values.size());

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/ui/gt...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:125: size_t line =
LineFromY(event->y);
nit: Any reason not to move this work into the body of the if-stmt?

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/ui/gt...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:127: if (event->button
== 1)  // Left click.
nit: Since the body wraps to multiple lines, please add curly braces for this
if-stmt.

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/common/autofi...
File chrome/common/autofill_messages.h (right):

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/common/autofi...
chrome/common/autofill_messages.h:101: // Tells the render that the next form
will be filled for real.
nit: "render" -> "renderer"

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/common/autofi...
chrome/common/autofill_messages.h:104: // Tells the render that the autofill
selection has been cleared.
nit: "autofill" -> "Autofill"

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/common/autofi...
chrome/common/autofill_messages.h:104: // Tells the render that the autofill
selection has been cleared.
It's not clear to me from reading this comment why the renderer needs to know
that the Autofill selection has been cleared.  Perhaps this should be named
something like "AutofillMsg_ClearPreviewedForm" instead?

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/renderer/auto...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/renderer/auto...
chrome/renderer/autofill/autofill_agent.cc:381: was_query_node_autofilled_ =
autofill_query_element_.isAutofilled();
Hmm, can we move this into AutofillAgent::OnFormDataFilled() -- and remove it
from AutofillAgent::FillAutofillFormData() -- or would that cause some sort of
race/inconsistency?

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/renderer/auto...
chrome/renderer/autofill/autofill_agent.cc:385:
didClearAutofillSelection(autofill_query_element_);
Likewise, I think we could move this into the AUTOFILL_PREVIEW case in
AutofillAgent::OnFormDataFilled() -- I think that would make more sense,
conceptually...  WDYT?

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/renderer/auto...
chrome/renderer/autofill/autofill_agent.cc:400: // TODO add a check that we are
setting the correct element
nit: "TODO" -> "TODO(csharp):"

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/renderer/auto...
chrome/renderer/autofill/autofill_agent.cc:400: // TODO add a check that we are
setting the correct element
What sort of check are you thinking of?

[csharp, 2012-02-07 22:30:58]

I didn't yet get a chance to play around with the old autofill model to make
sure that I didn't break it in a weird way by moving some of its code around.
I'll try that tomorrow morning.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.cc:35:
suggestions_clear_index_ == list_index)
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: This is probably fine as is, but I think we usually write conditionals
like
> this as "if (list_index == suggestions_options_index || ...)" (so that
> list_index is on the left rather than on the right).

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.cc:166: }
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> Have you verified that this behaves correctly in the case that unique_id is
> negative, i.e. if we're showing a warning message?  (It might be that this
code
> is unreachable in that case, which is fine, but would be even better with a
> DCHECK to that effect.)

I had forgotten about the warning message case. Added a test case and an early
check in this function that just returns when there are negative unique ids.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_external_delegate.h (right):

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.h:65: void
didAcceptAutofillSuggestions(
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: "didAccept..." -> "DidAccept"

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.h:66: string16 value,int
unique_id, unsigned index);
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: Since this is a header file, the parameters should be listed on separate
> lines.

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.h:69: // (i.e. there is now
Autofill selection now).
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: "there is now" -> "there is no"

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.h:102: void virtual
HideAutofillPopupInternal() = 0;
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: "void virtual" -> "virtual void"

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate.h:106: // if the form is a
preview. If |is_preview| is true we are just showing a
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: I find this first sentence a bit hard to make sense of.  Perhaps
something
> like "Fills the form with the Autofill data corresponding to |unique_id|" ?

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_external_delegate_unittest.cc (right):

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:5: #include
"chrome/browser/autofill/autofill_external_delegate_unittest.h"
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> Hmm, I don't see this file in the CL -- did you mean to add it or perhaps
> #include a different header here?  If this is meant to be used by multiple
> tests, it would probably be better to name it slightly differently -- perhaps
> "autofill_external_delegate_test_helpers.h" or something along those lines.

Ya, I missed including this file. I changed it up a bit to just be a basic
AutofilExternalDelegate with all the pure virtual functions filled in to
removing the repeated code redeclaring those as empty functions.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:26: class
AutofillManagerMock : public AutofillManager {
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: "AutofillManagerMock" -> "MockAutofillManager"

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:26: class
AutofillManagerMock : public AutofillManager {
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: Any reason not to move this into an anonymous namespace?

Nope, moved.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:90:
EXPECT_CALL(external_delegate, HideAutofillPopupInternal());
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: It's generally better to test the public interface rather than the
private
> interface -- any reason not to EXPECT_CALL(external_delegate,
HideAutofillPopup)
> instead?

Nope, fixed.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_popup_view.h (right):

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_popup_view.h:40: AutofillExternalDelegate*
external_delegate() { return external_delegate_; }
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: Can this have protected visibility rather than public visibility?

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_popup_view_browsertest.cc (right):

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:40: }
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: Is it not ok to just call AutofillPopupView::SetSelectedLine() directly?

Nope, because it is a protected member.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:41: };
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: Can this class be moved into an anonymous namespace?

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:48: virtual void
SetUpOnMainThread() OVERRIDE{
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: "OVERRIDE{" -> "OVERRIDE {"

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:101:
std::vector<string16> autofill_value;
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: Since this is a vector, let's call it |autofill_values|, even though it
> only has one element.

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:109: // be updated so
so it is selected.
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: "so so" -> "to show"?

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:114: // Ensure that
the row isn't invalidated if it didn't change.
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> If you want the test to actually fail if this doesn't happen, you'll need to
> call EXPECT_CALL(*autofill_popup_view_, InvalidateRow(_)).Times(0)

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/ui/gtk/autofi...
File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right):

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/ui/gtk/autofi...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:123: gboolean
AutofillPopupViewGtk::HandleButtonRelease(GtkWidget* widget,
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: Is there a GTK-specific reason why this function needs to return a
boolean?
>  It seems to always return true -- is that just to indicate that the event was
> handled?  If so... just to check... do we indeed want to indicate that we've
> handled all clicks, rather than just left-clicks?

I've changed it to ignore other clicks. I noticed that the current autofill
seems to consider any click a valid selection click (right, left and center seem
to all have the same result). 

Should the new UI duplicate this behaviour? Should I just drop the check for
left clicks?

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/ui/gtk/autofi...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:125: size_t line =
LineFromY(event->y);
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: Any reason not to move this work into the body of the if-stmt?

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/browser/ui/gtk/autofi...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:127: if (event->button
== 1)  // Left click.
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: Since the body wraps to multiple lines, please add curly braces for this
> if-stmt.

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/common/autofill_messa...
File chrome/common/autofill_messages.h (right):

http://codereview.chromium.org/9235072/diff/5001/chrome/common/autofill_messa...
chrome/common/autofill_messages.h:101: // Tells the render that the next form
will be filled for real.
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: "render" -> "renderer"

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/common/autofill_messa...
chrome/common/autofill_messages.h:104: // Tells the render that the autofill
selection has been cleared.
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> It's not clear to me from reading this comment why the renderer needs to know
> that the Autofill selection has been cleared.  Perhaps this should be named
> something like "AutofillMsg_ClearPreviewedForm" instead?

Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/renderer/autofill/aut...
File chrome/renderer/autofill/autofill_agent.cc (right):

http://codereview.chromium.org/9235072/diff/5001/chrome/renderer/autofill/aut...
chrome/renderer/autofill/autofill_agent.cc:381: was_query_node_autofilled_ =
autofill_query_element_.isAutofilled();
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> Hmm, can we move this into AutofillAgent::OnFormDataFilled() -- and remove it
> from AutofillAgent::FillAutofillFormData() -- or would that cause some sort of
> race/inconsistency?  

I don't think this can cause any trouble. The only problem I could see is that
the autofill_query_element_ gets out of sync with the node we are trying to work
on (which I don't think can happen). didAcceptAutofillSuggestions already had a
check they they were equal when the user is trying to clear the form, so I just
moved that check outside of the if, so they should be equal when it matters.

http://codereview.chromium.org/9235072/diff/5001/chrome/renderer/autofill/aut...
chrome/renderer/autofill/autofill_agent.cc:385:
didClearAutofillSelection(autofill_query_element_);
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> Likewise, I think we could move this into the AUTOFILL_PREVIEW case in
> AutofillAgent::OnFormDataFilled() -- I think that would make more sense,
> conceptually...  WDYT?

Seems like a good idea. Done.

http://codereview.chromium.org/9235072/diff/5001/chrome/renderer/autofill/aut...
chrome/renderer/autofill/autofill_agent.cc:400: // TODO add a check that we are
setting the correct element
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> What sort of check are you thinking of?

I had been thinking of making sure that autofill_query_element_ is the correct
element, but as early in this review it should also be the right element, so I'm
removing this todo.

[Ilya Sherman, 2012-02-07 23:35:55]

LGTM

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/ui/gt...
File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/ui/gt...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:123: gboolean
AutofillPopupViewGtk::HandleButtonRelease(GtkWidget* widget,
On 2012/02/07 22:30:58, csharp wrote:
> On 2012/02/04 04:10:52, Ilya Sherman wrote:
> > nit: Is there a GTK-specific reason why this function needs to return a
> boolean?
> >  It seems to always return true -- is that just to indicate that the event
was
> > handled?  If so... just to check... do we indeed want to indicate that we've
> > handled all clicks, rather than just left-clicks?
> 
> I've changed it to ignore other clicks. I noticed that the current autofill
> seems to consider any click a valid selection click (right, left and center
seem
> to all have the same result). 
> 
> Should the new UI duplicate this behaviour? Should I just drop the check for
> left clicks?

I think the new UI should behave like a system-native popup would -- which I'm
guessing means ignoring non-left clicks.

https://chromiumcodereview.appspot.com/9235072/diff/10001/chrome/browser/auto...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

https://chromiumcodereview.appspot.com/9235072/diff/10001/chrome/browser/auto...
chrome/browser/autofill/autofill_external_delegate.cc:145: // anything.
nit: "has a negative unique id" -> "is a warning"

https://chromiumcodereview.appspot.com/9235072/diff/10001/chrome/browser/auto...
File chrome/browser/autofill/autofill_external_delegate_test.h (right):

https://chromiumcodereview.appspot.com/9235072/diff/10001/chrome/browser/auto...
chrome/browser/autofill/autofill_external_delegate_test.h:10: #include
"testing/gmock/include/gmock/gmock.h"
nit: Looks like this isn't used, and it's a pretty expensive class to compile &
link.  (I think Nico once managed to improve the overall link-time memory
consumption when compiling unit tests by something like 25% just by fixing up
gmock.h includes.)

https://chromiumcodereview.appspot.com/9235072/diff/10001/chrome/browser/auto...
chrome/browser/autofill/autofill_external_delegate_test.h:18: class
AutofillExternalDelegateTest : public AutofillExternalDelegate {
nit: Generally a suffix of "Test" means the class is a test fixture class,
derived from testing::Test.  For testing versions of classes, we generally
prefix "Test" rather than suffix it.

https://chromiumcodereview.appspot.com/9235072/diff/10001/chrome/browser/auto...
File chrome/browser/autofill/autofill_popup_view.cc (right):

https://chromiumcodereview.appspot.com/9235072/diff/10001/chrome/browser/auto...
chrome/browser/autofill/autofill_popup_view.cc:65: if (selected_line_ != -1)
nit: Please add curly braces, since the body of this if-stmt is more than a
single line.

https://chromiumcodereview.appspot.com/9235072/diff/10001/chrome/browser/auto...
File chrome/browser/autofill/autofill_popup_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9235072/diff/10001/chrome/browser/auto...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:81: // Make this just
a member
nit: Is this meant as a TODO?

https://chromiumcodereview.appspot.com/9235072/diff/10001/chrome/renderer/aut...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/9235072/diff/10001/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:168: element.setValue(substring,
true);
nit: This code is essentially repeated in OnSetNodeText() -- please factor out a
shared method (or just call into OnSetNodeText() directly here).

[Elliot Glaysher, 2012-02-07 23:44:09]

the gtk part doesn't look like it's problematic and therefore lgtm.

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/ui/gt...
File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/9235072/diff/5001/chrome/browser/ui/gt...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:123: gboolean
AutofillPopupViewGtk::HandleButtonRelease(GtkWidget* widget,
On 2012/02/04 04:10:52, Ilya Sherman wrote:
> nit: Is there a GTK-specific reason why this function needs to return a
boolean?
>  It seems to always return true -- is that just to indicate that the event was
> handled?  If so... just to check... do we indeed want to indicate that we've
> handled all clicks, rather than just left-clicks?

Yes. The return value specifies whether other signal listeners (including the
default one) get to handle the event.

[csharp, 2012-02-08 16:11:16]

http://codereview.chromium.org/9235072/diff/10001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

http://codereview.chromium.org/9235072/diff/10001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_external_delegate.cc:145: // anything.
On 2012/02/07 23:35:55, Ilya Sherman wrote:
> nit: "has a negative unique id" -> "is a warning"

Done.

http://codereview.chromium.org/9235072/diff/10001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_external_delegate_test.h (right):

http://codereview.chromium.org/9235072/diff/10001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_external_delegate_test.h:10: #include
"testing/gmock/include/gmock/gmock.h"
On 2012/02/07 23:35:55, Ilya Sherman wrote:
> nit: Looks like this isn't used, and it's a pretty expensive class to compile
&
> link.  (I think Nico once managed to improve the overall link-time memory
> consumption when compiling unit tests by something like 25% just by fixing up
> gmock.h includes.)

Done.

http://codereview.chromium.org/9235072/diff/10001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_external_delegate_test.h:18: class
AutofillExternalDelegateTest : public AutofillExternalDelegate {
On 2012/02/07 23:35:55, Ilya Sherman wrote:
> nit: Generally a suffix of "Test" means the class is a test fixture class,
> derived from testing::Test.  For testing versions of classes, we generally
> prefix "Test" rather than suffix it.

Done.

http://codereview.chromium.org/9235072/diff/10001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_popup_view.cc (right):

http://codereview.chromium.org/9235072/diff/10001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_popup_view.cc:65: if (selected_line_ != -1)
On 2012/02/07 23:35:55, Ilya Sherman wrote:
> nit: Please add curly braces, since the body of this if-stmt is more than a
> single line.

Done.

http://codereview.chromium.org/9235072/diff/10001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_popup_view_browsertest.cc (right):

http://codereview.chromium.org/9235072/diff/10001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_popup_view_browsertest.cc:81: // Make this just
a member
On 2012/02/07 23:35:55, Ilya Sherman wrote:
> nit: Is this meant as a TODO?
I had make this change before uploading, I must have forgotten to save. Done

http://codereview.chromium.org/9235072/diff/10001/chrome/renderer/autofill/au...
File chrome/renderer/autofill/autofill_agent.cc (right):

http://codereview.chromium.org/9235072/diff/10001/chrome/renderer/autofill/au...
chrome/renderer/autofill/autofill_agent.cc:168: element.setValue(substring,
true);
On 2012/02/07 23:35:55, Ilya Sherman wrote:
> nit: This code is essentially repeated in OnSetNodeText() -- please factor out
a
> shared method (or just call into OnSetNodeText() directly here).

Done.

[Ilya Sherman, 2012-02-08 17:01:57]

(Still LGTM)

https://chromiumcodereview.appspot.com/9235072/diff/12005/chrome/renderer/aut...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/9235072/diff/12005/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:396: void
AutofillAgent::OnSetNodeText(string16 value) {
nit: Might as well pass the |value| by const-reference.

https://chromiumcodereview.appspot.com/9235072/diff/12005/chrome/renderer/aut...
File chrome/renderer/autofill/autofill_agent.h (right):

https://chromiumcodereview.appspot.com/9235072/diff/12005/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.h:155: void
SetNodeText(WebKit::WebInputElement node, string16 value);
nit: Might as well pass the |node| and the |value| by const-reference.

[csharp, 2012-02-08 19:05:43]

http://codereview.chromium.org/9235072/diff/12005/chrome/renderer/autofill/au...
File chrome/renderer/autofill/autofill_agent.cc (right):

http://codereview.chromium.org/9235072/diff/12005/chrome/renderer/autofill/au...
chrome/renderer/autofill/autofill_agent.cc:396: void
AutofillAgent::OnSetNodeText(string16 value) {
On 2012/02/08 17:01:57, Ilya Sherman wrote:
> nit: Might as well pass the |value| by const-reference.

Done.

http://codereview.chromium.org/9235072/diff/12005/chrome/renderer/autofill/au...
File chrome/renderer/autofill/autofill_agent.h (right):

http://codereview.chromium.org/9235072/diff/12005/chrome/renderer/autofill/au...
chrome/renderer/autofill/autofill_agent.h:155: void
SetNodeText(WebKit::WebInputElement node, string16 value);
On 2012/02/08 17:01:57, Ilya Sherman wrote:
> nit: Might as well pass the |node| and the |value| by const-reference.

changed value, but we modify node so it can't change

[Ilya Sherman, 2012-02-08 19:23:55]

http://codereview.chromium.org/9235072/diff/12005/chrome/renderer/autofill/au...
File chrome/renderer/autofill/autofill_agent.h (right):

http://codereview.chromium.org/9235072/diff/12005/chrome/renderer/autofill/au...
chrome/renderer/autofill/autofill_agent.h:155: void
SetNodeText(WebKit::WebInputElement node, string16 value);
On 2012/02/08 19:05:43, csharp wrote:
> On 2012/02/08 17:01:57, Ilya Sherman wrote:
> > nit: Might as well pass the |node| and the |value| by const-reference.
> 
> changed value, but we modify node so it can't change

In that case we should pass it as a pointer, and reverse the order of the
arguments (since the style guide specifies that outparams should appear at the
end of the list of params)... though it's interesting that the node is passed by
const-reference into AutofillAgent::didAcceptAutofillSuggestion...

[csharp, 2012-02-08 20:11:40]

http://codereview.chromium.org/9235072/diff/12005/chrome/renderer/autofill/au...
File chrome/renderer/autofill/autofill_agent.h (right):

http://codereview.chromium.org/9235072/diff/12005/chrome/renderer/autofill/au...
chrome/renderer/autofill/autofill_agent.h:155: void
SetNodeText(WebKit::WebInputElement node, string16 value);
On 2012/02/08 19:23:55, Ilya Sherman wrote:
> On 2012/02/08 19:05:43, csharp wrote:
> > On 2012/02/08 17:01:57, Ilya Sherman wrote:
> > > nit: Might as well pass the |node| and the |value| by const-reference.
> > 
> > changed value, but we modify node so it can't change
> 
> In that case we should pass it as a pointer, and reverse the order of the
> arguments (since the style guide specifies that outparams should appear at the
> end of the list of params)... though it's interesting that the node is passed
by
> const-reference into AutofillAgent::didAcceptAutofillSuggestion...

Done.

[Ilya Sherman, 2012-02-08 20:23:11]

lg, thanks

[commit-bot: I haz the power, 2012-02-09 13:40:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/9235072/10012

[commit-bot: I haz the power, 2012-02-09 15:21:56]

Change committed as 121234