Add "previewing on hover" support for single-field autocomplete input

Add "previewing on hover" support for single-field autocomplete input

In terms of previewing on hover, treat single-field autocomplete
the same as Autofill. Functions are introduced to handle when an
AutoComplete selection is hovered over and clear once preview is done.

This fix only handles autocomplete field. Preview support for password
autofill support mentioned in the bug descriptions will be handled in a
separate commit.

BUG=63421
R=estade@chromium.org, isherman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257274

[Evan Stade, 2014-01-27 17:43:16]

I don't really get why we'd bother showing a preview for a single field
autocomplete. The dropdown already shows you what will be filled in. Autofill is
different because without the preview you can't see 100% of the data that will
be filled in or where in the form it will go.

https://codereview.chromium.org/148413002/diff/1/chrome/browser/ui/autofill/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/148413002/diff/1/chrome/browser/ui/autofill/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2242: // TODO:
inline with ClearPreviewedForm() implementation.
TODOs need to have your name on them. That said, the new Autocomplete functions
don't make sense for the AutofillDialog. I think it's an intentional no-op, or
there should be a default no-op implementation.

[ziran.sun, 2014-01-27 18:18:05]

updated code as per review comments.

https://codereview.chromium.org/148413002/diff/1/chrome/browser/ui/autofill/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/148413002/diff/1/chrome/browser/ui/autofill/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2242: // TODO:
inline with ClearPreviewedForm() implementation.
On 2014/01/27 17:43:17, Evan Stade wrote:
> TODOs need to have your name on them. That said, the new Autocomplete
functions
> don't make sense for the AutofillDialog. I think it's an intentional no-op, or
> there should be a default no-op implementation.

Done.

[Ilya Sherman, 2014-02-14 02:22:28]

https://chromiumcodereview.appspot.com/148413002/diff/70001/chrome/browser/ui...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/148413002/diff/70001/chrome/browser/ui...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:452:
(selected_line_ < static_cast<int>(names_.size())))
nit: No need for the extra parens around the components of the conditional
expression in this if-stmt.

https://chromiumcodereview.appspot.com/148413002/diff/70001/chrome/browser/ui...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:452:
(selected_line_ < static_cast<int>(names_.size())))
Why do you need the second check?  When can selected_line_ be out-of-bounds?

https://chromiumcodereview.appspot.com/148413002/diff/70001/chrome/browser/ui...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:453:
delegate_->DidSelectAutocompleteSuggestion(names_[selected_line_]);
Rather than introducing a new method, it seems reasonable to parallel
DidAcceptSugestion(), which takes both the value and the identifier as a
parameter.

https://chromiumcodereview.appspot.com/148413002/diff/70001/chrome/browser/ui...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:458:
delegate_->ClearAutocompletePreviewedField();
Why can't ClearPreviewedForm() just handle clearing all previews?

https://chromiumcodereview.appspot.com/148413002/diff/70001/components/autofi...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://chromiumcodereview.appspot.com/148413002/diff/70001/components/autofi...
components/autofill/core/browser/autofill_external_delegate.cc:182:
autofill_driver_->RendererShouldSetNodeText(value);
This is incorrect.  The renderer should preview the text, not set it.  The
difference is essentially in whether the change is visible to JavaScript, though
this also makes it easier to restore the previous value prior to mouse-hover.

https://chromiumcodereview.appspot.com/148413002/diff/70001/components/autofi...
components/autofill/core/browser/autofill_external_delegate.cc:230:
autofill_driver_->RendererShouldSetNodeText(ASCIIToUTF16(""));
This is incorrect.  The field value shouldn't be cleared; it should return to
whatever it was prior to being previewed.

[ziran.sun, 2014-02-20 09:37:50]

On 2014/02/14 02:22:28, Ilya Sherman wrote:
>
https://chromiumcodereview.appspot.com/148413002/diff/70001/chrome/browser/ui...
> File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/148413002/diff/70001/chrome/browser/ui...
> chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:452:
> (selected_line_ < static_cast<int>(names_.size())))
> nit: No need for the extra parens around the components of the conditional
> expression in this if-stmt.
> 
>
https://chromiumcodereview.appspot.com/148413002/diff/70001/chrome/browser/ui...
> chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:452:
> (selected_line_ < static_cast<int>(names_.size())))
> Why do you need the second check?  When can selected_line_ be out-of-bounds?
> 
>
https://chromiumcodereview.appspot.com/148413002/diff/70001/chrome/browser/ui...
> chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:453:
> delegate_->DidSelectAutocompleteSuggestion(names_[selected_line_]);
> Rather than introducing a new method, it seems reasonable to parallel
> DidAcceptSugestion(), which takes both the value and the identifier as a
> parameter.
> 
>
https://chromiumcodereview.appspot.com/148413002/diff/70001/chrome/browser/ui...
> chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:458:
> delegate_->ClearAutocompletePreviewedField();
> Why can't ClearPreviewedForm() just handle clearing all previews?
> 
>
https://chromiumcodereview.appspot.com/148413002/diff/70001/components/autofi...
> File components/autofill/core/browser/autofill_external_delegate.cc (right):
> 
>
https://chromiumcodereview.appspot.com/148413002/diff/70001/components/autofi...
> components/autofill/core/browser/autofill_external_delegate.cc:182:
> autofill_driver_->RendererShouldSetNodeText(value);
> This is incorrect.  The renderer should preview the text, not set it.  The
> difference is essentially in whether the change is visible to JavaScript,
though
> this also makes it easier to restore the previous value prior to mouse-hover.
> 
>
https://chromiumcodereview.appspot.com/148413002/diff/70001/components/autofi...
> components/autofill/core/browser/autofill_external_delegate.cc:230:
> autofill_driver_->RendererShouldSetNodeText(ASCIIToUTF16(""));
> This is incorrect.  The field value shouldn't be cleared; it should return to
> whatever it was prior to being previewed.

Updated the code and all review comments were addressed. Please review the
updates. Many thanks!

[Ilya Sherman, 2014-02-22 05:59:51]

https://codereview.chromium.org/148413002/diff/130001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/148413002/diff/130001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:169: const
base::string16& value) OVERRIDE;
nit: Please update the parameters to have the same order as used in the two
methods immediately below this one.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl.h:64: const
base::string16& value) OVERRIDE;
nit: Let's name this method "RendererShouldPreviewFieldWithValue()", and rename
the method above to "RendererShouldFillFieldWithValue()".  Or, feel free to
brainstorm even better names.  However, I think it's useful to have the names
have a clear parallel between them, since they perform very similar functions.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:294: }
nit: Please add a blank line after this one.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:131:
IPC_MESSAGE_ROUTED1(AutofillMsg_PreviewAutoCompleteNode,
Again, let's try to have parallel names and documentation for this message and
the one above it.  I'd suggest "AutofillMsg_FillFieldWithValue" and
"AutofillMsg_PreviewFieldWithValue", or maybe  "AutofillMsg_FillFormField" and
"AutofillMsg_PreviewFormField", or even just AutofillMsg_FillField" and
"AutofillMsg_PreviewField".  Feel free to brainstorm other names as well if you
prefer.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:132: base::string16 /*
preview text */)
nit: The commented out part is meant to be a variable name, so "preview text" ->
"value".  Please also fix line 128 while you're here.  Thanks :)

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:447:
element_.setSuggestedValue(WebString());
Hmm, is this line actually necessary?  It seems like
ClearPreviewedFormWithElement() ought to already take care of this.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:463:
PreviewAutoCompleteNode(value, &element_);
nit: Why not just inline the implementation here?

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:630:
node->setSuggestedValue(substring);
You should also set 

  was_query_node_autofilled_ = element_.isAutofilled();

and set the node to have the autofilled appearance while it being previewed.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:630:
node->setSuggestedValue(substring);
Wow, the method you're cribbing from was needlessly verbose.  I'm fixing it in [
https://codereview.chromium.org/176213002 ].  Please update this method
similarly.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.h:108: void
OnPreviewAutoCompleteNode(const base::string16& value);
nit: Please move this to be right after "OnSetNodeText"

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.h:177: // Set preview value
for AutoComplete |node|.
nit: "Set |node| to display the given |value| as a preview.  The preview is
visible on screen to the user, but not visible to the page via the DOM or
JavaScript."

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
components/autofill/core/browser/autofill_driver.h:86: const base::string16&
value) = 0;
nit: Please move this to be below the corresponding method for filling the
field.  I'd like to consistently order methods as "fill" followed by "preview".

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:169: {
nit: The curly brace should be on the previous line.  If this causes the line to
exceed 80 characters in length, then the parameters should wrap.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:175: // Preview
Autocomplete field
nit: This comment is redundant with the code; please remove it.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:178: }
nit: No need for curly braces.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate_unittest.cc
(right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate_unittest.cc:354:
external_delegate_->DidSelectSuggestion(-1, ASCIIToUTF16(""));
nit: Prefer "base::string16()" to "ASCIIToUTF16("")".

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate_unittest.cc:381: //
Ensure selecting an AutoComplete entries will cause any previews to
nit: "an AutoComplete entries" -> "an autocomplete entry"

[ziran.sun, 2014-02-27 15:38:10]

Update code as per Ilya's 2nd round comments. All comments have been addressed.
Thanks!

https://codereview.chromium.org/148413002/diff/130001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/148413002/diff/130001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:169: const
base::string16& value) OVERRIDE;
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> nit: Please update the parameters to have the same order as used in the two
> methods immediately below this one.

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl.h:64: const
base::string16& value) OVERRIDE;
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> nit: Let's name this method "RendererShouldPreviewFieldWithValue()", and
rename
> the method above to "RendererShouldFillFieldWithValue()".  Or, feel free to
> brainstorm even better names.  However, I think it's useful to have the names
> have a clear parallel between them, since they perform very similar functions.

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl.h:64: const
base::string16& value) OVERRIDE;
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> nit: Let's name this method "RendererShouldPreviewFieldWithValue()", and
rename
> the method above to "RendererShouldFillFieldWithValue()".  Or, feel free to
> brainstorm even better names.  However, I think it's useful to have the names
> have a clear parallel between them, since they perform very similar functions.

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:294: }
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> nit: Please add a blank line after this one.

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:131:
IPC_MESSAGE_ROUTED1(AutofillMsg_PreviewAutoCompleteNode,
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> Again, let's try to have parallel names and documentation for this message and
> the one above it.  I'd suggest "AutofillMsg_FillFieldWithValue" and
> "AutofillMsg_PreviewFieldWithValue", or maybe  "AutofillMsg_FillFormField" and
> "AutofillMsg_PreviewFormField", or even just AutofillMsg_FillField" and
> "AutofillMsg_PreviewField".  Feel free to brainstorm other names as well if
you
> prefer.

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:131:
IPC_MESSAGE_ROUTED1(AutofillMsg_PreviewAutoCompleteNode,
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> Again, let's try to have parallel names and documentation for this message and
> the one above it.  I'd suggest "AutofillMsg_FillFieldWithValue" and
> "AutofillMsg_PreviewFieldWithValue", or maybe  "AutofillMsg_FillFormField" and
> "AutofillMsg_PreviewFormField", or even just AutofillMsg_FillField" and
> "AutofillMsg_PreviewField".  Feel free to brainstorm other names as well if
you
> prefer.

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:132: base::string16 /*
preview text */)
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> nit: The commented out part is meant to be a variable name, so "preview text"
->
> "value".  Please also fix line 128 while you're here.  Thanks :)

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:447:
element_.setSuggestedValue(WebString());
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> Hmm, is this line actually necessary?  It seems like
> ClearPreviewedFormWithElement() ought to already take care of this.

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:463:
PreviewAutoCompleteNode(value, &element_);
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> nit: Why not just inline the implementation here?

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:630:
node->setSuggestedValue(substring);
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> Wow, the method you're cribbing from was needlessly verbose.  I'm fixing it in
[
> https://codereview.chromium.org/176213002 ].  Please update this method
> similarly.

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.h:108: void
OnPreviewAutoCompleteNode(const base::string16& value);
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> nit: Please move this to be right after "OnSetNodeText"

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.h:177: // Set preview value
for AutoComplete |node|.
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> nit: "Set |node| to display the given |value| as a preview.  The preview is
> visible on screen to the user, but not visible to the page via the DOM or
> JavaScript."

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
components/autofill/core/browser/autofill_driver.h:86: const base::string16&
value) = 0;
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> nit: Please move this to be below the corresponding method for filling the
> field.  I'd like to consistently order methods as "fill" followed by
"preview".

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:169: {
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> nit: The curly brace should be on the previous line.  If this causes the line
to
> exceed 80 characters in length, then the parameters should wrap.

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:178: }
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> nit: No need for curly braces.

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate_unittest.cc
(right):

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate_unittest.cc:354:
external_delegate_->DidSelectSuggestion(-1, ASCIIToUTF16(""));
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> nit: Prefer "base::string16()" to "ASCIIToUTF16("")".

Done.

https://codereview.chromium.org/148413002/diff/130001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate_unittest.cc:381: //
Ensure selecting an AutoComplete entries will cause any previews to
On 2014/02/22 05:59:51, Ilya Sherman wrote:
> nit: "an AutoComplete entries" -> "an autocomplete entry"

Done.

[Ilya Sherman, 2014-03-01 02:57:37]

+tsepez for IPC changes

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:140:
AutofillMsg_FillFieldWithValue::Read(message, &autofill_param);
Please check the return value of Read(), and return false if it's false.  (The
rest of the calls to Read() in this file are being fixed by a CL that's in the
CQ.)

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:143:
AutofillMsg_PreviewFieldWithValue::Read(message, &autofill_param);
Ditto

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:130: // Sets suggested
value for the currently previewed node
nit: Please end the sentence with a period.

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:463:
PreviewFieldWithValue(value, &element_);
nit: Why not just inline the code?

https://codereview.chromium.org/148413002/diff/380001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate_unittest.cc
(right):

https://codereview.chromium.org/148413002/diff/380001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate_unittest.cc:392: //
Ensure selecting an autocomplete entries will cause any previews to
nit: "an autocomplete entries" -> "an autocomplete entry"

[Tom Sepez, 2014-03-03 19:28:45]

lgtm.

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:130: // Sets suggested
value for the currently previewed node
nit: Sets the suggested value ...

[ziran.sun, 2014-03-04 15:30:18]

Code updated. Please review. Thanks!

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
File components/autofill/content/browser/autofill_driver_impl_unittest.cc
(right):

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:140:
AutofillMsg_FillFieldWithValue::Read(message, &autofill_param);
On 2014/03/01 02:57:37, Ilya Sherman wrote:
> Please check the return value of Read(), and return false if it's false.  (The
> rest of the calls to Read() in this file are being fixed by a CL that's in the
> CQ.)

Done.

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl_unittest.cc:143:
AutofillMsg_PreviewFieldWithValue::Read(message, &autofill_param);
On 2014/03/01 02:57:37, Ilya Sherman wrote:
> Ditto

Done.

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:130: // Sets suggested
value for the currently previewed node
On 2014/03/03 19:28:46, Tom Sepez wrote:
> nit: Sets the suggested value ...

Done.

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:130: // Sets suggested
value for the currently previewed node
On 2014/03/01 02:57:37, Ilya Sherman wrote:
> nit: Please end the sentence with a period.

Done.

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/148413002/diff/380001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:463:
PreviewFieldWithValue(value, &element_);
On 2014/03/01 02:57:37, Ilya Sherman wrote:
> nit: Why not just inline the code?

Sorry, which part code you are referring to?

https://codereview.chromium.org/148413002/diff/380001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate_unittest.cc
(right):

https://codereview.chromium.org/148413002/diff/380001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate_unittest.cc:392: //
Ensure selecting an autocomplete entries will cause any previews to
On 2014/03/01 02:57:37, Ilya Sherman wrote:
> nit: "an autocomplete entries" -> "an autocomplete entry"

Done.

[Ilya Sherman, 2014-03-14 07:17:04]

LGTM with one final comment addressed.  Thanks very much for your patience!

https://codereview.chromium.org/148413002/diff/420001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/148413002/diff/420001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:584:
node->setEditingValue(value.substr(0, node->maxLength()));
Let's call setAutofilled() here too, so that the yellow background doesn't
vanish when actually filling the field.

[ziran.sun, 2014-03-14 13:50:56]

Rebased and updated code as per Ilya's review comments. Please review. Thanks!

https://codereview.chromium.org/148413002/diff/420001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/148413002/diff/420001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:584:
node->setEditingValue(value.substr(0, node->maxLength()));
On 2014/03/14 07:17:05, Ilya Sherman wrote:
> Let's call setAutofilled() here too, so that the yellow background doesn't
> vanish when actually filling the field.

Done.

[Ilya Sherman, 2014-03-14 21:31:40]

LGTM, thanks.

[Ilya Sherman, 2014-03-14 21:31:47]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-03-14 21:32:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/148413002/430001

[commit-bot: I haz the power, 2014-03-14 21:39:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-14 21:39:06]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[Ilya Sherman, 2014-03-14 22:42:52]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-03-14 22:43:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/148413002/430001

[commit-bot: I haz the power, 2014-03-14 23:23:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-14 23:23:37]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Ilya Sherman, 2014-03-14 23:24:20]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-03-14 23:25:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/148413002/430001

[commit-bot: I haz the power, 2014-03-15 00:48:33]

Change committed as 257274