Persist the choice of AutofillDataModel when using the requestAutocomplete dialog.

Persist the choice of AutofillDataModel when using the requestAutocomplete dialog.

If the user selects an autofill data model that we already know about, save that as the new default. Newly added addresses and cards will not be stickied.

Only takes effect if NOT paying with wallet.

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

[Ilya Sherman, 2013-05-21 03:54:35]

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

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1910:
SuggestionsMenuModel* model = SuggestionsMenuModelForSection(section);
nit: Perhaps move this into the if-stmt, since it's not used outside of there?

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1965: 
nit: Spurious diff

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2543: variant++;
nit: ++variant

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:438: // Until
a selection has been made, the defauilt shipping suggestion is the
nit: "default"

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:451: SetUp();
Hmm, these methods aren't really meant to be called except by the test harness. 
I'm worried that this will break at some point in the future as the contents of
SetUp() and TearDown() evolve.  Is there perhaps a way to write this test that
doesn't call into these methods?

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:453:
controller()->GetTestingManager()->AddTestingProfile(&full_profile2);
Please add a test verifying the behavior when the PersonalDataManager no longer
has the default profile saved.

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_models.cc (right):

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_models.cc:92: if (n-- <= 1)
nit: Please write this as two separate statements, rather than asking me to
think about post-increment vs. pre-increment operators.

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_models.h (right):

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_models.h:77: // becomes checked.
nit: Please mention that this is 1-indexed rather than 0-indexed (which,
incidentally, is a little surprising).

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_models.h:78: void
SetCheckedItemNthWithKey(const std::string& item_key, size_t n);
What happens if there are no items that share |item_key|?

[Evan Stade, 2013-05-21 17:34:41]

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

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1910:
SuggestionsMenuModel* model = SuggestionsMenuModelForSection(section);
On 2013/05/21 03:54:35, Ilya Sherman wrote:
> nit: Perhaps move this into the if-stmt, since it's not used outside of there?

changed this code a bit.

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1965: 
On 2013/05/21 03:54:35, Ilya Sherman wrote:
> nit: Spurious diff

Done.

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2543: variant++;
On 2013/05/21 03:54:35, Ilya Sherman wrote:
> nit: ++variant

this is not a rule that I'm aware of.

"For simple scalar (non-object) values there is no reason to prefer one form and
we allow either."[1]

By my crude count, chrome/browser/ has about 300 preincrements and 500
postincrements of values that are not in for loop headers.

[1] except actually there is, in the words of the style guide: "Some find
post-increment easier to read, since the "subject" (i) precedes the "verb" (++),
just like in English."

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:438: // Until
a selection has been made, the defauilt shipping suggestion is the
On 2013/05/21 03:54:35, Ilya Sherman wrote:
> nit: "default"

Done.

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:451: SetUp();
On 2013/05/21 03:54:35, Ilya Sherman wrote:
> Hmm, these methods aren't really meant to be called except by the test
harness. 
> I'm worried that this will break at some point in the future as the contents
of
> SetUp() and TearDown() evolve.  Is there perhaps a way to write this test that
> doesn't call into these methods?

If the contents of SetUp and TearDown evolve, we probably want the changes to
apply here as well. Conveniently, if someone does make a breaking change, we
will notice that this test is failing and be able to address that issue when it
arises (it would probably just consist of splitting out the functionality that's
currently in SetUp/TearDown into other functions that can be called separately).

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:453:
controller()->GetTestingManager()->AddTestingProfile(&full_profile2);
On 2013/05/21 03:54:35, Ilya Sherman wrote:
> Please add a test verifying the behavior when the PersonalDataManager no
longer
> has the default profile saved.

Done. Also improved the behavior in that case (instead of falling back to index
0, fall back to the normal default).

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_models.cc (right):

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_models.cc:92: if (n-- <= 1)
On 2013/05/21 03:54:35, Ilya Sherman wrote:
> nit: Please write this as two separate statements, rather than asking me to
> think about post-increment vs. pre-increment operators.

I don't see anything about this in the style guide. In fact it implicitly
condones it:

"When a variable is incremented (++i or i++) or decremented (--i or i--) <em>and
the value of the expression is not used</em>, one must decide whether to
preincrement (decrement) or postincrement (decrement)."

I don't feel strongly, but my general take on this type of matter is that it
causes confusion if you don't use the shortest form available (i.e. if you break
it out into a separate line, I wonder why that is and probably go back and
re-read to make sure I'm not missing something).

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_models.h (right):

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_models.h:77: // becomes checked.
On 2013/05/21 03:54:35, Ilya Sherman wrote:
> nit: Please mention that this is 1-indexed rather than 0-indexed (which,
> incidentally, is a little surprising).

done. I made it 1-indexed because Nth implies 1-indexing.

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_models.h:78: void
SetCheckedItemNthWithKey(const std::string& item_key, size_t n);
On 2013/05/21 03:54:35, Ilya Sherman wrote:
> What happens if there are no items that share |item_key|?

Done.

[Ilya Sherman, 2013-05-21 23:20:43]

lgtm

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

https://codereview.chromium.org/15500008/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2543: variant++;
On 2013/05/21 17:34:41, Evan Stade wrote:
> On 2013/05/21 03:54:35, Ilya Sherman wrote:
> > nit: ++variant
> 
> this is not a rule that I'm aware of.
> 
> "For simple scalar (non-object) values there is no reason to prefer one form
and
> we allow either."[1]
> 
> By my crude count, chrome/browser/ has about 300 preincrements and 500
> postincrements of values that are not in for loop headers.
> 
> [1] except actually there is, in the words of the style guide: "Some find
> post-increment easier to read, since the "subject" (i) precedes the "verb"
(++),
> just like in English."

I prefer to always use pre-increment because it's never wrong and hence is an
easy rule to stick to... but ok.

https://codereview.chromium.org/15500008/diff/18001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_models.h (right):

https://codereview.chromium.org/15500008/diff/18001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models.h:77: // becomes checked. If
there is no item with the key one, nothing happens.
nit: "with the key one" -> "with the key |item_key|"

[Evan Stade, 2013-05-22 22:24:02]

Dan, could you review the changes between patchset 3 and 4?

https://codereview.chromium.org/15500008/diff/18001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_models.h (right):

https://codereview.chromium.org/15500008/diff/18001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models.h:77: // becomes checked. If
there is no item with the key one, nothing happens.
On 2013/05/21 23:20:43, Ilya Sherman wrote:
> nit: "with the key one" -> "with the key |item_key|"

Done.

[Evan Stade, 2013-05-22 22:25:02]

On 2013/05/22 22:24:02, Evan Stade wrote:
> Dan, could you review the changes between patchset 3 and 4?

(existing tests plus a DCHECK in the new code I added uncovered some bugs in
intermediate state)

[Dan Beam, 2013-05-23 02:21:41]

3 -> 4 lgtm

[commit-bot: I haz the power, 2013-05-23 16:26:16]

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

[commit-bot: I haz the power, 2013-05-23 16:26:26]

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

Patch:       chrome/browser/ui/autofill/autofill_dialog_models.cc
Index: chrome/browser/ui/autofill/autofill_dialog_models.cc
diff --git a/chrome/browser/ui/autofill/autofill_dialog_models.cc
b/chrome/browser/ui/autofill/autofill_dialog_models.cc
index
18b05aff3a1acb70f20e5e48c8a5d99db01127f5..8965a559262504d191daa9800d3b6e773efcd214
100644
--- a/chrome/browser/ui/autofill/autofill_dialog_models.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_models.cc
@@ -76,14 +76,7 @@ std::string SuggestionsMenuModel::GetItemKeyForCheckedItem()
const {
 }
 
 void SuggestionsMenuModel::SetCheckedItem(const std::string& item_key) {
-  for (size_t i = 0; i < items_.size(); ++i) {
-    if (items_[i].first == item_key) {
-      checked_item_ = i;
-      return;
-    }
-  }
-
-  NOTREACHED();
+  SetCheckedItemNthWithKey(item_key, 1);
 }
 
 void SuggestionsMenuModel::SetCheckedIndex(size_t index) {
@@ -91,6 +84,17 @@ void SuggestionsMenuModel::SetCheckedIndex(size_t index) {
   checked_item_ = index;
 }
 
+void SuggestionsMenuModel::SetCheckedItemNthWithKey(const std::string&
item_key,
+                                                    size_t n) {
+  for (size_t i = 0; i < items_.size(); ++i) {
+    if (items_[i].first == item_key) {
+      checked_item_ = i;
+      if (n-- <= 1)
+        return;
+    }
+  }
+}
+
 bool SuggestionsMenuModel::IsCommandIdChecked(
     int command_id) const {
   return checked_item_ == command_id;

[commit-bot: I haz the power, 2013-05-23 18:14:40]

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

[commit-bot: I haz the power, 2013-05-23 18:37:39]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-23 22:34:52]

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

[commit-bot: I haz the power, 2013-05-24 06:46:00]

Change committed as 201991