Implemented inline login dialog for Chrome OS.

Implemented inline login dialog for Chrome OS.

Also StubUserAccountsDelegate was replaced with real implementation, so now we have fully functional version of mirror UI on Chrome OS.

BUG=344844
TEST=manually

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

[dzhioev (left Google), 2014-04-24 20:06:39]

Hi, please review.

xiyuan@:
chrome/browser/chromeos/*
chrome/browser/extensions/signin/*
chrome/browser/ui/webui/*

oshima@:
*/ash/*

ben@:
ui/*

[xiyuan, 2014-04-24 21:22:10]

https://codereview.chromium.org/256623002/diff/1/ash/system/user/user_account...
File ash/system/user/user_accounts_delegate.h (right):

https://codereview.chromium.org/256623002/diff/1/ash/system/user/user_account...
ash/system/user/user_accounts_delegate.h:47: // name "Test-us.er@gmail.com".
Why changing this? Anything no longer true with the existing comment?

https://codereview.chromium.org/256623002/diff/1/chrome/browser/chromeos/ui/i...
File chrome/browser/chromeos/ui/inline_login_dialog.h (right):

https://codereview.chromium.org/256623002/diff/1/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/inline_login_dialog.h:18: class InlineLoginDialog :
public ui::WebDialogDelegate {
Can we consolidate this one with the one in
chrome/browser/ui/sync/inline_login_dialog.h ?

They look the same except the size and URL. We probably can just update the old
one with the new params in this CL.

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/ash/user_a...
File chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc (right):

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/ash/user_a...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:35:
->GetAuthenticatedUsername();
Should this be GetAuthenticatedAccountId? Right now both returns the same thing
but that is going to change in the future.

The reason I ask because the value returned here is used as |account_id| for
GetAccountDisplayName below. That might break in the future.

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/ash/user_a...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:46: if
(std::distance(it, accounts.end()) != 1)
nit: LOG_IF since this "if" only do logging.

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc
(right):

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:7:
#include "base/strings/utf_string_conversions.h"
Is this used?

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:55: };
nit: DISALLOW_COPY_AND_ASSIGN since you are here. :p

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:81:
"https://www.google.com/intl/en-US/chrome/blank.html?source=10");
Should we get the url from GetLandingURL() in signin_promo.h?

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/sign...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/inline_login_handler.cc:8: #include
"base/strings/utf_string_conversions.h"
Is this used?

[dzhioev (left Google), 2014-04-24 22:06:19]

https://codereview.chromium.org/256623002/diff/1/ash/system/user/user_account...
File ash/system/user/user_accounts_delegate.h (right):

https://codereview.chromium.org/256623002/diff/1/ash/system/user/user_account...
ash/system/user/user_accounts_delegate.h:47: // name "Test-us.er@gmail.com".
On 2014/04/24 21:22:10, xiyuan wrote:
> Why changing this? Anything no longer true with the existing comment?
Yes, I figured out that GAIA doesn't accept "original_email+something@gmail.com"
emails.

https://codereview.chromium.org/256623002/diff/1/chrome/browser/chromeos/ui/i...
File chrome/browser/chromeos/ui/inline_login_dialog.h (right):

https://codereview.chromium.org/256623002/diff/1/chrome/browser/chromeos/ui/i...
chrome/browser/chromeos/ui/inline_login_dialog.h:18: class InlineLoginDialog :
public ui::WebDialogDelegate {
On 2014/04/24 21:22:10, xiyuan wrote:
> Can we consolidate this one with the one in
> chrome/browser/ui/sync/inline_login_dialog.h ?
> 
> They look the same except the size and URL. We probably can just update the
old
> one with the new params in this CL. 
I'm planning to customize this dialog later ( http://goo.gl/9r9eko ), so
implementation will change significantly.

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/ash/user_a...
File chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc (right):

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/ash/user_a...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:35:
->GetAuthenticatedUsername();
On 2014/04/24 21:22:10, xiyuan wrote:
> Should this be GetAuthenticatedAccountId? Right now both returns the same
thing
> but that is going to change in the future.
> 
> The reason I ask because the value returned here is used as |account_id| for
> GetAccountDisplayName below. That might break in the future.
I'm little confused. Comment for GetAuthenticatedAccountId says that AccountId
could be obfuscated GAIA id. On the other hand I need human-readable email to
display it to user. Is there some way to convert AccountId to email?

The |GetAccountDisplayName| is only needed now to convert canonized email to
"display email" for primary account.

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/ash/user_a...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:46: if
(std::distance(it, accounts.end()) != 1)
On 2014/04/24 21:22:10, xiyuan wrote:
> nit: LOG_IF since this "if" only do logging.

Done.

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc
(right):

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:7:
#include "base/strings/utf_string_conversions.h"
On 2014/04/24 21:22:10, xiyuan wrote:
> Is this used?
No, removed it.

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:55: };
On 2014/04/24 21:22:10, xiyuan wrote:
> nit: DISALLOW_COPY_AND_ASSIGN since you are here. :p

Done.

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:81:
"https://www.google.com/intl/en-US/chrome/blank.html?source=10");
On 2014/04/24 21:22:10, xiyuan wrote:
> Should we get the url from GetLandingURL() in signin_promo.h?
Yes, sorry I forgot about it. Could you please explain what "source" parameter
means? As long as I can understand it takes values from signin::Source enum.
Should I add new source for Chrome OS inline login?

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/sign...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/inline_login_handler.cc:8: #include
"base/strings/utf_string_conversions.h"
On 2014/04/24 21:22:10, xiyuan wrote:
> Is this used?
Yes, for base::UTF16ToASCII

[xiyuan, 2014-04-24 22:31:51]

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/ash/user_a...
File chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc (right):

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/ash/user_a...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:35:
->GetAuthenticatedUsername();
The plan is to make GetAuthenticatedAccountId return obfuscated GAIA id.
GetAuthenticatedUsername will be return Gaia email and can be used for display.
My impression is that account id to email mapping is not implemented at the
moment. In the future, we might put the id to User class for ChromeOS and could
use that to map id back to display name. rogerta@ knows more on this topic.

I guess this is okay for now since we always put canonicalized email to
SigninManagerBase.

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc
(right):

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:81:
"https://www.google.com/intl/en-US/chrome/blank.html?source=10");
On 2014/04/24 22:06:20, dzhioev wrote:
> On 2014/04/24 21:22:10, xiyuan wrote:
> > Should we get the url from GetLandingURL() in signin_promo.h?
> Yes, sorry I forgot about it. Could you please explain what "source" parameter
> means? As long as I can understand it takes values from signin::Source enum.
> Should I add new source for Chrome OS inline login?

It's the enum here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig...

Think it's for logging purpose. I think we should add a new field. But please
check with rogerta@.

[dzhioev (left Google), 2014-04-24 23:05:47]

+rogerta@

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc
(right):

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:81:
"https://www.google.com/intl/en-US/chrome/blank.html?source=10");
On 2014/04/24 22:31:51, xiyuan wrote:
> On 2014/04/24 22:06:20, dzhioev wrote:
> > On 2014/04/24 21:22:10, xiyuan wrote:
> > > Should we get the url from GetLandingURL() in signin_promo.h?
> > Yes, sorry I forgot about it. Could you please explain what "source"
parameter
> > means? As long as I can understand it takes values from signin::Source enum.
> > Should I add new source for Chrome OS inline login?
> 
> It's the enum here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig...
> 
> Think it's for logging purpose. I think we should add a new field. But please
> check with rogerta@.

Roger, should I add the new field?

[oshima, 2014-04-24 23:47:15]

ash/ lgtm

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/oauth2_login_manager.cc (right):

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/oauth2_login_manager.cc:223:
oauth2_token_fetcher_->StartExchangeFromCookies("");
std::string()

[dzhioev (left Google), 2014-04-28 15:36:12]

Oh, I've forgotten to actually add Roger as a reviewer.

Roger, should I add Chrome OS-specific "source" value for landing page? Should
it also be added somewhere in GAIA side?

[Roger Tawa OOO till Jul 10th, 2014-04-28 19:06:21]

Adding Hui for review of the inline login handler code.

Wrt "source", I guess you mean define a new source instead of using
signin::SOURCE_AVATAR_BUBBLE_ADD_ACCOUNT?  We use this for uma purposes to know
from where a sign-in happened, and we can filter uma based on platform, so I
don't see a great need to create a new source.

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/oauth2_token_fetcher.h (right):

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/oauth2_token_fetcher.h:69: };
This class should be consolidated with SigninOAuthHelper, since they both do the
same thing.

Can we move this class to //components/signin/core/browser and then merge the
two?  I know this is not necessarily trivial since this class depends on some
chromeos specific code and on //content.

Alternatively, could SigninOAuthHelper be extended so that OAuth2TokenFetcher
uses it?  That way OAuth2TokenFetcher can remain here and do only the cros
specific stuff.

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ui/inline_login_dialog.h (right):

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ui/inline_login_dialog.h:27: virtual ui::ModalType
GetDialogModalType() const OVERRIDE;
Why extra ui:: here and line 18 above if this is already in ui namespace?

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/ui/ash/us...
File chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc (right):

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/ui/ash/us...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:35:
->GetAuthenticatedUsername();
What is the intent of the return value?  Will the string be used to display
something to the user, or this just an id for internal purposes?  If the former,
I would rename GetPrimaryAccount() --> GetPrimaryUsermame().

If the later, use ->GetAuthenticatedAccountId() instead and rename to
GetPrimaryAccountId().

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/ui/ash/us...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:51: }
Same question as above.  Also I'd suggest changing the function to be called
either GetSecondaryUsernames() or GetSecondaryAccountIds() as appropriate.

[dzhioev (left Google), 2014-04-28 19:49:16]

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/oauth2_login_manager.cc (right):

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/oauth2_login_manager.cc:223:
oauth2_token_fetcher_->StartExchangeFromCookies("");
On 2014/04/24 23:47:16, oshima wrote:
> std::string()

Done.

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/oauth2_token_fetcher.h (right):

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/oauth2_token_fetcher.h:69: };
On 2014/04/28 19:06:22, Roger Tawa wrote:
> This class should be consolidated with SigninOAuthHelper, since they both do
the
> same thing.
> 
> Can we move this class to //components/signin/core/browser and then merge the
> two?  I know this is not necessarily trivial since this class depends on some
> chromeos specific code and on //content.
> 
> Alternatively, could SigninOAuthHelper be extended so that OAuth2TokenFetcher
> uses it?  That way OAuth2TokenFetcher can remain here and do only the cros
> specific stuff.
As I can see, OAuth2TokenFetcher makes only one request
(CookieForOAuthLoginTokenExchange) and SigninOAuthHelper makes two more
(OAuthLogin and GetUserInfo). What are these requests needed for?

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ui/inline_login_dialog.h (right):

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ui/inline_login_dialog.h:27: virtual ui::ModalType
GetDialogModalType() const OVERRIDE;
On 2014/04/28 19:06:22, Roger Tawa wrote:
> Why extra ui:: here and line 18 above if this is already in ui namespace?

Done.

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/ui/ash/us...
File chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc (right):

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/ui/ash/us...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:35:
->GetAuthenticatedUsername();
On 2014/04/28 19:06:22, Roger Tawa wrote:
> What is the intent of the return value?  Will the string be used to display
> something to the user, or this just an id for internal purposes?  If the
former,
> I would rename GetPrimaryAccount() --> GetPrimaryUsermame().
> 
> If the later, use ->GetAuthenticatedAccountId() instead and rename to
> GetPrimaryAccountId().
It is used as ID and isn't displayed to user. The value that is displayed to
user can be retrieved by call to GetAccountDisplayName. 

If I use GetAuthenticatedAccountId here how can I convert it to corresponding
account name in GetAccountDisplayName (comment to GetAuthenticatedAccountId says
that it will be returning some obfuscated value in future)?

[Roger Tawa OOO till Jul 10th, 2014-04-29 00:03:25]

lgtm

I think the rafactoring I was asking about should happen separately.

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/oauth2_token_fetcher.h (right):

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/oauth2_token_fetcher.h:69: };
On 2014/04/28 19:49:17, dzhioev wrote:
> On 2014/04/28 19:06:22, Roger Tawa wrote:
> > This class should be consolidated with SigninOAuthHelper, since they both do
> the
> > same thing.
> > 
> > Can we move this class to //components/signin/core/browser and then merge
the
> > two?  I know this is not necessarily trivial since this class depends on
some
> > chromeos specific code and on //content.
> > 
> > Alternatively, could SigninOAuthHelper be extended so that
OAuth2TokenFetcher
> > uses it?  That way OAuth2TokenFetcher can remain here and do only the cros
> > specific stuff.
> As I can see, OAuth2TokenFetcher makes only one request
> (CookieForOAuthLoginTokenExchange) and SigninOAuthHelper makes two more
> (OAuthLogin and GetUserInfo). What are these requests needed for?

OK, seems like this refactoring is bigger than I thought.  Lets leave it for
later.

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/ui/ash/us...
File chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc (right):

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/ui/ash/us...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:35:
->GetAuthenticatedUsername();
On 2014/04/28 19:49:17, dzhioev wrote:
> On 2014/04/28 19:06:22, Roger Tawa wrote:
> > What is the intent of the return value?  Will the string be used to display
> > something to the user, or this just an id for internal purposes?  If the
> former,
> > I would rename GetPrimaryAccount() --> GetPrimaryUsermame().
> > 
> > If the later, use ->GetAuthenticatedAccountId() instead and rename to
> > GetPrimaryAccountId().
> It is used as ID and isn't displayed to user. The value that is displayed to
> user can be retrieved by call to GetAccountDisplayName. 
> 
> If I use GetAuthenticatedAccountId here how can I convert it to corresponding
> account name in GetAccountDisplayName (comment to GetAuthenticatedAccountId
says
> that it will be returning some obfuscated value in future)?

Today username == account_id, so the code will work.  When chrome uses the gaia
obfuscated id as |account_id| this will need to be updated, but so will a lot of
places in chrome.  (The |User| class should have an |account_id()| method.)

So I still think its best to call GetAuthenticatedAccountId() here.

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/ui/ash/us...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:56: if (user->email()
== account_id && !user->display_email().empty())
Instead of using std::string::op==(), please use gaia::AreEmailsSame() to
compare emails.

[dzhioev (left Google), 2014-04-29 12:23:58]

Hui and Xiyuan,
I will be very grateful if you find a time to review this CL before 8am PDT
today. I wanted to land this CL in M36, but I'm leaving for long vacation today
and returning after branch point.

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc
(right):

https://codereview.chromium.org/256623002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:81:
"https://www.google.com/intl/en-US/chrome/blank.html?source=10");
On 2014/04/24 23:05:47, dzhioev wrote:
> On 2014/04/24 22:31:51, xiyuan wrote:
> > On 2014/04/24 22:06:20, dzhioev wrote:
> > > On 2014/04/24 21:22:10, xiyuan wrote:
> > > > Should we get the url from GetLandingURL() in signin_promo.h?
> > > Yes, sorry I forgot about it. Could you please explain what "source"
> parameter
> > > means? As long as I can understand it takes values from signin::Source
enum.
> > > Should I add new source for Chrome OS inline login?
> > 
> > It's the enum here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig...
> > 
> > Think it's for logging purpose. I think we should add a new field. But
please
> > check with rogerta@.
> 
> Roger, should I add the new field?
Replaced with GetLandingURL. Roger said that it is OK to use
SOURCE_AVATAR_BUBBLE_ADD_ACCOUNT here.

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/ui/ash/us...
File chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc (right):

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/ui/ash/us...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:35:
->GetAuthenticatedUsername();
On 2014/04/29 00:03:25, Roger Tawa wrote:
> On 2014/04/28 19:49:17, dzhioev wrote:
> > On 2014/04/28 19:06:22, Roger Tawa wrote:
> > > What is the intent of the return value?  Will the string be used to
display
> > > something to the user, or this just an id for internal purposes?  If the
> > former,
> > > I would rename GetPrimaryAccount() --> GetPrimaryUsermame().
> > > 
> > > If the later, use ->GetAuthenticatedAccountId() instead and rename to
> > > GetPrimaryAccountId().
> > It is used as ID and isn't displayed to user. The value that is displayed to
> > user can be retrieved by call to GetAccountDisplayName. 
> > 
> > If I use GetAuthenticatedAccountId here how can I convert it to
corresponding
> > account name in GetAccountDisplayName (comment to GetAuthenticatedAccountId
> says
> > that it will be returning some obfuscated value in future)?
> 
> Today username == account_id, so the code will work.  When chrome uses the
gaia
> obfuscated id as |account_id| this will need to be updated, but so will a lot
of
> places in chrome.  (The |User| class should have an |account_id()| method.)
> 
> So I still think its best to call GetAuthenticatedAccountId() here.

Done, used GetAuthenticatedAccountId here and renamed methods.

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/ui/ash/us...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:51: }
On 2014/04/28 19:06:22, Roger Tawa wrote:
> Same question as above.  Also I'd suggest changing the function to be called
> either GetSecondaryUsernames() or GetSecondaryAccountIds() as appropriate.

Done.

https://codereview.chromium.org/256623002/diff/20001/chrome/browser/ui/ash/us...
chrome/browser/ui/ash/user_accounts_delegate_chromeos.cc:56: if (user->email()
== account_id && !user->display_email().empty())
On 2014/04/29 00:03:25, Roger Tawa wrote:
> Instead of using std::string::op==(), please use gaia::AreEmailsSame() to
> compare emails.

Done.

[guohui, 2014-04-29 13:33:06]

lgtm with one comment.

https://codereview.chromium.org/256623002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc
(right):

https://codereview.chromium.org/256623002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:66:
params.SetInteger("authMode", InlineLoginHandler::kDesktopAuthMode);
most code here is duplicated in InlineLOginHandlerImpl::SetExtraInitParams, we
should extract common logic  into InlineLoginHandler::HandleInitializeMessage.

i am not sure if you could reuse the kDesktopAuthMode without any modification,
e.g. X-Cros-Auth-Ext-Support is not set in desktop mode, and i thought it was
required for enabling saml support on cros?

[dzhioev (left Google), 2014-04-29 14:27:21]

https://codereview.chromium.org/256623002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc
(right):

https://codereview.chromium.org/256623002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:66:
params.SetInteger("authMode", InlineLoginHandler::kDesktopAuthMode);
On 2014/04/29 13:33:07, guohui wrote:
> most code here is duplicated in InlineLOginHandlerImpl::SetExtraInitParams, we
> should extract common logic  into InlineLoginHandler::HandleInitializeMessage.
>

Done. After that I was able to remove
InlineLoginHandlerChromeOS::SetExtraInitParams completely. 

> i am not sure if you could reuse the kDesktopAuthMode without any
modification,
> e.g. X-Cros-Auth-Ext-Support is not set in desktop mode, and i thought it was
> required for enabling saml support on cros?

As long as desktop implementation for adding accounts doesn't support SAML, I
think it is OK if Chrome OS implementation doesn't support it as well.

[guohui, 2014-04-29 15:58:57]

On 2014/04/29 14:27:21, dzhioev wrote:
>
https://codereview.chromium.org/256623002/diff/60001/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc
> (right):
> 
>
https://codereview.chromium.org/256623002/diff/60001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:66:
> params.SetInteger("authMode", InlineLoginHandler::kDesktopAuthMode);
> On 2014/04/29 13:33:07, guohui wrote:
> > most code here is duplicated in InlineLOginHandlerImpl::SetExtraInitParams,
we
> > should extract common logic  into
InlineLoginHandler::HandleInitializeMessage.
> >
> 
> Done. After that I was able to remove
> InlineLoginHandlerChromeOS::SetExtraInitParams completely. 
> 
> > i am not sure if you could reuse the kDesktopAuthMode without any
> modification,
> > e.g. X-Cros-Auth-Ext-Support is not set in desktop mode, and i thought it
was
> > required for enabling saml support on cros?
> 
> As long as desktop implementation for adding accounts doesn't support SAML, I
> think it is OK if Chrome OS implementation doesn't support it as well.

desktop implementation does support SAML, we just don't set the
X-Cros-Auth-Ext-Support header to enable SAML support on gaia side, since Gaia
always enables X-Cros-Auth-Ext-Support for desktop chrome.

Anyway, i think you could add a comment and leave the SAML support to future
CLs.

[xiyuan, 2014-04-29 16:31:14]

On 2014/04/29 15:58:57, guohui wrote:
> desktop implementation does support SAML, we just don't set the
> X-Cros-Auth-Ext-Support header to enable SAML support on gaia side, since Gaia
> always enables X-Cros-Auth-Ext-Support for desktop chrome.
> 
> Anyway, i think you could add a comment and leave the SAML support to future
> CLs.

X-Cros-Auth-Ext-Support is only need for Cros login screen because Gaia disabled
SAML for Cros login before and we need to a way to let Gaia to re-enable it. The
header is created for this purpose. In adding account case, it is enabled by
default for Cros as well.

[xiyuan, 2014-04-29 16:43:15]

LGTM

ben@ please review the web_dialog part where a CanResizeDialog is exposed on
WebDialogDelegate so that we could create a non-resizable web dialog.

https://codereview.chromium.org/256623002/diff/70001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc
(right):

https://codereview.chromium.org/256623002/diff/70001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:11:
#include "chrome/browser/signin/signin_promo.h"
nit: is this still used?

https://codereview.chromium.org/256623002/diff/70001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:16:
#include "content/public/browser/web_contents.h"
nit: is this still used?

https://codereview.chromium.org/256623002/diff/70001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:18:
#include "google_apis/gaia/gaia_urls.h"
nit: is this still used?

https://codereview.chromium.org/256623002/diff/70001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:19:
#include "net/base/url_util.h"
nit: is this still used?

https://codereview.chromium.org/256623002/diff/70001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/256623002/diff/70001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:182: signin::Source
source = signin::GetSourceForPromoURL(
nit: you still need signin_promo.h for this.

[Ben Goodger (Google), 2014-04-29 16:57:20]

ui lgtm

[Nikita (slow), 2014-05-06 15:25:17]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-05-06 15:25:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dzhioev@chromium.org/256623002/70001

[commit-bot: I haz the power, 2014-05-06 15:52:27]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
  android_clang_dbg on tryserver.chromium
  android_dbg on tryserver.chromium
  chromium_presubmit on tryserver.chromium
  ios_dbg_simulator on tryserver.chromium
  ios_rel_device on tryserver.chromium
  ios_rel_device_ninja on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium
  mac_chromium_compile_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium
  win_chromium_compile_dbg on tryserver.chromium
  win_chromium_rel on tryserver.chromium
  win_chromium_x64_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-06 16:19:34]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
  android_clang_dbg on tryserver.chromium
  android_dbg on tryserver.chromium
  chromium_presubmit on tryserver.chromium
  ios_dbg_simulator on tryserver.chromium
  ios_rel_device on tryserver.chromium
  ios_rel_device_ninja on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium
  mac_chromium_compile_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium
  win_chromium_compile_dbg on tryserver.chromium
  win_chromium_rel on tryserver.chromium
  win_chromium_x64_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-06 16:22:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-06 16:22:41]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium

[dzhioev (left Google), 2014-05-12 12:41:25]

The CQ bit was checked by dzhioev@chromium.org

[commit-bot: I haz the power, 2014-05-12 12:41:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dzhioev@chromium.org/256623002/80001

[commit-bot: I haz the power, 2014-05-12 14:25:24]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-12 15:03:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-12 15:03:20]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[dzhioev (left Google), 2014-05-12 15:24:59]

The CQ bit was checked by dzhioev@chromium.org

[commit-bot: I haz the power, 2014-05-12 15:25:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dzhioev@chromium.org/256623002/80001

[commit-bot: I haz the power, 2014-05-12 16:08:31]

Change committed as 269792