Perform online reauth when password is changed for a locked profile.

Perform online reauth when password is changed for a locked profile.

BUG=472596
Committed: https://crrev.com/ce8820ca9f57406653a5983b0cd5bf4e5a60d390
Cr-Commit-Position: refs/heads/master@{#340411}

[Roger Tawa OOO till Jul 10th, 2015-07-16 19:18:35]

Patchset #1 (id:1) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-07-16 19:18:41]

Patchset #1 (id:20001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-07-16 19:18:50]

Patchset #1 (id:40001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-07-17 19:38:12]

Patchset #1 (id:60001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-07-17 19:38:21]

Patchset #1 (id:80001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-07-17 19:38:31]

Patchset #1 (id:100001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-07-17 21:14:55]

rogerta@chromium.org changed reviewers:
+ bcwhite@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-07-17 21:14:55]

Hi Brian,

Please take a look.  Mac is not yet implemented, and that will be handled in a
follow up CL.  Thanks,

[bcwhite, 2015-07-21 13:28:39]

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/signin_promo.h (right):

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/signin/...
chrome/browser/signin/signin_promo.h:66: GURL GetReauthURL(size_t profile_index,
const std::string& email);
These two methods are keyed based on the second parameter (email vs account-id)
but are differentiated by the compiler based solely on the first parameter.

I suggest different names for these methods and have the first parameters be the
same.

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:177: // webview and
observe it instead.  The webview may not be found in the
What URL will the embedded view have if not the ReauthURL?

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:191: if
(content_set.size() > 0)
DCHECK(content_set.size() <= 1)?

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:662:
FinishCompleteLogin(
Is this the "not called to reauthenticate" case?  If so, switch the if/else so
that it matches the comment.

[Roger Tawa OOO till Jul 10th, 2015-07-22 00:12:33]

Thanks Brian.  PTAL.

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/signin_promo.h (right):

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/signin/...
chrome/browser/signin/signin_promo.h:66: GURL GetReauthURL(size_t profile_index,
const std::string& email);
On 2015/07/21 13:28:39, bcwhite wrote:
> These two methods are keyed based on the second parameter (email vs
account-id)
> but are differentiated by the compiler based solely on the first parameter.
> 
> I suggest different names for these methods and have the first parameters be
the
> same.

Renamed new method.  I don't want to change the signature of the existing
GetReauthURL() function since it is used in lots of places not related to this
CL.  So I'll keep the args as is.

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:177: // webview and
observe it instead.  The webview may not be found in the
On 2015/07/21 13:28:39, bcwhite wrote:
> What URL will the embedded view have if not the ReauthURL?

ReauthDelegate first observes the WebContents of the |web_view_|, looking for
the embedded <webview> element.  Once found, ReauthDelegate then observers the
WebContents of the <webivew>.

The |web_view_| contains a URL of the form "chrome://chrome-signin/...".  The
<webview> element contains URLs of the form https://accounts.google.com/...,
URLs based on saml provider pages, and the eventually successful continue URL.

The term "web view" is overloaded in this case to mean different things, so it
gets a little confusing.

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:191: if
(content_set.size() > 0)
On 2015/07/21 13:28:39, bcwhite wrote:
> DCHECK(content_set.size() <= 1)?

The style guide says not to DCHECK if the code handles the cases with an if.

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:662:
FinishCompleteLogin(
On 2015/07/21 13:28:39, bcwhite wrote:
> Is this the "not called to reauthenticate" case?  If so, switch the if/else so
> that it matches the comment.

This is the "not called from the user manager to reauthenticate" case.  Let me
know if the comment at lines 656-659 could explain this better.

[bcwhite, 2015-07-22 13:42:21]

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:191: if
(content_set.size() > 0)
On 2015/07/22 00:12:32, Roger Tawa wrote:
> On 2015/07/21 13:28:39, bcwhite wrote:
> > DCHECK(content_set.size() <= 1)?
> 
> The style guide says not to DCHECK if the code handles the cases with an if.

Different conditions.  The DCHECK was to validate that there was only one
sub-view.  If there were two, the "content_set.begin()" could get the wrong one.

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:662:
FinishCompleteLogin(
The comment says "if A then B" but the code says "if not A then C else B".  Just
inverting the condition and switching the if/else blocks would make the code
match the comment.

[Roger Tawa OOO till Jul 10th, 2015-07-22 15:29:54]

Thanks Brian, PTAL.

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:191: if
(content_set.size() > 0)
On 2015/07/22 13:42:21, bcwhite wrote:
> On 2015/07/22 00:12:32, Roger Tawa wrote:
> > On 2015/07/21 13:28:39, bcwhite wrote:
> > > DCHECK(content_set.size() <= 1)?
> > 
> > The style guide says not to DCHECK if the code handles the cases with an if.
> 
> Different conditions.  The DCHECK was to validate that there was only one
> sub-view.  If there were two, the "content_set.begin()" could get the wrong
one.

Done.

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/1220843003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:662:
FinishCompleteLogin(
On 2015/07/22 13:42:21, bcwhite wrote:
> The comment says "if A then B" but the code says "if not A then C else B". 
Just
> inverting the condition and switching the if/else blocks would make the code
> match the comment.

Switched order of code.

[bcwhite, 2015-07-22 19:18:33]

lgtm

[Roger Tawa OOO till Jul 10th, 2015-07-23 16:03:21]

rogerta@chromium.org changed reviewers:
+ asvitkine@chromium.org, skylined@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-07-23 16:03:22]

Hi Alexei, Scott,

Can you please do owner review?

Alexei:
chrome/browser/ui/cocoa/profiles/user_manager_mac.mm

Scott:
chrome/browser/ui/user_manager.h
chrome/browser/ui/views/profiles/user_manager_view.cc

Thanks.

[Roger Tawa OOO till Jul 10th, 2015-07-23 16:03:53]

rogerta@chromium.org changed reviewers:
+ sky@chromium.org
- skylined@chromium.org

[sky, 2015-07-23 16:56:04]

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/user...
File chrome/browser/ui/user_manager.h (right):

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/user...
chrome/browser/ui/user_manager.h:41: size_t profile_index);
Is it common to pass around an index like this as identity? Might the index
shift around while the dialog is up? Say a profile is removed?

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:85: std::string
email_address_;
nit: const on this and profile_index_.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:87: };
nit: DISALLOW_...

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:122: delete web_view_;
Is there a reason you're deleting web_view_ here and not the constructor? Also,
document why you explicitly delete it.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:167:
url::Replacements<char> replacements;
This part seems generic and not specific to views. Should it be promoted to a
common place?

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:185:
std::set<content::WebContents*> content_set;
This part doesn't seem specific to views either.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:193: if
(content_set.size() > 0)
!empty

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:271:
views::DialogDelegate::CreateDialogWidget(delegate, NULL, parent);
nullptr

[Alexei Svitkine (slow), 2015-07-23 21:49:34]

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/signin/...
File chrome/browser/signin/signin_promo.cc (right):

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/signin/...
chrome/browser/signin/signin_promo.cc:209: if (profile_index !=
std::string::npos)
Nit: {}'s

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/user...
File chrome/browser/ui/user_manager.h (right):

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/user...
chrome/browser/ui/user_manager.h:41: size_t profile_index);
On 2015/07/23 16:56:04, sky wrote:
> Is it common to pass around an index like this as identity? Might the index
> shift around while the dialog is up? Say a profile is removed?

I share this concern. I think it can be a problem if we keep it as a member of a
class like a dialog. Either it would need to listen to notifications on changes
- or use the profile name string for the identifier instead.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:122: delete web_view_;
On 2015/07/23 16:56:04, sky wrote:
> Is there a reason you're deleting web_view_ here and not the constructor?
Also,
> document why you explicitly delete it.

Also, why not use a scoped_ptr?

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:189:
web_view_->GetWebContents()->GetBrowserContext());
Nit: You have a pointer to web_contents on line 186 - no need to get it again.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:269: new
ReauthDelegate(browser_context, email, profile_index);
What's the ownership of this? Worth putting a comment above otherwise just looks
like a leak.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:798:
ProfileInfoCache& cache = profile_manager->GetProfileInfoCache();
Nit: Just combine the two lines since you're only using cache once.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.h (right):

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.h:87: static void
FinishCompleteLogin(const FinishCompleteLoginParams& params,
I think the specific params should be documented somewhere. Probably in comments
above the struct members.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.h:89:
Profile::CreateStatus);
Name the last param.

[berendjanwever, 2015-07-23 22:45:42]

I have no idea what this is about: did you intend to add
sky@chromium.org, rather than me, SkyLined?

On 23-07-2015 18:03, rogerta@chromium.org wrote:
> Hi Alexei, Scott,
>
> Can you please do owner review?
>
> Alexei:
> chrome/browser/ui/cocoa/profiles/user_manager_mac.mm
>
> Scott:
> chrome/browser/ui/user_manager.h
> chrome/browser/ui/views/profiles/user_manager_view.cc
>
> Thanks.
>
> https://codereview.chromium.org/1220843003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Roger Tawa OOO till Jul 10th, 2015-07-24 16:12:49]

Patchset #8 (id:260001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-07-24 16:15:13]

Thanks guys.

Brian: I re-implemented without profile index as we discussed, please take
another look.

Scott: turns out I don't need to pass the profile index around to detect the
required condition.  I also replace the member index UserManagerScreenHandler
with a path and used the new interface of ProfileInfoCache in my new code.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/signin/...
File chrome/browser/signin/signin_promo.cc (right):

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/signin/...
chrome/browser/signin/signin_promo.cc:209: if (profile_index !=
std::string::npos)
On 2015/07/23 21:49:33, Alexei Svitkine wrote:
> Nit: {}'s

Changes in this file undone.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/user...
File chrome/browser/ui/user_manager.h (right):

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/user...
chrome/browser/ui/user_manager.h:41: size_t profile_index);
On 2015/07/23 21:49:33, Alexei Svitkine wrote:
> On 2015/07/23 16:56:04, sky wrote:
> > Is it common to pass around an index like this as identity? Might the index
> > shift around while the dialog is up? Say a profile is removed?
> 
> I share this concern. I think it can be a problem if we keep it as a member of
a
> class like a dialog. Either it would need to listen to notifications on
changes
> - or use the profile name string for the identifier instead.

I redid this code to no longer need an index.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:85: std::string
email_address_;
On 2015/07/23 16:56:04, sky wrote:
> nit: const on this and profile_index_.

Done.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:87: };
On 2015/07/23 16:56:04, sky wrote:
> nit: DISALLOW_...

Done.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:122: delete web_view_;
On 2015/07/23 16:56:04, sky wrote:
> Is there a reason you're deleting web_view_ here and not the constructor?
Also,
> document why you explicitly delete it.

Made it into a scoped_ptr<> so don't need to delete it explicitly.  Much better
this way!

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:122: delete web_view_;
On 2015/07/23 21:49:33, Alexei Svitkine wrote:
> On 2015/07/23 16:56:04, sky wrote:
> > Is there a reason you're deleting web_view_ here and not the constructor?
> Also,
> > document why you explicitly delete it.
> 
> Also, why not use a scoped_ptr?

Yup, that's what I did.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:167:
url::Replacements<char> replacements;
On 2015/07/23 16:56:04, sky wrote:
> This part seems generic and not specific to views. Should it be promoted to a
> common place?

Which part exactly?  The call at line 172 is views specific.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:185:
std::set<content::WebContents*> content_set;
On 2015/07/23 16:56:04, sky wrote:
> This part doesn't seem specific to views either.

Possibly, yes.  When the mac portion of this change is done in a follow up CL,
it might be possible to refactor this bit and the one above into a common
location.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:189:
web_view_->GetWebContents()->GetBrowserContext());
On 2015/07/23 21:49:33, Alexei Svitkine wrote:
> Nit: You have a pointer to web_contents on line 186 - no need to get it again.

Done.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:193: if
(content_set.size() > 0)
On 2015/07/23 16:56:04, sky wrote:
> !empty

Done.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:269: new
ReauthDelegate(browser_context, email, profile_index);
On 2015/07/23 21:49:33, Alexei Svitkine wrote:
> What's the ownership of this? Worth putting a comment above otherwise just
looks
> like a leak.

Done.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:271:
views::DialogDelegate::CreateDialogWidget(delegate, NULL, parent);
On 2015/07/23 16:56:04, sky wrote:
> nullptr

Done.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:798:
ProfileInfoCache& cache = profile_manager->GetProfileInfoCache();
On 2015/07/23 21:49:34, Alexei Svitkine wrote:
> Nit: Just combine the two lines since you're only using cache once.

Done.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.h (right):

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.h:87: static void
FinishCompleteLogin(const FinishCompleteLoginParams& params,
On 2015/07/23 21:49:34, Alexei Svitkine wrote:
> I think the specific params should be documented somewhere. Probably in
comments
> above the struct members.

Done.

https://codereview.chromium.org/1220843003/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.h:89:
Profile::CreateStatus);
On 2015/07/23 21:49:34, Alexei Svitkine wrote:
> Name the last param.

Done.

[Alexei Svitkine (slow), 2015-07-24 16:36:42]

lgtm % remaining comments

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/profile...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_window.cc:249: const base::string16&
profile_email = base::UTF8ToUTF16(email);
This shouldn't be a ref.

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_window.cc:252: for (auto entry : entries) {
Nit: I'd write out the auto here - since ProfileAttributesEntry* is not very
long and makes the code clearer.

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:67: // Overridden from
views::DialogDelegate:
Nit: I think new convention is just to have the comment be:

// views::DialogDelegate:

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:667:
FinishCompleteLoginParams params(NULL, partition, current_url, path,
Nit: nullptr

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:767: params.email,
&error_msg);
Nit: Align.

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:413:
g_browser_process->profile_manager()->GetProfileInfoCache();
Nit: Make this a pointer via
&g_browser_process->profile_manager()->GetProfileInfoCache().

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:455:
base::UTF16ToUTF8(email_address_));
What's the reason for doing the conversion at the call site that within the
function or even later?

For example, it looks like ReathDelegate takes a copy of that string. So maybe
it should take a string16 param and convert in its ctor, so that you copy from
string16 to std::string at that point and save an extra std::string copy.

[Roger Tawa OOO till Jul 10th, 2015-07-24 17:01:29]

Thanks Alexei.

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/profile...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_window.cc:249: const base::string16&
profile_email = base::UTF8ToUTF16(email);
On 2015/07/24 16:36:42, Alexei Svitkine wrote:
> This shouldn't be a ref.

Oops, my bad.  Copy and paste problem...

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_window.cc:252: for (auto entry : entries) {
On 2015/07/24 16:36:42, Alexei Svitkine wrote:
> Nit: I'd write out the auto here - since ProfileAttributesEntry* is not very
> long and makes the code clearer.

Done.

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:67: // Overridden from
views::DialogDelegate:
On 2015/07/24 16:36:42, Alexei Svitkine wrote:
> Nit: I think new convention is just to have the comment be:
> 
> // views::DialogDelegate:

Done.

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:667:
FinishCompleteLoginParams params(NULL, partition, current_url, path,
On 2015/07/24 16:36:42, Alexei Svitkine wrote:
> Nit: nullptr

Done.

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:767: params.email,
&error_msg);
On 2015/07/24 16:36:42, Alexei Svitkine wrote:
> Nit: Align.

Done.

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:413:
g_browser_process->profile_manager()->GetProfileInfoCache();
On 2015/07/24 16:36:42, Alexei Svitkine wrote:
> Nit: Make this a pointer via
> &g_browser_process->profile_manager()->GetProfileInfoCache().

I think I'll keep it as is.  All the code in chrome uses PIC as a ref.

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:455:
base::UTF16ToUTF8(email_address_));
On 2015/07/24 16:36:42, Alexei Svitkine wrote:
> What's the reason for doing the conversion at the call site that within the
> function or even later?
> 
> For example, it looks like ReathDelegate takes a copy of that string. So maybe
> it should take a string16 param and convert in its ctor, so that you copy from
> string16 to std::string at that point and save an extra std::string copy.

Everyone email is just an std::string (except when converted to show in UI).  I
took a look at why email is declared as base::string16, and it seem the only
reason is because it is read from a base::Value.  Is never used as a string16
here, and ReathDelegate also never displays it in UI.  So I changed the type of
the member to std::string.

[Alexei Svitkine (slow), 2015-07-24 17:09:08]

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1220843003/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:455:
base::UTF16ToUTF8(email_address_));
On 2015/07/24 17:01:29, Roger Tawa wrote:
> On 2015/07/24 16:36:42, Alexei Svitkine wrote:
> > What's the reason for doing the conversion at the call site that within the
> > function or even later?
> > 
> > For example, it looks like ReathDelegate takes a copy of that string. So
maybe
> > it should take a string16 param and convert in its ctor, so that you copy
from
> > string16 to std::string at that point and save an extra std::string copy.
> 
> Everyone email is just an std::string (except when converted to show in UI). 
I
> took a look at why email is declared as base::string16, and it seem the only
> reason is because it is read from a base::Value.  Is never used as a string16
> here, and ReathDelegate also never displays it in UI.  So I changed the type
of
> the member to std::string.

Woot, that's even better. :)

[bcwhite, 2015-07-24 17:53:39]

lgtm

[Roger Tawa OOO till Jul 10th, 2015-07-24 18:52:25]

rogerta@chromium.org changed reviewers:
- berendjanwever@gmail.com

[sky, 2015-07-24 20:24:03]

Glad to see the index gone!

https://codereview.chromium.org/1220843003/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1220843003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:59: class ReauthDelegate :
public views::DialogDelegate,
Your code would be marginally smaller if you make it a DialogDelegateView:
. you wouldn't need to override GetWidget or GetContentsView.
. You could add the WebView directly to 'this' and not make it client owned or
wrapped in a scoped_ptr.

https://codereview.chromium.org/1220843003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:128: DCHECK(!web_view_);
You shouldn't assume GetContentsView() is only called once.

[Roger Tawa OOO till Jul 10th, 2015-07-24 21:18:13]

Thanks Scott.  PTAL.

https://codereview.chromium.org/1220843003/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1220843003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:59: class ReauthDelegate :
public views::DialogDelegate,
On 2015/07/24 20:24:03, sky wrote:
> Your code would be marginally smaller if you make it a DialogDelegateView:
> . you wouldn't need to override GetWidget or GetContentsView.
> . You could add the WebView directly to 'this' and not make it client owned or
> wrapped in a scoped_ptr.

Done.

https://codereview.chromium.org/1220843003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:128: DCHECK(!web_view_);
On 2015/07/24 20:24:03, sky wrote:
> You shouldn't assume GetContentsView() is only called once.

Method removed.

[sky, 2015-07-24 21:43:34]

LGTM

[Roger Tawa OOO till Jul 10th, 2015-07-25 13:56:10]

The CQ bit was checked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-07-25 13:56:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, bcwhite@chromium.org
Link to the patchset: https://codereview.chromium.org/1220843003/#ps320001
(title: "Use DialogDelegateView")

[commit-bot: I haz the power, 2015-07-25 13:56:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1220843003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1220843003/320001

[commit-bot: I haz the power, 2015-07-25 14:47:41]

Committed patchset #10 (id:320001)

[commit-bot: I haz the power, 2015-07-25 14:48:30]

Patchset 10 (id:??) landed as
https://crrev.com/ce8820ca9f57406653a5983b0cd5bf4e5a60d390
Cr-Commit-Position: refs/heads/master@{#340411}