Remove "Use billing for shipping" checkbox in favor of item in suggestions menu.

Remove "Use billing for shipping" checkbox in favor of item in suggestions menu.

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

[Evan Stade, 2013-04-14 00:17:57]

dbeam: ui/autofill
sky: ui/views
aruslan: android

[tfarina, 2013-04-14 18:57:04]

https://codereview.chromium.org/14129005/diff/1/chrome/browser/ui/views/autof...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/14129005/diff/1/chrome/browser/ui/views/autof...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:507:
label_->SetFont(ResourceBundle::GetSharedInstance().GetFont(
nit: ui::ResourceBundle

[sky, 2013-04-15 13:59:47]

LGTM

[Dan Beam, 2013-04-16 03:01:14]

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1740:
l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_ADD_CREDIT_CARD));
^ hmm, where's IDS_AUTOFILL_DIALOG_MANAGE_CREDIT_CARD?

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1962: return
!key.empty() &&
when is key empty now?  why not DCHECK(!key.empty())?

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1965: key !=
kSameAsBillingKey;
nit:

  return !key.empty() &&
         key != kAddNewItemKey &&
         key != kManageItemsKey &&
         key != kSameAsBillingKey;

IMO (though I don't care much)

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:326: const;
opt. nit: I think there's something in the style guide about trying not to add
trailing `const`s on the next line, i.e. you'd wrap like:

  gfx::Font::FontStyle SuggestionTextStyleForSection(
      DialogSection section) const;

think I saw it fly past on a review... but can't procure evidence.

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_models.cc (right):

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_models.cc:83: break;
return;

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_models.cc:85: }
NOTREACHED(); IMO (unless this is valid for some reason)

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_models.h (right):

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_models.h:76: void
SetCheckedItem(const std::string& item_key);
maybe explain the difference between ExecuteCommand() and/or ActivatedAt()

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_types.cc (right):

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_types.cc:78: text_style(text_style),
btw, is there a less clunky way to do this without shadowing var names?  I
didn't find one...

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_types.h:143: gfx::Font::FontStyle
text_style;
nit: should these all be const? maybe it doesn't matter if SuggestionState's are
generally passed by value...

[aruslan, 2013-04-16 16:49:54]

Android changes ONLY -- LGTM -- with 2 places that should be changed to avoid
the breakage.

Could we actually have some way for the view to ask a controller/a model if a
given item is "special"?
Alternatively, I'll change the controller to #if Android etc and build the
correct UI model there.

Also, looks like the unit tests for the controller are broken somehow -- in
precisely the same place I see the crash right now (in CreateWrapper()).

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/android/jav...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java
(right):

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:449:
menuItems.length - 1 + mDefaultMenuItems[section].length);
It would have helped if there was a way to know that a given item is "Add.." or
"Manage.." from the controller.

Two options: #if !defined(OS_ANDROID) when adding the "Manage..." items in the
controller, or please make this "- 2" instead of "- 1" (assuming the "Manage.."
is always added; otherwise if (section != ...) should be added here).

https://chromiumcodereview.appspot.com/14129005/diff/17001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:452:
combinedItems.remove(menuItems.length - 1);
Please duplicate this to remove the "Manage..." and change the comment to say
that we remove the "Manage.." item and replace the provided "Add.." item with
ours.

[Evan Stade, 2013-04-16 19:06:20]

https://codereview.chromium.org/14129005/diff/1/chrome/browser/ui/views/autof...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/14129005/diff/1/chrome/browser/ui/views/autof...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:507:
label_->SetFont(ResourceBundle::GetSharedInstance().GetFont(
On 2013/04/14 18:57:05, tfarina wrote:
> nit: ui::ResourceBundle

Done.

https://codereview.chromium.org/14129005/diff/17001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java
(right):

https://codereview.chromium.org/14129005/diff/17001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:452:
combinedItems.remove(menuItems.length - 1);
On 2013/04/16 16:49:55, aruslan wrote:
> Please duplicate this to remove the "Manage..." and change the comment to say
> that we remove the "Manage.." item and replace the provided "Add.." item with
> ours.

why would you remove the manage item? I've done as you asked, but I don't see
why we wouldn't want that same link here to take the user to wallet's web
interface (or the settings page for managing autofill).

Why do you remove Add only to add it back? Is it just a string difference? if
so, you should implement that at the .grd layer.

https://codereview.chromium.org/14129005/diff/17001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/14129005/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1740:
l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_ADD_CREDIT_CARD));
On 2013/04/16 03:01:14, Dan Beam wrote:
> ^ hmm, where's IDS_AUTOFILL_DIALOG_MANAGE_CREDIT_CARD?

good catch, done.

https://codereview.chromium.org/14129005/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1962: return
!key.empty() &&
On 2013/04/16 03:01:14, Dan Beam wrote:
> when is key empty now?  why not DCHECK(!key.empty())?

GetKeyForCheckedItem will return an empty string if there's no checked item
(because the model is empty).

https://codereview.chromium.org/14129005/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1965: key !=
kSameAsBillingKey;
On 2013/04/16 03:01:14, Dan Beam wrote:
> nit:
> 
>   return !key.empty() &&
>          key != kAddNewItemKey &&
>          key != kManageItemsKey &&
>          key != kSameAsBillingKey;
> 
> IMO (though I don't care much)

that is harsher on my eyes, which have grown accustomed to a standard 4 space
indent.

https://codereview.chromium.org/14129005/diff/17001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/14129005/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:326: const;
On 2013/04/16 03:01:14, Dan Beam wrote:
> opt. nit: I think there's something in the style guide about trying not to add
> trailing `const`s on the next line, i.e. you'd wrap like:
> 
>   gfx::Font::FontStyle SuggestionTextStyleForSection(
>       DialogSection section) const;
> 
> think I saw it fly past on a review... but can't procure evidence.

that rule was removed from the style guide.

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

https://codereview.chromium.org/14129005/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models.cc:83: break;
On 2013/04/16 03:01:14, Dan Beam wrote:
> return;

sure, why not

https://codereview.chromium.org/14129005/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models.cc:85: }
On 2013/04/16 03:01:14, Dan Beam wrote:
> NOTREACHED(); IMO (unless this is valid for some reason)

Done.

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

https://codereview.chromium.org/14129005/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models.h:76: void
SetCheckedItem(const std::string& item_key);
On 2013/04/16 03:01:14, Dan Beam wrote:
> maybe explain the difference between ExecuteCommand() and/or ActivatedAt()

I don't really see how there could be confusion here unless you don't understand
what ExecuteCommand is. I believe documentation for ExecuteCommand belongs in
the SimpleMenuModel::Delegate interface.

https://codereview.chromium.org/14129005/diff/17001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_types.cc (right):

https://codereview.chromium.org/14129005/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_types.cc:78: text_style(text_style),
On 2013/04/16 03:01:14, Dan Beam wrote:
> btw, is there a less clunky way to do this without shadowing var names?  I
> didn't find one...

don't understand how this is clunky.

https://codereview.chromium.org/14129005/diff/17001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://codereview.chromium.org/14129005/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_types.h:143: gfx::Font::FontStyle
text_style;
On 2013/04/16 03:01:14, Dan Beam wrote:
> nit: should these all be const? maybe it doesn't matter if SuggestionState's
are
> generally passed by value...

these could be const, because at the moment nothing tries to change them. But I
don't think they should be. I can't think of a reason we'd specifically want to
disallow changing them.

[Dan Beam, 2013-04-17 00:18:15]

https://chromiumcodereview.appspot.com/14129005/diff/31001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/14129005/diff/31001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1359: //
TODO(estade): show chrome://settings or a wallet URL.
where's the bug for this and/or why can't we do this now?

https://chromiumcodereview.appspot.com/14129005/diff/31001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1692: //
TODO(estade): this should have a URL sublabel.
what does this mean? that we should be able to link directly to a user's wallet
items or something?

https://chromiumcodereview.appspot.com/14129005/diff/31001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1752: if
(!IsPayingWithWallet()) {
if we're ever going to add this, add a TODO

https://chromiumcodereview.appspot.com/14129005/diff/31001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2206: if
(model.GetItemKeyForCheckedItem().empty()) {
this should be

  if (model.GetItemKeyForCheckedItem() == kAddNewItemKey)

right?

[Evan Stade, 2013-04-17 21:54:38]

https://chromiumcodereview.appspot.com/14129005/diff/31001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/14129005/diff/31001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1359: //
TODO(estade): show chrome://settings or a wallet URL.
On 2013/04/17 00:18:16, Dan Beam wrote:
> where's the bug for this and/or why can't we do this now?

don't you think this CL is large enough as it is? Since the summary of the
problem fits succinctly on one line, I don't think a bug is necessary.

https://chromiumcodereview.appspot.com/14129005/diff/31001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1692: //
TODO(estade): this should have a URL sublabel.
On 2013/04/17 00:18:16, Dan Beam wrote:
> what does this mean? that we should be able to link directly to a user's
wallet
> items or something?

no. If you look at the mocks, you will see that the menu item has a bit of
secondary text off to the right.

https://chromiumcodereview.appspot.com/14129005/diff/31001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1752: if
(!IsPayingWithWallet()) {
On 2013/04/17 00:18:16, Dan Beam wrote:
> if we're ever going to add this, add a TODO

don't think we are.

https://chromiumcodereview.appspot.com/14129005/diff/31001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2206: if
(model.GetItemKeyForCheckedItem().empty()) {
On 2013/04/17 00:18:16, Dan Beam wrote:
> this should be
> 
>   if (model.GetItemKeyForCheckedItem() == kAddNewItemKey)
> 
> right?

Done.

[Dan Beam, 2013-04-17 22:24:26]

lgtm

https://chromiumcodereview.appspot.com/14129005/diff/45001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/14129005/diff/45001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1359: //
TODO(estade): show chrome://settings or a wallet URL.
can you file a bug for this?

https://chromiumcodereview.appspot.com/14129005/diff/45001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2215: //
shipping"?
can you file a bug for this?

[Evan Stade, 2013-04-19 22:23:16]

Committed patchset #8 manually as r195316 (presubmit successful).

[Mike Wittman, 2013-04-19 22:45:03]

On 2013/04/19 22:23:16, Evan Stade wrote:
> Committed patchset #8 manually as r195316 (presubmit successful).

Reverted in http://crrev.com/195320.  Breaks Mac ASAN compile.