Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14846020:
Add Views implementation of ProfileSigninConfirmationDialog. (Closed)
|
Created:
7 years, 7 months ago by dconnelly Modified:
7 years, 6 months ago CC:
chromium-reviews, Raghu Simha, haitaol1, akalin, tim (not reviewing), tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd Views implementation of ProfileSigninConfirmationDialog.
The existing WebUI implementation is still used on platforms that
don't support the Views toolkit.
Current screenshot:
https://code.google.com/p/chromium/issues/detail?id=179444#c25
BUG=179444
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203709
Patch Set 1 #Patch Set 2 : cleanup, remove weak ptrs #Patch Set 3 : more cleanup #Patch Set 4 : make new strings for new dialog #Patch Set 5 : no ifdef on function proto #
Total comments: 28
Patch Set 6 : bool to enum, split strings, cleanups #Patch Set 7 : no forward declared enums #
Total comments: 57
Patch Set 8 : pkasting nits #Patch Set 9 : better strings use #Patch Set 10 : #Patch Set 11 : rebase #Patch Set 12 : remove default button #
Total comments: 18
Patch Set 13 : more nits; use alphas instead of colors #Patch Set 14 : old explanation text #Patch Set 15 : move color calculation to helper file for multiplatform use #
Total comments: 5
Patch Set 16 : more nits #Messages
Total messages: 28 (0 generated)
Evan: do you mind reviewing the views stuff in ui/views/sync? James: this refactors some stuff in ui/webui/signin
I'm not a views owner.
Hi Peter, This CL adds a Views implementation of an existing WebUI dialog box. The plan is to incrementally replace the existing WebUI implementation with native toolkit implementations, starting with Windows (this one). This is my first experience with the Views toolkit---do you mind looking it over? Thanks!
-estade On Tue, May 7, 2013 at 5:41 PM, <estade@chromium.org> wrote: > I'm not a views owner. > > https://codereview.chromium.org/14846020/
Can you point out in that thread where agreement was reached to move to native UI?
On 2013/05/07 17:07:55, James Hawkins wrote: > Can you point out in that thread where agreement was reached to move to native > UI? Yup, I just forwarded it to you.
https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:160: delete this; This is really fragile. Don't we have a ScopedDeleter or something which we can cancel on the success case (or pass ownership through CreateConstrainedWebDialog)? https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h (right): https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:30: friend void chrome::ShowProfileSigninConfirmationDialog( Why is this necessary?
https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:160: delete this; On 2013/05/07 17:22:44, James Hawkins wrote: > This is really fragile. Don't we have a ScopedDeleter or something which we can > cancel on the success case (or pass ownership through > CreateConstrainedWebDialog)? I don't know anything about that, but it sounds good. I'll look into it first thing tomorrow. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h (right): https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:30: friend void chrome::ShowProfileSigninConfirmationDialog( On 2013/05/07 17:22:44, James Hawkins wrote: > Why is this necessary? This class has a private constructor and destructor and the friend function isn't part of the class. The friend function is declared once in browser_dialogs.h and is (will be) defined per-platform to show the right implementation. I'm happy to use a different approach, this is just the one suggested by ben@ in an aborted refactoring CL.
https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h (right): https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:10: #include "chrome/browser/ui/browser_dialogs.h" rm this include, if necessary, include in the source file. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:17: namespace content { also unused, remove. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:22: class TreeView; remove these unused forward declarations https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:32: // views::DialogDelegate: can all these overriddes be private? https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:81: bool responded; s/responded/responded_ Could you name it better though?
https://codereview.chromium.org/14846020/diff/7002/chrome/app/chromium_string... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/14846020/diff/7002/chrome/app/chromium_string... chrome/app/chromium_strings.grd:885: Your bookmarks, history and other Chromium data can be saved to your Google Account and will then be controlled by the <ph name="DOMAIN">$1<ex>example.com</ex></ph> administrator. You can optionally create a new profile to keep your existing Chrome data separate. <ph name="LEARN_MORE">$2<ex>Learn more</ex></ph> Just to document our offline discussion - we probably need to break out the "You can optionally create a new profile..." text into a separate string so we can omit/hide it in the case that this is a new/clean profile. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc (right): https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:54: #if defined(TOOLKIT_VIEWS) Is this needed? I would have thought that _views.cc files were not compiled in if TOOLKIT_VIEWS is not defined. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:153: void ProfileSigninConfirmationDialogViews::Show(bool prompt) { BTW, I don't like bools that much for things like this - better to have an enum: enum DisplayCreateProfilePrompt { PROMPT_TO_CREATE_PROFILE, DO_NOT_PROMPT_TO_CREATE_PROFILE } which makes it easier to parse when reading the code (i.e. when you see Show(true) you don't have to go look up the definition of Show() to know what |true| means. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:158: cancel_signin_.Run(); This is possibly OK, although I suspect it might break inappropriately if the user switches desktops while signing in? Perhaps if we can't find a browser, we should create one? Also consider either passing in a Browser (tricky, because you may need to listen for that browser getting closed), or passing in a desktop instead of using chrome::GetActiveDesktop() since the caller knows what browser/desktop should be used. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:199: responded = true; Consider just Reset()-ing the callbacks once you've called one instead of keeping a responded flag. It's more fail-safe, and also probably clearer as to your intent. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:221: if (source == link_) { What does it mean if source != link? Is this possible? https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h (right): https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:32: // views::DialogDelegate: On 2013/05/07 18:16:18, tfarina wrote: > can all these overriddes be private? No objection to moving these, but wanted to note for dconnelly's benefit that the style guide says all inheritance should be public, and our last discussion of this topic on chromium-dev resulted in the conclusion that either approach is acceptable, at the discretion of the author. That said, you could probably make your own private implementation of DialogDelegateView inside the .cc file (since most of the functions don't require access to this class) and hide the implementation entirely (use composition instead of having this class implement DialogDelegateView).
https://codereview.chromium.org/14846020/diff/7002/chrome/app/chromium_string... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/14846020/diff/7002/chrome/app/chromium_string... chrome/app/chromium_strings.grd:885: Your bookmarks, history and other Chromium data can be saved to your Google Account and will then be controlled by the <ph name="DOMAIN">$1<ex>example.com</ex></ph> administrator. You can optionally create a new profile to keep your existing Chrome data separate. <ph name="LEARN_MORE">$2<ex>Learn more</ex></ph> On 2013/05/08 14:23:48, Andrew T Wilson wrote: > Just to document our offline discussion - we probably need to break out the "You > can optionally create a new profile..." text into a separate string so we can > omit/hide it in the case that this is a new/clean profile. Done. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc (right): https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:54: #if defined(TOOLKIT_VIEWS) On 2013/05/08 14:23:48, Andrew T Wilson wrote: > Is this needed? I would have thought that _views.cc files were not compiled in > if TOOLKIT_VIEWS is not defined. Done. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:153: void ProfileSigninConfirmationDialogViews::Show(bool prompt) { On 2013/05/08 14:23:48, Andrew T Wilson wrote: > BTW, I don't like bools that much for things like this - better to have an enum: > > enum DisplayCreateProfilePrompt { > PROMPT_TO_CREATE_PROFILE, > DO_NOT_PROMPT_TO_CREATE_PROFILE > } > > which makes it easier to parse when reading the code (i.e. when you see > Show(true) you don't have to go look up the definition of Show() to know what > |true| means. Done. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:158: cancel_signin_.Run(); On 2013/05/08 14:23:48, Andrew T Wilson wrote: > This is possibly OK, although I suspect it might break inappropriately if the > user switches desktops while signing in? > > Perhaps if we can't find a browser, we should create one? Also consider either > passing in a Browser (tricky, because you may need to listen for that browser > getting closed), or passing in a desktop instead of using > chrome::GetActiveDesktop() since the caller knows what browser/desktop should be > used. Done. Browser now passed by the caller. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:199: responded = true; On 2013/05/08 14:23:48, Andrew T Wilson wrote: > Consider just Reset()-ing the callbacks once you've called one instead of > keeping a responded flag. It's more fail-safe, and also probably clearer as to > your intent. Done. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:221: if (source == link_) { On 2013/05/08 14:23:48, Andrew T Wilson wrote: > What does it mean if source != link? Is this possible? Done. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h (right): https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:10: #include "chrome/browser/ui/browser_dialogs.h" On 2013/05/07 18:16:18, tfarina wrote: > rm this include, if necessary, include in the source file. Done. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:17: namespace content { On 2013/05/07 18:16:18, tfarina wrote: > also unused, remove. Done. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:22: class TreeView; On 2013/05/07 18:16:18, tfarina wrote: > remove these unused forward declarations Done. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:32: // views::DialogDelegate: On 2013/05/08 14:23:48, Andrew T Wilson wrote: > On 2013/05/07 18:16:18, tfarina wrote: > > can all these overriddes be private? > > No objection to moving these, but wanted to note for dconnelly's benefit that > the style guide says all inheritance should be public, and our last discussion > of this topic on chromium-dev resulted in the conclusion that either approach is > acceptable, at the discretion of the author. > > That said, you could probably make your own private implementation of > DialogDelegateView inside the .cc file (since most of the functions don't > require access to this class) and hide the implementation entirely (use > composition instead of having this class implement DialogDelegateView). If I'm doing that, is there any reason to not just hide the entire implementation? The only use of this class is through the single ShowDialog function; there are no other clients. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/views/sy... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:81: bool responded; On 2013/05/07 18:16:18, tfarina wrote: > s/responded/responded_ > > Could you name it better though? Removed. https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://codereview.chromium.org/14846020/diff/7002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:160: delete this; On 2013/05/07 17:31:30, dconnelly wrote: > On 2013/05/07 17:22:44, James Hawkins wrote: > > This is really fragile. Don't we have a ScopedDeleter or something which we > can > > cancel on the success case (or pass ownership through > > CreateConstrainedWebDialog)? > > I don't know anything about that, but it sounds good. I'll look into it first > thing tomorrow. No longer needed
https://codereview.chromium.org/14846020/diff/25001/chrome/app/chromium_strin... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/14846020/diff/25001/chrome/app/chromium_strin... chrome/app/chromium_strings.grd:881: <message name="IDS_ENTERPRISE_SIGNIN_PROFILE_LINK_DIALOG_TITLE_NEW_STYLE" desc="The title of the dialog to confirm linking the browser profile with the signed-in enterprise account"> Make sure you come back and edit all these files to remove the "new style" bits as soon as the old strings can be removed. https://codereview.chromium.org/14846020/diff/25001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/14846020/diff/25001/chrome/app/generated_reso... chrome/app/generated_resources.grd:11168: + <message name="IDS_ENTERPRISE_SIGNIN_CREATE_NEW_PROFILE_NO_NEW_STYLE" desc="Text of the button to link the current profile with the signed-in enterprise account"> Nit: "CREATE_NEW_PROFILE_{YES|NO}" is confusing, especially since the text on these buttons isn't YES and NO. Use better names, like IDS_ENTERPRISE_SIGNIN_CONTINUE or something. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/on... File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_sync_starter.cc:102: // This callback is only invoked for the web-based signin flow - for the old Nit: This comment is now somewhat inaccurate (in that your new flow is not web-based), right? https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_sync_starter.cc:148: DLOG(WARNING) << "No browser found to display the confirmation dialog"; In most Chrome code we try to avoid logging because we usually don't have a good way of getting the log data and doing something useful with it anyway. Do you really need these logs? It would be clearer to either just handle these cases and return (if they can actually ever happen) or DCHECK and don't handle them if they can't actually occur. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/pr... File chrome/browser/ui/sync/profile_signin_confirmation_helper.cc (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/pr... chrome/browser/ui/sync/profile_signin_confirmation_helper.cc:182: : ui::DO_NOT_PROMPT_TO_CREATE_PROFILE); Nit: Illegal wrapping style; operators go on the ends of lines. Wrap after '?' instead of ':'. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/pr... File chrome/browser/ui/sync/profile_signin_confirmation_helper.h (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/pr... chrome/browser/ui/sync/profile_signin_confirmation_helper.h:29: const base::Callback<void(DisplayCreateProfilePrompt)>& cb); Nit: Personally I slightly prefer the old bool, since the function name makes it pretty clear what it means (and you can add an explicit comment if you think that's not enough). I'm more in favor of using enums in place of bools when e.g. there are multiple bool args in a row so it's easy to accidentally swap them without the compile catching you, or when it's impossible to name/comment things sufficiently clearly. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:95: const int kDialogWidth = 440; Nit: Declare just above first use. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:111: views::View* prompt_container = MakeFixedWidth(prompt_label, kDialogWidth); Why are these MakeFixedWidth() calls necessary? https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:114: 1, 0, 1, 0, SkColorSetRGB(0xE1, 0xE1, 0xE1))); You can't hardcode colors. Please use appropriate colors from the theme service or other globally defined/flexible constants (even if this means you can't use the precise color in whatever mocks you've been given). As a last resort, add new colors to the theming system that are suitably general for others to use as well. (2 places) https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:118: AddChildView(prompt_container); Normally we have views not create or add any children except in response to being added themselves in ViewHierarchyChanged(). This prevents any possibility of children leaking if the view never gets properly added to a hierarchy. I believe doing this would also eliminate your need to have things like UpdateExplanationLabel() since you could just take those actions directly while creating the children. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:147: label_text += ASCIIToUTF16(" ") + create_explanation_text; You can't paste together strings this way, it's not localizable. You need to have one big string, or have a "wrapper string" that you substitute other strings into, or something else that's entirely .grd-controlled and doesn't involve manual concatenation. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:164: void ProfileSigninConfirmationDialogViews::UpdateCreateProfileLink( Seems like we could just do all this in CreateExtraView() directly. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:190: return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL; Isn't this just what the base class method already does? https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:195: switch (button) { Nit: Simpler and more readable: return l10n_util::GetStringUTF16((button == ui::DIALOG_BUTTON_OK) ? IDS_ENTERPRISE_SIGNIN_CREATE_NEW_PROFILE_NO_NEW_STYLE : IDS_ENTERPRISE_SIGNIN_CREATE_NEW_PROFILE_CANCEL); https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:213: delete this; Isn't this just what the base class method already does? https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:253: "http://support.google.com/chromeos/bin/answer.py?hl=en&answer=1331549"; Nit: Just inline this directly. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:255: browser_, GURL(url), content::PAGE_TRANSITION_AUTO_TOPLEVEL); Why AUTO_TOPLEVEL? Why is this not just a LINK? https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:27: void ShowProfileSigninConfirmationDialog( See comments in non-views .h file. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:39: // views::DialogDelegate: Nit: Do these functions all need to be public? In general, make overriding functions as private as possible (e.g. if they're only ever called through polymorphic base class pointers). https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:58: friend void ::chrome::ShowProfileSigninConfirmationDialog( See comments in non-views .h file. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:134: : web_contents_(web_contents), Tiny nit: It would be nice if this function's arg order and the class member declaration order agreed. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:28: void ShowProfileSigninConfirmationDialog( Why do we have this declaration both here and in browser_dialogs.h? This function should only be declared in one place. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:29: Browser* browser, content::WebContents* web_contents, Nit: While this wrapping style is legal, I think putting one arg per line is more common and more readable. Also, please name all arguments. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:42: friend void ::chrome::ShowProfileSigninConfirmationDialog( Nit: It seems like we could avoid the need for this friend declaration by either making the constructor public or making the creation function be a public static method of this class instead of a global within the chrome namespace. Either way seems better; I'd rather avoid friend declarations for things other than unit tests unless there's no other good way. (I don't know offhand whether perhaps the "public static method" route doesn't make sense, based on what further cleanup you'd like to do after this patch. I will note that it's still OK to make the function a public static method on this class but actually define it for TOOLKIT_VIEWS off in the *_views.cc file; we do that with lots of other UI objects.)
LGTM, once you resolve Peter's feedback. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/on... File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_sync_starter.cc:102: // This callback is only invoked for the web-based signin flow - for the old On 2013/05/15 00:11:03, Peter Kasting wrote: > Nit: This comment is now somewhat inaccurate (in that your new flow is not > web-based), right? This comment has nothing to do with the confirmation dialog - it's talking about the signin flow (GAIA web-form based vs old webui client-login based). https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_sync_starter.cc:148: DLOG(WARNING) << "No browser found to display the confirmation dialog"; On 2013/05/15 00:11:03, Peter Kasting wrote: > In most Chrome code we try to avoid logging because we usually don't have a good > way of getting the log data and doing something useful with it anyway. Do you > really need these logs? > > It would be clearer to either just handle these cases and return (if they can > actually ever happen) or DCHECK and don't handle them if they can't actually > occur. Do we really have any restrictions on adding DLOGs()? This is often useful when debugging failed tests, especially for exceptional conditions. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/pr... File chrome/browser/ui/sync/profile_signin_confirmation_helper.h (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/pr... chrome/browser/ui/sync/profile_signin_confirmation_helper.h:29: const base::Callback<void(DisplayCreateProfilePrompt)>& cb); On 2013/05/15 00:11:03, Peter Kasting wrote: > Nit: Personally I slightly prefer the old bool, since the function name makes it > pretty clear what it means (and you can add an explicit comment if you think > that's not enough). > > I'm more in favor of using enums in place of bools when e.g. there are multiple > bool args in a row so it's easy to accidentally swap them without the compile > catching you, or when it's impossible to name/comment things sufficiently > clearly. Peter has a reasonable point - if you also prefer the old bool, I have no objections either way.
https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/on... File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_sync_starter.cc:148: DLOG(WARNING) << "No browser found to display the confirmation dialog"; On 2013/05/16 08:00:30, Andrew T Wilson wrote: > On 2013/05/15 00:11:03, Peter Kasting wrote: > > In most Chrome code we try to avoid logging because we usually don't have a > good > > way of getting the log data and doing something useful with it anyway. Do you > > really need these logs? > > > > It would be clearer to either just handle these cases and return (if they can > > actually ever happen) or DCHECK and don't handle them if they can't actually > > occur. > > Do we really have any restrictions on adding DLOGs()? This is often useful when > debugging failed tests, especially for exceptional conditions. In the particular code here, it seems like either these conditions are impossible -- in which case we should at most DCHECK() them, and not handle them, which will be just as informative in a test failure -- or they're reasonable for some particular scenario(s), in which case we really shouldn't be logging them, we should just silently handle them. Logging here would be more appropriate if you're in the process of trying to make these conditions hold, but you think they currently don't, and you think there are important cases that will break if using a DCHECK, but you have a specific plan on how to locate the log output and use it to finish off the fixes. In general, there aren't hard-and-fast rules so much as a desire to have no middle ground between "this can happen" and "this cannot happen". Something like DLOG(WARNING) is worrisome in the same sort of way that handling DCHECK failures is worrisome -- it becomes unclear precisely what the intent of the code or the magnitude of any problem is, and over time we tend toward console spew about issues no one is sure are important or aware of how to deal with. For short-term changes, logs are welcome if they're useful, it's just not good to have them persist for a significant length of time.
https://codereview.chromium.org/14846020/diff/25001/chrome/app/chromium_strin... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/14846020/diff/25001/chrome/app/chromium_strin... chrome/app/chromium_strings.grd:881: <message name="IDS_ENTERPRISE_SIGNIN_PROFILE_LINK_DIALOG_TITLE_NEW_STYLE" desc="The title of the dialog to confirm linking the browser profile with the signed-in enterprise account"> On 2013/05/15 00:11:03, Peter Kasting wrote: > Make sure you come back and edit all these files to remove the "new style" bits > as soon as the old strings can be removed. TODO added. https://codereview.chromium.org/14846020/diff/25001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/14846020/diff/25001/chrome/app/generated_reso... chrome/app/generated_resources.grd:11168: + <message name="IDS_ENTERPRISE_SIGNIN_CREATE_NEW_PROFILE_NO_NEW_STYLE" desc="Text of the button to link the current profile with the signed-in enterprise account"> On 2013/05/15 00:11:03, Peter Kasting wrote: > Nit: "CREATE_NEW_PROFILE_{YES|NO}" is confusing, especially since the text on > these buttons isn't YES and NO. Use better names, like > IDS_ENTERPRISE_SIGNIN_CONTINUE or something. Done. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/on... File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_sync_starter.cc:148: DLOG(WARNING) << "No browser found to display the confirmation dialog"; On 2013/05/16 21:57:39, Peter Kasting wrote: > On 2013/05/16 08:00:30, Andrew T Wilson wrote: > > On 2013/05/15 00:11:03, Peter Kasting wrote: > > > In most Chrome code we try to avoid logging because we usually don't have a > > good > > > way of getting the log data and doing something useful with it anyway. Do > you > > > really need these logs? > > > > > > It would be clearer to either just handle these cases and return (if they > can > > > actually ever happen) or DCHECK and don't handle them if they can't actually > > > occur. > > > > Do we really have any restrictions on adding DLOGs()? This is often useful > when > > debugging failed tests, especially for exceptional conditions. > > In the particular code here, it seems like either these conditions are > impossible -- in which case we should at most DCHECK() them, and not handle > them, which will be just as informative in a test failure -- or they're > reasonable for some particular scenario(s), in which case we really shouldn't be > logging them, we should just silently handle them. > > Logging here would be more appropriate if you're in the process of trying to > make these conditions hold, but you think they currently don't, and you think > there are important cases that will break if using a DCHECK, but you have a > specific plan on how to locate the log output and use it to finish off the > fixes. > > In general, there aren't hard-and-fast rules so much as a desire to have no > middle ground between "this can happen" and "this cannot happen". Something > like DLOG(WARNING) is worrisome in the same sort of way that handling DCHECK > failures is worrisome -- it becomes unclear precisely what the intent of the > code or the magnitude of any problem is, and over time we tend toward console > spew about issues no one is sure are important or aware of how to deal with. > For short-term changes, logs are welcome if they're useful, it's just not good > to have them persist for a significant length of time. Done: removed the logging and kept the handling. This situation can occur if the user starts signin and then closes the browser before this is reached. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/pr... File chrome/browser/ui/sync/profile_signin_confirmation_helper.cc (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/pr... chrome/browser/ui/sync/profile_signin_confirmation_helper.cc:182: : ui::DO_NOT_PROMPT_TO_CREATE_PROFILE); On 2013/05/15 00:11:03, Peter Kasting wrote: > Nit: Illegal wrapping style; operators go on the ends of lines. Wrap after '?' > instead of ':'. Removed now that I brought back the bool. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/pr... File chrome/browser/ui/sync/profile_signin_confirmation_helper.h (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/sync/pr... chrome/browser/ui/sync/profile_signin_confirmation_helper.h:29: const base::Callback<void(DisplayCreateProfilePrompt)>& cb); On 2013/05/15 00:11:03, Peter Kasting wrote: > Nit: Personally I slightly prefer the old bool, since the function name makes it > pretty clear what it means (and you can add an explicit comment if you think > that's not enough). > > I'm more in favor of using enums in place of bools when e.g. there are multiple > bool args in a row so it's easy to accidentally swap them without the compile > catching you, or when it's impossible to name/comment things sufficiently > clearly. Done. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:95: const int kDialogWidth = 440; On 2013/05/15 00:11:03, Peter Kasting wrote: > Nit: Declare just above first use. Done. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:111: views::View* prompt_container = MakeFixedWidth(prompt_label, kDialogWidth); On 2013/05/15 00:11:03, Peter Kasting wrote: > Why are these MakeFixedWidth() calls necessary? Was trying to reduce the amount of code to write. GridLayout::CreatePanel will set up a container with the right margins and spacing for the "new dialog style", and if that container has a fixed width specified then the StyledLabel will automatically resize properly. MakeFixedWidth does both of these. I think to get rid of this I would need to use GridLayout for this entire view, and manually set the margins for the relevant child labels. I think that's not necessarily more clear, and is more code, but if you think it's better then I can do it. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:114: 1, 0, 1, 0, SkColorSetRGB(0xE1, 0xE1, 0xE1))); On 2013/05/15 00:11:03, Peter Kasting wrote: > You can't hardcode colors. Please use appropriate colors from the theme service > or other globally defined/flexible constants (even if this means you can't use > the precise color in whatever mocks you've been given). > > As a last resort, add new colors to the theming system that are suitably general > for others to use as well. > > (2 places) "git grep SkColorSetRGB -- chrome/browser/ui" shows quite a few files that define color constants for specific views. Is that not an acceptable approach? https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:118: AddChildView(prompt_container); On 2013/05/15 00:11:03, Peter Kasting wrote: > Normally we have views not create or add any children except in response to > being added themselves in ViewHierarchyChanged(). This prevents any possibility > of children leaking if the view never gets properly added to a hierarchy. > > I believe doing this would also eliminate your need to have things like > UpdateExplanationLabel() since you could just take those actions directly while > creating the children. Done. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:147: label_text += ASCIIToUTF16(" ") + create_explanation_text; On 2013/05/15 00:11:03, Peter Kasting wrote: > You can't paste together strings this way, it's not localizable. > > You need to have one big string, or have a "wrapper string" that you substitute > other strings into, or something else that's entirely .grd-controlled and > doesn't involve manual concatenation. Done. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:164: void ProfileSigninConfirmationDialogViews::UpdateCreateProfileLink( On 2013/05/15 00:11:03, Peter Kasting wrote: > Seems like we could just do all this in CreateExtraView() directly. Done. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:190: return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL; On 2013/05/15 00:11:03, Peter Kasting wrote: > Isn't this just what the base class method already does? Done. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:195: switch (button) { On 2013/05/15 00:11:03, Peter Kasting wrote: > Nit: Simpler and more readable: > > return l10n_util::GetStringUTF16((button == ui::DIALOG_BUTTON_OK) ? > IDS_ENTERPRISE_SIGNIN_CREATE_NEW_PROFILE_NO_NEW_STYLE : > IDS_ENTERPRISE_SIGNIN_CREATE_NEW_PROFILE_CANCEL); Done. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:213: delete this; On 2013/05/15 00:11:03, Peter Kasting wrote: > Isn't this just what the base class method already does? Done. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:253: "http://support.google.com/chromeos/bin/answer.py?hl=en&answer=1331549"; On 2013/05/15 00:11:03, Peter Kasting wrote: > Nit: Just inline this directly. Done. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:255: browser_, GURL(url), content::PAGE_TRANSITION_AUTO_TOPLEVEL); On 2013/05/15 00:11:03, Peter Kasting wrote: > Why AUTO_TOPLEVEL? Why is this not just a LINK? From the descriptions of the transition types: // This is a toplevel navigation. This is any content that is automatically // loaded in a toplevel frame. For example, opening a tab to show the ASH // screen saver, opening the devtools window, opening the NTP after the safe // browsing warning, opening web-based dialog boxes are examples of // AUTO_TOPLEVEL navigations. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:27: void ShowProfileSigninConfirmationDialog( On 2013/05/15 00:11:03, Peter Kasting wrote: > See comments in non-views .h file. Responded there. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:39: // views::DialogDelegate: On 2013/05/15 00:11:03, Peter Kasting wrote: > Nit: Do these functions all need to be public? In general, make overriding > functions as private as possible (e.g. if they're only ever called through > polymorphic base class pointers). Done. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:58: friend void ::chrome::ShowProfileSigninConfirmationDialog( On 2013/05/15 00:11:03, Peter Kasting wrote: > See comments in non-views .h file. Responded there. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:134: : web_contents_(web_contents), On 2013/05/15 00:11:03, Peter Kasting wrote: > Tiny nit: It would be nice if this function's arg order and the class member > declaration order agreed. Done. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:28: void ShowProfileSigninConfirmationDialog( On 2013/05/15 00:11:03, Peter Kasting wrote: > Why do we have this declaration both here and in browser_dialogs.h? This > function should only be declared in one place. It's declared in browser_dialogs.h. It's re-declared here because tfarina asked me to remove the browser_dialogs.h include, and the friend declaration requires functions in a different namespace to be forward-declared. See other comment below on the friend declaration. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:29: Browser* browser, content::WebContents* web_contents, On 2013/05/15 00:11:03, Peter Kasting wrote: > Nit: While this wrapping style is legal, I think putting one arg per line is > more common and more readable. > > Also, please name all arguments. Done. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:42: friend void ::chrome::ShowProfileSigninConfirmationDialog( On 2013/05/15 00:11:03, Peter Kasting wrote: > Nit: It seems like we could avoid the need for this friend declaration by either > making the constructor public or making the creation function be a public static > method of this class instead of a global within the chrome namespace. Either > way seems better; I'd rather avoid friend declarations for things other than > unit tests unless there's no other good way. (I don't know offhand whether > perhaps the "public static method" route doesn't make sense, based on what > further cleanup you'd like to do after this patch. I will note that it's still > OK to make the function a public static method on this class but actually define > it for TOOLKIT_VIEWS off in the *_views.cc file; we do that with lots of other > UI objects.) Here's the idea: there is one top-level function (the friended one) that clients can call. That function is declared in browser_dialogs.h. Its definition is in the separate toolkit implementation files. I dislike the friending too. Based on your suggestion, how about I do the following: - create public static methods on both this webui class and on the views class - keep the declaration of the global Show() function in browser_dialogs.h - implement the global Show() function in both the webui impl and the views impl, which just calls the respective static methods.
https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:111: views::View* prompt_container = MakeFixedWidth(prompt_label, kDialogWidth); On 2013/05/17 13:01:55, dconnelly wrote: > On 2013/05/15 00:11:03, Peter Kasting wrote: > > Why are these MakeFixedWidth() calls necessary? > > Was trying to reduce the amount of code to write. GridLayout::CreatePanel will > set up a container with the right margins and spacing for the "new dialog > style", and if that container has a fixed width specified then the StyledLabel > will automatically resize properly. MakeFixedWidth does both of these. > > I think to get rid of this I would need to use GridLayout for this entire view, > and manually set the margins for the relevant child labels. I think that's not > necessarily more clear, and is more code, but if you think it's better then I > can do it. I think using a GridLayout for everything is slightly better in the sense that nested layout managers are somewhat difficult to understand. I also think that the vertical padding in the screenshot looks too large -- e.g. around the "account is managed by X" label. I'm surprised this padding results from using the "panel" constants, because supposedly kPanelVertMargin = 13, and that looks like WAY more than 13 px from text to border. Are we somehow getting more padding in here than that? (The horizontal padding looks way bigger than 13 px on each side, too.) I ask this because with one layout for everything it's easier to use different layout constants as padding to customize the within-dialog spacing of everything. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:114: 1, 0, 1, 0, SkColorSetRGB(0xE1, 0xE1, 0xE1))); On 2013/05/17 13:01:55, dconnelly wrote: > On 2013/05/15 00:11:03, Peter Kasting wrote: > > You can't hardcode colors. Please use appropriate colors from the theme > service > > or other globally defined/flexible constants (even if this means you can't use > > the precise color in whatever mocks you've been given). > > > > As a last resort, add new colors to the theming system that are suitably > general > > for others to use as well. > > > > (2 places) > > "git grep SkColorSetRGB -- chrome/browser/ui" shows quite a few files that > define color constants for specific views. Is that not an acceptable approach? Not in general. Some specific cases might be OK, e.g. if we have an image resource and there is code that declares what the color of that resource is (but even then it would be better to extract the color from the resource than to hardcode). https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:255: browser_, GURL(url), content::PAGE_TRANSITION_AUTO_TOPLEVEL); On 2013/05/17 13:01:55, dconnelly wrote: > On 2013/05/15 00:11:03, Peter Kasting wrote: > > Why AUTO_TOPLEVEL? Why is this not just a LINK? > > From the descriptions of the transition types: > > // This is a toplevel navigation. This is any content that is automatically > // loaded in a toplevel frame. For example, opening a tab to show the ASH > // screen saver, opening the devtools window, opening the NTP after the safe > // browsing warning, opening web-based dialog boxes are examples of > // AUTO_TOPLEVEL navigations. But this content isn't automatically loaded, is the point. This is explicitly loaded based on a user's click of a link. AUTO_TOPLEVEL is for when we just open something without a user having taken any action, or in response to doing something very unlike clicking a link (e.g. pressing "OK" in a dialog box). https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:28: void ShowProfileSigninConfirmationDialog( On 2013/05/17 13:01:55, dconnelly wrote: > On 2013/05/15 00:11:03, Peter Kasting wrote: > > Why do we have this declaration both here and in browser_dialogs.h? This > > function should only be declared in one place. > > It's declared in browser_dialogs.h. It's re-declared here because tfarina asked > me to remove the browser_dialogs.h include The #include should not have been removed, but this is probably moot pending the below comment. https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:42: friend void ::chrome::ShowProfileSigninConfirmationDialog( On 2013/05/17 13:01:55, dconnelly wrote: > I dislike the friending too. Based on your suggestion, how about I do the > following: > > - create public static methods on both this webui class and on the views class > - keep the declaration of the global Show() function in browser_dialogs.h > - implement the global Show() function in both the webui impl and the views > impl, which just calls the respective static methods. I'm OK with this.
On 2013/05/17 18:45:53, Peter Kasting wrote: > https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... > File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc > (right): > > https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... > chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:111: > views::View* prompt_container = MakeFixedWidth(prompt_label, kDialogWidth); > On 2013/05/17 13:01:55, dconnelly wrote: > > On 2013/05/15 00:11:03, Peter Kasting wrote: > > > Why are these MakeFixedWidth() calls necessary? > > > > Was trying to reduce the amount of code to write. GridLayout::CreatePanel > will > > set up a container with the right margins and spacing for the "new dialog > > style", and if that container has a fixed width specified then the StyledLabel > > will automatically resize properly. MakeFixedWidth does both of these. > > > > I think to get rid of this I would need to use GridLayout for this entire > view, > > and manually set the margins for the relevant child labels. I think that's > not > > necessarily more clear, and is more code, but if you think it's better then I > > can do it. > > I think using a GridLayout for everything is slightly better in the sense that > nested layout managers are somewhat difficult to understand. > > I also think that the vertical padding in the screenshot looks too large -- e.g. > around the "account is managed by X" label. I'm surprised this padding results > from using the "panel" constants, because supposedly kPanelVertMargin = 13, and > that looks like WAY more than 13 px from text to border. Are we somehow getting > more padding in here than that? (The horizontal padding looks way bigger than > 13 px on each side, too.) > > I ask this because with one layout for everything it's easier to use different > layout constants as padding to customize the within-dialog spacing of > everything. I just measured the padding in MSPaint. The vertical spacing from border to text is indeed only 13 pixels. And the horizontal spacing is 20 pixels, which is also correct, since the new style dialogs use kButtonHEdgeMarginNew. > > https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... > chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:114: 1, > 0, 1, 0, SkColorSetRGB(0xE1, 0xE1, 0xE1))); > On 2013/05/17 13:01:55, dconnelly wrote: > > On 2013/05/15 00:11:03, Peter Kasting wrote: > > > You can't hardcode colors. Please use appropriate colors from the theme > > service > > > or other globally defined/flexible constants (even if this means you can't > use > > > the precise color in whatever mocks you've been given). > > > > > > As a last resort, add new colors to the theming system that are suitably > > general > > > for others to use as well. > > > > > > (2 places) > > > > "git grep SkColorSetRGB -- chrome/browser/ui" shows quite a few files that > > define color constants for specific views. Is that not an acceptable > approach? > > Not in general. Some specific cases might be OK, e.g. if we have an image > resource and there is code that declares what the color of that resource is (but > even then it would be better to extract the color from the resource than to > hardcode). > > https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... > chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:255: > browser_, GURL(url), content::PAGE_TRANSITION_AUTO_TOPLEVEL); > On 2013/05/17 13:01:55, dconnelly wrote: > > On 2013/05/15 00:11:03, Peter Kasting wrote: > > > Why AUTO_TOPLEVEL? Why is this not just a LINK? > > > > From the descriptions of the transition types: > > > > // This is a toplevel navigation. This is any content that is automatically > > // loaded in a toplevel frame. For example, opening a tab to show the ASH > > // screen saver, opening the devtools window, opening the NTP after the safe > > // browsing warning, opening web-based dialog boxes are examples of > > // AUTO_TOPLEVEL navigations. > > But this content isn't automatically loaded, is the point. This is explicitly > loaded based on a user's click of a link. > > AUTO_TOPLEVEL is for when we just open something without a user having taken any > action, or in response to doing something very unlike clicking a link (e.g. > pressing "OK" in a dialog box). > > https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... > File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h > (right): > > https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... > chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:28: void > ShowProfileSigninConfirmationDialog( > On 2013/05/17 13:01:55, dconnelly wrote: > > On 2013/05/15 00:11:03, Peter Kasting wrote: > > > Why do we have this declaration both here and in browser_dialogs.h? This > > > function should only be declared in one place. > > > > It's declared in browser_dialogs.h. It's re-declared here because tfarina > asked > > me to remove the browser_dialogs.h include > > The #include should not have been removed, but this is probably moot pending the > below comment. > > https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/webui/s... > chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:42: friend > void ::chrome::ShowProfileSigninConfirmationDialog( > On 2013/05/17 13:01:55, dconnelly wrote: > > I dislike the friending too. Based on your suggestion, how about I do the > > following: > > > > - create public static methods on both this webui class and on the views class > > - keep the declaration of the global Show() function in browser_dialogs.h > > - implement the global Show() function in both the webui impl and the views > > impl, which just calls the respective static methods. > > I'm OK with this.
On 2013/05/22 12:56:21, dconnelly wrote: > I just measured the padding in MSPaint. The vertical spacing from border to > text is indeed only 13 pixels. And the horizontal spacing is 20 pixels, which > is also correct, since the new style dialogs use kButtonHEdgeMarginNew. I see, we have to measure from below the descender or above the internal leading, not just from the baseline or cap height. It looks as if the distance from the "dialog title" at the top to the first separator is 8 px. That matches the kRelatedControlVerticalSpacing constant which is also 8 px. What if we use 8 px. for all of these instead of 13? I attached a shot of what this would look like to the bug. To me it looks significantly better. I pinged Glen about this but haven't heard anything from him yet.
https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc (right): https://codereview.chromium.org/14846020/diff/25001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:111: views::View* prompt_container = MakeFixedWidth(prompt_label, kDialogWidth); On 2013/05/17 18:45:53, Peter Kasting wrote: > On 2013/05/17 13:01:55, dconnelly wrote: > > On 2013/05/15 00:11:03, Peter Kasting wrote: > > > Why are these MakeFixedWidth() calls necessary? > > > > Was trying to reduce the amount of code to write. GridLayout::CreatePanel > will > > set up a container with the right margins and spacing for the "new dialog > > style", and if that container has a fixed width specified then the StyledLabel > > will automatically resize properly. MakeFixedWidth does both of these. > > > > I think to get rid of this I would need to use GridLayout for this entire > view, > > and manually set the margins for the relevant child labels. I think that's > not > > necessarily more clear, and is more code, but if you think it's better then I > > can do it. > > I think using a GridLayout for everything is slightly better in the sense that > nested layout managers are somewhat difficult to understand. > > I also think that the vertical padding in the screenshot looks too large -- e.g. > around the "account is managed by X" label. I'm surprised this padding results > from using the "panel" constants, because supposedly kPanelVertMargin = 13, and > that looks like WAY more than 13 px from text to border. Are we somehow getting > more padding in here than that? (The horizontal padding looks way bigger than > 13 px on each side, too.) > > I ask this because with one layout for everything it's easier to use different > layout constants as padding to customize the within-dialog spacing of > everything. After some investigation I don't think I can avoid using nested layout managers. The gray bar needs insets of zero with respect to the dialog view, the label it contains needs positive insets, and the explanation text beneath the gray bar needs positive insets. GridLayout only supports insets for the entire container that it lays out. So I can replace the BoxLayout of the dialog view with a GridLayout, but not much will change. What do you think? (Still waiting for feedback from UI on the paddings)
On 2013/05/27 12:49:56, dconnelly wrote: > After some investigation I don't think I can avoid using nested layout managers. > The gray bar needs insets of zero with respect to the dialog view, the label it > contains needs positive insets, and the explanation text beneath the gray bar > needs positive insets. GridLayout only supports insets for the entire container > that it lays out. So I can replace the BoxLayout of the dialog view with a > GridLayout, but not much will change. What do you think? What about having insets of 0 everywhere and then using padding columns inside them to inset the contents of things? If that won't work, then nested layout managers it is.
LGTM, save for a couple issues (the color calculation and the language code ones). I don't need to re-review once those are fixed. https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc (right): https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:38: const SkColor kDialogAlertBarBorder = SkColorSetRGB(0xE1, 0xE1, 0xE1); OK, I have an idea for how you could do these without hardcoding colors and without trying to add more colors to the theme. Use color_utils::GetLuminanceForColor() with the dialog background color and check whether it's <128. Then color_utils::AlphaBlend() white or black atop that color (depending) with the appropriate percentages to get roughly the results you want here -- should be about 4% and 12% for the background and border, respectively, I think. This ought to work for any background color. Of course, if there are appropriate theme colors we have or should add, that's another option too. The UI designers ought to be aware of how we're aiming to calculate these colors so they can try and keep the UI everywhere in sync and not have one-off dialogs. https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:177: if (details.is_add && details.child == this) { Nit: {} unnecessary https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:197: "hl=en&answer=1331549"), "hl=en" isn't right. Per conops, don't send a language code at all ( https://code.google.com/p/chromium/issues/detail?id=164939 ), though even if we did want to, we'd want to use AppendGoogleLocaleParam() to do it automatically. https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:224: prompt_label->AddStyleRange( I'd direct you to set the background color of the label here, but StyledLabel doesn't allow that right now. I filed http://crbug.com/244630 for this, in case that gets fixed before this lands (or you want to watch and fix up later). https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h (right): https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:50: // views::DialogDelegate: Tiny nit: You don't actually inherit directly from DialogDelegate or View, so I'd combine these two lists as "// DialogDelegateView:", listed in the order that class lists its own parennt views (which is the order you already have things in, so no change there). https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:72: void Init(); Nit: Personally, I don't think this adds much to just doing it inline in ViewHierarchyChanged9), since that function is so simple -- it just makes it look like this could be called by more than one place. But up to you. https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:26: class ProfileSigninConfirmationHandler : public content::WebUIMessageHandler { Nit: Since you have multiple classes in this file, consider putting "// ClassName -----------------..." dividers above each; see e.g. chrome/browser/ui/gtk/infobars/alternate_nav_infobar_gtk.cc for how to space these. https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:75: ProfileSigninConfirmationHandler::ProfileSigninConfirmationHandler( Nit: Put these function definitions next to the class declaration (i.e. inside the anonymous namespace). https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:90: web_ui()->RegisterMessageCallback("cancel", Nit: Per c-style, all lines of arguments should start at the same horizontal place (so you'll want to move "cancel" to a new line indented 4).
https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc (right): https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:38: const SkColor kDialogAlertBarBorder = SkColorSetRGB(0xE1, 0xE1, 0xE1); On 2013/05/28 23:25:11, Peter Kasting wrote: > OK, I have an idea for how you could do these without hardcoding colors and > without trying to add more colors to the theme. > > Use color_utils::GetLuminanceForColor() with the dialog background color and > check whether it's <128. Then color_utils::AlphaBlend() white or black atop > that color (depending) with the appropriate percentages to get roughly the > results you want here -- should be about 4% and 12% for the background and > border, respectively, I think. > > This ought to work for any background color. > > Of course, if there are appropriate theme colors we have or should add, that's > another option too. The UI designers ought to be aware of how we're aiming to > calculate these colors so they can try and keep the UI everywhere in sync and > not have one-off dialogs. Cool. Done. FWIW I grepped for these colors and couldn't find that they already exist. https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:177: if (details.is_add && details.child == this) { On 2013/05/28 23:25:11, Peter Kasting wrote: > Nit: {} unnecessary Done. https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:197: "hl=en&answer=1331549"), On 2013/05/28 23:25:11, Peter Kasting wrote: > "hl=en" isn't right. Per conops, don't send a language code at all ( > https://code.google.com/p/chromium/issues/detail?id=164939 ), though even if we > did want to, we'd want to use AppendGoogleLocaleParam() to do it automatically. Done. https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc:224: prompt_label->AddStyleRange( On 2013/05/28 23:25:11, Peter Kasting wrote: > I'd direct you to set the background color of the label here, but StyledLabel > doesn't allow that right now. I filed http://crbug.com/244630 for this, in case > that gets fixed before this lands (or you want to watch and fix up later). Added TODO https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... File chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h (right): https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:50: // views::DialogDelegate: On 2013/05/28 23:25:11, Peter Kasting wrote: > Tiny nit: You don't actually inherit directly from DialogDelegate or View, so > I'd combine these two lists as "// DialogDelegateView:", listed in the order > that class lists its own parennt views (which is the order you already have > things in, so no change there). Done. https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/views/s... chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h:72: void Init(); On 2013/05/28 23:25:11, Peter Kasting wrote: > Nit: Personally, I don't think this adds much to just doing it inline in > ViewHierarchyChanged9), since that function is so simple -- it just makes it > look like this could be called by more than one place. But up to you. Done. https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:26: class ProfileSigninConfirmationHandler : public content::WebUIMessageHandler { On 2013/05/28 23:25:11, Peter Kasting wrote: > Nit: Since you have multiple classes in this file, consider putting "// > ClassName -----------------..." dividers above each; see e.g. > chrome/browser/ui/gtk/infobars/alternate_nav_infobar_gtk.cc for how to space > these. Done. https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:75: ProfileSigninConfirmationHandler::ProfileSigninConfirmationHandler( On 2013/05/28 23:25:11, Peter Kasting wrote: > Nit: Put these function definitions next to the class declaration (i.e. inside > the anonymous namespace). Done. https://codereview.chromium.org/14846020/diff/68001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:90: web_ui()->RegisterMessageCallback("cancel", On 2013/05/28 23:25:11, Peter Kasting wrote: > Nit: Per c-style, all lines of arguments should start at the same horizontal > place (so you'll want to move "cancel" to a new line indented 4). Done.
On 2013/05/28 18:19:26, Peter Kasting wrote: > On 2013/05/27 12:49:56, dconnelly wrote: > > After some investigation I don't think I can avoid using nested layout > managers. > > The gray bar needs insets of zero with respect to the dialog view, the label > it > > contains needs positive insets, and the explanation text beneath the gray bar > > needs positive insets. GridLayout only supports insets for the entire > container > > that it lays out. So I can replace the BoxLayout of the dialog view with a > > GridLayout, but not much will change. What do you think? > > What about having insets of 0 everywhere and then using padding columns inside > them to inset the contents of things? > > If that won't work, then nested layout managers it is. Forgot to respond to this: since the prompt label is contained inside another view (the gray bar), it still needs a nested layout manager.
jhawkins: PTAL
LGTM with nits. https://codereview.chromium.org/14846020/diff/93001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc (right): https://codereview.chromium.org/14846020/diff/93001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.cc:24: // ProfileSigninConfirmationHandler -------------------------------------------- Optional nit: FWIW I would remove this; it's superfluous and noisy. https://codereview.chromium.org/14846020/diff/93001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h (right): https://codereview.chromium.org/14846020/diff/93001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:71: // The containing view. Also weak pointer? https://codereview.chromium.org/14846020/diff/93001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:85: // Weak ptr to delegate. Be consistent: you use 'pointer' above.
https://codereview.chromium.org/14846020/diff/93001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h (right): https://codereview.chromium.org/14846020/diff/93001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:71: // The containing view. On 2013/05/31 16:51:10, James Hawkins wrote: > Also weak pointer? Done. https://codereview.chromium.org/14846020/diff/93001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h:85: // Weak ptr to delegate. On 2013/05/31 16:51:10, James Hawkins wrote: > Be consistent: you use 'pointer' above. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/14846020/66004
Message was sent while issue was closed.
Change committed as 203709 |
