[autofill] Hook the sign-in button up to required actions.

[autofill] Hook the sign-in button up to required actions.

- Actually use the info we're getting from wallet to tell whether a user needs
  to authenticate further (in this case only in a non-passive way).

- Add a DialogSignedInState to autofill_dialog_types.h.

- Add a DialogSignedInState SignedInState() to AutofillDialogController interface
  and implements AutofillDialogControllerImpl.

- Adds a UpdateAccountChooser() to AutofillDialogView and implements in
  AutofillDialogViews.

- Use SignedInState() from AutofillDialogViews.

BUG=157270, 163508, 171528
R=isherman@chromium.org,sky@chromium.org

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

[Ilya Sherman, 2013-01-31 02:33:07]

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:103: virtual
std::vector<wallet::RequiredAction> RequiredActions() const = 0;
nit: Shouldn't this be defined on the Impl class?

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:106: virtual
DialogNotification CurrentNotification() const = 0;
nit: If this is only for security notifications, perhaps name the method
CurrentSecurityNotification?

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:279: return
wallet_items_;
nit: Implicitly converting to a boolean is generally not what you want, since
some code will explicitly compare to false (i.e. 0) or true (i.e. 1).  To
support such cases, you should write something more verbose, but safer, like
"return wallet_items_ != NULL;"

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:151:
std::vector<wallet::RequiredAction> RequiredActions() const;
Hmm, this makes a copy.  Is that really what you want?  IMO it would be better
to just inline the code.

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:154: bool
RequiredActionApplies(wallet::RequiredAction action) const;
Optional nit: When I read this method name and comment, I sort of expect it to
be called on actions that the server thinks are required, but which might not
apply for some reason.  Instead, you're basically checking whether the server
returned a specific required action or not.  Perhaps name this something like
HasRequiredAction() and update the comment to reduce ambiguity?

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/v...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:482: DCHECK(source);
// TODO(dbeam): needed?
nit: Don't think this is needed, no.

[Dan Beam, 2013-01-31 02:48:51]

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:103: virtual
std::vector<wallet::RequiredAction> RequiredActions() const = 0;
On 2013/01/31 02:33:07, Ilya Sherman wrote:
> nit: Shouldn't this be defined on the Impl class?

D'oh, was using in |view_| before but forgot to remove (did it 2 other places,
but not the *third*, >:/).

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:106: virtual
DialogNotification CurrentNotification() const = 0;
On 2013/01/31 02:33:07, Ilya Sherman wrote:
> nit: If this is only for security notifications, perhaps name the method
> CurrentSecurityNotification?

It's not, there's required actions and there will be other things as well (see:
autofill_dialog_types.h for definition of the types of DialogNotifications).

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:279: return
wallet_items_;
On 2013/01/31 02:33:07, Ilya Sherman wrote:
> nit: Implicitly converting to a boolean is generally not what you want, since
> some code will explicitly compare to false (i.e. 0) or true (i.e. 1).  To
> support such cases, you should write something more verbose, but safer, like
> "return wallet_items_ != NULL;"

Done.

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:151:
std::vector<wallet::RequiredAction> RequiredActions() const;
On 2013/01/31 02:33:07, Ilya Sherman wrote:
> Hmm, this makes a copy.  Is that really what you want?  IMO it would be better
> to just inline the code.

Done. (this was used quite a few places before, now it's not)

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:154: bool
RequiredActionApplies(wallet::RequiredAction action) const;
On 2013/01/31 02:33:07, Ilya Sherman wrote:
> Optional nit: When I read this method name and comment, I sort of expect it to
> be called on actions that the server thinks are required, but which might not
> apply for some reason.  Instead, you're basically checking whether the server
> returned a specific required action or not.  Perhaps name this something like
> HasRequiredAction() and update the comment to reduce ambiguity?

Done.

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/v...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:482: DCHECK(source);
// TODO(dbeam): needed?
On 2013/01/31 02:33:07, Ilya Sherman wrote:
> nit: Don't think this is needed, no.

Done.

[Ilya Sherman, 2013-01-31 04:35:44]

LGTM with da nitz:

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:106: virtual
DialogNotification CurrentNotification() const = 0;
On 2013/01/31 02:48:51, Dan Beam wrote:
> On 2013/01/31 02:33:07, Ilya Sherman wrote:
> > nit: If this is only for security notifications, perhaps name the method
> > CurrentSecurityNotification?
> 
> It's not, there's required actions and there will be other things as well
(see:
> autofill_dialog_types.h for definition of the types of DialogNotifications).

In that case, maybe don't call out security explicitly in the comment?

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:12: #include
"chrome/browser/autofill/wallet/required_action.h"
nit: Remove dis guy

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:54: virtual bool
HasReceivedWalletResponse() const = 0;
nit: Docs.  Also, perhaps name this method something closer to what the view
cares about, e.g. IsSigninStateKnown() (or something like that but maybe less
clunky).

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:739: const
std::vector<wallet::RequiredAction>& required =
Optional nit: IMO "actions" is a slightly better short name than "required".

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:151:
std::vector<wallet::RequiredAction> RequiredActions() const;
nit: Remove dis guy.

[sky, 2013-01-31 16:49:53]

LGTM

[Dan Beam, 2013-01-31 21:04:26]

isherman@: ptaql (q = quick)

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://chromiumcodereview.appspot.com/12094069/diff/3001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:106: virtual
DialogNotification CurrentNotification() const = 0;
On 2013/01/31 04:35:44, Ilya Sherman wrote:
> On 2013/01/31 02:48:51, Dan Beam wrote:
> > On 2013/01/31 02:33:07, Ilya Sherman wrote:
> > > nit: If this is only for security notifications, perhaps name the method
> > > CurrentSecurityNotification?
> > 
> > It's not, there's required actions and there will be other things as well
> (see:
> > autofill_dialog_types.h for definition of the types of DialogNotifications).
> 
> In that case, maybe don't call out security explicitly in the comment?

Done.

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:12: #include
"chrome/browser/autofill/wallet/required_action.h"
On 2013/01/31 04:35:44, Ilya Sherman wrote:
> nit: Remove dis guy

Done.

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:54: virtual bool
HasReceivedWalletResponse() const = 0;
On 2013/01/31 04:35:44, Ilya Sherman wrote:
> nit: Docs.  Also, perhaps name this method something closer to what the view
> cares about, e.g. IsSigninStateKnown() (or something like that but maybe less
> clunky).

Added a DialogSignedInState, should make this prettier.

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:739: const
std::vector<wallet::RequiredAction>& required =
On 2013/01/31 04:35:44, Ilya Sherman wrote:
> Optional nit: IMO "actions" is a slightly better short name than "required".

Yeah, thought that actions + action was a little unclear, but looking at it
again it's not so bad.

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://chromiumcodereview.appspot.com/12094069/diff/5002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:151:
std::vector<wallet::RequiredAction> RequiredActions() const;
On 2013/01/31 04:35:44, Ilya Sherman wrote:
> nit: Remove dis guy.

Done.

[Ilya Sherman, 2013-01-31 23:46:41]

SLGTM (s=still)

https://chromiumcodereview.appspot.com/12094069/diff/3016/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://chromiumcodereview.appspot.com/12094069/diff/3016/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:54: virtual
DialogSignedInState SignedIn() const = 0;
nit: Name the method SignedInState?

[tfarina, 2013-01-31 23:47:38]

On Thu, Jan 31, 2013 at 9:46 PM,  <isherman@chromium.org> wrote:
> SLGTM (s=still)
>
>
>
https://chromiumcodereview.appspot.com/12094069/diff/3016/chrome/browser/ui/a...
> File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):
>
>
https://chromiumcodereview.appspot.com/12094069/diff/3016/chrome/browser/ui/a...
> chrome/browser/ui/autofill/autofill_dialog_controller.h:54: virtual
> DialogSignedInState SignedIn() const = 0;
> nit: Name the method SignedInState?
>
GetSignInState?

> https://chromiumcodereview.appspot.com/12094069/



-- 
Thiago

[Dan Beam, 2013-01-31 23:54:49]

On 2013/01/31 23:47:38, tfarina wrote:
> On Thu, Jan 31, 2013 at 9:46 PM,  <mailto:isherman@chromium.org> wrote:
> > SLGTM (s=still)
> >
> >
> >
>
https://chromiumcodereview.appspot.com/12094069/diff/3016/chrome/browser/ui/a...
> > File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):
> >
> >
>
https://chromiumcodereview.appspot.com/12094069/diff/3016/chrome/browser/ui/a...
> > chrome/browser/ui/autofill/autofill_dialog_controller.h:54: virtual
> > DialogSignedInState SignedIn() const = 0;
> > nit: Name the method SignedInState?
> >
> GetSignInState?

I'll talk with estade@ and change all of these to Get*() if he wants to (right
now none of the accessors are using a Get prefix).

> 
> > https://chromiumcodereview.appspot.com/12094069/
> 
> 
> 
> -- 
> Thiago

[Dan Beam, 2013-02-01 00:30:39]

https://chromiumcodereview.appspot.com/12094069/diff/3016/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://chromiumcodereview.appspot.com/12094069/diff/3016/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:54: virtual
DialogSignedInState SignedIn() const = 0;
On 2013/01/31 23:46:41, Ilya Sherman wrote:
> nit: Name the method SignedInState?

Done.

[commit-bot: I haz the power, 2013-02-01 00:49:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/12094069/14001

[commit-bot: I haz the power, 2013-02-01 05:24:38]

Change committed as 180083