Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
93633007:
Created optional multiprofiles introduction dialog. (Closed)
|
Created:
7 years ago by merkulova Modified:
6 years, 11 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCreated optional multiprofiles introduction dialog.
BUG=230862
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245502
Patch Set 1 #
Total comments: 26
Patch Set 2 : Multiprofiles dialog view and interface polished. Nits fixed. #
Total comments: 15
Patch Set 3 : Nits #
Total comments: 18
Patch Set 4 : Nikita nits #
Total comments: 6
Patch Set 5 : Dialog is shown only if ALL profiles in multi-session haven't dismissed it. #
Total comments: 2
Patch Set 6 : Line move #Patch Set 7 : Obsolete SetFont changed to SetFontList #Patch Set 8 : Non POD variables are not allowed to be set as static. #
Messages
Total messages: 20 (0 generated)
Used UserProfile as it was used in the changed files. It's still discussable issue.
You need more owner's reviews by the way. https://codereview.chromium.org/93633007/diff/1/chrome/app/chromeos_strings.grdp File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/93633007/diff/1/chrome/app/chromeos_strings.g... chrome/app/chromeos_strings.grdp:3840: <message name="IDS_MULTIPROFILES_INTRO_HEADLINE" desc="Describes what the intro dialog explains."> Please add more information to description. Translators don't see |name| of string, so it's not clear which dialog are you talking about. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/multi_profile_first_run_notification.cc (right): https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/multi_profile_first_run_notification.cc:70: prefs::kMultiProfileIntroShowDismissed, I think kMultiProfileNeverShowIntroNotice is better name for pref. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... File chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc (right): https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc:86: views::DialogDelegate::CreateDialogWidget(dialog_view, NULL, owning_window); I think your choice of owning_window is not right. It's probably should be one of RootWindows, ideally RootWindow where user clicked on systray, by primary RootWindow will be OK too. Also, as I can understand, it should be passed as |context| parameter of CreateDialogWidget, not |parent|. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc:96: no_show_checkbox_->checked()); nit: fix indent https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc:110: return gfx::Size(kDefaultWidth, kDefaultHeight); I think default implementation of GetPreferredSize asks layout_manager about size. Isn't it better? Why do we need fixed size here? https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc:124: label->SetFont(ui::ResourceBundle::GetSharedInstance().GetFont( Is it necessary? Isn't BaseFont set by default? Moreover SetFont marked as OBSOLETE. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc:152: grid_layout->Layout(this); Is this call needed? https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc:165: return true; Why do we need return value if it is always true and never checked? https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... File chrome/browser/chromeos/profiles/multiprofiles_intro_view.h (right): https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. File should be renamed, it's name now doesn't correlate with it's interface. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.h:9: namespace multiprofiles { I don't think that we need new namespace for one function. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.h:12: bool IntroDialog(); Function name should contain verb. https://codereview.chromium.org/93633007/diff/1/chrome/chrome_browser_chromeo... File chrome/chrome_browser_chromeos.gypi (right): https://codereview.chromium.org/93633007/diff/1/chrome/chrome_browser_chromeo... chrome/chrome_browser_chromeos.gypi:782: 'browser/chromeos/profiles/multiprofiles_intro_view.h', I think these files should be moved to chrome/browser/chromeos/login or chrome/browser/chromeos/ui as well as avatar_menu_* files. https://codereview.chromium.org/93633007/diff/1/chrome/chrome_browser_chromeo... chrome/chrome_browser_chromeos.gypi:826: 'browser/chromeos/profiles/multiprofiles_intro_view.h', You've added these files twice.
https://codereview.chromium.org/93633007/diff/1/chrome/app/chromeos_strings.grdp File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/93633007/diff/1/chrome/app/chromeos_strings.g... chrome/app/chromeos_strings.grdp:3840: <message name="IDS_MULTIPROFILES_INTRO_HEADLINE" desc="Describes what the intro dialog explains."> On 2013/12/20 03:39:01, dzhioev wrote: > Please add more information to description. Translators don't see |name| of > string, so it's not clear which dialog are you talking about. Done. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/multi_profile_first_run_notification.cc (right): https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/multi_profile_first_run_notification.cc:70: prefs::kMultiProfileIntroShowDismissed, On 2013/12/20 03:39:01, dzhioev wrote: > I think kMultiProfileNeverShowIntroNotice is better name for pref. As it's bool, kMultiProfileNeverShowIntro might be enough. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... File chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc (right): https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc:86: views::DialogDelegate::CreateDialogWidget(dialog_view, NULL, owning_window); On 2013/12/20 03:39:01, dzhioev wrote: > I think your choice of owning_window is not right. It's probably should be one > of RootWindows, ideally RootWindow where user clicked on systray, by primary > RootWindow will be OK too. Also, as I can understand, it should be passed as > |context| parameter of CreateDialogWidget, not |parent|. Done. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc:96: no_show_checkbox_->checked()); On 2013/12/20 03:39:01, dzhioev wrote: > nit: fix indent Done. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc:110: return gfx::Size(kDefaultWidth, kDefaultHeight); On 2013/12/20 03:39:01, dzhioev wrote: > I think default implementation of GetPreferredSize asks layout_manager about > size. Isn't it better? Why do we need fixed size here? As discussed offline, it's the easiest way to set sizes here. HeightFromWidth methods don't support checkbox view. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc:124: label->SetFont(ui::ResourceBundle::GetSharedInstance().GetFont( On 2013/12/20 03:39:01, dzhioev wrote: > Is it necessary? Isn't BaseFont set by default? > Moreover SetFont marked as OBSOLETE. Done. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc:152: grid_layout->Layout(this); On 2013/12/20 03:39:01, dzhioev wrote: > Is this call needed? Yes, it's necessary. Done. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.cc:165: return true; On 2013/12/20 03:39:01, dzhioev wrote: > Why do we need return value if it is always true and never checked? Done. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... File chrome/browser/chromeos/profiles/multiprofiles_intro_view.h (right): https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/12/20 03:39:01, dzhioev wrote: > File should be renamed, it's name now doesn't correlate with it's interface. Done. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.h:9: namespace multiprofiles { On 2013/12/20 03:39:01, dzhioev wrote: > I don't think that we need new namespace for one function. Done. https://codereview.chromium.org/93633007/diff/1/chrome/browser/chromeos/profi... chrome/browser/chromeos/profiles/multiprofiles_intro_view.h:12: bool IntroDialog(); On 2013/12/20 03:39:01, dzhioev wrote: > Function name should contain verb. Done. https://codereview.chromium.org/93633007/diff/1/chrome/chrome_browser_chromeo... File chrome/chrome_browser_chromeos.gypi (right): https://codereview.chromium.org/93633007/diff/1/chrome/chrome_browser_chromeo... chrome/chrome_browser_chromeos.gypi:782: 'browser/chromeos/profiles/multiprofiles_intro_view.h', On 2013/12/20 03:39:01, dzhioev wrote: > I think these files should be moved to chrome/browser/chromeos/login or > chrome/browser/chromeos/ui as well as avatar_menu_* files. Made preps for easy moving: removed chrome/browser includes from multiprofiles_intro... file Place for relocation is being discussed. https://codereview.chromium.org/93633007/diff/1/chrome/chrome_browser_chromeo... chrome/chrome_browser_chromeos.gypi:826: 'browser/chromeos/profiles/multiprofiles_intro_view.h', On 2013/12/20 03:39:01, dzhioev wrote: > You've added these files twice. Done.
https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... File chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc (right): https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:27: const gfx::Insets kInsets = gfx::Insets(40, 40, 40, 40); non POD global object is not allowed. You may move inside the function with static const though https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:33: MultiprofilesIntroView(base::Callback<void(bool)> on_accept); explicit https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:50: base::Callback<void(bool)> on_accept_; can this be const? https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:51: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:101: title_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT); Any RTL/i18n issue here? https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... File chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.h (right): https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. 2014, no (c) https://codereview.chromium.org/93633007/diff/50001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/93633007/diff/50001/chrome/common/pref_names.... chrome/common/pref_names.cc:859: // itroduction dialog show. The comment looks incorrect.
https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... File chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc (right): https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:27: const gfx::Insets kInsets = gfx::Insets(40, 40, 40, 40); On 2014/01/10 19:14:07, oshima wrote: > non POD global object is not allowed. You may move inside the function with > static const though Done. https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:33: MultiprofilesIntroView(base::Callback<void(bool)> on_accept); On 2014/01/10 19:14:07, oshima wrote: > explicit Done. https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:50: base::Callback<void(bool)> on_accept_; On 2014/01/10 19:14:07, oshima wrote: > can this be const? Done. https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:51: }; On 2014/01/10 19:14:07, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:101: title_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT); On 2014/01/10 19:14:07, oshima wrote: > Any RTL/i18n issue here? It will automatically mirror the dialog for RTL locale. So, no additional alignment changes required. https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... File chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.h (right): https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2014/01/10 19:14:07, oshima wrote: > 2014, no (c) Done. https://codereview.chromium.org/93633007/diff/50001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/93633007/diff/50001/chrome/common/pref_names.... chrome/common/pref_names.cc:859: // itroduction dialog show. On 2014/01/10 19:14:07, oshima wrote: > The comment looks incorrect. Could you clarify your doubts?
https://codereview.chromium.org/93633007/diff/160001/chrome/app/chromeos_stri... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/93633007/diff/160001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:3839: <!-- Multiprofiles mode --> multi-profiles https://codereview.chromium.org/93633007/diff/160001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:3840: <message name="IDS_MULTIPROFILES_INTRO_HEADLINE" desc="Describes which feature multiprofiles intro dialog presents."> nit: multi-profiles https://codereview.chromium.org/93633007/diff/160001/chrome/browser/chromeos/... File chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc (right): https://codereview.chromium.org/93633007/diff/160001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:39: virtual bool Accept() OVERRIDE; nit: // views::DialogDelegate overrides. https://codereview.chromium.org/93633007/diff/160001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:41: // views::WidgetDelegate overrides nit: Add dot at each comment sentence (here and below). https://codereview.chromium.org/93633007/diff/160001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:97: void MultiprofilesIntroView::InitDialog() { Please make layout RTL friendly. https://codereview.chromium.org/93633007/diff/160001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:115: // Explanation string nit: Dot at the and here and below. https://codereview.chromium.org/93633007/diff/160001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:150: Layout(); nit: I don't think that you need to call Layout() explicitly here. Please check that it still works fine without this call. https://codereview.chromium.org/93633007/diff/160001/chrome/browser/ui/ash/sy... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/93633007/diff/160001/chrome/browser/ui/ash/sy... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:242: void onAcceptMultiprofilesIntro(bool no_show_again) { nit: Capitalize. https://codereview.chromium.org/93633007/diff/160001/chrome/browser/ui/ash/sy... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:642: base::Callback<void(bool)> on_accept = base::Bind( nit: Move base::Bind( to next line so that whole call fits on one line.
https://codereview.chromium.org/93633007/diff/160001/chrome/app/chromeos_stri... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/93633007/diff/160001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:3839: <!-- Multiprofiles mode --> On 2014/01/14 13:44:11, Nikita Kostylev wrote: > multi-profiles Done. https://codereview.chromium.org/93633007/diff/160001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:3840: <message name="IDS_MULTIPROFILES_INTRO_HEADLINE" desc="Describes which feature multiprofiles intro dialog presents."> On 2014/01/14 13:44:11, Nikita Kostylev wrote: > nit: multi-profiles Done. https://codereview.chromium.org/93633007/diff/160001/chrome/browser/chromeos/... File chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc (right): https://codereview.chromium.org/93633007/diff/160001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:39: virtual bool Accept() OVERRIDE; On 2014/01/14 13:44:11, Nikita Kostylev wrote: > nit: // views::DialogDelegate overrides. Done. https://codereview.chromium.org/93633007/diff/160001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:41: // views::WidgetDelegate overrides On 2014/01/14 13:44:11, Nikita Kostylev wrote: > nit: Add dot at each comment sentence (here and below). Done. https://codereview.chromium.org/93633007/diff/160001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:97: void MultiprofilesIntroView::InitDialog() { On 2014/01/14 13:44:11, Nikita Kostylev wrote: > Please make layout RTL friendly. It is already friendly. The dialog is automatically mirrored for RTL languages. If I set it manually, I get it wrong. https://codereview.chromium.org/93633007/diff/160001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:115: // Explanation string On 2014/01/14 13:44:11, Nikita Kostylev wrote: > nit: Dot at the and here and below. Done. https://codereview.chromium.org/93633007/diff/160001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:150: Layout(); On 2014/01/14 13:44:11, Nikita Kostylev wrote: > nit: I don't think that you need to call Layout() explicitly here. > Please check that it still works fine without this call. It is necessary actually. Without this call, we get only clear dialog without any labels, checkboxes and so on. https://codereview.chromium.org/93633007/diff/160001/chrome/browser/ui/ash/sy... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/93633007/diff/160001/chrome/browser/ui/ash/sy... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:242: void onAcceptMultiprofilesIntro(bool no_show_again) { On 2014/01/14 13:44:11, Nikita Kostylev wrote: > nit: Capitalize. Done. https://codereview.chromium.org/93633007/diff/160001/chrome/browser/ui/ash/sy... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:642: base::Callback<void(bool)> on_accept = base::Bind( On 2014/01/14 13:44:11, Nikita Kostylev wrote: > nit: Move base::Bind( to next line so that whole call fits on one line. Done.
lgtm
LGTM after moving new files to proper place.
https://codereview.chromium.org/93633007/diff/240001/chrome/browser/chromeos/... File chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc (right): https://codereview.chromium.org/93633007/diff/240001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:59: kInset, kInset); no static initializer please. since this is used only in InitDialog, you can move this to there (a function scope static does not create static initializer) And since this is cont, the name should be kDialogInsets https://codereview.chromium.org/93633007/diff/240001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:65: const base::Callback<void(bool)> on_accept) : on_accept_(on_accept) { nit : move ": on_accept_ .." to next line. https://codereview.chromium.org/93633007/diff/240001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:77: dialog_view, ash::Shell::GetPrimaryRootWindow(), NULL); this should probably be TargetRootWindow?
As discussed in bug thread, logic for dialog show has changed. Nikita, Pasha, could you have one more look on it? Pasha, there will be no file-move. Discussed it with Stefan. https://codereview.chromium.org/93633007/diff/240001/chrome/browser/chromeos/... File chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc (right): https://codereview.chromium.org/93633007/diff/240001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:59: kInset, kInset); On 2014/01/14 19:06:14, oshima wrote: > no static initializer please. since this is used only in InitDialog, you can > move this to there (a function scope static does not create static initializer) > > And since this is cont, the name should be kDialogInsets Done. https://codereview.chromium.org/93633007/diff/240001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:65: const base::Callback<void(bool)> on_accept) : on_accept_(on_accept) { On 2014/01/14 19:06:14, oshima wrote: > nit : move ": on_accept_ .." to next line. Done. https://codereview.chromium.org/93633007/diff/240001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:77: dialog_view, ash::Shell::GetPrimaryRootWindow(), NULL); On 2014/01/14 19:06:14, oshima wrote: > this should probably be TargetRootWindow? Sounds reasonable. Does TargetRootWindow represent current monitor base?
lgtm
lgtm with a nit https://codereview.chromium.org/93633007/diff/360001/chrome/browser/chromeos/... File chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc (right): https://codereview.chromium.org/93633007/diff/360001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:96: static const gfx::Insets kDialogInsets(kInset, kInset, kInset, kInset); nit: move this to the top of the function.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/93633007/450001
Retried try job too often on linux_chromeos_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/93633007/400002
Retried try job too often on linux_chromeos_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... File chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc (right): https://codereview.chromium.org/93633007/diff/50001/chrome/browser/chromeos/p... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:27: const gfx::Insets kInsets = gfx::Insets(40, 40, 40, 40); On 2014/01/10 19:14:07, oshima wrote: > non POD global object is not allowed. You may move inside the function with > static const though Hi, Oshima. On committing I discovered that non POD static objects are not allowed. Compilation logs http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos_clang/... So I just changed it to const. Insets object creation isn't heavy, so I think it would be better than applying CR_DEFINE_STATIC_LOCAL. https://codereview.chromium.org/93633007/diff/360001/chrome/browser/chromeos/... File chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc (right): https://codereview.chromium.org/93633007/diff/360001/chrome/browser/chromeos/... chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc:96: static const gfx::Insets kDialogInsets(kInset, kInset, kInset, kInset); On 2014/01/15 18:44:44, oshima wrote: > nit: move this to the top of the function. > Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/93633007/530004
Message was sent while issue was closed.
Change committed as 245502 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
