Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12221111:
Add a modal confirmation dialog to the enterprise profile sign-in flow. (Closed)
|
Created:
7 years, 10 months ago by dconnelly Modified:
7 years, 10 months ago CC:
chromium-reviews, tfarina, arv+watch_chromium.org, sail+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a modal confirmation dialog to the enterprise profile sign-in flow.
When signing in with an enterprise user account, the user is prompted to
create a new Chrome profile, continue signing in with the current
profile, or cancel the signin process.
BUG=171236
TBR=jhawkins
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182702
Patch Set 1 #Patch Set 2 : revert native ui experiment #Patch Set 3 : more cleanup #
Total comments: 27
Patch Set 4 : fix dcheck #Patch Set 5 : Move WebUI handler into WebDialogDelegate implementation #
Total comments: 10
Patch Set 6 : move handler into anon namespace and satisfy the compiler with consts #Patch Set 7 : cancel signin when user closes the tab; class comments #
Total comments: 19
Patch Set 8 : nits #Patch Set 9 : forgot to git add a few #
Total comments: 12
Patch Set 10 : nits #Patch Set 11 : copyright header #Messages
Total messages: 34 (0 generated)
This is very rough, but it's 90% done and I think I can finish the rest on Monday. I'm sending it out for review now to save time. Also my C++ is not yet very good, so any tips are appreciated. What works: - The modal dialog pops up at the right time - It has the right strings - The buttons invoke the proper callbacks Remaining: - Comments - How does one test a modal dialog box? - CSS for the dialog - Should the tab be redirected if the user cancels? - Cancel signin if the tab is closed
Looks pretty good so far, although I'm not the world's greatest web_ui expert. I'd also get rogerta's feedback about this style of dialog. Finally, if you are ready for a final review before I'm back in the office on Tuesday, you can send this over to jhawkins or dbeam for their feedback - they know the most about the web_ui stuff. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/resources/p... File chrome/browser/resources/profile_signin_confirmation.html (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/resources/p... chrome/browser/resources/profile_signin_confirmation.html:6: <link rel="stylesheet" href="hello_world.css"> I know that we don't have CSS setup yet, but should this point to profile_signin_confirmation.css anyway? https://codereview.chromium.org/12221111/diff/1023/chrome/browser/resources/p... File chrome/browser/resources/profile_signin_confirmation.js (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/resources/p... chrome/browser/resources/profile_signin_confirmation.js:10: $('createButton').addEventListener('click', function() { This is fine as is, but one question:Does the modal dialog allow for keyboard navigation (i.e. should we use 'activate' instead of 'click')? https://codereview.chromium.org/12221111/diff/1023/chrome/browser/signin/sign... File chrome/browser/signin/signin_manager.cc (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/signin/sign... chrome/browser/signin/signin_manager.cc:736: base::Passed(policy_client_.Pass()))); So, this is the root of the DCHECK you are hitting - policy_client_.Pass() clears policy_client_ and moves the pointer into a scoped_ptr within the callback you are creating. I'd say the right way to proceed is to change LoadPolicyWithCachedClient() to not take any parameters, and just used the cached policy_client_ instead. It means that CompleteSigninForNewProfile() will need to move policy_client_ to the newly created profile (see my next comment). https://codereview.chromium.org/12221111/diff/1023/chrome/browser/signin/sign... chrome/browser/signin/signin_manager.cc:794: signin_manager->temp_oauth_login_tokens_ = temp_oauth_login_tokens_; So, when you change LoadPolicyWithCachedClient() to not take any parameters, you'll want to add: signin_manager->policy_client_(policy_client_); to transfer ownership of the client to the new profile. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:49: Profile* profile = ProfileManager::GetLastUsedProfile(); You should probably have the caller pass in a profile. GetLastUsedProfile() would work in this case, but we try not to use it. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2012->2013 https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:13: #include "chrome/browser/ui/webui/signin/profile_signin_confirmation_handler.h" Can you use a forward decl (i.e. "class ProfileSigninConfirmationHandler" instead of including the header file? Likewise for WebUIMessageHandler? https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:28: explicit ProfileSigninConfirmationDialog( nit: remove explicit (only use if the constructor takes only one arg) https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:52: mutable scoped_ptr<ProfileSigninConfirmationHandler> handler_; One thing to consider is to do one of the following: a) Get rid of ProfileSIgninConfirmationHandler and move that functionality into this class (that would also allow you to easily fire off the "cancel signin" callback if the user closes the tab and it hasn't been fired off yet). or b) Move the ProfileSIgninConfirmationHandler definition and implementation into profile_signin_confirmation_dialog.cc instead of in a separate file, since it won't be used by any external class. I'd probably do a) unless there's a technical reason not to. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_handler.cc (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_handler.cc:28: base::Bind(&ProfileSigninConfirmationHandler::OnCancelButtonClicked, See my earlier point - if you roll this into the Dialog class, then you can invoke this callback on things like "dialog closed prematurely/via esc key". https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_handler.cc:29: base::Unretained(this))); Are we guaranteed that we'll outlive the web_ui? I think so, but just checking. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_handler.cc:40: // TODO(dconnelly): redirect back to NTP? Yeah, I'm not sure about what to do about redirects, since the user could be signing in to gmail and in that case we don't want to redirect. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_handler.h (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_handler.h:16: explicit ProfileSigninConfirmationHandler( nit: remove explicit. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_handler.h:29: // Deletes itself. Typically, if you don't own an object (like this object does not own dialog_) the typical idiom is to add a comment like: // Weak ptr to parent dialog. This means that we don't own the dialog and don't manage its lifecycle (so we probably shouldn't document when it frees itself here). https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_ui.h (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_ui.h:12: explicit ProfileSigninConfirmationUI(content::WebUI* web_ui); This is the correct use of "explicit" - with a single-parameter constructor. This is to make sure that people don't accidentally create ProfileSigningConfirmationUI objects via an implicit cast like this: content::WebUI* web_ui = some_web_ui_value; ProfileSigninConfirmationUI ui = web_ui;
https://codereview.chromium.org/12221111/diff/1023/chrome/browser/resources/p... File chrome/browser/resources/profile_signin_confirmation.html (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/resources/p... chrome/browser/resources/profile_signin_confirmation.html:6: <link rel="stylesheet" href="hello_world.css"> On 2013/02/10 20:47:26, Andrew T Wilson wrote: > I know that we don't have CSS setup yet, but should this point to > profile_signin_confirmation.css anyway? Done. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/signin/sign... File chrome/browser/signin/signin_manager.cc (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/signin/sign... chrome/browser/signin/signin_manager.cc:736: base::Passed(policy_client_.Pass()))); On 2013/02/10 20:47:26, Andrew T Wilson wrote: > So, this is the root of the DCHECK you are hitting - policy_client_.Pass() > clears policy_client_ and moves the pointer into a scoped_ptr within the > callback you are creating. > > I'd say the right way to proceed is to change LoadPolicyWithCachedClient() to > not take any parameters, and just used the cached policy_client_ instead. It > means that CompleteSigninForNewProfile() will need to move policy_client_ to the > newly created profile (see my next comment). Done. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/signin/sign... chrome/browser/signin/signin_manager.cc:794: signin_manager->temp_oauth_login_tokens_ = temp_oauth_login_tokens_; On 2013/02/10 20:47:26, Andrew T Wilson wrote: > So, when you change LoadPolicyWithCachedClient() to not take any parameters, > you'll want to add: > > signin_manager->policy_client_(policy_client_); > > to transfer ownership of the client to the new profile. Done. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:49: Profile* profile = ProfileManager::GetLastUsedProfile(); On 2013/02/10 20:47:26, Andrew T Wilson wrote: > You should probably have the caller pass in a profile. GetLastUsedProfile() > would work in this case, but we try not to use it. Done. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/02/10 20:47:26, Andrew T Wilson wrote: > 2012->2013 Done. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:13: #include "chrome/browser/ui/webui/signin/profile_signin_confirmation_handler.h" On 2013/02/10 20:47:26, Andrew T Wilson wrote: > Can you use a forward decl (i.e. "class ProfileSigninConfirmationHandler" > instead of including the header file? Likewise for WebUIMessageHandler? Done. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:28: explicit ProfileSigninConfirmationDialog( On 2013/02/10 20:47:26, Andrew T Wilson wrote: > nit: remove explicit (only use if the constructor takes only one arg) Done. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_handler.h (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_handler.h:16: explicit ProfileSigninConfirmationHandler( On 2013/02/10 20:47:26, Andrew T Wilson wrote: > nit: remove explicit. Done. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_handler.h:29: // Deletes itself. On 2013/02/10 20:47:26, Andrew T Wilson wrote: > Typically, if you don't own an object (like this object does not own dialog_) > the typical idiom is to add a comment like: > > // Weak ptr to parent dialog. > > This means that we don't own the dialog and don't manage its lifecycle (so we > probably shouldn't document when it frees itself here). Done. https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_ui.h (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_ui.h:12: explicit ProfileSigninConfirmationUI(content::WebUI* web_ui); On 2013/02/10 20:47:26, Andrew T Wilson wrote: > This is the correct use of "explicit" - with a single-parameter constructor. > > This is to make sure that people don't accidentally create > ProfileSigningConfirmationUI objects via an implicit cast like this: > > content::WebUI* web_ui = some_web_ui_value; > ProfileSigninConfirmationUI ui = web_ui; Cool, thanks
https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:52: mutable scoped_ptr<ProfileSigninConfirmationHandler> handler_; On 2013/02/10 20:47:26, Andrew T Wilson wrote: > One thing to consider is to do one of the following: > > a) Get rid of ProfileSIgninConfirmationHandler and move that functionality into > this class (that would also allow you to easily fire off the "cancel signin" > callback if the user closes the tab and it hasn't been fired off yet). > > or b) Move the ProfileSIgninConfirmationHandler definition and implementation > into profile_signin_confirmation_dialog.cc instead of in a separate file, since > it won't be used by any external class. > > I'd probably do a) unless there's a technical reason not to. There needs to be two distinct objects, because both are owned by different objects: - The WebDialogDelegate (ProfileSigninConfirmationDialog) is owned by the ConstrainedWebDialogDelegate - The WebUIMessageHandler (ProfileSigninConfirmationHandler) is owned by the ConstrainedWebDialogUI So if I merge them it will be deleted twice. I'll go ahead and move the handler into profile_signin_confirmation_dialog.cc, though.
On 2013/02/11 09:59:20, dconnelly wrote: > https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... > File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h > (right): > > https://codereview.chromium.org/12221111/diff/1023/chrome/browser/ui/webui/si... > chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:52: mutable > scoped_ptr<ProfileSigninConfirmationHandler> handler_; > On 2013/02/10 20:47:26, Andrew T Wilson wrote: > > One thing to consider is to do one of the following: > > > > a) Get rid of ProfileSIgninConfirmationHandler and move that functionality > into > > this class (that would also allow you to easily fire off the "cancel signin" > > callback if the user closes the tab and it hasn't been fired off yet). > > > > or b) Move the ProfileSIgninConfirmationHandler definition and implementation > > into profile_signin_confirmation_dialog.cc instead of in a separate file, > since > > it won't be used by any external class. > > > > I'd probably do a) unless there's a technical reason not to. > > There needs to be two distinct objects, because both are owned by different > objects: > - The WebDialogDelegate (ProfileSigninConfirmationDialog) is owned by the > ConstrainedWebDialogDelegate > - The WebUIMessageHandler (ProfileSigninConfirmationHandler) is owned by the > ConstrainedWebDialogUI > > So if I merge them it will be deleted twice. I'll go ahead and move the handler > into profile_signin_confirmation_dialog.cc, though. Done
https://codereview.chromium.org/12221111/diff/1023/chrome/browser/resources/p... File chrome/browser/resources/profile_signin_confirmation.js (right): https://codereview.chromium.org/12221111/diff/1023/chrome/browser/resources/p... chrome/browser/resources/profile_signin_confirmation.js:10: $('createButton').addEventListener('click', function() { On 2013/02/10 20:47:26, Andrew T Wilson wrote: > This is fine as is, but one question:Does the modal dialog allow for keyboard > navigation (i.e. should we use 'activate' instead of 'click')? Whoops, missed this comment. Yes, 'click' seems to be picking up the keyboard actions. I tabbed to the buttons and hit return and it did the right things.
Hi Roger, Drew suggested that I ask you to review this CL. Can you offer any feedback? Thanks! Daniel Connelly
a Few nits, but I'm not very familiar with webui either, so an owner from chrome\browser\ui\webui would be best to look at that. SigninManager changes lgtm. Question about refactoring it below. https://codereview.chromium.org/12221111/diff/11002/chrome/browser/resources/... File chrome/browser/resources/profile_signin_confirmation.html (right): https://codereview.chromium.org/12221111/diff/11002/chrome/browser/resources/... chrome/browser/resources/profile_signin_confirmation.html:16: <a i18n-content="learnMoreText" href="#"></a> might want to keep the <a> inside the <p> so that it flows with the message. https://codereview.chromium.org/12221111/diff/11002/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://codereview.chromium.org/12221111/diff/11002/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:32: class ProfileSigninConfirmationHandler : public content::WebUIMessageHandler { put class in anon namespace too? https://codereview.chromium.org/12221111/diff/11002/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:132: profile, this, NULL, browser->tab_strip_model()->GetActiveWebContents()); Ideally you would want to pass the web_contents directly. Doing that would probably mean refactoring the signin manager code, and I'm not exactly sure how to best do that. Drew: is this something you are looking at when you mentioned that signin manager needs to be refactored to remove dependencies on enterprise policies? https://codereview.chromium.org/12221111/diff/11002/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:143: return ui::MODAL_TYPE_WINDOW; is this modal app-wide? If not, should it be?
Sorry for the delay. I was working down to the wire trying to get another CL submitted before the branch point. https://chromiumcodereview.appspot.com/12221111/diff/11002/chrome/browser/res... File chrome/browser/resources/profile_signin_confirmation.html (right): https://chromiumcodereview.appspot.com/12221111/diff/11002/chrome/browser/res... chrome/browser/resources/profile_signin_confirmation.html:16: <a i18n-content="learnMoreText" href="#"></a> On 2013/02/11 16:24:39, Roger Tawa wrote: > might want to keep the <a> inside the <p> so that it flows with the message. I thought that too, but I sent a screenshot to Cyrus this morning, and he said that the UI is fine like it is. Which is funny, since I didn't even write any CSS for this. https://chromiumcodereview.appspot.com/12221111/diff/11002/chrome/browser/ui/... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://chromiumcodereview.appspot.com/12221111/diff/11002/chrome/browser/ui/... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:32: class ProfileSigninConfirmationHandler : public content::WebUIMessageHandler { On 2013/02/11 16:24:39, Roger Tawa wrote: > put class in anon namespace too? Done. https://chromiumcodereview.appspot.com/12221111/diff/11002/chrome/browser/ui/... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:143: return ui::MODAL_TYPE_WINDOW; On 2013/02/11 16:24:39, Roger Tawa wrote: > is this modal app-wide? If not, should it be? It's local to the tab. Good question. I don't know if it should be app-wide, but I hate app-wide modals and I can't see any technical reason why this isn't okay.
On 2013/02/12 10:04:36, dconnelly wrote: > Sorry for the delay. I was working down to the wire trying to get another CL > submitted before the branch point. > > https://chromiumcodereview.appspot.com/12221111/diff/11002/chrome/browser/res... > File chrome/browser/resources/profile_signin_confirmation.html (right): > > https://chromiumcodereview.appspot.com/12221111/diff/11002/chrome/browser/res... > chrome/browser/resources/profile_signin_confirmation.html:16: <a > i18n-content="learnMoreText" href="#"></a> > On 2013/02/11 16:24:39, Roger Tawa wrote: > > might want to keep the <a> inside the <p> so that it flows with the message. > > I thought that too, but I sent a screenshot to Cyrus this morning, and he said > that the UI is fine like it is. Which is funny, since I didn't even write any > CSS for this. > > https://chromiumcodereview.appspot.com/12221111/diff/11002/chrome/browser/ui/... > File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc > (right): > > https://chromiumcodereview.appspot.com/12221111/diff/11002/chrome/browser/ui/... > chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:32: class > ProfileSigninConfirmationHandler : public content::WebUIMessageHandler { > On 2013/02/11 16:24:39, Roger Tawa wrote: > > put class in anon namespace too? > > Done. > > https://chromiumcodereview.appspot.com/12221111/diff/11002/chrome/browser/ui/... > chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:143: return > ui::MODAL_TYPE_WINDOW; > On 2013/02/11 16:24:39, Roger Tawa wrote: > > is this modal app-wide? If not, should it be? > > It's local to the tab. Good question. I don't know if it should be app-wide, > but I hate app-wide modals and I can't see any technical reason why this isn't > okay. I've added some class comments and logic to cancel sign-in when the tab is closed before any of the buttons are clicked.
https://codereview.chromium.org/12221111/diff/11002/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://codereview.chromium.org/12221111/diff/11002/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:132: profile, this, NULL, browser->tab_strip_model()->GetActiveWebContents()); On 2013/02/11 16:24:39, Roger Tawa wrote: > Ideally you would want to pass the web_contents directly. Doing that would > probably mean refactoring the signin manager code, and I'm not exactly sure how > to best do that. > > Drew: is this something you are looking at when you mentioned that signin > manager needs to be refactored to remove dependencies on enterprise policies? Yeah, currently SigninManager doesn't know anything about what Browser is initiating signin, which is a PITA when you want to display some UI because you end up having to do some hokey stuff like "FindBrowserWithProfile" to try to guess at which Browser is in-use. To add further complexity, some platforms (Android/iOS) don't use Browser objects to surface their UI, so you can't just pass one in to SigninManager. I haven't had the time to really think about this yet but I'm sure we can figure something out. One option I'm strongly considering is to put all the signin UI logic (including integration with loading policy) off in OneClickSignin (and maybe change the name of this class :), which already knows all about the UI/active browser/whether the user is initiating an "Advanced" signin. But that's a task for M27, I think. https://codereview.chromium.org/12221111/diff/11002/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:143: return ui::MODAL_TYPE_WINDOW; On 2013/02/12 10:04:36, dconnelly wrote: > On 2013/02/11 16:24:39, Roger Tawa wrote: > > is this modal app-wide? If not, should it be? > > It's local to the tab. Good question. I don't know if it should be app-wide, > but I hate app-wide modals and I can't see any technical reason why this isn't > okay. Let's leave as-is until the UX guys get back to us.
On 2013/02/12 10:46:40, Andrew T Wilson wrote: > https://codereview.chromium.org/12221111/diff/11002/chrome/browser/ui/webui/s... > File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc > (right): > > https://codereview.chromium.org/12221111/diff/11002/chrome/browser/ui/webui/s... > chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:132: > profile, this, NULL, browser->tab_strip_model()->GetActiveWebContents()); > On 2013/02/11 16:24:39, Roger Tawa wrote: > > Ideally you would want to pass the web_contents directly. Doing that would > > probably mean refactoring the signin manager code, and I'm not exactly sure > how > > to best do that. > > > > Drew: is this something you are looking at when you mentioned that signin > > manager needs to be refactored to remove dependencies on enterprise policies? > > Yeah, currently SigninManager doesn't know anything about what Browser is > initiating signin, which is a PITA when you want to display some UI because you > end up having to do some hokey stuff like "FindBrowserWithProfile" to try to > guess at which Browser is in-use. To add further complexity, some platforms > (Android/iOS) don't use Browser objects to surface their UI, so you can't just > pass one in to SigninManager. > > I haven't had the time to really think about this yet but I'm sure we can figure > something out. One option I'm strongly considering is to put all the signin UI > logic (including integration with loading policy) off in OneClickSignin (and > maybe change the name of this class :), which already knows all about the > UI/active browser/whether the user is initiating an "Advanced" signin. > > But that's a task for M27, I think. > > https://codereview.chromium.org/12221111/diff/11002/chrome/browser/ui/webui/s... > chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:143: return > ui::MODAL_TYPE_WINDOW; > On 2013/02/12 10:04:36, dconnelly wrote: > > On 2013/02/11 16:24:39, Roger Tawa wrote: > > > is this modal app-wide? If not, should it be? > > > > It's local to the tab. Good question. I don't know if it should be app-wide, > > but I hate app-wide modals and I can't see any technical reason why this isn't > > okay. > > Let's leave as-is until the UX guys get back to us. Does everything look fine before I send this off to a WebUI owner?
LGTM here - it's ready for webui review.
Hi James, I have a WebUI modal dialog that needs review from a WebUI expert. Do you have time? Thanks!
https://codereview.chromium.org/12221111/diff/11002/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://codereview.chromium.org/12221111/diff/11002/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:132: profile, this, NULL, browser->tab_strip_model()->GetActiveWebContents()); On 2013/02/12 10:46:41, Andrew T Wilson wrote: > On 2013/02/11 16:24:39, Roger Tawa wrote: > > Ideally you would want to pass the web_contents directly. Doing that would > > probably mean refactoring the signin manager code, and I'm not exactly sure > how > > to best do that. > > > > Drew: is this something you are looking at when you mentioned that signin > > manager needs to be refactored to remove dependencies on enterprise policies? > > Yeah, currently SigninManager doesn't know anything about what Browser is > initiating signin, which is a PITA when you want to display some UI because you > end up having to do some hokey stuff like "FindBrowserWithProfile" to try to > guess at which Browser is in-use. To add further complexity, some platforms > (Android/iOS) don't use Browser objects to surface their UI, so you can't just > pass one in to SigninManager. > > I haven't had the time to really think about this yet but I'm sure we can figure > something out. One option I'm strongly considering is to put all the signin UI > logic (including integration with loading policy) off in OneClickSignin (and > maybe change the name of this class :), which already knows all about the > UI/active browser/whether the user is initiating an "Advanced" signin. > > But that's a task for M27, I think. Yes, sounds like something for M27. For M26, does this code make sure the window containing the tab is brought to the front, and that the particular tab is made active? Or are these things a given since the user is probably interacting with that tab?
Is codereview messing up? I see the first CSS file as being empty.
On 2013/02/12 18:58:08, James Hawkins wrote: > Is codereview messing up? I see the first CSS file as being empty. It's intentional for now (waiting for UX to provide styles if they want any).
On 2013/02/12 20:57:07, Andrew T Wilson wrote: > On 2013/02/12 18:58:08, James Hawkins wrote: > > Is codereview messing up? I see the first CSS file as being empty. > > It's intentional for now (waiting for UX to provide styles if they want any). Please don't add an empty file. Only add the file when/if we do get styles.
On 2013/02/12 21:11:46, James Hawkins wrote: > On 2013/02/12 20:57:07, Andrew T Wilson wrote: > > On 2013/02/12 18:58:08, James Hawkins wrote: > > > Is codereview messing up? I see the first CSS file as being empty. > > > > It's intentional for now (waiting for UX to provide styles if they want any). > > Please don't add an empty file. Only add the file when/if we do get styles. Seems reasonable. I presume you can still review the rest of the CL with that as one of the comments?
https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/reso... File chrome/browser/resources/profile_signin_confirmation.html (right): https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/reso... chrome/browser/resources/profile_signin_confirmation.html:15: <p id="dialogMessage"></p> nit: id must be dash-form. Here and elsewhere. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/reso... chrome/browser/resources/profile_signin_confirmation.html:16: <a i18n-content="learnMoreText" href="http://support.google.com/chromeos/bin/answer.py?hl=en&answer=1331549" target="_blank"></a> nit: 80 cols. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/reso... chrome/browser/resources/profile_signin_confirmation.html:21: <script src="chrome://resources/js/i18n_template2.js"></script> Why is this here and not in head? https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:25: const int kMinimumDialogWidth = 480; nit: Blank line above this to be consistent with the close of the namespace. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h (right): https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:23: // Create and show the modal dialog. |profile| is the current Chrome nit: Creates and shows https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:32: // Programmatically lose the dialog, which will delete itself. nit: Not sure what you mean by 'lose' here, and this needs to be in declarative, not imperative, form. On second look I'm assuming you meant s/lose/close/. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:44: // Overridden from WebDialogDelegate: // WebDialogDelegate implementation. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:60: std::string username_; nit: Document member variables. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... File chrome/browser/ui/webui/signin/profile_signin_confirmation_ui.h (right): https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_ui.h:15: private: nit: Blank line between access sections, i.e., above this line.
https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/reso... File chrome/browser/resources/profile_signin_confirmation.html (right): https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/reso... chrome/browser/resources/profile_signin_confirmation.html:15: <p id="dialogMessage"></p> On 2013/02/12 21:40:40, James Hawkins wrote: > nit: id must be dash-form. Here and elsewhere. Done. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/reso... chrome/browser/resources/profile_signin_confirmation.html:16: <a i18n-content="learnMoreText" href="http://support.google.com/chromeos/bin/answer.py?hl=en&answer=1331549" target="_blank"></a> On 2013/02/12 21:40:40, James Hawkins wrote: > nit: 80 cols. Done. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/reso... chrome/browser/resources/profile_signin_confirmation.html:21: <script src="chrome://resources/js/i18n_template2.js"></script> On 2013/02/12 21:40:40, James Hawkins wrote: > Why is this here and not in head? Was copying the WebUI tutorial from dev.chromium.org. Fixed. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:25: const int kMinimumDialogWidth = 480; On 2013/02/12 21:40:40, James Hawkins wrote: > nit: Blank line above this to be consistent with the close of the namespace. Done. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h (right): https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:23: // Create and show the modal dialog. |profile| is the current Chrome On 2013/02/12 21:40:40, James Hawkins wrote: > nit: Creates and shows Done. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:32: // Programmatically lose the dialog, which will delete itself. On 2013/02/12 21:40:40, James Hawkins wrote: > nit: Not sure what you mean by 'lose' here, and this needs to be in declarative, > not imperative, form. > > On second look I'm assuming you meant s/lose/close/. Done. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:44: // Overridden from WebDialogDelegate: On 2013/02/12 21:40:40, James Hawkins wrote: > // WebDialogDelegate implementation. Done. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:60: std::string username_; On 2013/02/12 21:40:40, James Hawkins wrote: > nit: Document member variables. Done. https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... File chrome/browser/ui/webui/signin/profile_signin_confirmation_ui.h (right): https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_ui.h:15: private: On 2013/02/12 21:40:40, James Hawkins wrote: > nit: Blank line between access sections, i.e., above this line. Done.
https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/reso... File chrome/browser/resources/profile_signin_confirmation.html (right): https://chromiumcodereview.appspot.com/12221111/diff/5018/chrome/browser/reso... chrome/browser/resources/profile_signin_confirmation.html:21: <script src="chrome://resources/js/i18n_template2.js"></script> On 2013/02/12 21:40:40, James Hawkins wrote: > Why is this here and not in head? Just figured out why---it processes the elements in the DOM and inserts strings, so it needs to go last.
Empty CSS file removed. On Tue, Feb 12, 2013 at 10:11 PM, <jhawkins@chromium.org> wrote: > On 2013/02/12 20:57:07, Andrew T Wilson wrote: >> >> On 2013/02/12 18:58:08, James Hawkins wrote: >> > Is codereview messing up? I see the first CSS file as being empty. > > >> It's intentional for now (waiting for UX to provide styles if they want >> any). > > > Please don't add an empty file. Only add the file when/if we do get styles. > > https://chromiumcodereview.appspot.com/12221111/
Okay---all finished. On Wed, Feb 13, 2013 at 10:21 AM, Daniel Connelly <dconnelly@google.com> wrote: > Empty CSS file removed. > > On Tue, Feb 12, 2013 at 10:11 PM, <jhawkins@chromium.org> wrote: >> On 2013/02/12 20:57:07, Andrew T Wilson wrote: >>> >>> On 2013/02/12 18:58:08, James Hawkins wrote: >>> > Is codereview messing up? I see the first CSS file as being empty. >> >> >>> It's intentional for now (waiting for UX to provide styles if they want >>> any). >> >> >> Please don't add an empty file. Only add the file when/if we do get styles. >> >> https://chromiumcodereview.appspot.com/12221111/
https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/reso... File chrome/browser/resources/profile_signin_confirmation.html (right): https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/reso... chrome/browser/resources/profile_signin_confirmation.html:16: href="http://support.google.com/chromeos/bin/answer.py?hl=en&answer=1331549" nit: Indentation must be 4 spaces in. https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/reso... chrome/browser/resources/profile_signin_confirmation.html:17: target="_blank"></a> nit: Closing tag of a wrapped element must be on a new line. https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/reso... chrome/browser/resources/profile_signin_confirmation.html:22: <script src="chrome://resources/js/i18n_template2.js"></script> You moved it back. What is the reason? https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/ui/w... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:26: const int kMinimumDialogWidth = 480; These are only used in one method, so declare them in that method. https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:37: virtual void RegisterMessages() OVERRIDE; // content::WebUIMessageHandler implementation. https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:47: base::Closure cancel_signin_; nit: Document member variables.
I thought I responded about the script tag, but maybe not. It processes the DOM elements on the page and inserts localized strings, so it has to go last in the body. You can check out the script to see. On Wednesday, February 13, 2013, wrote: > > https://chromiumcodereview.**appspot.com/12221111/diff/** > 4022/chrome/browser/resources/**profile_signin_confirmation.**html<https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/resources/profile_signin_confirmation.html> > File chrome/browser/resources/**profile_signin_confirmation.**html > (right): > > https://chromiumcodereview.**appspot.com/12221111/diff/** > 4022/chrome/browser/resources/**profile_signin_confirmation.** > html#newcode16<https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/resources/profile_signin_confirmation.html#newcode16> > chrome/browser/resources/**profile_signin_confirmation.**html:16: > href="http://support.google.**com/chromeos/bin/answer.py?hl=** > en&answer=1331549<http://support.google.com/chromeos/bin/answer.py?hl=en&answer=1331549> > " > nit: Indentation must be 4 spaces in. > > https://chromiumcodereview.**appspot.com/12221111/diff/** > 4022/chrome/browser/resources/**profile_signin_confirmation.** > html#newcode17<https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/resources/profile_signin_confirmation.html#newcode17> > chrome/browser/resources/**profile_signin_confirmation.**html:17: > target="_blank"></a> > nit: Closing tag of a wrapped element must be on a new line. > > https://chromiumcodereview.**appspot.com/12221111/diff/** > 4022/chrome/browser/resources/**profile_signin_confirmation.** > html#newcode22<https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/resources/profile_signin_confirmation.html#newcode22> > chrome/browser/resources/**profile_signin_confirmation.**html:22: <script > src="chrome://resources/js/**i18n_template2.js"></script> > You moved it back. What is the reason? > > https://chromiumcodereview.**appspot.com/12221111/diff/** > 4022/chrome/browser/ui/webui/**signin/profile_signin_** > confirmation_dialog.cc<https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc> > File > chrome/browser/ui/webui/**signin/profile_signin_**confirmation_dialog.cc > (right): > > https://chromiumcodereview.**appspot.com/12221111/diff/** > 4022/chrome/browser/ui/webui/**signin/profile_signin_** > confirmation_dialog.cc#**newcode26<https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc#newcode26> > chrome/browser/ui/webui/**signin/profile_signin_** > confirmation_dialog.cc:26: > const int kMinimumDialogWidth = 480; > These are only used in one method, so declare them in that method. > > https://chromiumcodereview.**appspot.com/12221111/diff/** > 4022/chrome/browser/ui/webui/**signin/profile_signin_** > confirmation_dialog.cc#**newcode37<https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc#newcode37> > chrome/browser/ui/webui/**signin/profile_signin_** > confirmation_dialog.cc:37: > virtual void RegisterMessages() OVERRIDE; > // content::WebUIMessageHandler implementation. > > https://chromiumcodereview.**appspot.com/12221111/diff/** > 4022/chrome/browser/ui/webui/**signin/profile_signin_** > confirmation_dialog.cc#**newcode47<https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc#newcode47> > chrome/browser/ui/webui/**signin/profile_signin_** > confirmation_dialog.cc:47: > base::Closure cancel_signin_; > nit: Document member variables. > > https://chromiumcodereview.**appspot.com/12221111/<https://chromiumcodereview... >
PTAL https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/reso... File chrome/browser/resources/profile_signin_confirmation.html (right): https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/reso... chrome/browser/resources/profile_signin_confirmation.html:16: href="http://support.google.com/chromeos/bin/answer.py?hl=en&answer=1331549" On 2013/02/13 21:41:31, James Hawkins wrote: > nit: Indentation must be 4 spaces in. Done. https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/reso... chrome/browser/resources/profile_signin_confirmation.html:17: target="_blank"></a> On 2013/02/13 21:41:31, James Hawkins wrote: > nit: Closing tag of a wrapped element must be on a new line. Done. https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/reso... chrome/browser/resources/profile_signin_confirmation.html:22: <script src="chrome://resources/js/i18n_template2.js"></script> On 2013/02/13 21:41:31, James Hawkins wrote: > You moved it back. What is the reason? Added a comment. https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/ui/w... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:26: const int kMinimumDialogWidth = 480; On 2013/02/13 21:41:31, James Hawkins wrote: > These are only used in one method, so declare them in that method. Done. https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:37: virtual void RegisterMessages() OVERRIDE; On 2013/02/13 21:41:31, James Hawkins wrote: > // content::WebUIMessageHandler implementation. Done. https://chromiumcodereview.appspot.com/12221111/diff/4022/chrome/browser/ui/w... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:47: base::Closure cancel_signin_; On 2013/02/13 21:41:31, James Hawkins wrote: > nit: Document member variables. Done.
James, we need to land this ASAP as this addresses a beta blocker, so if it's possible to give your LGTM pending fixing of any further nits it would help prevent another 24-hour turnaround.
Hi James, We're going to go ahead and commit this. Please let me know if you have more feedback and I'll address it. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12221111/17002
Presubmit check for 12221111-17002 failed and returned exit status 1. INFO:root:Found 13 file(s). Running presubmit commit checks ... Running /b/commit-queue/workdir/chromium/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py ** Presubmit Messages ** --tbr was specified, skipping OWNERS check ** Presubmit Warnings ** License must match: .*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: \*/)?\n Found a bad license header in these files: chrome/browser/resources/profile_signin_confirmation.js Presubmit checks took 1.0s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12221111/25002
Message was sent while issue was closed.
Committed manually as r182702 (presubmit successful).
Message was sent while issue was closed.
lgtm |
