Upstream changes needed by Android auto-login.

Upstream changes needed by Android auto-login.

This is part of Chrome for Android upstreaming.

This adapts AutoLoginInfoBarDelegate/AutoLoginPrompter so that the Android
implementation of the auto-login bar on the app side can reuse these classes.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144907

[Nico, 2012-06-27 15:14:33]

LGTM

Looks great! Please wait for Roger too though.

https://chromiumcodereview.appspot.com/10690006/diff/2001/chrome/browser/ui/a...
File chrome/browser/ui/auto_login_info_bar_delegate.h (right):

https://chromiumcodereview.appspot.com/10690006/diff/2001/chrome/browser/ui/a...
chrome/browser/ui/auto_login_info_bar_delegate.h:69: std::string username_;
Nit: This probably isn't always ascii. In that case, this should be a string16.
If that's not true, then GetMessageText() above should get a string.

https://chromiumcodereview.appspot.com/10690006/diff/2001/chrome/browser/ui/a...
File chrome/browser/ui/auto_login_prompter.cc (right):

https://chromiumcodereview.appspot.com/10690006/diff/2001/chrome/browser/ui/a...
chrome/browser/ui/auto_login_prompter.cc:176: output->realm =
net::UnescapeURLComponent(
nit: functions with a bool result generally shouldn't modify output parameters
when they return false

[bulach, 2012-06-27 15:56:33]

lgtm for the android side, thanks!

[John Grabowski, 2012-06-27 16:37:02]

Another LGTM from the Android side

[Philippe, 2012-06-27 16:52:02]

Thanks for the quick review guys!

Please take another quick look Nico. I made some small changes after your
comments.
Let me know what you think about the potential encoding issue during header
parsing.

I'm also adding sky@ who was missing for chrome/browser/ui (which is actually
the main part).

https://chromiumcodereview.appspot.com/10690006/diff/2001/chrome/browser/ui/a...
File chrome/browser/ui/auto_login_info_bar_delegate.h (right):

https://chromiumcodereview.appspot.com/10690006/diff/2001/chrome/browser/ui/a...
chrome/browser/ui/auto_login_info_bar_delegate.h:69: std::string username_;
On 2012/06/27 15:14:33, Nico wrote:
> Nit: This probably isn't always ascii. In that case, this should be a
string16.
> If that's not true, then GetMessageText() above should get a string.

Indeed it seems possible to have non-ASCII characters in e-mail addresses. I
double checked on Wikipedia (http://en.wikipedia.org/wiki/Email_address) where I
saw this example: Rδοκιμή@παράδειγμα.δοκιμή :)

This field, username_, is not extracted from the x-auto-login header. However
some others which might contain non-ASCII are (e.g. the "account" field). In
that case we might have an internationalization issue when we parse the header.

We use SplitStringIntoKeyValuePairs() which is not unicode aware I suppose (I
haven't checked).

Can I add a TODO(pliard) and investigate/fix these internationalization issues
(if any) in a next CL?

https://chromiumcodereview.appspot.com/10690006/diff/2001/chrome/browser/ui/a...
File chrome/browser/ui/auto_login_prompter.cc (right):

https://chromiumcodereview.appspot.com/10690006/diff/2001/chrome/browser/ui/a...
chrome/browser/ui/auto_login_prompter.cc:176: output->realm =
net::UnescapeURLComponent(
On 2012/06/27 15:14:33, Nico wrote:
> nit: functions with a bool result generally shouldn't modify output parameters
> when they return false

Good point.

[Nico, 2012-06-27 16:54:51]

LGTM

https://chromiumcodereview.appspot.com/10690006/diff/2001/chrome/browser/ui/a...
File chrome/browser/ui/auto_login_info_bar_delegate.h (right):

https://chromiumcodereview.appspot.com/10690006/diff/2001/chrome/browser/ui/a...
chrome/browser/ui/auto_login_info_bar_delegate.h:69: std::string username_;
On 2012/06/27 16:52:02, Philippe wrote:
> On 2012/06/27 15:14:33, Nico wrote:
> > Nit: This probably isn't always ascii. In that case, this should be a
> string16.
> > If that's not true, then GetMessageText() above should get a string.
> 
> Indeed it seems possible to have non-ASCII characters in e-mail addresses. I
> double checked on Wikipedia (http://en.wikipedia.org/wiki/Email_address) where
I
> saw this example: Rδοκιμή@παράδειγμα.δοκιμή :)
> 
> This field, username_, is not extracted from the x-auto-login header. However
> some others which might contain non-ASCII are (e.g. the "account" field). In
> that case we might have an internationalization issue when we parse the
header.
> 
> We use SplitStringIntoKeyValuePairs() which is not unicode aware I suppose (I
> haven't checked).
> 
> Can I add a TODO(pliard) and investigate/fix these internationalization issues
> (if any) in a next CL? 

That's fine (if it works, adding a test would be nice).

[sky, 2012-06-27 17:05:41]

I'm swapping myself for Peter. He is better suited for infobar related patches.

[Peter Kasting, 2012-06-27 17:39:05]

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/inf...
File chrome/browser/infobars/infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/inf...
chrome/browser/infobars/infobar_delegate.h:101: virtual
AutoLoginInfoBarDelegate* AsAutoLoginInfoBarDelegate();
Why are you adding this?  You don't call it directly and we try very hard not to
add new ones of these.  Normally code that needs to cast can just static_cast<>
because it knows it has an auto-login infobar.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/auto_login_info_bar_delegate.cc (right):

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.cc:160:
AutoLoginInfoBarDelegate::AsAutoLoginInfoBarDelegate() {
Nit: Indent 4

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/auto_login_info_bar_delegate.h (right):

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.h:29: virtual
AutoLoginInfoBarDelegate* AsAutoLoginInfoBarDelegate() OVERRIDE;
Nit: AutoLoginInfoBarDelegate doesn't inherit directly from this class, so don't
create a section for it.  Place this function in the appropriate place in the
list below.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.h:40: string16
GetMessageText(const std::string& username) const;
Similar to my question about the downcast routine: you don't call any of these
new functions directly.  You need to either remove them or document precisely
why you need them and how some non-Android maintainer can know how to
refactor/modify/remove them in the future.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.h:42: const std::string& realm()
const {
Nit: All on one line (3 places)

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.h:45: 
Nit: Remove blank lines between getters

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/auto_login_prompter.cc (right):

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.cc:122: Params params_copy(params);
Nit: Why not pass this in by value?

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.cc:124: // On Android, the username is
fetched later from the AccountManager on the
Nit: What does "later" mean?  This function must be called twice on Android? 
This function must be called before Android then does something different to
create the infobar that doesn't use this function at all?

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.cc:133: params_copy.username !=
params.account) {
Nit: {} not needed

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.cc:152: infobar_helper,
Nit: Strange wrapping.  Put more args on this line.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/auto_login_prompter.h (right):

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.h:43: // Extracted from X-Auto-Login
header.
Nit: Blank line above

[Philippe, 2012-06-28 09:24:00]

Thanks Peter.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/inf...
File chrome/browser/infobars/infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/inf...
chrome/browser/infobars/infobar_delegate.h:101: virtual
AutoLoginInfoBarDelegate* AsAutoLoginInfoBarDelegate();
On 2012/06/27 17:39:05, Peter Kasting wrote:
> Why are you adding this?  You don't call it directly and we try very hard not
to
> add new ones of these.  Normally code that needs to cast can just
static_cast<>
> because it knows it has an auto-login infobar.

I understand your concern here. This CL only upstreams the changes we made on
the chromium side. We also have the app side which directly uses this class (and
this method).

Unfortunately our current implementation on the app side does something like
that:

if (delegate->AsAutoLoginInfoBarDelegate())
  AddAutoLoginInfoBar(delegate);
else if (delegate->AsConfirmInfoBarDelegate())
  AddConfirmInfoBar(delegate);

I don't think we can easily get rid of this method without introducing a visitor
which wouldn't be better in this particular case (this would probably involve
much more boilerplate code).

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/auto_login_info_bar_delegate.cc (right):

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.cc:160:
AutoLoginInfoBarDelegate::AsAutoLoginInfoBarDelegate() {
On 2012/06/27 17:39:05, Peter Kasting wrote:
> Nit: Indent 4

Done.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/auto_login_info_bar_delegate.h (right):

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.h:29: virtual
AutoLoginInfoBarDelegate* AsAutoLoginInfoBarDelegate() OVERRIDE;
On 2012/06/27 17:39:05, Peter Kasting wrote:
> Nit: AutoLoginInfoBarDelegate doesn't inherit directly from this class, so
don't
> create a section for it.  Place this function in the appropriate place in the
> list below.

Done.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.h:40: string16
GetMessageText(const std::string& username) const;
On 2012/06/27 17:39:05, Peter Kasting wrote:
> Similar to my question about the downcast routine: you don't call any of these
> new functions directly.  You need to either remove them or document precisely
> why you need them and how some non-Android maintainer can know how to
> refactor/modify/remove them in the future.

We use that in the Android implementation of the auto bar on the app side. I
added a comment.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.h:42: const std::string& realm()
const {
On 2012/06/27 17:39:05, Peter Kasting wrote:
> Nit: All on one line (3 places)

Done.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.h:45: 
On 2012/06/27 17:39:05, Peter Kasting wrote:
> Nit: Remove blank lines between getters

Done.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/auto_login_prompter.cc (right):

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.cc:122: Params params_copy(params);
On 2012/06/27 17:39:05, Peter Kasting wrote:
> Nit: Why not pass this in by value?

Done.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.cc:124: // On Android, the username is
fetched later from the AccountManager on the
On 2012/06/27 17:39:05, Peter Kasting wrote:
> Nit: What does "later" mean?  This function must be called twice on Android? 
> This function must be called before Android then does something different to
> create the infobar that doesn't use this function at all?

I removed 'later' and made the comment clearer (I think).

We do use this function. We just fetch the username ourselves and pass it to the
newly added GetMessageText function which takes a username in rather than
directly using |username_|.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.cc:133: params_copy.username !=
params.account) {
On 2012/06/27 17:39:05, Peter Kasting wrote:
> Nit: {} not needed

Done.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.cc:152: infobar_helper,
On 2012/06/27 17:39:05, Peter Kasting wrote:
> Nit: Strange wrapping.  Put more args on this line.

Done.

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/auto_login_prompter.h (right):

https://chromiumcodereview.appspot.com/10690006/diff/14001/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.h:43: // Extracted from X-Auto-Login
header.
On 2012/06/27 17:39:05, Peter Kasting wrote:
> Nit: Blank line above

Done.

[Roger Tawa OOO till Jul 10th, 2012-06-28 14:39:56]

Salut Philippe,

Please see comments below.  I didn't see where the params.username field gets
set in the OS_ANDROID case.

http://codereview.chromium.org/10690006/diff/14002/chrome/browser/ui/auto_log...
File chrome/browser/ui/auto_login_info_bar_delegate.h (right):

http://codereview.chromium.org/10690006/diff/14002/chrome/browser/ui/auto_log...
chrome/browser/ui/auto_login_info_bar_delegate.h:61: std::string username_;
why not replace all the members here with a params instance, and also pass a
params const ref in ctor?

http://codereview.chromium.org/10690006/diff/14002/chrome/browser/ui/auto_log...
File chrome/browser/ui/auto_login_prompter.cc (right):

http://codereview.chromium.org/10690006/diff/14002/chrome/browser/ui/auto_log...
chrome/browser/ui/auto_login_prompter.cc:44: bool
FetchUsernameThroughSync(Profile* profile, std::string* output) {
Please rename this to remove "sync".  The concepts of "signing in" and
"sync'ing" are being separated.  Maybe FetchUsernameThroughSigninManager()?

http://codereview.chromium.org/10690006/diff/14002/chrome/browser/ui/auto_log...
chrome/browser/ui/auto_login_prompter.cc:151: params_.username));
why not pass the params as an arg instead of all the individual members?

http://codereview.chromium.org/10690006/diff/14002/chrome/browser/ui/auto_log...
File chrome/browser/ui/auto_login_prompter.h (right):

http://codereview.chromium.org/10690006/diff/14002/chrome/browser/ui/auto_log...
chrome/browser/ui/auto_login_prompter.h:57: static void
ShowInfoBarUIThread(Params params,
const ref params?

[Philippe, 2012-06-28 15:18:26]

Thanks Roger.

Please let me know what you think about the use of Params in
AutoLoginInfoBarDelegate (extra dependency "issue") so that I can upload a new
patch set if needed. I don't feel strongly about it.

https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
File chrome/browser/ui/auto_login_info_bar_delegate.h (right):

https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.h:61: std::string username_;
On 2012/06/28 14:39:56, Roger Tawa wrote:
> why not replace all the members here with a params instance, and also pass a
> params const ref in ctor?

I wanted to avoid making AutoLoginInfoBarDelegate depend on AutoLoginPrompter.
Should I? I agree that it would remove quite a lot of code.

https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
File chrome/browser/ui/auto_login_prompter.cc (right):

https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.cc:44: bool
FetchUsernameThroughSync(Profile* profile, std::string* output) {
On 2012/06/28 14:39:56, Roger Tawa wrote:
> Please rename this to remove "sync".  The concepts of "signing in" and
> "sync'ing" are being separated.  Maybe FetchUsernameThroughSigninManager()?

Done.

https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.cc:151: params_.username));
On 2012/06/28 14:39:56, Roger Tawa wrote:
> why not pass the params as an arg instead of all the individual members?

I'm ok with doing this change. As I said before I wanted to avoid making
AutoLoginInfoBarDelegate depend on AutoLoginPrompter. Let me know what you
prefer. Moving Params to its own file might be overkill. What do you think?

https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
File chrome/browser/ui/auto_login_prompter.h (right):

https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.h:57: static void
ShowInfoBarUIThread(Params params,
On 2012/06/28 14:39:56, Roger Tawa wrote:
> const ref params?

That's what we had a few patch sets ago :) Peter suggested that I take |params|
by copy since I was making a copy in the method's body anyway.

[Philippe, 2012-06-28 15:20:52]

On 2012/06/28 15:18:26, Philippe wrote:
> Thanks Roger.
> 
> Please let me know what you think about the use of Params in
> AutoLoginInfoBarDelegate (extra dependency "issue") so that I can upload a new
> patch set if needed. I don't feel strongly about it.
> 
>
https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
> File chrome/browser/ui/auto_login_info_bar_delegate.h (right):
> 
>
https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
> chrome/browser/ui/auto_login_info_bar_delegate.h:61: std::string username_;
> On 2012/06/28 14:39:56, Roger Tawa wrote:
> > why not replace all the members here with a params instance, and also pass a
> > params const ref in ctor?
> 
> I wanted to avoid making AutoLoginInfoBarDelegate depend on AutoLoginPrompter.
> Should I? I agree that it would remove quite a lot of code.
> 
>
https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
> File chrome/browser/ui/auto_login_prompter.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
> chrome/browser/ui/auto_login_prompter.cc:44: bool
> FetchUsernameThroughSync(Profile* profile, std::string* output) {
> On 2012/06/28 14:39:56, Roger Tawa wrote:
> > Please rename this to remove "sync".  The concepts of "signing in" and
> > "sync'ing" are being separated.  Maybe FetchUsernameThroughSigninManager()?
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
> chrome/browser/ui/auto_login_prompter.cc:151: params_.username));
> On 2012/06/28 14:39:56, Roger Tawa wrote:
> > why not pass the params as an arg instead of all the individual members?
> 
> I'm ok with doing this change. As I said before I wanted to avoid making
> AutoLoginInfoBarDelegate depend on AutoLoginPrompter. Let me know what you
> prefer. Moving Params to its own file might be overkill. What do you think?
> 
>
https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
> File chrome/browser/ui/auto_login_prompter.h (right):
> 
>
https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
> chrome/browser/ui/auto_login_prompter.h:57: static void
> ShowInfoBarUIThread(Params params,
> On 2012/06/28 14:39:56, Roger Tawa wrote:
> > const ref params?
> 
> That's what we had a few patch sets ago :) Peter suggested that I take
|params|
> by copy since I was making a copy in the method's body anyway.

I clarified the use of |username_| on Android too (not used actually). 
|username_| is used only in GetMessageText(). On Android we use
GetMessageText(const string& username) instead.

[Roger Tawa OOO till Jul 10th, 2012-06-28 15:31:51]

lgtm

I think its worth moving the Params struct from one class to the other so that
dependencies are not affected and the code is simpler and cleaner as you say. 
But I don't want to hold up your upstreaming for this.

btw, you didn't mention anything about the android code path for
params.username.  I'm curious how it works.

http://codereview.chromium.org/10690006/diff/14002/chrome/browser/ui/auto_log...
File chrome/browser/ui/auto_login_info_bar_delegate.h (right):

http://codereview.chromium.org/10690006/diff/14002/chrome/browser/ui/auto_log...
chrome/browser/ui/auto_login_info_bar_delegate.h:61: std::string username_;
On 2012/06/28 15:18:27, Philippe wrote:
> On 2012/06/28 14:39:56, Roger Tawa wrote:
> > why not replace all the members here with a params instance, and also pass a
> > params const ref in ctor?
> 
> I wanted to avoid making AutoLoginInfoBarDelegate depend on AutoLoginPrompter.
> Should I? I agree that it would remove quite a lot of code.

If you move the declaration of struct Params to here, them I believe this does
not change the dependencies.

http://codereview.chromium.org/10690006/diff/14002/chrome/browser/ui/auto_log...
File chrome/browser/ui/auto_login_prompter.h (right):

http://codereview.chromium.org/10690006/diff/14002/chrome/browser/ui/auto_log...
chrome/browser/ui/auto_login_prompter.h:57: static void
ShowInfoBarUIThread(Params params,
On 2012/06/28 15:18:27, Philippe wrote:
> On 2012/06/28 14:39:56, Roger Tawa wrote:
> > const ref params?
> 
> That's what we had a few patch sets ago :) Peter suggested that I take
|params|
> by copy since I was making a copy in the method's body anyway.

OK sorry, I didn't see Peter's comment.  It just seemed weird with the const
GURL& right below it.

[Philippe, 2012-06-28 16:37:34]

I moved Params. Indeed it's nicer. About |username_| you might not have seen my
latest message:

I clarified the use of |username_| on Android too (not used actually). 
|username_| is used only in GetMessageText(). On Android we use
GetMessageText(const string& username) instead.

Here are some additional details:
|username| is fetched on the Java side using Android's AccountManager after
AutoLoginInfoBarDelegate's instantiation. We have our own logic when we fetch
the username to figure out whether we should show the info bar.

To give you more context, we had initially our own implementations of
AutoLoginInfoBarDelegate and AutoLoginPrompter that were actually implemented
before the ones we now have upstream.

We did a recent refactoring to get rid of our private duplicate implementation.
To be able to do that we had to slightly adapt these classes upstream to match
our internal implementation.

https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
File chrome/browser/ui/auto_login_info_bar_delegate.h (right):

https://chromiumcodereview.appspot.com/10690006/diff/14002/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.h:61: std::string username_;
On 2012/06/28 15:31:51, Roger Tawa wrote:
> On 2012/06/28 15:18:27, Philippe wrote:
> > On 2012/06/28 14:39:56, Roger Tawa wrote:
> > > why not replace all the members here with a params instance, and also pass
a
> > > params const ref in ctor?
> > 
> > I wanted to avoid making AutoLoginInfoBarDelegate depend on
AutoLoginPrompter.
> > Should I? I agree that it would remove quite a lot of code.
> 
> If you move the declaration of struct Params to here, them I believe this does
> not change the dependencies.

Indeed.

[Roger Tawa OOO till Jul 10th, 2012-06-28 18:51:02]

lgtm

Awesome, all looks good Philippe.

[Peter Kasting, 2012-06-28 19:13:45]

LGTM

https://chromiumcodereview.appspot.com/10690006/diff/21011/chrome/browser/ui/...
File chrome/browser/ui/auto_login_info_bar_delegate.cc (right):

https://chromiumcodereview.appspot.com/10690006/diff/21011/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.cc:139:
AutoLoginInfoBarDelegate::Params::Params() {}
Nit: Make an AutoLoginInfoBarDelegate::Params section divider for these

https://chromiumcodereview.appspot.com/10690006/diff/21011/chrome/browser/ui/...
File chrome/browser/ui/auto_login_prompter.cc (right):

https://chromiumcodereview.appspot.com/10690006/diff/21011/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.cc:174: params.realm =
net::UnescapeURLComponent(
Nit: You could factor out:

    std::string unescaped_value(net::UnescapeURLComponent(pair.second,
        net::UnescapeRule::URL_SPECIAL_CHARS);
    if (pair.first == "realm") {
      params.realm = unescaped_value;
      ...

And so on, using |unescaped_value| in all three arms

[Philippe, 2012-06-29 08:48:10]

Thanks for the review guys!

https://chromiumcodereview.appspot.com/10690006/diff/21011/chrome/browser/ui/...
File chrome/browser/ui/auto_login_info_bar_delegate.cc (right):

https://chromiumcodereview.appspot.com/10690006/diff/21011/chrome/browser/ui/...
chrome/browser/ui/auto_login_info_bar_delegate.cc:139:
AutoLoginInfoBarDelegate::Params::Params() {}
On 2012/06/28 19:13:45, Peter Kasting wrote:
> Nit: Make an AutoLoginInfoBarDelegate::Params section divider for these

Done.

https://chromiumcodereview.appspot.com/10690006/diff/21011/chrome/browser/ui/...
File chrome/browser/ui/auto_login_prompter.cc (right):

https://chromiumcodereview.appspot.com/10690006/diff/21011/chrome/browser/ui/...
chrome/browser/ui/auto_login_prompter.cc:174: params.realm =
net::UnescapeURLComponent(
On 2012/06/28 19:13:45, Peter Kasting wrote:
> Nit: You could factor out:
> 
>     std::string unescaped_value(net::UnescapeURLComponent(pair.second,
>         net::UnescapeRule::URL_SPECIAL_CHARS);
>     if (pair.first == "realm") {
>       params.realm = unescaped_value;
>       ...
> 
> And so on, using |unescaped_value| in all three arms

Done.

[commit-bot: I haz the power, 2012-06-29 11:02:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10690006/2018

[commit-bot: I haz the power, 2012-06-29 12:07:06]

Try job failure for 10690006-2018 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-06-29 13:21:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10690006/2018

[commit-bot: I haz the power, 2012-06-29 14:23:32]

Change committed as 144907