rAc: don't clobber user input when accepting autofill popup suggestion

rAc: don't clobber user input when accepting autofill popup suggestion

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

[Evan Stade, 2013-04-04 03:22:01]

this matches web Autofill behavior: only fill Autofill data into empty form
fields.

[Ilya Sherman, 2013-04-04 03:59:55]

https://codereview.chromium.org/13607006/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/13607006/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1262:
view_->UpdateSection(section_showing_popup_, false);
Why is it the view's responsibility to deal with whether or not to clear the
user input?  How will the view remember which fields to clobber or not if it
ever needs to re-query the controller later?  It seems like the controller
should just correctly update the inputs returned by
MutableRequestFieldsForSection().

https://codereview.chromium.org/13607006/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_view.h (right):

https://codereview.chromium.org/13607006/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_view.h:41: virtual void
UpdateSection(DialogSection section, bool clear_user_input) = 0;
nit: If you keep this method, please use an enumerated constant rather than a
boolean for the type of the second variable.  As is, call sites are quite hard
to understand at a glance.

[sky, 2013-04-04 15:30:07]

LGTM

[Evan Stade, 2013-04-04 17:56:50]

https://codereview.chromium.org/13607006/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/13607006/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1262:
view_->UpdateSection(section_showing_popup_, false);
On 2013/04/04 03:59:55, Ilya Sherman wrote:
> Why is it the view's responsibility to deal with whether or not to clear the
> user input?

because the view handles user input

> How will the view remember which fields to clobber or not if it
> ever needs to re-query the controller later?

I'm not quite sure what you mean by this. Could you clarify?

> It seems like the controller
> should just correctly update the inputs returned by
> MutableRequestFieldsForSection().

DetailInput is a struct that describes how to init a textfield. The textfields
(and their contents) are part of the presentation layer.

[Ilya Sherman, 2013-04-04 23:11:02]

https://codereview.chromium.org/13607006/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/13607006/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1262:
view_->UpdateSection(section_showing_popup_, false);
On 2013/04/04 17:56:51, Evan Stade wrote:
> On 2013/04/04 03:59:55, Ilya Sherman wrote:
> > Why is it the view's responsibility to deal with whether or not to clear the
> > user input?
> 
> because the view handles user input
> 
> > How will the view remember which fields to clobber or not if it
> > ever needs to re-query the controller later?
> 
> I'm not quite sure what you mean by this. Could you clarify?
> 
> > It seems like the controller
> > should just correctly update the inputs returned by
> > MutableRequestFieldsForSection().
> 
> DetailInput is a struct that describes how to init a textfield. The textfields
> (and their contents) are part of the presentation layer.

Ok, I guess the view code isn't likely to ever be updated to try to read the
initial_value field from this struct at some other time, so this is probably not
an issue.

It's just weird that it *could* try to read this cached data from the controller
at any time... even though the cache might not be relevant anymore, and would be
missing the context of whether or not user input should be clobbered.

https://codereview.chromium.org/13607006/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_view.h (right):

https://codereview.chromium.org/13607006/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_view.h:41: virtual void
UpdateSection(DialogSection section, bool clear_user_input) = 0;
On 2013/04/04 03:59:55, Ilya Sherman wrote:
> nit: If you keep this method, please use an enumerated constant rather than a
> boolean for the type of the second variable.  As is, call sites are quite hard
> to understand at a glance.

^^^

[Evan Stade, 2013-04-05 23:24:01]

https://codereview.chromium.org/13607006/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_view.h (right):

https://codereview.chromium.org/13607006/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_view.h:41: virtual void
UpdateSection(DialogSection section, bool clear_user_input) = 0;
On 2013/04/04 23:11:02, Ilya Sherman wrote:
> On 2013/04/04 03:59:55, Ilya Sherman wrote:
> > nit: If you keep this method, please use an enumerated constant rather than
a
> > boolean for the type of the second variable.  As is, call sites are quite
hard
> > to understand at a glance.
> 
> ^^^

Done.

https://codereview.chromium.org/13607006/diff/20001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_models.cc (right):

https://codereview.chromium.org/13607006/diff/20001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models.cc:198: return
ASCIIToUTF16(base::StringPrintf("%.2d", index));
isherman: NB

https://codereview.chromium.org/13607006/diff/20001/ui/views/controls/combobo...
File ui/views/controls/combobox/native_combobox_views.cc (right):

https://codereview.chromium.org/13607006/diff/20001/ui/views/controls/combobo...
ui/views/controls/combobox/native_combobox_views.cc:222: SchedulePaint();
sky: could you also review this change?

[Ilya Sherman, 2013-04-05 23:29:52]

LGTM, thanks.

https://codereview.chromium.org/13607006/diff/20001/chrome/browser/ui/android...
File chrome/browser/ui/android/autofill/autofill_dialog_view_android.h (right):

https://codereview.chromium.org/13607006/diff/20001/chrome/browser/ui/android...
chrome/browser/ui/android/autofill/autofill_dialog_view_android.h:31: UserInput
action) OVERRIDE;
nit: UserInput -> UserInputAction

[commit-bot: I haz the power, 2013-04-08 18:58:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/13607006/35001

[commit-bot: I haz the power, 2013-04-08 18:58:08]

Failed to apply patch for
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
  Hunk #1 succeeded at 844 (offset -9 lines).
  Hunk #2 succeeded at 853 (offset -9 lines).
  Hunk #3 FAILED at 1271.
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc.rej

Patch:       chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
Index: chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
index
197287dea24cf9f251051aab334906416f4acc41..aa820f32101e1bbe6f2f2190bde1fbf4b66fd86a
100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
@@ -853,7 +853,7 @@ void AutofillDialogControllerImpl::EditClickedForSection(
   scoped_ptr<DataModelWrapper> model = CreateWrapper(section);
   model->FillInputs(inputs);
   section_editing_state_[section] = true;
-  view_->UpdateSection(section);
+  view_->UpdateSection(section, CLEAR_USER_INPUT);
 }
 
 void AutofillDialogControllerImpl::EditCancelledForSection(
@@ -862,7 +862,7 @@ void AutofillDialogControllerImpl::EditCancelledForSection(
   for (size_t i = 0; i < inputs->size(); ++i)
     (*inputs)[i].initial_value.clear();
   section_editing_state_[section] = false;
-  view_->UpdateSection(section);
+  view_->UpdateSection(section, CLEAR_USER_INPUT);
 }
 
 gfx::Image AutofillDialogControllerImpl::IconForField(
@@ -1271,7 +1271,7 @@ void
AutofillDialogControllerImpl::DidAcceptSuggestion(const string16& value,
   FillInputFromFormGroup(
       form_group,
       MutableRequestedFieldsForSection(section_showing_popup_));
-  view_->UpdateSection(section_showing_popup_);
+  view_->UpdateSection(section_showing_popup_, KEEP_USER_INPUT);
 
   metric_logger_.LogDialogPopupEvent(
       dialog_type_, AutofillMetrics::DIALOG_POPUP_FORM_FILLED);

[commit-bot: I haz the power, 2013-04-09 00:41:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/13607006/40001

[commit-bot: I haz the power, 2013-04-09 01:02:41]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2013-04-09 22:31:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/13607006/56001

[commit-bot: I haz the power, 2013-04-10 01:55:52]

Change committed as 193282