[rAc] Fetch username concurrently with fetching Wallet items.

[rAc] Fetch username concurrently with fetching Wallet items.

BUG=270859
TEST=Autofill dialog should still work correctly, but a little bit faster.
R=groby@chromium.org

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

[Ilya Sherman, 2013-09-20 06:49:56]

Please hold off on this review... I need to figure out why all the unit tests
are crashing :/

https://chromiumcodereview.appspot.com/23653052/diff/1/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm (right):

https://chromiumcodereview.appspot.com/23653052/diff/1/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:112:
//DCHECK([label isKindOfClass:[NSTextView class]]);
Augh, please ignore this diff.  Btw, this DCHECK consistently fails in Wallet
mode :P

[groby-ooo-7-16, 2013-09-20 16:07:13]

https://chromiumcodereview.appspot.com/23653052/diff/1/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm (right):

https://chromiumcodereview.appspot.com/23653052/diff/1/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:112:
//DCHECK([label isKindOfClass:[NSTextView class]]);
That is not good, because we _expect_ it to be an NSTextView. Ugh.

And if you look at setMessages: all the subviews are created as labels.
On 2013/09/20 06:49:56, Ilya Sherman wrote:
> Augh, please ignore this diff.  Btw, this DCHECK consistently fails in Wallet
> mode :P

[groby-ooo-7-16, 2013-09-20 16:10:19]

https://chromiumcodereview.appspot.com/23653052/diff/1/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm (right):

https://chromiumcodereview.appspot.com/23653052/diff/1/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:112:
//DCHECK([label isKindOfClass:[NSTextView class]]);
And by "created as labels" I mean created as NSTextView. I better get some
coffee :)

On 2013/09/20 16:07:13, groby wrote:
> That is not good, because we _expect_ it to be an NSTextView. Ugh.
> 
> And if you look at setMessages: all the subviews are created as labels.
> On 2013/09/20 06:49:56, Ilya Sherman wrote:
> > Augh, please ignore this diff.  Btw, this DCHECK consistently fails in
Wallet
> > mode :P
>

[Ilya Sherman, 2013-09-20 19:29:53]

Evan, if you're around, could you review this CL?  If not, I'll ask Rachel to
review, but I think you're more familiar with the code.

[Ilya Sherman, 2013-09-21 01:08:37]

Rachel, please take a look -- I think I've finally squashed all the bugs...

[Evan Stade, 2013-09-23 18:13:44]

lgtm

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

https://codereview.chromium.org/23653052/diff/24001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:993:
username_fetcher_.reset(new wallet::WalletSigninHelper(
DCHECK(!username_fetcher_)

https://codereview.chromium.org/23653052/diff/24001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1009:
username_fetcher_.reset();
I don't think username_fetcher_ can be non-null here. If it were, the signed in
state would be REQUIRES_RESPONSE. You could add a DCHECK like the signin_helper_
one though.

[Ilya Sherman, 2013-09-24 00:50:58]

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

https://codereview.chromium.org/23653052/diff/24001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:993:
username_fetcher_.reset(new wallet::WalletSigninHelper(
On 2013/09/23 18:13:44, Evan Stade wrote:
> DCHECK(!username_fetcher_)

Done.

https://codereview.chromium.org/23653052/diff/24001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1009:
username_fetcher_.reset();
On 2013/09/23 18:13:44, Evan Stade wrote:
> I don't think username_fetcher_ can be non-null here. If it were, the signed
in
> state would be REQUIRES_RESPONSE. You could add a DCHECK like the
signin_helper_
> one though.

Oops, that was a bug in SignedInState().  It ought to be possible for
username_fetcher_ to be non-null here, since the username fetch is started
before we get a response from the Wallet server, and there's no reason to wait
for the username fetch to complete once we know that a passive sign-in is
needed.

[Evan Stade, 2013-09-24 01:24:04]

I think MenuModelForAccountChooser() is now broken, because it will return a
menu model when we don't have wallet items (if we just know the account name for
example).

[Ilya Sherman, 2013-09-24 23:06:46]

On 2013/09/24 01:24:04, Evan Stade wrote:
> I think MenuModelForAccountChooser() is now broken, because it will return a
> menu model when we don't have wallet items (if we just know the account name
for
> example).

Okay, fixed.  I audited all the other uses of the account_chooser_model_, and
they all seem to be ok as-is.

[Evan Stade, 2013-09-25 18:24:14]

lgtm

[groby-ooo-7-16, 2013-09-25 18:54:39]

rubberstamp LGTM :)

[commit-bot: I haz the power, 2013-09-25 22:19:50]

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

[commit-bot: I haz the power, 2013-09-26 04:16:15]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-26 06:02:12]

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

[commit-bot: I haz the power, 2013-09-26 09:19:36]

Change committed as 225413