Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2351173004:
Display local signin error without browser and record the path of selected profile in user manager. (Closed)
DescriptionDisplay local signin error without browser and record the path of selected profile in user manager.
BUG=642059
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/c105747d69ec6c66856327ba51765ab6e628e0f3
Cr-Commit-Position: refs/heads/master@{#420713}
Patch Set 1 #
Total comments: 34
Patch Set 2 : cr+fix trybot #
Total comments: 13
Patch Set 3 : cr #
Total comments: 4
Patch Set 4 : cr #
Total comments: 6
Patch Set 5 : cr #
Total comments: 3
Patch Set 6 #Patch Set 7 #
Total comments: 15
Patch Set 8 : cr #
Total comments: 1
Messages
Total messages: 51 (21 generated)
Description was changed from ========== Display local signin error without browser and record the path of selected profile in user manager. BUG=642059 ========== to ========== Display local signin error without browser and record the path of selected profile in user manager. BUG=642059 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
zmin@chromium.org changed reviewers: + anthonyvd@chromium.org, rogerta@chromium.org, sky@chromium.org, tommycli@chromium.org
Hi, With this CL, the new error ui(2315393002) can be shown without the main browser window. It will be used by force-sign-in which can't open browser window with any error occurred. Note that force-sign-in is an feature for enterprise users and will be controlled by group policy. I appreciate all your comments. tommycli: chrome/browser/resources/signin/signin_error/signin_error.js sky: chrome/browser/ui/user_manager.h chrome/browser/ui/views/profiles/user_manager_view.h chrome/browser/ui/views/profiles/user_manager_view.cc rogerta: chrome/browser/ui/webui/signin/signin_error_handler.h chrome/browser/ui/webui/signin/signin_error_handler.cc chrome/browser/ui/webui/signin/signin_error_ui.cc chrome/browser/ui/webui/signin/user_manager_screen_handler.cc anthonyvd: Can you review the whole CL as we have discussed a lot about it.
https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/user_mana... File chrome/browser/ui/user_manager.h (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/user_mana... chrome/browser/ui/user_manager.h:69: // TODO(noms): Figure out if this size can be computed dynamically or adjusted Sorry for random comment, just noticing this now. Constants should be before functions (see style guide). https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:224: if (instance_ && !instance_->GetWidget()->IsClosed()) Can you motivate why this change is needed? https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:238: instance_->SetSigninProfilePath(profile_path); How do you know instance_ is non-null at the time this is called? https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:239: ShowReauthDialog(browser_context, std::string(), As someone not intimitely familiar with this code I find it confusing that ShowSigninDialog calls ShowReauthDialog. Is there a reason we can't have a single ShowReauthDialog with the appropriate details? https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:261: signin_profile_path_(base::FilePath()) { This is the default, so you don't need it. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:307: void UserManagerView::DisplayErrorMessage() { Style guide says declaration and definition order should match. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:308: if (delegate_) { no {} https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/user_manager_view.h (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.h:103: // Display local sign in error message withou browser. without. What do you mean by local? https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.h:106: void SetSigninProfilePath(const base::FilePath profile_path); const &
Reply some comments, will do the rest tomorrow. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:224: if (instance_ && !instance_->GetWidget()->IsClosed()) On 2016/09/20 23:30:19, sky wrote: > Can you motivate why this change is needed? IsShowing will check the activate of User Manager window. bool UserManager::IsShowing() { return instance_ ? instance_->GetWidget()->IsActive() : false; 194 return instance_ ? instance_->GetWidget()->IsActive() : false; }} However, it's not true when gaia window(ReauthDialog) popup. Because when gaia window(ReauthDialog) displayed, the user manager window will behind the gaia window(ReauthDialog) and not activate and I can't call this function to hide the dialog. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:238: instance_->SetSigninProfilePath(profile_path); On 2016/09/20 23:30:19, sky wrote: > How do you know instance_ is non-null at the time this is called? Good catch, I shall add a check here. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:239: ShowReauthDialog(browser_context, std::string(), On 2016/09/20 23:30:19, sky wrote: > As someone not intimitely familiar with this code I find it confusing that > ShowSigninDialog calls ShowReauthDialog. Is there a reason we can't have a > single ShowReauthDialog with the appropriate details? User manager only has ShowReauthDialog before because all sign in in User Manager are reauth. However, it's not true anymore with force sign in which is the reason I create ShowSigninDialog. But I call ShowReauthDialog inside the ShowSigninDialog to avoid code copy. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/user_manager_view.h (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.h:103: // Display local sign in error message withou browser. On 2016/09/20 23:30:19, sky wrote: > without. > What do you mean by local? Local means the error not from Gaia(i..e password error, account doesn't exist) but from Chrome(i.e. email has been used by another profile, email doesn't match the policy rule)
https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:239: ShowReauthDialog(browser_context, std::string(), On 2016/09/21 00:09:35, zmin wrote: > On 2016/09/20 23:30:19, sky wrote: > > As someone not intimitely familiar with this code I find it confusing that > > ShowSigninDialog calls ShowReauthDialog. Is there a reason we can't have a > > single ShowReauthDialog with the appropriate details? > > User manager only has ShowReauthDialog before because all sign in in User > Manager are reauth. However, it's not true anymore with force sign in which is > the reason I create ShowSigninDialog. But I call ShowReauthDialog inside the > ShowSigninDialog to avoid code copy. I think it's more clear for the API or it will become "It's not reauth but fresh sign in, why call UserManager::ShowReauthDialog()?" Another solution is change the name of ShowReauthDialog but that is not a simply refactor.
pastarmovj@chromium.org changed reviewers: + pastarmovj@chromium.org
A few drive-by comments. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/user_mana... File chrome/browser/ui/user_manager.h (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/user_mana... chrome/browser/ui/user_manager.h:63: // Display local sign in error message withou browser. nit: s/withou/without/ https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/signin_error_handler.cc (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/signin_error_handler.cc:21: return new SigninErrorWithoutBrowserHandler(); I haven't looked at the calling site but it would be nice if we can wrap the result in a unique_ptr. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/signin_error_handler.h (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/signin_error_handler.h:22: static SigninErrorHandler* Create(bool is_system_profile); I think a comment why you need this factory function is going to be useful here. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/signin_error_handler.h:36: SigninErrorHandler() {} nit: new line after the constructor. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/signin_error_handler.h:65: class SigninErrorWithoutBrowserHandler : public SigninErrorHandler { Comment this class please. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/signin_error_ui.cc (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/signin_error_ui.cc:32: } nit: // namespace
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/user_mana... File chrome/browser/ui/user_manager.h (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/user_mana... chrome/browser/ui/user_manager.h:63: // Display local sign in error message withou browser. On 2016/09/21 08:44:10, pastarmovj wrote: > nit: s/withou/without/ Done. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/user_mana... chrome/browser/ui/user_manager.h:69: // TODO(noms): Figure out if this size can be computed dynamically or adjusted On 2016/09/20 23:30:19, sky wrote: > Sorry for random comment, just noticing this now. Constants should be before > functions (see style guide). Done. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:238: instance_->SetSigninProfilePath(profile_path); On 2016/09/20 23:30:19, sky wrote: > How do you know instance_ is non-null at the time this is called? Done. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:261: signin_profile_path_(base::FilePath()) { On 2016/09/20 23:30:19, sky wrote: > This is the default, so you don't need it. Done. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:307: void UserManagerView::DisplayErrorMessage() { On 2016/09/20 23:30:19, sky wrote: > Style guide says declaration and definition order should match. Done. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:308: if (delegate_) { On 2016/09/20 23:30:19, sky wrote: > no {} Done. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/user_manager_view.h (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.h:103: // Display local sign in error message withou browser. On 2016/09/21 00:09:35, zmin wrote: > On 2016/09/20 23:30:19, sky wrote: > > without. > > What do you mean by local? > > Local means the error not from Gaia(i..e password error, account doesn't exist) > but from Chrome(i.e. email has been used by another profile, email doesn't match > the policy rule) Remove "local" as it may cause confusion https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.h:106: void SetSigninProfilePath(const base::FilePath profile_path); On 2016/09/20 23:30:19, sky wrote: > const & Done. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/signin_error_handler.cc (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/signin_error_handler.cc:21: return new SigninErrorWithoutBrowserHandler(); On 2016/09/21 08:44:10, pastarmovj wrote: > I haven't looked at the calling site but it would be nice if we can wrap the > result in a unique_ptr. The handler will be assigned to webui https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/signin/signin_er... And eventually stored in a scopedvector https://cs.chromium.org/chromium/src/content/browser/webui/web_ui_impl.h?cl=G... https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/signin_error_handler.h (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/signin_error_handler.h:22: static SigninErrorHandler* Create(bool is_system_profile); On 2016/09/21 08:44:10, pastarmovj wrote: > I think a comment why you need this factory function is going to be useful here. > Done. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/signin_error_handler.h:36: SigninErrorHandler() {} On 2016/09/21 08:44:10, pastarmovj wrote: > nit: new line after the constructor. Done. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/signin_error_handler.h:65: class SigninErrorWithoutBrowserHandler : public SigninErrorHandler { On 2016/09/21 08:44:10, pastarmovj wrote: > Comment this class please. Done. https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/signin_error_ui.cc (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/signin_error_ui.cc:32: } On 2016/09/21 08:44:10, pastarmovj wrote: > nit: // namespace Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/signin_error_handler.h (right): https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_error_handler.h:76: void CloseDialog() override; nit: Such a small difference does not seem worth making a class hierarchy for. Maybe just a boolean flag within SigninErrorHandler? https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/signin_error_ui.cc (right): https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_error_ui.cc:32: } // namespace nit: I don't think this shortening is worth it. It's only used in two places. https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_error_ui.cc:45: if (is_system_profile) { nit: Maybe clearer with a ternary expression? Not required, just a maybe suggestion. https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:475: #if !defined(OS_MACOSX) Why the mac vs. non-mac distinction? Also the non-mac version lost the comment. Maybe the comment should be at the top level.
https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/signin_error_handler.h (right): https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_error_handler.h:76: void CloseDialog() override; On 2016/09/21 18:26:59, tommycli wrote: > nit: Such a small difference does not seem worth making a class hierarchy for. > Maybe just a boolean flag within SigninErrorHandler? I think you're right. There is only one conditional statement anyway. Subclass do looks heavy. https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/signin_error_ui.cc (right): https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_error_ui.cc:32: } // namespace On 2016/09/21 18:26:59, tommycli wrote: > nit: I don't think this shortening is worth it. It's only used in two places. Why not? That is still a code copy. https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_error_ui.cc:45: if (is_system_profile) { On 2016/09/21 18:26:59, tommycli wrote: > nit: Maybe clearer with a ternary expression? Not required, just a maybe > suggestion. I'm personally not a fan of ternary expression. Especially when the expression is longer than one line. https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:475: #if !defined(OS_MACOSX) On 2016/09/21 18:26:59, tommycli wrote: > Why the mac vs. non-mac distinction? > > Also the non-mac version lost the comment. Maybe the comment should be at the > top level. Because the user_manager implementation on Mac is different but I don't have Mac for now. All Mac related condition will be removed in a separated CL once the Mac part is finished. Since this one is big enough.
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm except: https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/signin_error_ui.cc (right): https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_error_ui.cc:32: } // namespace On 2016/09/21 21:21:37, zmin wrote: > On 2016/09/21 18:26:59, tommycli wrote: > > nit: I don't think this shortening is worth it. It's only used in two places. > > Why not? That is still a code copy. I think since it's so short, it would be easier to read to just inline profile->GetOriginalProfile()->IsSystemProfile(). https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_error_ui.cc:45: if (is_system_profile) { On 2016/09/21 21:21:37, zmin wrote: > On 2016/09/21 18:26:59, tommycli wrote: > > nit: Maybe clearer with a ternary expression? Not required, just a maybe > > suggestion. > > I'm personally not a fan of ternary expression. Especially when the expression > is longer than one line. Acknowledged. https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:475: #if !defined(OS_MACOSX) On 2016/09/21 21:21:37, zmin wrote: > On 2016/09/21 18:26:59, tommycli wrote: > > Why the mac vs. non-mac distinction? > > > > Also the non-mac version lost the comment. Maybe the comment should be at the > > top level. > > Because the user_manager implementation on Mac is different but I don't have > Mac for now. All Mac related condition will be removed in a separated CL once > the Mac part is finished. Since this one is big enough. Okay that's fine, please add a TODO for yourself explaining that. https://codereview.chromium.org/2351173004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/signin_error_handler.h (right): https://codereview.chromium.org/2351173004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_error_handler.h:57: virtual void CloseDialog(); nit: no need for virtual anymore. https://codereview.chromium.org/2351173004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/signin_error_ui.cc (right): https://codereview.chromium.org/2351173004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_error_ui.cc:47: #if !defined(OS_MACOSX) Please add a TODO with an explanation about the separate Mac implementation.
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/signin_error_ui.cc (right): https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_error_ui.cc:32: } // namespace On 2016/09/21 21:26:16, tommycli wrote: > On 2016/09/21 21:21:37, zmin wrote: > > On 2016/09/21 18:26:59, tommycli wrote: > > > nit: I don't think this shortening is worth it. It's only used in two > places. > > > > Why not? That is still a code copy. > > I think since it's so short, it would be easier to read to just inline > profile->GetOriginalProfile()->IsSystemProfile(). Done. https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/2351173004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:475: #if !defined(OS_MACOSX) On 2016/09/21 21:26:16, tommycli wrote: > On 2016/09/21 21:21:37, zmin wrote: > > On 2016/09/21 18:26:59, tommycli wrote: > > > Why the mac vs. non-mac distinction? > > > > > > Also the non-mac version lost the comment. Maybe the comment should be at > the > > > top level. > > > > Because the user_manager implementation on Mac is different but I don't have > > Mac for now. All Mac related condition will be removed in a separated CL once > > the Mac part is finished. Since this one is big enough. > > Okay that's fine, please add a TODO for yourself explaining that. Done. https://codereview.chromium.org/2351173004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/signin_error_handler.h (right): https://codereview.chromium.org/2351173004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_error_handler.h:57: virtual void CloseDialog(); On 2016/09/21 21:26:16, tommycli wrote: > nit: no need for virtual anymore. Done. https://codereview.chromium.org/2351173004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/signin_error_ui.cc (right): https://codereview.chromium.org/2351173004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/signin_error_ui.cc:47: #if !defined(OS_MACOSX) On 2016/09/21 21:26:16, tommycli wrote: > Please add a TODO with an explanation about the separate Mac implementation. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm chrome/browser/ui/webui thanks
https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/user_manager_view.cc:224: if (instance_ && !instance_->GetWidget()->IsClosed()) On 2016/09/21 00:09:35, zmin wrote: > On 2016/09/20 23:30:19, sky wrote: > > Can you motivate why this change is needed? > > IsShowing will check the activate of User Manager window. > bool UserManager::IsShowing() { > return instance_ ? instance_->GetWidget()->IsActive() : false; 194 return > instance_ ? instance_->GetWidget()->IsActive() : false; }} > > However, it's not true when gaia window(ReauthDialog) popup. Because when gaia > window(ReauthDialog) displayed, the user manager window will behind the gaia > window(ReauthDialog) and not activate and I can't call this function to hide the > dialog. I think you are arguing for checking IsVisible(), which makes sense. https://codereview.chromium.org/2351173004/diff/80001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.h (right): https://codereview.chromium.org/2351173004/diff/80001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.h:23: static const int kWindowWidth = 800; constexpr on all these. https://codereview.chromium.org/2351173004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/user_manager_view.cc:247: // This method should only be called if the user manager is already showing. This comment implies a DCHECK and not a conditional. Or am I missing something?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2351173004/diff/80001/chrome/browser/ui/user_... File chrome/browser/ui/user_manager.h (right): https://codereview.chromium.org/2351173004/diff/80001/chrome/browser/ui/user_... chrome/browser/ui/user_manager.h:23: static const int kWindowWidth = 800; On 2016/09/21 23:00:02, sky wrote: > constexpr on all these. Will do. https://codereview.chromium.org/2351173004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/user_manager_view.cc:247: // This method should only be called if the user manager is already showing. On 2016/09/21 23:00:02, sky wrote: > This comment implies a DCHECK and not a conditional. Or am I missing something? I use condition here because of the consistence with ShowReauthDialog. What's more, IsShowing() not only check the instance_ pointer but also whether the UI is activated or not which I believe is also necessary.
https://codereview.chromium.org/2351173004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/user_manager_view.cc:247: // This method should only be called if the user manager is already showing. On 2016/09/21 23:49:55, zmin wrote: > On 2016/09/21 23:00:02, sky wrote: > > This comment implies a DCHECK and not a conditional. Or am I missing > something? > > I use condition here because of the consistence with ShowReauthDialog. What's > more, IsShowing() not only check the instance_ pointer but also whether the UI > is activated or not which I believe is also necessary. That's fine, but then update the comment. The comment implies a DCHECK.
https://codereview.chromium.org/2351173004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/user_manager_view.cc:247: // This method should only be called if the user manager is already showing. On 2016/09/21 23:52:40, sky wrote: > On 2016/09/21 23:49:55, zmin wrote: > > On 2016/09/21 23:00:02, sky wrote: > > > This comment implies a DCHECK and not a conditional. Or am I missing > > something? > > > > I use condition here because of the consistence with ShowReauthDialog. What's > > more, IsShowing() not only check the instance_ pointer but also whether the UI > > is activated or not which I believe is also necessary. > > That's fine, but then update the comment. The comment implies a DCHECK. OOPS, you're right. I shall use DCHECK here. I thought we were talking about the function above(ShowSigninDialog). Guess I shouldn't read the code from phone. Change it to DCHECK.
https://codereview.chromium.org/2351173004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/user_manager_view.cc:224: if (instance_ && !instance_->GetWidget()->IsClosed()) I think you missed my earlier comment here. I think you want IsVisible. That said. Should IsShowing change to check IsVisible() and not IsActive()? It's very confusing to have IsShowing check active state, not visibility.
https://codereview.chromium.org/2351173004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/user_manager_view.cc:224: if (instance_ && !instance_->GetWidget()->IsClosed()) On 2016/09/22 13:08:46, sky wrote: > I think you missed my earlier comment here. I think you want IsVisible. That > said. Should IsShowing change to check IsVisible() and not IsActive()? It's very > confusing to have IsShowing check active state, not visibility. Sorry for missing comment. Yes, I agree IsVisible is better than IsClosed for HideReauthDialog(). I have changed it. However, I think IsActive is better than IsVisible for ShowReauthDialog() because the gaia dialog should only be opened when user click the profile in user manager window which means the user manager window should be active. Yes, I agree the name of function is a little bit confusing, maybe IsActive is better. However, given it's a public API that existed for a long time, I don't really want to change its name. So I want to use IsVisible for HideReauthDialog and IsActive(IsShowing) for ShowReauhDialog and ShowSigninDialog. In the mean time, I will update IsShowing()'s comments from "Returns whether the User Manager is showing" to "Returns whether the User Manager is showing and active".
https://codereview.chromium.org/2351173004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/user_manager_view.cc:224: if (instance_ && !instance_->GetWidget()->IsClosed()) On 2016/09/22 15:56:23, zmin wrote: > On 2016/09/22 13:08:46, sky wrote: > > I think you missed my earlier comment here. I think you want IsVisible. That > > said. Should IsShowing change to check IsVisible() and not IsActive()? It's > very > > confusing to have IsShowing check active state, not visibility. > > Sorry for missing comment. > Yes, I agree IsVisible is better than IsClosed for HideReauthDialog(). I have > changed it. > However, I think IsActive is better than IsVisible for ShowReauthDialog() > because the gaia dialog should only be opened when user click the profile in > user manager window which means the user manager window should be active. > Yes, I agree the name of function is a little bit confusing, maybe IsActive is > better. However, given it's a public API that existed for a long time, I don't > really want to change its name. > > So I want to use IsVisible for HideReauthDialog and IsActive(IsShowing) for > ShowReauhDialog and ShowSigninDialog. In the mean time, I will update > IsShowing()'s comments from "Returns whether the User Manager is showing" to > "Returns whether the User Manager is showing and active". Please add file a bug to clean it up and add a TODO referencing said to bug to rename IsShowing() to something less confusing.
On 2016/09/22 16:33:50, sky wrote: > https://codereview.chromium.org/2351173004/diff/100001/chrome/browser/ui/view... > File chrome/browser/ui/views/profiles/user_manager_view.cc (right): > > https://codereview.chromium.org/2351173004/diff/100001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/user_manager_view.cc:224: if (instance_ && > !instance_->GetWidget()->IsClosed()) > On 2016/09/22 15:56:23, zmin wrote: > > On 2016/09/22 13:08:46, sky wrote: > > > I think you missed my earlier comment here. I think you want IsVisible. That > > > said. Should IsShowing change to check IsVisible() and not IsActive()? It's > > very > > > confusing to have IsShowing check active state, not visibility. > > > > Sorry for missing comment. > > Yes, I agree IsVisible is better than IsClosed for HideReauthDialog(). I have > > changed it. > > However, I think IsActive is better than IsVisible for ShowReauthDialog() > > because the gaia dialog should only be opened when user click the profile in > > user manager window which means the user manager window should be active. > > Yes, I agree the name of function is a little bit confusing, maybe IsActive is > > better. However, given it's a public API that existed for a long time, I don't > > really want to change its name. > > > > So I want to use IsVisible for HideReauthDialog and IsActive(IsShowing) for > > ShowReauhDialog and ShowSigninDialog. In the mean time, I will update > > IsShowing()'s comments from "Returns whether the User Manager is showing" to > > "Returns whether the User Manager is showing and active". > > Please add file a bug to clean it up and add a TODO referencing said to bug to > rename IsShowing() to something less confusing. Done.
Looks good, just a few comments/questions! https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/user_manager_view.cc:436: void UserManagerView::DisplayErrorMessage() { It looks like doing it this way makes the delegate load the reauth URL in its constructor, then switch to the error URL when an error happens. Does the dialog resize itself properly? https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/user_manager_view.h (right): https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/user_manager_view.h:103: // Display sign in error message without browser. Can you expand a bit on what that comment means? It's pretty obvious now but it might not be for an outsider reading it in a few months. https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/user_manager_view.h:106: void SetSigninProfilePath(const base::FilePath& profile_path); Can you please document what those do with a comment? https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:484: // Fresh sign in via user manager without existed email address. nit: existed -> existing
https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/signin_error_handler.cc (right): https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/signin_error_handler.cc:83: } Can you add a comment why this change is needed? I don't see how it fits into this CL. https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:491: } Don't you need to check the "force" flag in ::HandleLaunchUser() and make sure to show the sign in dialog there too? Or is that change planned for a follow up CL?
https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/user_manager_view.cc:436: void UserManagerView::DisplayErrorMessage() { On 2016/09/22 18:38:45, anthonyvd wrote: > It looks like doing it this way makes the delegate load the reauth URL in its > constructor, then switch to the error URL when an error happens. Does the dialog > resize itself properly? As far as I can see, it fits well: https://drive.google.com/file/d/0B7mk_V3OvgKRVFh2c1NLdmdsWHc/view?usp=sharing https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/user_manager_view.h (right): https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/user_manager_view.h:103: // Display sign in error message without browser. On 2016/09/22 18:38:45, anthonyvd wrote: > Can you expand a bit on what that comment means? It's pretty obvious now but it > might not be for an outsider reading it in a few months. Done. https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/user_manager_view.h:106: void SetSigninProfilePath(const base::FilePath& profile_path); On 2016/09/22 18:38:45, anthonyvd wrote: > Can you please document what those do with a comment? Done. https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/signin_error_handler.cc (right): https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/signin_error_handler.cc:83: } On 2016/09/22 18:59:13, Roger Tawa wrote: > Can you add a comment why this change is needed? I don't see how it fits into > this CL. With the old CloseDialog logic, the user manager window will be closed with error dialog. Because with force-signin enabled, there is no browser window at all which means the Chrome will be closed completely. That's why I make the change to close the dialog without closing user manager here. https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:484: // Fresh sign in via user manager without existed email address. On 2016/09/22 18:38:46, anthonyvd wrote: > nit: existed -> existing Done. https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:491: } On 2016/09/22 18:59:13, Roger Tawa wrote: > Don't you need to check the "force" flag in ::HandleLaunchUser() and make sure > to show the sign in dialog there too? Or is that change planned for a follow up > CL? Yes, this CL is focusing on the error dialog. I change the code here just because the refactor in the UserManager.
lgtm, one nit below. https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/signin_error_handler.cc (right): https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/signin_error_handler.cc:83: } On 2016/09/22 19:14:56, zmin wrote: > On 2016/09/22 18:59:13, Roger Tawa wrote: > > Can you add a comment why this change is needed? I don't see how it fits into > > this CL. > > With the old CloseDialog logic, the user manager window will be closed with > error dialog. Because with force-signin enabled, there is no browser window at > all which means the Chrome will be closed completely. > > That's why I make the change to close the dialog without closing user manager > here. Cool. Can you add a comment to the code explaining that?
https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/signin_error_handler.cc (right): https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/signin_error_handler.cc:83: } On 2016/09/22 19:27:20, Roger Tawa wrote: > On 2016/09/22 19:14:56, zmin wrote: > > On 2016/09/22 18:59:13, Roger Tawa wrote: > > > Can you add a comment why this change is needed? I don't see how it fits > into > > > this CL. > > > > With the old CloseDialog logic, the user manager window will be closed with > > error dialog. Because with force-signin enabled, there is no browser window at > > all which means the Chrome will be closed completely. > > > > That's why I make the change to close the dialog without closing user manager > > here. > > Cool. Can you add a comment to the code explaining that? Done.
LGTM https://codereview.chromium.org/2351173004/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/user_manager_view.cc:223: // This method should only be called if the user manager is already showing. Remove comment for similar reasons about DCHECKs?
lgtm
lgtm https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/user_manager_view.cc (right): https://codereview.chromium.org/2351173004/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/user_manager_view.cc:436: void UserManagerView::DisplayErrorMessage() { On 2016/09/22 19:14:56, zmin wrote: > On 2016/09/22 18:38:45, anthonyvd wrote: > > It looks like doing it this way makes the delegate load the reauth URL in its > > constructor, then switch to the error URL when an error happens. Does the > dialog > > resize itself properly? > > As far as I can see, it fits well: > https://drive.google.com/file/d/0B7mk_V3OvgKRVFh2c1NLdmdsWHc/view?usp=sharing I imagine we can modify the content so that it can take up all available space but no need to do it in this CL. Thanks for the screenshot!
The CQ bit was checked by zmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/2351173004/#ps150001 (title: "cr")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:150001)
Message was sent while issue was closed.
Description was changed from ========== Display local signin error without browser and record the path of selected profile in user manager. BUG=642059 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Display local signin error without browser and record the path of selected profile in user manager. BUG=642059 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/c105747d69ec6c66856327ba51765ab6e628e0f3 Cr-Commit-Position: refs/heads/master@{#420713} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c105747d69ec6c66856327ba51765ab6e628e0f3 Cr-Commit-Position: refs/heads/master@{#420713} |
