[rAc] Disable sign-in link while the user's signed-in state is unknown.

[rAc] Disable sign-in link while the user's signed-in state is unknown.

BUG=281698
TEST=Link should be disabled while the user's signed-in state is unknown.
R=estade@chromium.org

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

[Ilya Sherman, 2013-08-30 01:44:02]

+Rachel for the cocoa changes.

[groby-ooo-7-16, 2013-08-30 01:48:11]

LGTM w/ nit

https://codereview.chromium.org/23806002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_account_chooser.mm (right):

https://codereview.chromium.org/23806002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_account_chooser.mm:111:
setEnabled:(delegate_->SignedInState() != autofill::REQUIRES_RESPONSE)];
nit: line break after setEnabled:

i.e.
[link_ setEnabled:
    (delegate_->SignedInState() != autofill::REQUIRES_RESPONSE)];

[Ilya Sherman, 2013-08-30 03:24:40]

https://codereview.chromium.org/23806002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_account_chooser.mm (right):

https://codereview.chromium.org/23806002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_account_chooser.mm:111:
setEnabled:(delegate_->SignedInState() != autofill::REQUIRES_RESPONSE)];
On 2013/08/30 01:48:12, groby wrote:
> nit: line break after setEnabled:
> 
> i.e.
> [link_ setEnabled:
>     (delegate_->SignedInState() != autofill::REQUIRES_RESPONSE)];

Done.

[Evan Stade, 2013-08-30 03:34:07]

On 2013/08/30 03:24:40, Ilya Sherman wrote:
>
https://codereview.chromium.org/23806002/diff/1/chrome/browser/ui/cocoa/autof...
> File chrome/browser/ui/cocoa/autofill/autofill_account_chooser.mm (right):
> 
>
https://codereview.chromium.org/23806002/diff/1/chrome/browser/ui/cocoa/autof...
> chrome/browser/ui/cocoa/autofill/autofill_account_chooser.mm:111:
> setEnabled:(delegate_->SignedInState() != autofill::REQUIRES_RESPONSE)];
> On 2013/08/30 01:48:12, groby wrote:
> > nit: line break after setEnabled:
> > 
> > i.e.
> > [link_ setEnabled:
> >     (delegate_->SignedInState() != autofill::REQUIRES_RESPONSE)];
> 
> Done.

SignedInState() is a function that no longer needs to exist as a part of the
AutofillDialogViewDelegate interface. Please remove it from that interface. You
can modify SignInLinkText() to also return a boolean for enabled/disabled state.
This will allow the view to be a little dumber and exposes less internal state
of the controller.

[Ilya Sherman, 2013-08-30 04:07:05]

On 2013/08/30 03:34:07, Evan Stade wrote:
> SignedInState() is a function that no longer needs to exist as a part of the
> AutofillDialogViewDelegate interface. Please remove it from that interface.
You
> can modify SignInLinkText() to also return a boolean for enabled/disabled
state.
> This will allow the view to be a little dumber and exposes less internal state
> of the controller.

I'm not a fan of returning multiple values from a single function, but I've
replaced SignedInState() with ShouldDisableSignInLink() so that the view is a
little bit dumber.  LG, or would you really prefer changing SignInLinkText() to
return multiple values?

[Evan Stade, 2013-08-30 19:47:17]

On 2013/08/30 04:07:05, Ilya Sherman wrote:
> On 2013/08/30 03:34:07, Evan Stade wrote:
> > SignedInState() is a function that no longer needs to exist as a part of the
> > AutofillDialogViewDelegate interface. Please remove it from that interface.
> You
> > can modify SignInLinkText() to also return a boolean for enabled/disabled
> state.
> > This will allow the view to be a little dumber and exposes less internal
state
> > of the controller.
> 
> I'm not a fan of returning multiple values from a single function, but I've
> replaced SignedInState() with ShouldDisableSignInLink() so that the view is a
> little bit dumber.  LG, or would you really prefer changing SignInLinkText()
to
> return multiple values?

It's a matter of taste, so this LGTM. FWIW, the reason I suggested returning
multiple values is because

a) I prefer fewer functions in the interface
b) the two functions are always called together

There are many functions in the interface which return a struct of related data.
If they were split into separate functions it would explode the size of the
interface.

https://codereview.chromium.org/23806002/diff/16001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_view_delegate.h (left):

https://codereview.chromium.org/23806002/diff/16001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_view_delegate.h:55: virtual
DialogSignedInState SignedInState() const = 0;
since the enum is now only used privately in the controller, perhaps you can
remove the enum from the public dialog_types header

[Ilya Sherman, 2013-08-30 21:45:58]

https://codereview.chromium.org/23806002/diff/16001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_view_delegate.h (left):

https://codereview.chromium.org/23806002/diff/16001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_view_delegate.h:55: virtual
DialogSignedInState SignedInState() const = 0;
On 2013/08/30 19:47:17, Evan Stade wrote:
> since the enum is now only used privately in the controller, perhaps you can
> remove the enum from the public dialog_types header

Done.

[commit-bot: I haz the power, 2013-08-30 21:46:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/23806002/22001

[commit-bot: I haz the power, 2013-08-30 21:46:06]

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 FAILED at 716.
  Hunk #2 succeeded at 951 (offset 47 lines).
  1 out of 2 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
450878109a89597df085498832a059a898b83da7..8fd5ecfe9295f6387db77ef644359e6b534e180a
100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
@@ -716,20 +716,8 @@ string16 AutofillDialogControllerImpl::LegalDocumentsText()
{
   return legal_documents_text_;
 }
 
-DialogSignedInState AutofillDialogControllerImpl::SignedInState() const {
-  if (account_chooser_model_.HadWalletError())
-    return SIGN_IN_DISABLED;
-
-  if (signin_helper_ || !wallet_items_)
-    return REQUIRES_RESPONSE;
-
-  if (wallet_items_->HasRequiredAction(wallet::GAIA_AUTH))
-    return REQUIRES_SIGN_IN;
-
-  if (wallet_items_->HasRequiredAction(wallet::PASSIVE_GAIA_AUTH))
-    return REQUIRES_PASSIVE_SIGN_IN;
-
-  return SIGNED_IN;
+bool AutofillDialogControllerImpl::ShouldDisableSignInLink() const {
+  return SignedInState() == REQUIRES_RESPONSE;
 }
 
 bool AutofillDialogControllerImpl::ShouldShowSpinner() const {
@@ -904,6 +892,23 @@ void AutofillDialogControllerImpl::HideSignIn() {
   view_->UpdateAccountChooser();
 }
 
+AutofillDialogControllerImpl::DialogSignedInState
+    AutofillDialogControllerImpl::SignedInState() const {
+  if (account_chooser_model_.HadWalletError())
+    return SIGN_IN_DISABLED;
+
+  if (signin_helper_ || !wallet_items_)
+    return REQUIRES_RESPONSE;
+
+  if (wallet_items_->HasRequiredAction(wallet::GAIA_AUTH))
+    return REQUIRES_SIGN_IN;
+
+  if (wallet_items_->HasRequiredAction(wallet::PASSIVE_GAIA_AUTH))
+    return REQUIRES_PASSIVE_SIGN_IN;
+
+  return SIGNED_IN;
+}
+
 void AutofillDialogControllerImpl::SignedInStateUpdated() {
   switch (SignedInState()) {
     case SIGNED_IN:

[commit-bot: I haz the power, 2013-08-30 22:00:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/23806002/26001

[commit-bot: I haz the power, 2013-09-02 05:05:16]

Failed to apply patch for chrome/browser/ui/autofill/autofill_dialog_types.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/autofill/autofill_dialog_types.h
  Hunk #1 FAILED at 184.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/autofill/autofill_dialog_types.h.rej

Patch:       chrome/browser/ui/autofill/autofill_dialog_types.h
Index: chrome/browser/ui/autofill/autofill_dialog_types.h
diff --git a/chrome/browser/ui/autofill/autofill_dialog_types.h
b/chrome/browser/ui/autofill/autofill_dialog_types.h
index
914ee75d393d293a9c0bc11497f1299756d80daa..3c3c2000fed919b4e3f63dd91ea8ed9d6efb1e0c
100644
--- a/chrome/browser/ui/autofill/autofill_dialog_types.h
+++ b/chrome/browser/ui/autofill/autofill_dialog_types.h
@@ -184,14 +184,6 @@ class DialogAutocheckoutStep {
 
 extern SkColor const kWarningColor;
 
-enum DialogSignedInState {
-  REQUIRES_RESPONSE,
-  REQUIRES_SIGN_IN,
-  REQUIRES_PASSIVE_SIGN_IN,
-  SIGNED_IN,
-  SIGN_IN_DISABLED,
-};
-
 // Overall state of the Autocheckout flow.
 enum AutocheckoutState {
   AUTOCHECKOUT_ERROR,        // There was an error in the flow.

[commit-bot: I haz the power, 2013-09-03 22:50:11]

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

[commit-bot: I haz the power, 2013-09-04 17:16:41]

Change committed as 221213