Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
40053002:
Implements the dialog view for logout button tray in public sessions. (Closed)
|
Created:
7 years, 2 months ago by binjin Modified:
6 years, 11 months ago CC:
chromium-reviews, kalyank, sadrul, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplements the dialog view for logout confirmation dialog in public sessions.
Implements ash::internal::LogoutConfirmationDialogView which is supposed
to be used by ash::internal::LogoutButtonTray.
BUG=223846
TEST=new unit test
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242286
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243530
Patch Set 1 #
Total comments: 104
Patch Set 2 : Rewrite the implementations of logout confirmation #Patch Set 3 : Add preference for dialog duration #
Total comments: 29
Patch Set 4 : Minor improvements #Patch Set 5 : Immediately reflect duration updates #
Total comments: 30
Patch Set 6 : minor fixes; use scoped_ptr; fix timer #
Total comments: 4
Patch Set 7 : add tests; simplify code #Patch Set 8 : minor improvement on comments #
Total comments: 73
Patch Set 9 : minor fixes #Patch Set 10 : rebase #Patch Set 11 : enhence tests; simplify code; fix win build(hope so) #
Total comments: 71
Patch Set 12 : fixes; use OnClosed() approach #Patch Set 13 : drop weak pointer for owner #
Total comments: 8
Patch Set 14 : minor fixes #
Total comments: 24
Patch Set 15 : more fixes #
Total comments: 44
Patch Set 16 : more improvements #Patch Set 17 : rebase; remove scoped_ptr; remove GetUpdateInterval() #
Total comments: 12
Patch Set 18 : more fixes #Patch Set 19 : minor fix #
Total comments: 28
Patch Set 20 : more fixes; make delegate pure; add ShowDialog() to delegate(); add TODO in tests #
Total comments: 8
Patch Set 21 : fixes; attempt to fix win build #Patch Set 22 : second attempt to fix win build #Patch Set 23 : attempt to fix presubmit trybot warning #Patch Set 24 : first attempt to fix memory leak in tests #Patch Set 25 : rebase #Messages
Total messages: 59 (0 generated)
please take a look, thanks
https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.cc:11: #include "ash/system/tray/system_tray_delegate.h" No longer used. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.cc:115: void LogoutButtonTray::ShowConfirmDialog() { How about calling this EnsureConfirmationDialogShown instead? https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.cc:122: void LogoutButtonTray::CloseConfirmDialog() { How about calling this EnsureConfirmationDialogClosed instead? https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.cc:130: if (confirm_dialog_ == instance) { - No curly braces for single-line conditionals. - No need to pass the |instance| here. There can only ever be one. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.cc:177: if (!show_logout_button_in_tray_) { No curly braces for single-line conditionals. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.h:34: void ShowConfirmDialog(); - Here and elsewhere: s/confirm/confirmation/ - All three methods: Why public, not private? https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.h:61: LogoutConfirmationDialogView* confirm_dialog_; As commented in logout_confirmation_dialog_view.cc, I think this should be a base::WeakPtr. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:10: #include "base/strings/string_number_conversions.h" This is not used anywhere. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:24: Remove duplicated blank line. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:25: const int kLogoutConfirmationDialogMaxWidth = 300; Could you use one of the existing ash/aura/views constants here? Hard-coding pixel values is frowned upon. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:26: const int kCountdownUpdateIntervalMs = 1000; Add a comment (prefixed by two spaces): // 1 second. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:27: const int kAutoLogoutDurationMs = 5 * 1000; - This will need to be adjustable by pref. - Add a comment explaining what this timeout is for. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:29: } Replace with: } // namespace https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:35: Remove duplicated blank line. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:42: return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL; This is the default. No need to override the method. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:46: return ui::MODAL_TYPE_WINDOW; - It would seem that ui::MODAL_TYPE_SYSTEM is the correct choice here. - #include "ui/base/ui_base_types.h" https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:55: if (button == ui::DIALOG_BUTTON_OK) { No curly braces for single-line conditionals. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:58: if (button == ui::DIALOG_BUTTON_CANCEL) { No curly braces for single-line conditionals. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:59: return l10n_util::GetStringUTF16(IDS_APP_CANCEL); You can defer to the inherited method instead here. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:61: return base::string16(); In this implementation, there should be a NOTREACHED(). But you can defer to the inherited method instead here. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:65: LogoutButtonTray *owner) : display_label_(NULL), owner_(owner) { Break at the : and the , https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:69: if (owner_) { No curly braces for single-line conditionals. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:70: owner_->RemoveConfirmDialogInstance(this); - The "Instance" in this method name is unnecessary. - Instead of keeping track of the owner and manually unregistering from it, why not have the owner keep a WeakPtr to this? https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:74: void LogoutConfirmationDialogView::InitAndShow() { Why is this initialization not done in the constructor? https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:75: ui::ResourceBundle &rb = ui::ResourceBundle::GetSharedInstance(); Abbreviations are strongly frowned upon. Please expand the name "rb". https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:77: display_label_ = new views::Label(); No () needed for new. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:78: display_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT); #include "ui/gfx/text_constants.h" https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:80: display_label_->SetFont(rb.GetFont(ui::ResourceBundle::BaseFont)); - In this implementation, #include "ui/gfx/font.h". - Is this not the default font anyway, making this unnecessary? https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:82: views::GridLayout *layout = views::GridLayout::CreatePanel(this); - Put the * on the data type, not the variable name. - Is a GridLayout really the best choice here? You are using one column only. Could you not use a simpler layout class? https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:85: views::ColumnSet *column_set = layout->AddColumnSet(0); Put the * on the data type, not the variable name. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:90: layout->AddPaddingRow(0, views::kRelatedControlHorizontalSpacing); You are not adding spacing between related controls. Is views::kRelatedControlHorizontalSpacing really the correct constant to use here? https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:93: layout->AddPaddingRow(0, views::kRelatedControlHorizontalSpacing); You are not adding spacing between related controls. Is views::kRelatedControlHorizontalSpacing really the correct constant to use here? https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:100: base::TimeDelta::FromMilliseconds(kAutoLogoutDurationMs); Indent 4 spaces relative to the line above, not 2. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:106: GetWidget()->SetAlwaysOnTop(true); Is this really necessary when the dialog is marked as modal? https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:109: timer_.Start(FROM_HERE, #include "base/location.h" https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:116: // 500ms is a workaround to handle inaccurate time measuring. The time What are you trying to work around here? https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:118: // kCountdownUpdateIntervalMs Add full stop at the end of the sentence. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:119: base::TimeDelta logout_warning_time = countdown_end_time_ - Mark as const. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:124: IDS_ASH_LOGOUT_CONFIRMATION_WARNING, Indent 4 spaces, not 6. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:128: IDS_ASH_LOGOUT_CONFIRMATION_WARNING_NOW)); Indent 4 spaces, not 6. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:135: } // namespace ash Add blank line at end of file. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:12: class Label; Do not indent class declarations. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:21: // dialog view class for logout confirmation This should be a complete sentence, starting with a capital letter and ending with a full stop. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:24: Remove blank line. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:26: virtual bool Accept(); Add OVERRIDE. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:27: virtual int GetDialogButtons() const OVERRIDE; #include "base/compiler_specific.h" https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:33: Remove blank line. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:34: friend class LogoutButtonTray; Make the methods public and remove the friend declaration. We only mark methods internal to a class as private. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:36: LogoutConfirmationDialogView(LogoutButtonTray *owner); - Put the * on the data type, not the argument name. - Mark the constructor as explicit. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:45: views::Label *display_label_; Put the * on the data type, not the member name. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:47: base::Time countdown_end_time_; - Use base::TimeTicks instead: base::Time changes when the timezone or the clock is adjusted. base::TimeTicks is immune to such changes. - #include "base/time.h" https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:51: LogoutButtonTray *owner_; Put the * on the data type, not the member name. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:53: DISALLOW_COPY_AND_ASSIGN(LogoutConfirmationDialogView); #include "base/basictypes.h" https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:60: #endif // ASH_SYSTEM_LOGOUT_BUTTON_LOGOUT_CONFIRMATION_DIALOG_VIEW_H_ Add blank line at end of file.
I rewrote the code to follow the coding style. Also I used weak pointer and FillLayout as you advised. But the current UI looks quite different from the mocks here: https://folio.googleplex.com/chrome-ux/mocks/139-cros-public-sessions/#/4_end... It's mainly regarding the width and height padding(or preferred size) setting. It's quite easy to fix with some hard-coded constants but I'd like to see if there are better ways or not. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.cc:11: #include "ash/system/tray/system_tray_delegate.h" On 2013/10/24 13:11:02, bartfab wrote: > No longer used. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.cc:115: void LogoutButtonTray::ShowConfirmDialog() { On 2013/10/24 13:11:02, bartfab wrote: > How about calling this EnsureConfirmationDialogShown instead? Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.cc:122: void LogoutButtonTray::CloseConfirmDialog() { On 2013/10/24 13:11:02, bartfab wrote: > How about calling this EnsureConfirmationDialogClosed instead? Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.cc:130: if (confirm_dialog_ == instance) { On 2013/10/24 13:11:02, bartfab wrote: > - No curly braces for single-line conditionals. > - No need to pass the |instance| here. There can only ever be one. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.cc:177: if (!show_logout_button_in_tray_) { On 2013/10/24 13:11:02, bartfab wrote: > No curly braces for single-line conditionals. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.h:34: void ShowConfirmDialog(); On 2013/10/24 13:11:02, bartfab wrote: > - Here and elsewhere: s/confirm/confirmation/ > - All three methods: Why public, not private? They are supposed to be accessed by LogoutConfirmationDialogView, and they are already in ash::internal namespace. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_button_tray.h:61: LogoutConfirmationDialogView* confirm_dialog_; On 2013/10/24 13:11:02, bartfab wrote: > As commented in logout_confirmation_dialog_view.cc, I think this should be a > base::WeakPtr. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:10: #include "base/strings/string_number_conversions.h" On 2013/10/24 13:11:02, bartfab wrote: > This is not used anywhere. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:24: On 2013/10/24 13:11:02, bartfab wrote: > Remove duplicated blank line. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:26: const int kCountdownUpdateIntervalMs = 1000; On 2013/10/24 13:11:02, bartfab wrote: > Add a comment (prefixed by two spaces): > > // 1 second. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:27: const int kAutoLogoutDurationMs = 5 * 1000; On 2013/10/24 13:11:02, bartfab wrote: > - This will need to be adjustable by pref. > - Add a comment explaining what this timeout is for. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:29: } On 2013/10/24 13:11:02, bartfab wrote: > Replace with: > > } // namespace Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:35: On 2013/10/24 13:11:02, bartfab wrote: > Remove duplicated blank line. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:42: return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL; On 2013/10/24 13:11:02, bartfab wrote: > This is the default. No need to override the method. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:46: return ui::MODAL_TYPE_WINDOW; On 2013/10/24 13:11:02, bartfab wrote: > - It would seem that ui::MODAL_TYPE_SYSTEM is the correct choice here. > - #include "ui/base/ui_base_types.h" Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:55: if (button == ui::DIALOG_BUTTON_OK) { On 2013/10/24 13:11:02, bartfab wrote: > No curly braces for single-line conditionals. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:58: if (button == ui::DIALOG_BUTTON_CANCEL) { On 2013/10/24 13:11:02, bartfab wrote: > No curly braces for single-line conditionals. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:59: return l10n_util::GetStringUTF16(IDS_APP_CANCEL); On 2013/10/24 13:11:02, bartfab wrote: > You can defer to the inherited method instead here. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:61: return base::string16(); On 2013/10/24 13:11:02, bartfab wrote: > In this implementation, there should be a NOTREACHED(). But you can defer to the > inherited method instead here. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:65: LogoutButtonTray *owner) : display_label_(NULL), owner_(owner) { On 2013/10/24 13:11:02, bartfab wrote: > Break at the : and the , Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:69: if (owner_) { On 2013/10/24 13:11:02, bartfab wrote: > No curly braces for single-line conditionals. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:70: owner_->RemoveConfirmDialogInstance(this); On 2013/10/24 13:11:02, bartfab wrote: > - The "Instance" in this method name is unnecessary. > - Instead of keeping track of the owner and manually unregistering from it, why > not have the owner keep a WeakPtr to this? Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:74: void LogoutConfirmationDialogView::InitAndShow() { On 2013/10/24 13:11:02, bartfab wrote: > Why is this initialization not done in the constructor? Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:75: ui::ResourceBundle &rb = ui::ResourceBundle::GetSharedInstance(); On 2013/10/24 13:11:02, bartfab wrote: > Abbreviations are strongly frowned upon. Please expand the name "rb". Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:77: display_label_ = new views::Label(); On 2013/10/24 13:11:02, bartfab wrote: > No () needed for new. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:78: display_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT); On 2013/10/24 13:11:02, bartfab wrote: > #include "ui/gfx/text_constants.h" Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:80: display_label_->SetFont(rb.GetFont(ui::ResourceBundle::BaseFont)); On 2013/10/24 13:11:02, bartfab wrote: > - In this implementation, #include "ui/gfx/font.h". > - Is this not the default font anyway, making this unnecessary? Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:82: views::GridLayout *layout = views::GridLayout::CreatePanel(this); On 2013/10/24 13:11:02, bartfab wrote: > - Put the * on the data type, not the variable name. > - Is a GridLayout really the best choice here? You are using one column only. > Could you not use a simpler layout class? Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:85: views::ColumnSet *column_set = layout->AddColumnSet(0); On 2013/10/24 13:11:02, bartfab wrote: > Put the * on the data type, not the variable name. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:100: base::TimeDelta::FromMilliseconds(kAutoLogoutDurationMs); On 2013/10/24 13:11:02, bartfab wrote: > Indent 4 spaces relative to the line above, not 2. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:106: GetWidget()->SetAlwaysOnTop(true); On 2013/10/24 13:11:02, bartfab wrote: > Is this really necessary when the dialog is marked as modal? Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:109: timer_.Start(FROM_HERE, On 2013/10/24 13:11:02, bartfab wrote: > #include "base/location.h" Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:118: // kCountdownUpdateIntervalMs On 2013/10/24 13:11:02, bartfab wrote: > Add full stop at the end of the sentence. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:119: base::TimeDelta logout_warning_time = countdown_end_time_ - On 2013/10/24 13:11:02, bartfab wrote: > Mark as const. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:124: IDS_ASH_LOGOUT_CONFIRMATION_WARNING, On 2013/10/24 13:11:02, bartfab wrote: > Indent 4 spaces, not 6. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:128: IDS_ASH_LOGOUT_CONFIRMATION_WARNING_NOW)); On 2013/10/24 13:11:02, bartfab wrote: > Indent 4 spaces, not 6. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.cc:135: } // namespace ash On 2013/10/24 13:11:02, bartfab wrote: > Add blank line at end of file. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:12: class Label; On 2013/10/24 13:11:02, bartfab wrote: > Do not indent class declarations. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:21: // dialog view class for logout confirmation On 2013/10/24 13:11:02, bartfab wrote: > This should be a complete sentence, starting with a capital letter and ending > with a full stop. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:24: On 2013/10/24 13:11:02, bartfab wrote: > Remove blank line. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:26: virtual bool Accept(); On 2013/10/24 13:11:02, bartfab wrote: > Add OVERRIDE. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:27: virtual int GetDialogButtons() const OVERRIDE; On 2013/10/24 13:11:02, bartfab wrote: > #include "base/compiler_specific.h" Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:33: On 2013/10/24 13:11:02, bartfab wrote: > Remove blank line. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:34: friend class LogoutButtonTray; On 2013/10/24 13:11:02, bartfab wrote: > Make the methods public and remove the friend declaration. We only mark methods > internal to a class as private. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:36: LogoutConfirmationDialogView(LogoutButtonTray *owner); On 2013/10/24 13:11:02, bartfab wrote: > - Put the * on the data type, not the argument name. > - Mark the constructor as explicit. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:45: views::Label *display_label_; On 2013/10/24 13:11:02, bartfab wrote: > Put the * on the data type, not the member name. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:47: base::Time countdown_end_time_; On 2013/10/24 13:11:02, bartfab wrote: > - Use base::TimeTicks instead: base::Time changes when the timezone or the clock > is adjusted. base::TimeTicks is immune to such changes. > - #include "base/time.h" Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:51: LogoutButtonTray *owner_; On 2013/10/24 13:11:02, bartfab wrote: > Put the * on the data type, not the member name. Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:53: DISALLOW_COPY_AND_ASSIGN(LogoutConfirmationDialogView); On 2013/10/24 13:11:02, bartfab wrote: > #include "base/basictypes.h" Done. https://codereview.chromium.org/40053002/diff/1/ash/system/logout_button/logo... ash/system/logout_button/logout_confirmation_dialog_view.h:60: #endif // ASH_SYSTEM_LOGOUT_BUTTON_LOGOUT_CONFIRMATION_DIALOG_VIEW_H_ On 2013/10/24 13:11:02, bartfab wrote: > Add blank line at end of file. Done.
https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... File ash/system/logout_button/logout_button_observer.h (right): https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_button_observer.h:10: // Observer for the value of the kShowLogoutButtonInTray pref and Nit: s/value/values/ https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_button_observer.h:11: // kLogoutDialogDurationMs pref. kShowLogoutButtonInTray determines whether a Nit: s/pref/prefs/ https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_button_observer.h:23: virtual void OnLogoutDialogDurationChanged(int duration) = 0; Instead of an int, use a base::TimeDelta here. https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... File ash/system/logout_button/logout_button_tray.cc (right): https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_button_tray.cc:114: confirm_dialog_ = (new LogoutConfirmationDialogView())->GetWeakPtr(); The construction in this line is hard to follow. Also, while WeakPtr works well here, I realize that it makes the ownership relation very ambiguous. Sorry for the fore-and-back but I actually think it would be best if LogoutButtonTray had a scoped_ptr that owns the LogoutConfirmationDialogView. When the dialog closes, it notifies the button which clears the scoped_ptr, as you had originally implemented it. https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_button_tray.cc:120: if (!confirm_dialog_) { You want the exact reverse: if (confirm_dialog_) Also, nit: No braces for single-line conditionals. https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_button_tray.cc:148: dialog_duration_ = base::TimeDelta::FromMilliseconds(duration); You need to update the dialog here. It should immediately reflect the updated duration. https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_button_tray.cc:154: if (dialog_duration_.InSeconds() <= 0) { The correct way to do this would be something like: if (dialog_duration < base::TimeDelta::FromSeconds(0)) Also, nit 1: No braces for single-line conditionals. Also, nit 2: Add a comment that explains what is happening here. https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... File ash/system/logout_button/logout_button_tray.h (right): https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_button_tray.h:36: void EnsureConfirmationDialogShown(); Nit: I know I suggested the name but I think it can be improved further a little bit: EnsureConfirmationDialogIsShowing(); https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_button_tray.h:37: void EnsureConfirmationDialogClosed(); Nit: Add "Is" to the name: EnsureConfirmationDialogIsClosed(); https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_button_tray.h:64: base::WeakPtr<LogoutConfirmationDialogView> confirm_dialog_; Nit: s/confirm_dialog_/confirmation_dialog_/ https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.cc:74: weak_factory_.InvalidateWeakPtrs(); This happens automatically when the weak_factory_ dies. https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.h:25: // This class implements dialog view for logout confirmation, supposed to be I do not think the part "supposed to be used by logout button tray directly" is necessary. https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.h:29: base::WeakPtr<LogoutConfirmationDialogView> GetWeakPtr(); SupportsWeakPtr provides this. But as commented elsewhere, I actually think the WeakPtr was a bad idea here - sorry. https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.h:37: LogoutConfirmationDialogView(); The constructor and destructor should be the first members. https://chromiumcodereview.appspot.com/40053002/diff/110001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.h:45: base::TimeTicks countdown_end_time_; Instead of having to pass the time to the dialog and running a timer here, how about LogoutButton runs a timer and manages the dialog?
I updated the code. Regarding using of weak pointer, I think The current approach fits the model well and I added some code to prevent potential memory leaks in the future. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... File ash/system/logout_button/logout_button_observer.h (right): https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_button_observer.h:10: // Observer for the value of the kShowLogoutButtonInTray pref and On 2013/10/28 16:24:07, bartfab wrote: > Nit: s/value/values/ Done. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_button_observer.h:11: // kLogoutDialogDurationMs pref. kShowLogoutButtonInTray determines whether a On 2013/10/28 16:24:07, bartfab wrote: > Nit: s/pref/prefs/ Done. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_button_observer.h:23: virtual void OnLogoutDialogDurationChanged(int duration) = 0; On 2013/10/28 16:24:07, bartfab wrote: > Instead of an int, use a base::TimeDelta here. I think the interface should be aware of the type preference only, assigning high level types is not proper here. If I use base::TimeDelta here I had to do the conversation in a lower level which is not proper I think. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:120: if (!confirm_dialog_) { On 2013/10/28 16:24:07, bartfab wrote: > You want the exact reverse: if (confirm_dialog_) > > Also, nit: No braces for single-line conditionals. Done. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:148: dialog_duration_ = base::TimeDelta::FromMilliseconds(duration); On 2013/10/28 16:24:07, bartfab wrote: > You need to update the dialog here. It should immediately reflect the updated > duration. Done. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:154: if (dialog_duration_.InSeconds() <= 0) { On 2013/10/28 16:24:07, bartfab wrote: > The correct way to do this would be something like: > > if (dialog_duration < base::TimeDelta::FromSeconds(0)) > > Also, nit 1: No braces for single-line conditionals. > > Also, nit 2: Add a comment that explains what is happening here. Done. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:36: void EnsureConfirmationDialogShown(); On 2013/10/28 16:24:07, bartfab wrote: > Nit: I know I suggested the name but I think it can be improved further a little > bit: > > EnsureConfirmationDialogIsShowing(); Done. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:37: void EnsureConfirmationDialogClosed(); On 2013/10/28 16:24:07, bartfab wrote: > Nit: Add "Is" to the name: > > EnsureConfirmationDialogIsClosed(); Done. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:64: base::WeakPtr<LogoutConfirmationDialogView> confirm_dialog_; On 2013/10/28 16:24:07, bartfab wrote: > Nit: s/confirm_dialog_/confirmation_dialog_/ Done. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:74: weak_factory_.InvalidateWeakPtrs(); On 2013/10/28 16:24:07, bartfab wrote: > This happens automatically when the weak_factory_ dies. Done. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:25: // This class implements dialog view for logout confirmation, supposed to be On 2013/10/28 16:24:07, bartfab wrote: > I do not think the part "supposed to be used by logout button tray directly" is > necessary. Done. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:29: base::WeakPtr<LogoutConfirmationDialogView> GetWeakPtr(); On 2013/10/28 16:24:07, bartfab wrote: > SupportsWeakPtr provides this. But as commented elsewhere, I actually think the > WeakPtr was a bad idea here - sorry. Done. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:37: LogoutConfirmationDialogView(); On 2013/10/28 16:24:07, bartfab wrote: > The constructor and destructor should be the first members. Done. https://codereview.chromium.org/40053002/diff/110001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:45: base::TimeTicks countdown_end_time_; On 2013/10/28 16:24:07, bartfab wrote: > Instead of having to pass the time to the dialog and running a timer here, how > about LogoutButton runs a timer and manages the dialog? I think the current approach is fine. The life time of logout confirmation dialog and logout button is different and I believe the timer should be with the dialog.
Almost there. Tests are still missing though. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... File ash/system/logout_button/logout_button_observer.h (right): https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_observer.h:23: virtual void OnLogoutDialogDurationChanged(int duration) = 0; I think the conversion should happen at a lower level. This is an interface for passing configuration settings to the power button. The fact that these settings come from prefs is an implementation detail. The settings should be read from their source (a pref in this case), converted to the most appropriate data format (base::TimeDelta in this case) and then sent to the button. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (left): https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:10: #include "ash/system/tray/system_tray_delegate.h" This is still used. system_tray_delegate() returns a SystemTrayDelegate. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:111: confirmation_dialog_->DeleteDelegate(); This is the wrong method. DeleteDelegate() is how a widget notifies its delegate that the widget is going away. You will need to do something different here. From a cursory look at the code GetWidget()->Close() followed by a DeleteSoon might be the correct soluton. Also, no curly braces for single-line conditionals. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:124: confirmation_dialog_->GetWidget()->Close(); Can you destroy the dialog here? Otherwise, we are using memory for no good reason - the dialog is no longer showing, might as well destroy it. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:159: if (dialog_duration_ <= base::TimeDelta::FromSeconds(0)) { Nit: No curly braces for single-line conditionals. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:25: class LogoutConfirmationDialogView; Nit: alphabetize https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:64: base::WeakPtr<LogoutConfirmationDialogView> confirmation_dialog_; This needs to be a scoped_ptr. Making the ownership explicit is more important than simplifying the code. And the code will not become significantly more complex anyway - you just need to add one method that allows the dialog to notify its owner when it is going away. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:26: const int kCountdownUpdateIntervalMs = 100; // 100 millisecond * If the UI only shows seconds, why update it every 100ms? * Nit: s/millisecond/milliseconds./ https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:85: const base::TimeDelta logout_warning_time = countdown_start_time_ Could you rename this to something like "time_remaining" or "remaining_time"? https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:87: if (logout_warning_time.InMilliseconds() > 0) { This should be: if (logout_warning_time > base::TimeDelta::FromSeconds(0)) { https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:90: ui::TimeFormat::TimeDurationLong(logout_warning_time))); Nit: un-indent 2 spaces https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:26: class LogoutConfirmationDialogView : Nit: The : goes on the next line. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:30: LogoutConfirmationDialogView(); clang will complain that you need a virtual ~LogoutConfirmationDialogView(), even if it ends up being empty. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:39: void UpdateCountdown(); This should be private. https://codereview.chromium.org/40053002/diff/200001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/40053002/diff/200001/chrome/common/pref_names... chrome/common/pref_names.cc:2375: // Integer value in milliseconds indicating the duration of logout confirmation Grammar nit: "Integer value in milliseconds indicating the length of time for which a confirmation dialog should be shown when the user presses the logout button. A value of 0 indicates that logout should happen immediately, without showing a confirmation dialog."
I fixed all the problems that you pointed out. tests is still missing though. The approach I used for replacing weak pointers is a little bit complicated. Please check it carefully. Bin https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... File ash/system/logout_button/logout_button_observer.h (right): https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_observer.h:23: virtual void OnLogoutDialogDurationChanged(int duration) = 0; On 2013/10/30 11:17:28, bartfab wrote: > I think the conversion should happen at a lower level. This is an interface for > passing configuration settings to the power button. The fact that these settings > come from prefs is an implementation detail. The settings should be read from > their source (a pref in this case), converted to the most appropriate data > format (base::TimeDelta in this case) and then sent to the button. Done. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (left): https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:10: #include "ash/system/tray/system_tray_delegate.h" On 2013/10/30 11:17:28, bartfab wrote: > This is still used. system_tray_delegate() returns a SystemTrayDelegate. Done. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:111: confirmation_dialog_->DeleteDelegate(); On 2013/10/30 11:17:28, bartfab wrote: > This is the wrong method. DeleteDelegate() is how a widget notifies its delegate > that the widget is going away. You will need to do something different here. > From a cursory look at the code GetWidget()->Close() followed by a DeleteSoon > might be the correct soluton. > > Also, no curly braces for single-line conditionals. Done. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:124: confirmation_dialog_->GetWidget()->Close(); On 2013/10/30 11:17:28, bartfab wrote: > Can you destroy the dialog here? Otherwise, we are using memory for no good > reason - the dialog is no longer showing, might as well destroy it. Done. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:159: if (dialog_duration_ <= base::TimeDelta::FromSeconds(0)) { On 2013/10/30 11:17:28, bartfab wrote: > Nit: No curly braces for single-line conditionals. Done. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:25: class LogoutConfirmationDialogView; On 2013/10/30 11:17:28, bartfab wrote: > Nit: alphabetize Done. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:64: base::WeakPtr<LogoutConfirmationDialogView> confirmation_dialog_; On 2013/10/30 11:17:28, bartfab wrote: > This needs to be a scoped_ptr. Making the ownership explicit is more important > than simplifying the code. And the code will not become significantly more > complex anyway - you just need to add one method that allows the dialog to > notify its owner when it is going away. Done. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:26: const int kCountdownUpdateIntervalMs = 100; // 100 millisecond On 2013/10/30 11:17:28, bartfab wrote: > * If the UI only shows seconds, why update it every 100ms? > * Nit: s/millisecond/milliseconds./ Done. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:85: const base::TimeDelta logout_warning_time = countdown_start_time_ On 2013/10/30 11:17:28, bartfab wrote: > Could you rename this to something like "time_remaining" or "remaining_time"? Done. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:87: if (logout_warning_time.InMilliseconds() > 0) { On 2013/10/30 11:17:28, bartfab wrote: > This should be: > > if (logout_warning_time > base::TimeDelta::FromSeconds(0)) { Done. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:90: ui::TimeFormat::TimeDurationLong(logout_warning_time))); On 2013/10/30 11:17:28, bartfab wrote: > Nit: un-indent 2 spaces Done. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:26: class LogoutConfirmationDialogView : On 2013/10/30 11:17:28, bartfab wrote: > Nit: The : goes on the next line. Done. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:30: LogoutConfirmationDialogView(); On 2013/10/30 11:17:28, bartfab wrote: > clang will complain that you need a virtual ~LogoutConfirmationDialogView(), > even if it ends up being empty. Done. https://codereview.chromium.org/40053002/diff/200001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:39: void UpdateCountdown(); On 2013/10/30 11:17:28, bartfab wrote: > This should be private. Done. https://codereview.chromium.org/40053002/diff/200001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/40053002/diff/200001/chrome/common/pref_names... chrome/common/pref_names.cc:2375: // Integer value in milliseconds indicating the duration of logout confirmation On 2013/10/30 11:17:28, bartfab wrote: > Grammar nit: > > "Integer value in milliseconds indicating the length of time for which a > confirmation dialog should be shown when the user presses the logout button. A > value of 0 indicates that logout should happen immediately, without showing a > confirmation dialog." Done.
https://codereview.chromium.org/40053002/diff/330001/ash/system/logout_button... File ash/system/logout_button/logout_button_observer.h (right): https://codereview.chromium.org/40053002/diff/330001/ash/system/logout_button... ash/system/logout_button/logout_button_observer.h:12: // Observer for the values of the kShowLogoutButtonInTray pref and Nit 1: s/pref// Nit 2: The reference to kShowLogoutButtonInTray made sense before because this class really just tracked a pref 1:1. Now it tracks two things and one is a base::TimeDelta derived from a pref, not just a raw pref value. I think you can reword the entire description and remove the references to prefs by name. It makes sense to keep them in the comments above the individual methods though so it is clear when exactly they get called. https://codereview.chromium.org/40053002/diff/330001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/330001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:124: confirmation_dialog_->GetWidget()->Close(); Are you sure that closing the widget will not destroy the delegate for you? Looking at the code, Widget::OnNativeWidgetDestroyed() calls DialogDelegateView::DeleteDelegate which destroys the delegate. Is it correct to explicitly destroy the delegate? Will this not produce a double-free? https://codereview.chromium.org/40053002/diff/330001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/330001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:56: LogoutConfirmationDialogView* UnbindWithConfirmationDialog(); In general, the approach looks good but is overly complex: * If the LogoutConfirmationDialogView is destroyed first, it would be sufficient if it called a method named something like OnConfirmationDialogViewDestroyed() which simply releases the confirmation_dialog_ pointer. * You could make the LogoutConfirmationDialogView's owner_ pointer a WeakPtr. LogoutButtonTray would have a weak_factory_ that this WeakPtr comes from. That way, there would be no need to tell the dialog when its owner goes away. You could then fold UnbindWithConfirmationDialog() into EnsureConfirmationDialogIsClosed() and simplify it to: void LogoutButtonTray::EnsureConfirmationDialogIsClosed() { if (confirmation_dialog_) { weak_factory_.InvalidateWeakPtrs(); confirmation_dialog_->GetWidget()->Close(); base::MessageLoop::current()->DeleteSoon(FROM_HERE, confirmation_dialog_.release()); } Finally, three more nits: 1/ "Bind" has a very specific meaning in Chrome (it refers to base::Bind), so you should not be using "bind" and "unbind" for naming unrelated methods. 2/ There is no need to warn that a method is not thread-safe. All Chrome code is assumed to be non-thread-safe unless explicitly stated otherwise. 3/ The method is actually not atomic (i.e., it performs a sequence of operations, not a single atomic one), so better do not document it as such :). https://codereview.chromium.org/40053002/diff/330001/ash/system/tray/system_t... File ash/system/tray/system_tray_notifier.cc (right): https://codereview.chromium.org/40053002/diff/330001/ash/system/tray/system_t... ash/system/tray/system_tray_notifier.cc:237: base::TimeDelta duration_ = base::TimeDelta::FromMilliseconds(duration); Nit 1: const Nit 2: The _ at the end is used for member variables only, not for local variables.
Hello, I send another changeset, please help review it. I'm still not sure if I set up the testing environments correctly or not, and the code was ugly hacked to for testing purpose. On 2013/11/20 18:00:44, bartfab wrote: > https://codereview.chromium.org/40053002/diff/330001/ash/system/logout_button... > File ash/system/logout_button/logout_button_observer.h (right): > > https://codereview.chromium.org/40053002/diff/330001/ash/system/logout_button... > ash/system/logout_button/logout_button_observer.h:12: // Observer for the values > of the kShowLogoutButtonInTray pref and > Nit 1: s/pref// > > Nit 2: The reference to kShowLogoutButtonInTray made sense before because this > class really just tracked a pref 1:1. Now it tracks two things and one is a > base::TimeDelta derived from a pref, not just a raw pref value. I think you can > reword the entire description and remove the references to prefs by name. It > makes sense to keep them in the comments above the individual methods though so > it is clear when exactly they get called. > > https://codereview.chromium.org/40053002/diff/330001/ash/system/logout_button... > File ash/system/logout_button/logout_button_tray.cc (right): > > https://codereview.chromium.org/40053002/diff/330001/ash/system/logout_button... > ash/system/logout_button/logout_button_tray.cc:124: > confirmation_dialog_->GetWidget()->Close(); > Are you sure that closing the widget will not destroy the delegate for you? > > Looking at the code, > Widget::OnNativeWidgetDestroyed() > calls > DialogDelegateView::DeleteDelegate > which destroys the delegate. Is it correct to explicitly destroy the delegate? > Will this not produce a double-free? > > https://codereview.chromium.org/40053002/diff/330001/ash/system/logout_button... > File ash/system/logout_button/logout_button_tray.h (right): > > https://codereview.chromium.org/40053002/diff/330001/ash/system/logout_button... > ash/system/logout_button/logout_button_tray.h:56: LogoutConfirmationDialogView* > UnbindWithConfirmationDialog(); > In general, the approach looks good but is overly complex: > > * If the LogoutConfirmationDialogView is destroyed first, it would be sufficient > if it called a method named something like OnConfirmationDialogViewDestroyed() > which simply releases the confirmation_dialog_ pointer. > > * You could make the LogoutConfirmationDialogView's owner_ pointer a WeakPtr. > LogoutButtonTray would have a weak_factory_ that this WeakPtr comes from. That > way, there would be no need to tell the dialog when its owner goes away. You > could then fold UnbindWithConfirmationDialog() into > EnsureConfirmationDialogIsClosed() and simplify it to: > > void LogoutButtonTray::EnsureConfirmationDialogIsClosed() { > if (confirmation_dialog_) { > weak_factory_.InvalidateWeakPtrs(); > confirmation_dialog_->GetWidget()->Close(); > base::MessageLoop::current()->DeleteSoon(FROM_HERE, > confirmation_dialog_.release()); > } > > Finally, three more nits: > > 1/ "Bind" has a very specific meaning in Chrome (it refers to base::Bind), so > you should not be using "bind" and "unbind" for naming unrelated methods. > > 2/ There is no need to warn that a method is not thread-safe. All Chrome code is > assumed to be non-thread-safe unless explicitly stated otherwise. > > 3/ The method is actually not atomic (i.e., it performs a sequence of > operations, not a single atomic one), so better do not document it as such :). > > https://codereview.chromium.org/40053002/diff/330001/ash/system/tray/system_t... > File ash/system/tray/system_tray_notifier.cc (right): > > https://codereview.chromium.org/40053002/diff/330001/ash/system/tray/system_t... > ash/system/tray/system_tray_notifier.cc:237: base::TimeDelta duration_ = > base::TimeDelta::FromMilliseconds(duration); > Nit 1: const > Nit 2: The _ at the end is used for member variables only, not for local > variables.
The tests look very nice. If you fix up the nits and comments, I think the code is ready. By the way: When you address a comment, please mark it as "Done" or add an explanation what you changed if a simple "Done" is not sufficient. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_observer.h (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_observer.h:22: // 'duration' is the duration of the logout confirmation dialog after user Nit 1: We use | to delimit argument names, so |duration|. Nit 2: Wording: s/of the logout confirmation dialog after user pressed logout button/for which the logout confirmation dialog is shown after the user has pressed the logout button/ https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:132: return (bool)confirmation_dialog_; Nit: We use C++-style casts. In this case though, no explicit casting is needed at all. You can just do: return confirmation_dialog_; https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:204: Nit: No blank line needed. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:62: void OnUserLogoutEvent(); This method name is rather cryptic. Also, the method is entirely unnecessary. It gets invoked by ButtonPressed() and by tests. Why not simply have the tests call ButtonPressed(), doing away with this method altogether? https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:64: LogoutConfirmationDialogView* GetConfirmationDialog(); Nit: This is used in tests only. You could rename it to GetConfirmationDialogForTest() to ensure it does not get called in production code. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray_unittest.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:19: class MockTimeSingleThreadTaskRunner : public base::SingleThreadTaskRunner { This entire class is a copy & paste from another place. Please move it to a shared place instead. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:56: : public LogoutConfirmationSettingsProvider { Nit: s/SettingsProvider/Delegate/. This name is more appropriate because the helper class not only supplies settings but also handles commands sent to it (such as LogoutCurrentUser). https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:58: MockLogoutConfirmationSettingsProvider( Single-argument constructors should be marked explicit. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:61: // LogoutConfirmationSettingsProvider Nit: Add a colon at the end of the line. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:62: virtual void LogoutCurrentUser(LogoutConfirmationDialogView*) OVERRIDE; Nit: Add argument name. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:65: void SetLogoutCalled(bool called); Nit: This is never used. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:66: bool IsLogoutCalled(); Nit 1: WasLogoutCalled() would be more correct. Nit 2: const https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:72: }; Nit: Add DISALLOW_COPY_AND_ASSIGN(MockLogoutConfirmationSettingsProvider); https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:158: LogoutConfirmationDialogTest() {} Nit: You should be consistent with your inlining style. Since you did not inline any other methods, the constructor and destructor here should not be inlined either. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:161: // testing::Test Nit: Add a colon at the end of the line. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:165: void ChangeDialogDuration(int duration_ms); Nit: Make the argument a base::TimeDelta. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:168: scoped_ptr<LogoutButtonTray> tray_; Nit: s/tray_/button_/ or s/tray_/logout_button_/ would be more appropriate. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:170: scoped_ptr<MockLogoutConfirmationSettingsProvider> provider_; Nit: In line with renaming the class, this member should be renamed to |delegate_|. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:190: base::ThreadTaskRunnerHandle runner_handle(runner_); Nit: Could this be handled by the LogoutConfirmationDialogTest class? As it stands, you have to duplicate it in every test case. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:196: runner_->FastForwardBy(1 * 1000); // 1 second after start Nit 1: Here and elsewhere: Terminate comments with a full stop. Nit 2: "after start" of what? How about "1 second since the logout button was pressed?" https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:201: runner_->FastForwardBy(18 * 1000); // 19 seconds after start Nit: Fast-forwarding by 1s and then 18s is rather arbitrary. I presume you want to verify that the dialog shows immediately and then want to verify that shortly before the countdown expires, is still being seen. How about this: runner_->FastForwardBy(0); // Now the dialog should have popped up runner_->FastForwardBy(19 * 1000); // Now the dialog should still be showing runner_->FastForwardBy(2 * 1000); // Now the user should have been logged out. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:215: ChangeDialogDuration(0); // set preference to 0 Nit: The comment does not really add much. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:223: runner_->FastForwardBy(1 * 1000); // 1 second Nit: Same as above: Why not runner_->FastForwardBy(0) to see if there is any immediate reaction? https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:227: } Nit: Add runner_->FastForwardUntilNoTasksRemain() and verify that the the dialog is not shown and the user is not logged out. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:232: ChangeDialogDuration(5 * 1000); // set dialog duration to 5 seconds Nit: Here and elsewhere: If you change the argument to a base::TimeDelta, the call will be very self-documenting and no explicit comment will be needed. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:238: runner_->FastForwardBy(3 * 1000); // 3 seconds after start Nit: Same as above: Why not runner_->FastForwardBy(0) to see if there is any immediate reaction? https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:261: runner_->FastForwardBy(3 * 1000); // 3 seconds after start Nit: Same as above: Why not runner_->FastForwardBy(0) to see if there is any immediate reaction? https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:267: runner_->FastForwardBy(1 * 1000); // 4 seconds after start Nit: Same as above: Why not runner_->FastForwardBy(0) to see if there is any immediate reaction? https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:45: dialog->GetWidget()->Close(); Nit: Can the dialog not just call GetWidget()->Close() itself? Then you could remove the |dialog| argument altogether. I know that in tests, you call DeleteDelegate() instead. However, that simply translates into delete dialog and can easily be done explicitly in tests (best with a scoped_ptr). You would then only have to make sure that either GetWidget()->Close() is also valid to do in tests or make it conditional, e.g.: if (GetWidget()) GetWidget()->close() This way, the code will not run in tests. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:28: class ASH_EXPORT LogoutConfirmationSettingsProvider { 1) This should be an inner class of LogoutConfirmationDialogView. 2) This class should be called Delegate. 3) Instead of statics getters/setters, pass the Delegate in the constructor. This means that you will have to pass it to LogoutButtonTray first, which will take ownership of the Delegate and will pass a Delegate* to the LogoutConfirmationDialogView whenever it creates it. https://codereview.chromium.org/40053002/diff/450001/ash/system/tray/system_t... File ash/system/tray/system_tray_notifier.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/tray/system_t... ash/system/tray/system_tray_notifier.cc:237: const base::TimeDelta duration = base::TimeDelta::FromMilliseconds(duration_ms); Nit: This line is more than 80 characters long now. It must be wrapped. https://codereview.chromium.org/40053002/diff/450001/chrome/browser/chromeos/... File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right): https://codereview.chromium.org/40053002/diff/450001/chrome/browser/chromeos/... chrome/browser/chromeos/system/ash_system_tray_delegate.cc:958: user_pref_registrar_->prefs()->GetInteger( Nit: Why not convert to base::TimeDelta() here? You have to do the conversion somewhere - best do it as early as possible so the value you pass around is as unambiguous as possible.
Hello, Here is quick fixes to some of the comments you left, and two comments to seek your help on additional suggestion. Bin https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_observer.h (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_observer.h:22: // 'duration' is the duration of the logout confirmation dialog after user On 2013/12/03 19:46:04, bartfab wrote: > Nit 1: We use | to delimit argument names, so |duration|. > > Nit 2: Wording: s/of the logout confirmation dialog after user pressed logout > button/for which the logout confirmation dialog is shown after the user has > pressed the logout button/ Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:132: return (bool)confirmation_dialog_; On 2013/12/03 19:46:04, bartfab wrote: > Nit: We use C++-style casts. In this case though, no explicit casting is needed > at all. You can just do: > > return confirmation_dialog_; Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:204: On 2013/12/03 19:46:04, bartfab wrote: > Nit: No blank line needed. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:62: void OnUserLogoutEvent(); On 2013/12/03 19:46:04, bartfab wrote: > This method name is rather cryptic. Also, the method is entirely unnecessary. It > gets invoked by ButtonPressed() and by tests. Why not simply have the tests call > ButtonPressed(), doing away with this method altogether? The reason I didn't call ButtonPressed() directly is that there is a DCHECK_EQ statement I don't want to remove or bypass it in ugly way. Can you suggest some better method name? https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:64: LogoutConfirmationDialogView* GetConfirmationDialog(); On 2013/12/03 19:46:04, bartfab wrote: > Nit: This is used in tests only. You could rename it to > GetConfirmationDialogForTest() to ensure it does not get called in production > code. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray_unittest.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:19: class MockTimeSingleThreadTaskRunner : public base::SingleThreadTaskRunner { On 2013/12/03 19:46:04, bartfab wrote: > This entire class is a copy & paste from another place. Please move it to a > shared place instead. Can you suggest a proper place to move the shared class to? https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:58: MockLogoutConfirmationSettingsProvider( On 2013/12/03 19:46:04, bartfab wrote: > Single-argument constructors should be marked explicit. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:61: // LogoutConfirmationSettingsProvider On 2013/12/03 19:46:04, bartfab wrote: > Nit: Add a colon at the end of the line. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:62: virtual void LogoutCurrentUser(LogoutConfirmationDialogView*) OVERRIDE; On 2013/12/03 19:46:04, bartfab wrote: > Nit: Add argument name. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:65: void SetLogoutCalled(bool called); On 2013/12/03 19:46:04, bartfab wrote: > Nit: This is never used. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:66: bool IsLogoutCalled(); On 2013/12/03 19:46:04, bartfab wrote: > Nit 1: WasLogoutCalled() would be more correct. > Nit 2: const Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:72: }; On 2013/12/03 19:46:04, bartfab wrote: > Nit: Add DISALLOW_COPY_AND_ASSIGN(MockLogoutConfirmationSettingsProvider); Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:158: LogoutConfirmationDialogTest() {} On 2013/12/03 19:46:04, bartfab wrote: > Nit: You should be consistent with your inlining style. Since you did not inline > any other methods, the constructor and destructor here should not be inlined > either. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:161: // testing::Test On 2013/12/03 19:46:04, bartfab wrote: > Nit: Add a colon at the end of the line. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:168: scoped_ptr<LogoutButtonTray> tray_; On 2013/12/03 19:46:04, bartfab wrote: > Nit: s/tray_/button_/ or s/tray_/logout_button_/ would be more appropriate. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/tray/system_t... File ash/system/tray/system_tray_notifier.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/tray/system_t... ash/system/tray/system_tray_notifier.cc:237: const base::TimeDelta duration = base::TimeDelta::FromMilliseconds(duration_ms); On 2013/12/03 19:46:04, bartfab wrote: > Nit: This line is more than 80 characters long now. It must be wrapped. Done. https://codereview.chromium.org/40053002/diff/450001/chrome/browser/chromeos/... File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right): https://codereview.chromium.org/40053002/diff/450001/chrome/browser/chromeos/... chrome/browser/chromeos/system/ash_system_tray_delegate.cc:958: user_pref_registrar_->prefs()->GetInteger( On 2013/12/03 19:46:04, bartfab wrote: > Nit: Why not convert to base::TimeDelta() here? You have to do the conversion > somewhere - best do it as early as possible so the value you pass around is as > unambiguous as possible. Done.
Another hint on the way we do code reviews: When you address a comment and mark it as "Done," you should upload a new CL that has the fix in it first and only then hit "Publish all my drafts." https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:62: void OnUserLogoutEvent(); On 2013/12/04 10:47:03, bjin wrote: > On 2013/12/03 19:46:04, bartfab wrote: > > This method name is rather cryptic. Also, the method is entirely unnecessary. > It > > gets invoked by ButtonPressed() and by tests. Why not simply have the tests > call > > ButtonPressed(), doing away with this method altogether? > > The reason I didn't call ButtonPressed() directly is that there is a DCHECK_EQ > statement I don't want to remove or bypass it in ugly way. Can you suggest some > better method name? You can make the test a friend of LogoutButtonTray. This would allow the test to access the private button_ so that you can directly call tray_->ButtonPressed(tray_->button_, ui::Event()); https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray_unittest.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:19: class MockTimeSingleThreadTaskRunner : public base::SingleThreadTaskRunner { On 2013/12/04 10:47:03, bjin wrote: > On 2013/12/03 19:46:04, bartfab wrote: > > This entire class is a copy & paste from another place. Please move it to a > > shared place instead. > > Can you suggest a proper place to move the shared class to? It should probably move to a file ending in test_util.h/test_util.cc. Since this is used in different places throughout the chromeos code, chrome/browser/chromeos/test_util.h would be a good start. But that is a generic file name, so some reviewer may object and ask you to move it into a subdirectory. This is easy to do, so I suggest you start with that name and see how it goes.
Hello, I rewrite the code according to most of comments you left, but there are still three comments I need your opinion or further explanation. PTAL Bin https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:62: void OnUserLogoutEvent(); On 2013/12/04 12:51:56, bartfab wrote: > On 2013/12/04 10:47:03, bjin wrote: > > On 2013/12/03 19:46:04, bartfab wrote: > > > This method name is rather cryptic. Also, the method is entirely > unnecessary. > > It > > > gets invoked by ButtonPressed() and by tests. Why not simply have the tests > > call > > > ButtonPressed(), doing away with this method altogether? > > > > The reason I didn't call ButtonPressed() directly is that there is a DCHECK_EQ > > statement I don't want to remove or bypass it in ugly way. Can you suggest > some > > better method name? > > You can make the test a friend of LogoutButtonTray. This would allow the test to > access the private button_ so that you can directly call > tray_->ButtonPressed(tray_->button_, ui::Event()); Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray_unittest.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:19: class MockTimeSingleThreadTaskRunner : public base::SingleThreadTaskRunner { I'm not sure if it's okay to use chrome/* classes from ash/*, but I think the better idea is implementing a priority_queue(instead of deque) version of TestSimpleTaskRunner (base/test/test_simple_task_runner.h), like TestForwardableTaskRunner or TestOrderedTaskRunner, upon the existing TestPendingTask(base/test/test_pending_task.h). On 2013/12/04 12:51:56, bartfab wrote: > On 2013/12/04 10:47:03, bjin wrote: > > On 2013/12/03 19:46:04, bartfab wrote: > > > This entire class is a copy & paste from another place. Please move it to a > > > shared place instead. > > > > Can you suggest a proper place to move the shared class to? > > It should probably move to a file ending in test_util.h/test_util.cc. Since this > is used in different places throughout the chromeos code, > chrome/browser/chromeos/test_util.h would be a good start. But that is a generic > file name, so some reviewer may object and ask you to move it into a > subdirectory. This is easy to do, so I suggest you start with that name and see > how it goes. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:56: : public LogoutConfirmationSettingsProvider { On 2013/12/03 19:46:04, bartfab wrote: > Nit: s/SettingsProvider/Delegate/. This name is more appropriate because the > helper class not only supplies settings but also handles commands sent to it > (such as LogoutCurrentUser). Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:165: void ChangeDialogDuration(int duration_ms); On 2013/12/03 19:46:04, bartfab wrote: > Nit: Make the argument a base::TimeDelta. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:170: scoped_ptr<MockLogoutConfirmationSettingsProvider> provider_; On 2013/12/03 19:46:04, bartfab wrote: > Nit: In line with renaming the class, this member should be renamed to > |delegate_|. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:190: base::ThreadTaskRunnerHandle runner_handle(runner_); On 2013/12/03 19:46:04, bartfab wrote: > Nit: Could this be handled by the LogoutConfirmationDialogTest class? As it > stands, you have to duplicate it in every test case. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:196: runner_->FastForwardBy(1 * 1000); // 1 second after start On 2013/12/03 19:46:04, bartfab wrote: > Nit 1: Here and elsewhere: Terminate comments with a full stop. > > Nit 2: "after start" of what? How about "1 second since the logout button was > pressed?" Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:201: runner_->FastForwardBy(18 * 1000); // 19 seconds after start On 2013/12/03 19:46:04, bartfab wrote: > Nit: Fast-forwarding by 1s and then 18s is rather arbitrary. I presume you want > to verify that the dialog shows immediately and then want to verify that shortly > before the countdown expires, is still being seen. How about this: > > runner_->FastForwardBy(0); > // Now the dialog should have popped up > runner_->FastForwardBy(19 * 1000); > // Now the dialog should still be showing > runner_->FastForwardBy(2 * 1000); > // Now the user should have been logged out. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:215: ChangeDialogDuration(0); // set preference to 0 On 2013/12/03 19:46:04, bartfab wrote: > Nit: The comment does not really add much. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:223: runner_->FastForwardBy(1 * 1000); // 1 second On 2013/12/03 19:46:04, bartfab wrote: > Nit: Same as above: Why not runner_->FastForwardBy(0) to see if there is any > immediate reaction? Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:227: } I didn't follow you on this. On 2013/12/03 19:46:04, bartfab wrote: > Nit: Add runner_->FastForwardUntilNoTasksRemain() and verify that the the dialog > is not shown and the user is not logged out. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:232: ChangeDialogDuration(5 * 1000); // set dialog duration to 5 seconds On 2013/12/03 19:46:04, bartfab wrote: > Nit: Here and elsewhere: If you change the argument to a base::TimeDelta, the > call will be very self-documenting and no explicit comment will be needed. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:238: runner_->FastForwardBy(3 * 1000); // 3 seconds after start On 2013/12/03 19:46:04, bartfab wrote: > Nit: Same as above: Why not runner_->FastForwardBy(0) to see if there is any > immediate reaction? Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:261: runner_->FastForwardBy(3 * 1000); // 3 seconds after start On 2013/12/03 19:46:04, bartfab wrote: > Nit: Same as above: Why not runner_->FastForwardBy(0) to see if there is any > immediate reaction? Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:267: runner_->FastForwardBy(1 * 1000); // 4 seconds after start On 2013/12/03 19:46:04, bartfab wrote: > Nit: Same as above: Why not runner_->FastForwardBy(0) to see if there is any > immediate reaction? Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:45: dialog->GetWidget()->Close(); On 2013/12/03 19:46:04, bartfab wrote: > Nit: Can the dialog not just call GetWidget()->Close() itself? Then you could > remove the |dialog| argument altogether. > > I know that in tests, you call DeleteDelegate() instead. However, that simply > translates into delete dialog and can easily be done explicitly in tests (best > with a scoped_ptr). You would then only have to make sure that either > GetWidget()->Close() is also valid to do in tests or make it conditional, e.g.: > > if (GetWidget()) > GetWidget()->close() > > This way, the code will not run in tests. I'm not sure I understand, it's the dialog it self I want ensure closed(then I can verify if the dialog is showing easily). I think the |dialog| argument is necessary since there is a situation to logout without an existing LogoutConfirmationDialogView(ButtonPressed() with zero dialog duration preference). In this situation I can simply pass NULL to LogoutCurrentUser(). https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:28: class ASH_EXPORT LogoutConfirmationSettingsProvider { On 2013/12/03 19:46:04, bartfab wrote: > 1) This should be an inner class of LogoutConfirmationDialogView. > > 2) This class should be called Delegate. > > 3) Instead of statics getters/setters, pass the Delegate in the constructor. > This means that you will have to pass it to LogoutButtonTray first, which will > take ownership of the Delegate and will pass a Delegate* to the > LogoutConfirmationDialogView whenever it creates it. Done.
https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray_unittest.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:19: class MockTimeSingleThreadTaskRunner : public base::SingleThreadTaskRunner { On 2013/12/05 10:08:05, bjin wrote: > I'm not sure if it's okay to use chrome/* classes from ash/*, but I think the > better idea is implementing a priority_queue(instead of deque) version of > TestSimpleTaskRunner (base/test/test_simple_task_runner.h), like > TestForwardableTaskRunner or TestOrderedTaskRunner, upon the existing > TestPendingTask(base/test/test_pending_task.h). > > On 2013/12/04 12:51:56, bartfab wrote: > > On 2013/12/04 10:47:03, bjin wrote: > > > On 2013/12/03 19:46:04, bartfab wrote: > > > > This entire class is a copy & paste from another place. Please move it to > a > > > > shared place instead. > > > > > > Can you suggest a proper place to move the shared class to? > > > > It should probably move to a file ending in test_util.h/test_util.cc. Since > this > > is used in different places throughout the chromeos code, > > chrome/browser/chromeos/test_util.h would be a good start. But that is a > generic > > file name, so some reviewer may object and ask you to move it into a > > subdirectory. This is easy to do, so I suggest you start with that name and > see > > how it goes. > As discussed offline, I agree that having this code copy & pasted is probably our best solution for now. It is needed by tests under chrome/ and tests under ash/ but is not generic enough to warrant placing it in a shared place like base/. Hence, there will be two implementations of MockTimeSingleThreadTaskRunner, one here and one in chrome/. Feel free to reimplement the MockTimeSingleThreadTaskRunner here in a nicer way. What you copy & pasted from the tests I wrote works but if you want to improve it, by all means, go ahead. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:227: } On 2013/12/05 10:08:05, bjin wrote: > I didn't follow you on this. > > On 2013/12/03 19:46:04, bartfab wrote: > > Nit: Add runner_->FastForwardUntilNoTasksRemain() and verify that the the > dialog > > is not shown and the user is not logged out. > At the end of the test, you verify that the confirmation dialog is not showing *at this moment* and logout has not been called *at this moment*. But maybe the dialog would show or a logout would happen a few moments later? My suggestion was to add this: runner_->FastForwardUntilNoTasksRemain(); EXPECT_FALSE(tray_->IsConfirmationDialogShowing()); EXPECT_TRUE(provider_->IsLogoutCalled()); This fast-forwards time once more, until absolutely all tasks that were ever posted have been run. There are guaranteed to be zero tasks in the queue. If at that point, no dialog is showing and no logout has happened, we know that the user will not be logged out. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:45: dialog->GetWidget()->Close(); On 2013/12/05 10:08:05, bjin wrote: > On 2013/12/03 19:46:04, bartfab wrote: > > Nit: Can the dialog not just call GetWidget()->Close() itself? Then you could > > remove the |dialog| argument altogether. > > > > I know that in tests, you call DeleteDelegate() instead. However, that simply > > translates into delete dialog and can easily be done explicitly in tests (best > > with a scoped_ptr). You would then only have to make sure that either > > GetWidget()->Close() is also valid to do in tests or make it conditional, > e.g.: > > > > if (GetWidget()) > > GetWidget()->close() > > > > This way, the code will not run in tests. > > I'm not sure I understand, it's the dialog it self I want ensure closed(then I > can verify if the dialog is showing easily). > > I think the |dialog| argument is necessary since there is a situation to logout > without an existing LogoutConfirmationDialogView(ButtonPressed() with zero > dialog duration preference). In this situation I can simply pass NULL to > LogoutCurrentUser(). What I meant is this: The LogoutCurrentUser() call can come from two places. In one place, there is no dialog and there is no need to call Close(). In the other place (when the call comes from the dialog), the dialog exists and Close() has to be called. my suggestion was to remove the |dialog| argument here and do the following instead: * In the first case, where there is no dialog, do nothing special. Just call LogoutCurrentUser(). * In the second case, where the dialog wants to close itself and log out, the dialog would call two things: GetWidget()->Close(); LogoutCurrentUser();
hello, just fixed the remaining two issues. PTAL https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray_unittest.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:227: } On 2013/12/06 13:50:04, bartfab wrote: > On 2013/12/05 10:08:05, bjin wrote: > > I didn't follow you on this. > > > > On 2013/12/03 19:46:04, bartfab wrote: > > > Nit: Add runner_->FastForwardUntilNoTasksRemain() and verify that the the > > dialog > > > is not shown and the user is not logged out. > > > > At the end of the test, you verify that the confirmation dialog is not showing > *at this moment* and logout has not been called *at this moment*. But maybe the > dialog would show or a logout would happen a few moments later? My suggestion > was to add this: > > runner_->FastForwardUntilNoTasksRemain(); > > EXPECT_FALSE(tray_->IsConfirmationDialogShowing()); > EXPECT_TRUE(provider_->IsLogoutCalled()); > > This fast-forwards time once more, until absolutely all tasks that were ever > posted have been run. There are guaranteed to be zero tasks in the queue. If at > that point, no dialog is showing and no logout has happened, we know that the > user will not be logged out. Done. https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/450001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:45: dialog->GetWidget()->Close(); On 2013/12/06 13:50:04, bartfab wrote: > On 2013/12/05 10:08:05, bjin wrote: > > On 2013/12/03 19:46:04, bartfab wrote: > > > Nit: Can the dialog not just call GetWidget()->Close() itself? Then you > could > > > remove the |dialog| argument altogether. > > > > > > I know that in tests, you call DeleteDelegate() instead. However, that > simply > > > translates into delete dialog and can easily be done explicitly in tests > (best > > > with a scoped_ptr). You would then only have to make sure that either > > > GetWidget()->Close() is also valid to do in tests or make it conditional, > > e.g.: > > > > > > if (GetWidget()) > > > GetWidget()->close() > > > > > > This way, the code will not run in tests. > > > > I'm not sure I understand, it's the dialog it self I want ensure closed(then I > > can verify if the dialog is showing easily). > > > > I think the |dialog| argument is necessary since there is a situation to > logout > > without an existing LogoutConfirmationDialogView(ButtonPressed() with zero > > dialog duration preference). In this situation I can simply pass NULL to > > LogoutCurrentUser(). > > What I meant is this: The LogoutCurrentUser() call can come from two places. In > one place, there is no dialog and there is no need to call Close(). In the other > place (when the call comes from the dialog), the dialog exists and Close() has > to be called. my suggestion was to remove the |dialog| argument here and do the > following instead: > > * In the first case, where there is no dialog, do nothing special. Just call > LogoutCurrentUser(). > * In the second case, where the dialog wants to close itself and log out, the > dialog would call two things: > GetWidget()->Close(); > LogoutCurrentUser(); Done.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
I made another pass over all the files except the test for now. https://codereview.chromium.org/40053002/diff/500001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/40053002/diff/500001/ash/ash.gyp#newcode808 ash/ash.gyp:808: "system/logout_button/logout_button_tray_unittest.cc", 1/ Use single quotes ('), not double quotes ("). 2/ Move down 8 lines. This file should be in alphabetic order. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (left): https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:99: button_(NULL), Nit: Do not remove this. You will be setting |button_| a few lines down. But it should have a well-defined value until then as well. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:96: LogoutButtonTray::LogoutButtonTray(StatusAreaWidget* status_area_widget, Nit: Indentation: All arguments must be aligned. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:104: new LogoutConfirmationDialogView::LogoutConfirmationDelegate()); Nit: No need for (). https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:105: } else Nit 1: Style guide: If the if part has curly braces, the else part must have curly braces as well. Nit 2: Why not set confirmation_delegate_ in the initializer list, using a ternary operator, such as: confirmation_delegate_( delegate ? delegate : new LogoutConfirmationDialogView::LogoutConfirmationDelegate) https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:110: // For testing purpose. Nit: "For testing purpose" is slightly misleading. The AddLogoutButtonObserver() call is not for testing purposes only. It is the check that is necessary for tests. How about a comment like "The Shell may not exist in some unit tests." instead? https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:117: if (Shell::HasInstance()) Nit: Style guide: Multi-line conditionals require curly braces. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:132: confirmation_dialog_->GetWidget()->Close(); 1) After this method has run, the dialog is closed but |confirmation_dialog_| is still non-NULL. This means that IsConfirmationDialogShowing() will keep returning |true| (which is wrong) and EnsureConfirmationDialogIsShowing() would think that the dialog is still showing (which is wrong). Why not release the pointer here, i.e.: if (confirmation_dialog_ && Shell::HasInstance()) confirmation_dialog_->release()->GetWidget()->Close(); That way, |confirmation_dialog_| becomes NULL but the dialog is *not* actually deleted. The dialog now manages its own lifetime and will destroy itself eventually. This would also completely remove the need for the DeleteConfirmationDialog() method, for passing |this| to the dialog, for the weak_ptr_factory_... It would simplify the code a lot. 2) This method does nothing when Shell::HasInstance() is |false|. There is no widget to close but there still is a |confirmation_dialog_| that should be deleted. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:170: // Sign out immediately if kLogoutDialogDurationMs is non-positive. Nit: Do not refer to the pref name here. Ash knows nothing about Chrome prefs. Just say "if |duration_| is non-positive." https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:171: if (dialog_duration_ <= base::TimeDelta::FromSeconds(0)) Nit: The default constructor base::TimeDelta() produces a zero-length time delta for you. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:191: LogoutConfirmationDialogView *LogoutButtonTray::GetConfirmationDialogForTest() { Nit: Style guide: The * goes on the type. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:28: class LogoutConfirmationDialogTest; Nit: This is not needed. Stating that LogoutConfirmationDialogTest is a friend will automatically forward-declare it. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:36: friend class LogoutConfirmationDialogTest; Nit: The friend declaration should go to the start of the private: section, best followed by a blank line, i.e.: private: friend class LogoutConfirmationDialogTest; void UpdateVisibility(); [...] https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:37: LogoutButtonTray(StatusAreaWidget* status_area_widget, Nit: Indentation: All arguments must be aligned. In this case, you would want: LogoutButtonTray( StatusAreaWidget* status_area_widget, LogoutConfirmationDialogView::LogoutConfirmationDelegate* delegate); https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:38: LogoutConfirmationDialogView::LogoutConfirmationDelegate* delegate); 1) You are passing ownership of |delegate| here. Use a scoped_ptr. 2) Could you add a SetDelegateForTest() method instead? That way, there will be no need to pass a NULL in in the default code flow. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:43: bool IsConfirmationDialogShowing(); Nit 1: Getters usually go before setters. Thus, IsConfirmationDialogShowing() should be first. Nit 2: Make IsConfirmationDialogShowing() const. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:8: #include "ash/system/logout_button/logout_button_tray.h" Nit: Actually not needed once you remove |owner| and |owner_|. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:13: #include "grit/ui_strings.h" Nit: Where is this used? https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:21: #include "ui/views/layout/layout_constants.h" Nit: Where is this used? https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:26: const int kCountdownUpdateIntervalMs = 1000; // 1 second. Nit: Two spaces between inline comment. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:54: return base::TimeDelta::FromMilliseconds(kCountdownUpdateIntervalMs); Nit: You could use FromSeconds(1), removing the need for a constant. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:60: Nit: Remove blank line. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:63: views::Border::CreateEmptyBorder(0, kTrayPopupPaddingHorizontal, Nit: #include "ui/views/border.h" https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:76: void LogoutConfirmationDialogView::DeleteDelegate() { This is actually not needed. You could post a DeleteSoon for |this| instead. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:77: if (owner_) { Nit: No curly braces for single-line conditionals. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:108: // For testint purpose, only run the actually display the dialog if an Nit 1: s/testint/testing/. Nit 2: s/run the//. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:140: const base::TimeDelta time_remaining = countdown_start_time_ In its destructor, LogoutButtonTray calls confirmation_dialog_->GetWidget()->Close(). From that point on, the LogoutButtonTray and the LogoutConfirmationDialogView::LogoutConfirmationDelegate that it owns are gone. But the |timer_| here is still running. This may cause UpdateCountdown() to be called, which will try to access a |delegate_| that no longer exists. This class should react to OnClosed(). When the dialog is closed, the timer should stop and the |delegate_| pointer should be reset. All accesses to |delegate_| should check first whether it is not NULL so that even if one of these methods is called somehow after the dialog has been closed, there are no invalid accesses. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:143: // the count down display. Nit 1: s/count down/countdown/. Nit 2: Contrary to the comment, you use the rounded value for actual enforcement as well. Either update the enforcement or the comment. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:144: int seconds_remaining = (time_remaining Nit: Why not round(InSecondsF())? https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:11: #include "base/memory/weak_ptr.h" Nit: Actually not needed once you remove |owner| and |owner_|. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:23: class LogoutButtonTray; Nit: Actually not needed once you remove |owner| and |owner_|. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:24: class LogoutConfirmationDialogView; Nit: This is not needed. You are about to declare the class two lines down :). https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:30: Nit: No blank line needed here. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:46: LogoutConfirmationDialogView(base::WeakPtr<LogoutButtonTray> owner, Nit 1: As noted in my comment for LogoutButtonTray, the |owner| pointer is not necessary. Nit 2: Add a comment explaining the relationship between the lifetimes of |this| and |delegate|. Right now, the relationship is wrong (|this| may access |delegate| when it is no longer valid) but once you fix this, you can say that |delegate| must live until the dialog is closed. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:71: base::WeakPtr<LogoutButtonTray> owner_; Nit: As noted in my comment for LogoutButtonTray, the |owner_| pointer is not necessary. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:73: LogoutConfirmationDelegate* delegate_; // Not owned Nit 1: No need for the "Not owned" comment. I know I was using it a lot myself but I have since been convinced by others that it is a bad idea. Nit 2: Comments should end with a full stop. Nit 3: There should be two spaces before an inline comment. https://codereview.chromium.org/40053002/diff/500001/ash/system/tray/system_t... File ash/system/tray/system_tray_notifier.cc (right): https://codereview.chromium.org/40053002/diff/500001/ash/system/tray/system_t... ash/system/tray/system_tray_notifier.cc:6: #include "base/time/time.h" Nit 1: This is #included by the header file already. Nit 2: The first #include (the one for the header of the class you are implementing) should always be followed by a blank line. https://codereview.chromium.org/40053002/diff/500001/chrome/browser/chromeos/... File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right): https://codereview.chromium.org/40053002/diff/500001/chrome/browser/chromeos/... chrome/browser/chromeos/system/ash_system_tray_delegate.cc:974: int duration_ms = user_pref_registrar_->prefs()->GetInteger( Nit: const.
hello bartfab, I fixed most of issues you pointed out. For the lifetime problem I used the second approach that we discussed offline. PTAL Bin https://codereview.chromium.org/40053002/diff/500001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/40053002/diff/500001/ash/ash.gyp#newcode808 ash/ash.gyp:808: "system/logout_button/logout_button_tray_unittest.cc", On 2013/12/10 12:41:16, bartfab wrote: > 1/ Use single quotes ('), not double quotes ("). > 2/ Move down 8 lines. This file should be in alphabetic order. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (left): https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:99: button_(NULL), On 2013/12/10 12:41:16, bartfab wrote: > Nit: Do not remove this. You will be setting |button_| a few lines down. But it > should have a well-defined value until then as well. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:96: LogoutButtonTray::LogoutButtonTray(StatusAreaWidget* status_area_widget, On 2013/12/10 12:41:16, bartfab wrote: > Nit: Indentation: All arguments must be aligned. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:104: new LogoutConfirmationDialogView::LogoutConfirmationDelegate()); On 2013/12/10 12:41:16, bartfab wrote: > Nit: No need for (). Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:105: } else On 2013/12/10 12:41:16, bartfab wrote: > Nit 1: Style guide: If the if part has curly braces, the else part must have > curly braces as well. > > Nit 2: Why not set confirmation_delegate_ in the initializer list, using a > ternary operator, such as: > > confirmation_delegate_( > delegate ? delegate > : new LogoutConfirmationDialogView::LogoutConfirmationDelegate) Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:110: // For testing purpose. On 2013/12/10 12:41:16, bartfab wrote: > Nit: "For testing purpose" is slightly misleading. The AddLogoutButtonObserver() > call is not for testing purposes only. It is the check that is necessary for > tests. How about a comment like "The Shell may not exist in some unit tests." > instead? Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:117: if (Shell::HasInstance()) On 2013/12/10 12:41:16, bartfab wrote: > Nit: Style guide: Multi-line conditionals require curly braces. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:132: confirmation_dialog_->GetWidget()->Close(); On 2013/12/10 12:41:16, bartfab wrote: > 1) After this method has run, the dialog is closed but |confirmation_dialog_| is > still non-NULL. This means that IsConfirmationDialogShowing() will keep > returning |true| (which is wrong) and EnsureConfirmationDialogIsShowing() would > think that the dialog is still showing (which is wrong). > > Why not release the pointer here, i.e.: > > if (confirmation_dialog_ && Shell::HasInstance()) > confirmation_dialog_->release()->GetWidget()->Close(); > > That way, |confirmation_dialog_| becomes NULL but the dialog is *not* actually > deleted. The dialog now manages its own lifetime and will destroy itself > eventually. This would also completely remove the need for the > DeleteConfirmationDialog() method, for passing |this| to the dialog, for the > weak_ptr_factory_... It would simplify the code a lot. > > 2) This method does nothing when Shell::HasInstance() is |false|. There is no > widget to close but there still is a |confirmation_dialog_| that should be > deleted. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:170: // Sign out immediately if kLogoutDialogDurationMs is non-positive. On 2013/12/10 12:41:16, bartfab wrote: > Nit: Do not refer to the pref name here. Ash knows nothing about Chrome prefs. > Just say "if |duration_| is non-positive." Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:171: if (dialog_duration_ <= base::TimeDelta::FromSeconds(0)) On 2013/12/10 12:41:16, bartfab wrote: > Nit: The default constructor base::TimeDelta() produces a zero-length time delta > for you. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:191: LogoutConfirmationDialogView *LogoutButtonTray::GetConfirmationDialogForTest() { On 2013/12/10 12:41:16, bartfab wrote: > Nit: Style guide: The * goes on the type. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:28: class LogoutConfirmationDialogTest; On 2013/12/10 12:41:16, bartfab wrote: > Nit: This is not needed. Stating that LogoutConfirmationDialogTest is a friend > will automatically forward-declare it. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:36: friend class LogoutConfirmationDialogTest; On 2013/12/10 12:41:16, bartfab wrote: > Nit: The friend declaration should go to the start of the private: section, best > followed by a blank line, i.e.: > > private: > friend class LogoutConfirmationDialogTest; > > void UpdateVisibility(); > [...] Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:37: LogoutButtonTray(StatusAreaWidget* status_area_widget, On 2013/12/10 12:41:16, bartfab wrote: > Nit: Indentation: All arguments must be aligned. In this case, you would want: > > LogoutButtonTray( > StatusAreaWidget* status_area_widget, > LogoutConfirmationDialogView::LogoutConfirmationDelegate* delegate); Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:38: LogoutConfirmationDialogView::LogoutConfirmationDelegate* delegate); On 2013/12/10 12:41:16, bartfab wrote: > 1) You are passing ownership of |delegate| here. Use a scoped_ptr. > 2) Could you add a SetDelegateForTest() method instead? That way, there will be > no need to pass a NULL in in the default code flow. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:43: bool IsConfirmationDialogShowing(); On 2013/12/10 12:41:16, bartfab wrote: > Nit 1: Getters usually go before setters. Thus, IsConfirmationDialogShowing() > should be first. > > Nit 2: Make IsConfirmationDialogShowing() const. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:13: #include "grit/ui_strings.h" On 2013/12/10 12:41:16, bartfab wrote: > Nit: Where is this used? Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:21: #include "ui/views/layout/layout_constants.h" On 2013/12/10 12:41:16, bartfab wrote: > Nit: Where is this used? Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:26: const int kCountdownUpdateIntervalMs = 1000; // 1 second. On 2013/12/10 12:41:16, bartfab wrote: > Nit: Two spaces between inline comment. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:54: return base::TimeDelta::FromMilliseconds(kCountdownUpdateIntervalMs); On 2013/12/10 12:41:16, bartfab wrote: > Nit: You could use FromSeconds(1), removing the need for a constant. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:60: On 2013/12/10 12:41:16, bartfab wrote: > Nit: Remove blank line. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:63: views::Border::CreateEmptyBorder(0, kTrayPopupPaddingHorizontal, On 2013/12/10 12:41:16, bartfab wrote: > Nit: #include "ui/views/border.h" Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:77: if (owner_) { On 2013/12/10 12:41:16, bartfab wrote: > Nit: No curly braces for single-line conditionals. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:108: // For testint purpose, only run the actually display the dialog if an On 2013/12/10 12:41:16, bartfab wrote: > Nit 1: s/testint/testing/. > Nit 2: s/run the//. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:140: const base::TimeDelta time_remaining = countdown_start_time_ On 2013/12/10 12:41:16, bartfab wrote: > In its destructor, LogoutButtonTray calls > confirmation_dialog_->GetWidget()->Close(). From that point on, the > LogoutButtonTray and the > LogoutConfirmationDialogView::LogoutConfirmationDelegate that it owns are gone. > But the |timer_| here is still running. This may cause UpdateCountdown() to be > called, which will try to access a |delegate_| that no longer exists. > > This class should react to OnClosed(). When the dialog is closed, the timer > should stop and the |delegate_| pointer should be reset. All accesses to > |delegate_| should check first whether it is not NULL so that even if one of > these methods is called somehow after the dialog has been closed, there are no > invalid accesses. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:143: // the count down display. On 2013/12/10 12:41:16, bartfab wrote: > Nit 1: s/count down/countdown/. > Nit 2: Contrary to the comment, you use the rounded value for actual enforcement > as well. Either update the enforcement or the comment. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:144: int seconds_remaining = (time_remaining On 2013/12/10 12:41:16, bartfab wrote: > Nit: Why not round(InSecondsF())? Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:11: #include "base/memory/weak_ptr.h" On 2013/12/10 12:41:16, bartfab wrote: > Nit: Actually not needed once you remove |owner| and |owner_|. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:24: class LogoutConfirmationDialogView; On 2013/12/10 12:41:16, bartfab wrote: > Nit: This is not needed. You are about to declare the class two lines down :). Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:30: On 2013/12/10 12:41:16, bartfab wrote: > Nit: No blank line needed here. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:73: LogoutConfirmationDelegate* delegate_; // Not owned On 2013/12/10 12:41:16, bartfab wrote: > Nit 1: No need for the "Not owned" comment. I know I was using it a lot myself > but I have since been convinced by others that it is a bad idea. > Nit 2: Comments should end with a full stop. > Nit 3: There should be two spaces before an inline comment. Done. https://codereview.chromium.org/40053002/diff/500001/ash/system/tray/system_t... File ash/system/tray/system_tray_notifier.cc (right): https://codereview.chromium.org/40053002/diff/500001/ash/system/tray/system_t... ash/system/tray/system_tray_notifier.cc:6: #include "base/time/time.h" On 2013/12/10 12:41:16, bartfab wrote: > Nit 1: This is #included by the header file already. > > Nit 2: The first #include (the one for the header of the class you are > implementing) should always be followed by a blank line. Done. https://codereview.chromium.org/40053002/diff/500001/chrome/browser/chromeos/... File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right): https://codereview.chromium.org/40053002/diff/500001/chrome/browser/chromeos/... chrome/browser/chromeos/system/ash_system_tray_delegate.cc:974: int duration_ms = user_pref_registrar_->prefs()->GetInteger( On 2013/12/10 12:41:16, bartfab wrote: > Nit: const. Done.
hello, just send another CL without using weak pointer for owner in LogoutConfirmationDialogView. PTAL Bin
https://codereview.chromium.org/40053002/diff/520001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/520001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:28: // This class provide setting and interface for LogoutConfirmationDialogView. Nit 1: s/provide/provides/ Nit 2: s/setting and interface/a setting and an interface/ Nit 3: I think this entire comment is unnecessary. It is clear what Delegates are for. https://codereview.chromium.org/40053002/diff/520001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:36: virtual base::TimeTicks GetCurrentTime(); Nit: const. https://codereview.chromium.org/40053002/diff/520001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:37: virtual base::TimeDelta GetUpdateInterval(); Nit: const. https://codereview.chromium.org/40053002/diff/520001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:56: void NullifyOwner(); This method is unnecessary. The *only* code path that reaches it is: LogoutConfirmationDialogView::OnClosed() owner_->DeleteConfirmationDialog() LogoutConfirmationDialogView::NullifyOwner() which sets: owner_ = NULL You could simplify this and remove the entirey NullifyOwner() by having OnClosed() set owner_ = NULL.
https://codereview.chromium.org/40053002/diff/520001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/520001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:28: // This class provide setting and interface for LogoutConfirmationDialogView. On 2013/12/12 15:16:40, bartfab wrote: > Nit 1: s/provide/provides/ > Nit 2: s/setting and interface/a setting and an interface/ > Nit 3: I think this entire comment is unnecessary. It is clear what Delegates > are for. Done. https://codereview.chromium.org/40053002/diff/520001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:36: virtual base::TimeTicks GetCurrentTime(); On 2013/12/12 15:16:40, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/40053002/diff/520001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:37: virtual base::TimeDelta GetUpdateInterval(); On 2013/12/12 15:16:40, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/40053002/diff/520001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:56: void NullifyOwner(); On 2013/12/12 15:16:40, bartfab wrote: > This method is unnecessary. The *only* code path that reaches it is: > > LogoutConfirmationDialogView::OnClosed() > owner_->DeleteConfirmationDialog() > LogoutConfirmationDialogView::NullifyOwner() > which sets: owner_ = NULL > > You could simplify this and remove the entirey NullifyOwner() by having > OnClosed() set owner_ = NULL. > Done.
https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:135: confirmation_dialog_->DeleteDelegate(); Would it be possible to call confirmation_dialog_->Close() here instead? It seems cleaner to tell the dialog to do an orderly close rather than forcing it to delete immediately. https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:58: void DeleteConfirmationDialog(); The name is misleading. This method does not delete the dialog. It releases the scoped_ptr. The method also gets called *before* the dialog has been deleted, so anything with "Delete" in the name would be misleading. How about ReleaseConfirmationDialog()? https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:68: OnClosed(); As discussed offline, it is sufficient to just do this instead: if (owner_) owner_->DeleteConfirmationDialog(); Plus, add a comment explaining that this will be triggered in tests only. https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:93: if (owner_) { How can the owner_ ever be NULL? https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:97: // nullify the delegate to prevent future activities of the dialog. Nit: s/nullify/Nullify/ https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:138: delegate->LogoutCurrentUser(); Why did you choose this ordering? If you called LogoutCurrentUser() first, you would not need to store the |delegate_| in a temporary. https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:141: DeleteDelegate(); Could you call Close() here instead? https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:149: + duration_ - delegate_->GetCurrentTime(); Nit: The style guide says the + should go on the previous line. https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:162: LogoutCurrentUser(); At this point, the dialog is still open and the delegate_ is non-NULL. Could the user click one of the buttons, causing unexpected behavior? https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:53: void UpdateDialogDuration(base::TimeDelta duration); Nit: We usually separate unrelated functions like these two by a blank line.
hello, Here is the fixes, I left explaining comments instead for some comments you left in logout_confirmation_dialog_view.cc. PTAL bin https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:135: confirmation_dialog_->DeleteDelegate(); On 2013/12/12 16:04:13, bartfab wrote: > Would it be possible to call confirmation_dialog_->Close() here instead? It > seems cleaner to tell the dialog to do an orderly close rather than forcing it > to delete immediately. Done. https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:58: void DeleteConfirmationDialog(); On 2013/12/12 16:04:13, bartfab wrote: > The name is misleading. This method does not delete the dialog. It releases the > scoped_ptr. The method also gets called *before* the dialog has been deleted, so > anything with "Delete" in the name would be misleading. How about > ReleaseConfirmationDialog()? Done. https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:68: OnClosed(); On 2013/12/12 16:04:13, bartfab wrote: > As discussed offline, it is sufficient to just do this instead: > > if (owner_) > owner_->DeleteConfirmationDialog(); > > Plus, add a comment explaining that this will be triggered in tests only. Done. https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:93: if (owner_) { On 2013/12/12 16:04:13, bartfab wrote: > How can the owner_ ever be NULL? I'm not sure the reason, but OnClosed() is actually called twice during session logout. And I believe the times OnClosed() is called is kind of undefined, so I added this condition check. https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:97: // nullify the delegate to prevent future activities of the dialog. On 2013/12/12 16:04:13, bartfab wrote: > Nit: s/nullify/Nullify/ Done. https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:138: delegate->LogoutCurrentUser(); On 2013/12/12 16:04:13, bartfab wrote: > Why did you choose this ordering? If you called LogoutCurrentUser() first, you > would not need to store the |delegate_| in a temporary. I choose this ordering because I think the session logout might be complicated and kind of undefined in real browser environment, so I want to make sure the dialog is closed properly before actually try to log out the session. And I think a temporary pointer with comments didn't make the code much more complicated and hard to follow. https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:141: DeleteDelegate(); On 2013/12/12 16:04:13, bartfab wrote: > Could you call Close() here instead? I think the default implementation of DialogDelegate::Close() didn't actually do clean up work, and in tests that's the only necessary task. https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:149: + duration_ - delegate_->GetCurrentTime(); On 2013/12/12 16:04:13, bartfab wrote: > Nit: The style guide says the + should go on the previous line. Done. https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:162: LogoutCurrentUser(); The LogoutCurrentUser() here is LogoutConfirmationDialogView::LogoutCurrentUser, instead of the LogoutCurrentUser() from Delegate. The |delegate_| is nullified and dialog is closed here. https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:53: void UpdateDialogDuration(base::TimeDelta duration); On 2013/12/12 16:04:13, bartfab wrote: > Nit: We usually separate unrelated functions like these two by a blank line. Done.
This is very close to done now. I have three actual comments. Everything else is just nits. https://chromiumcodereview.appspot.com/40053002/diff/530001/ash/system/logout... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://chromiumcodereview.appspot.com/40053002/diff/530001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.cc:93: if (owner_) { On 2013/12/17 10:29:49, bjin wrote: > On 2013/12/12 16:04:13, bartfab wrote: > > How can the owner_ ever be NULL? > > I'm not sure the reason, but OnClosed() is actually called twice during session > logout. And I believe the times OnClosed() is called is kind of undefined, so I > added this condition check. This is a bit worrying. It would seem that OnClosed() should be called exactly once, when the dialog is getting closed. Could you quickly check in a debugger or print out a stack trace whenever OnClosed() is called to see why it gets called twice during logout (Philipp always remembers by heart how to print a stack trace, you can check with him for the right code snippet)? https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... File ash/system/logout_button/logout_button_tray.cc (right): https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray.cc:128: this, confirmation_delegate_.get())); Nit: Indentation should be 4 characters from previous line, not 6. https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... File ash/system/logout_button/logout_button_tray.h (right): https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray.h:34: LogoutButtonTray(StatusAreaWidget* status_area_widget); Nit: Style guide: Single-argument constructors must be marked |explicit|. https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... File ash/system/logout_button/logout_button_tray_unittest.cc (right): https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:7: #include "ash/system/logout_button/logout_button_observer.h" Nit: Already included by logout_button_tray.h. https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:8: #include "ash/system/logout_button/logout_confirmation_dialog_view.h" Nit: Already included by logout_button_tray.h. https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:9: #include "base/memory/scoped_ptr.h" Nit: Already included by logout_button_tray.h. https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:12: #include "base/time/time.h" Nit: Already included by logout_button_tray.h. https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:14: Nit: The unittest file should have includes for all classes that have been forward-declared in the header. In this case, the missing includes are: #include "ash/system/status_area_widget.h" #include "ui/views/controls/button/label_button.h" https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:36: void FastForwardBy(int64 milliseconds); Nit: I know you inherited this from my code in its current form. Could you improve it by changing the argument to a base::TimeDelta though? https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:51: std::priority_queue<std::pair<base::TimeTicks, base::Closure>, Nit 1: #include <queue> Nit 2: #include <util> Nit 3: #include "base/callback.h" (for base::Closure) https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:52: std::vector<std::pair<base::TimeTicks, base::Closure> >, Nit: #include <vector> https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:60: MockTimeSingleThreadTaskRunner* runner); Why not use a scoped_refptr to ensure the |runner| outlives |this|? https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:95: NOTREACHED(); Nit: #include "base/logging.h" https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:165: scoped_refptr<MockTimeSingleThreadTaskRunner> runner_; Nit: #include "base/memory/ref_counted.h" https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:179: ui::TranslatedKeyEvent faked_event(false, Nit 1: #include "ui/events/event.h" Nit 2: const https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:180: static_cast<ui::KeyboardCode>(0), 0); Nit: #include "ui/events/keycodes/keyboard_codes.h" https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:191: delegate_ = static_cast<MockLogoutConfirmationDelegate*>( Why is this necessary? The |delegate_| you get back here is the same one you set a couple of lines above. If you remove this, you can remove the GetConfirmationDelegateForTest() method as well. https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:195: void LogoutConfirmationDialogTest::TearDown() { Nit: If your TearDown() is empty, there is no need to override it. https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:240: EXPECT_TRUE(delegate_->WasLogoutCalled()); Nit: This is a no-op because WasLogoutCalled() returns a value that is only ever set and never cleared. So if delegate_->WasLogoutCalled() was true above, it is guaranteed to be true here again. https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_button_tray_unittest.cc:294: logout_button_->GetConfirmationDialogForTest()->Accept(); Nit: This is the only use of GetConfirmationDialogForTest() in the entire CL. Since the method is used so rarely, how about you access logout_button_->confirmation_dialog_.get() directly instead (in a helper method provided by the LogoutConfirmationDialogTest class of course, because only this one is a friend of LogoutButtonTray)? https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.cc:139: // For testing purpose. Nit: Could you rephrase the comment so that it is clear which of the two branches exists for test purposes only, and why? https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://chromiumcodereview.appspot.com/40053002/diff/550001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.h:35: virtual base::TimeDelta GetUpdateInterval() const; Nit: AFAICT, this is never overridden. Hence, it can be removed from the Delegate.
hello bartfab, just send out another CL, PTAL binjin https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:93: if (owner_) { On 2013/12/17 13:21:03, bartfab wrote: > On 2013/12/17 10:29:49, bjin wrote: > > On 2013/12/12 16:04:13, bartfab wrote: > > > How can the owner_ ever be NULL? > > > > I'm not sure the reason, but OnClosed() is actually called twice during > session > > logout. And I believe the times OnClosed() is called is kind of undefined, so > I > > added this condition check. > > This is a bit worrying. It would seem that OnClosed() should be called exactly > once, when the dialog is getting closed. Could you quickly check in a debugger > or print out a stack trace whenever OnClosed() is called to see why it gets > called twice during logout (Philipp always remembers by heart how to print a > stack trace, you can check with him for the right code snippet)? I checked the stack trace for both calls. here is the different part: [0x7fe15aeef893] ash::internal::LogoutConfirmationDialogView::OnClosed() [0x7fe158c69060] views::DialogClientView::CanClose() [0x7fe158c6ca19] views::NonClientView::CanClose() [0x7fe158c5d8f7] views::Widget::Close() [0x7fe15aeefb3f] ash::internal::LogoutConfirmationDialogView::LogoutCurrentUser() [0x7fe15aeef7de] ash::internal::LogoutConfirmationDialogView::Accept() [0x7fe158c6ab86] views::DialogDelegate::Accept() [0x7fe158c68c63] views::DialogClientView::AcceptWindow() [0x7fe158c6a00a] views::DialogClientView::ButtonPressed() vs. [0x7fe15aeef893] ash::internal::LogoutConfirmationDialogView::OnClosed() [0x7fe158c6a8b4] views::DialogClientView::Close() [0x7fe158c68c8e] views::DialogClientView::AcceptWindow() [0x7fe158c6a00a] views::DialogClientView::ButtonPressed() So it turns out it's unnecessary to close the dialog in LogoutConfirmationDialogView::LogoutCurrentUser() for real browser environment after user clicked Yes button and Accept() was called, but it's necessary for tests with my current approach. I think it's okay and I prefer to just left everything unchanged here. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:128: this, confirmation_delegate_.get())); On 2013/12/17 13:21:03, bartfab wrote: > Nit: Indentation should be 4 characters from previous line, not 6. Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:34: LogoutButtonTray(StatusAreaWidget* status_area_widget); On 2013/12/17 13:21:03, bartfab wrote: > Nit: Style guide: Single-argument constructors must be marked |explicit|. Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray_unittest.cc (right): https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:7: #include "ash/system/logout_button/logout_button_observer.h" On 2013/12/17 13:21:03, bartfab wrote: > Nit: Already included by logout_button_tray.h. Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:8: #include "ash/system/logout_button/logout_confirmation_dialog_view.h" On 2013/12/17 13:21:03, bartfab wrote: > Nit: Already included by logout_button_tray.h. Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:9: #include "base/memory/scoped_ptr.h" On 2013/12/17 13:21:03, bartfab wrote: > Nit: Already included by logout_button_tray.h. Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:12: #include "base/time/time.h" On 2013/12/17 13:21:03, bartfab wrote: > Nit: Already included by logout_button_tray.h. Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:14: On 2013/12/17 13:21:03, bartfab wrote: > Nit: The unittest file should have includes for all classes that have been > forward-declared in the header. In this case, the missing includes are: > > #include "ash/system/status_area_widget.h" > #include "ui/views/controls/button/label_button.h" Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:36: void FastForwardBy(int64 milliseconds); On 2013/12/17 13:21:03, bartfab wrote: > Nit: I know you inherited this from my code in its current form. Could you > improve it by changing the argument to a base::TimeDelta though? Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:51: std::priority_queue<std::pair<base::TimeTicks, base::Closure>, On 2013/12/17 13:21:03, bartfab wrote: > Nit 1: #include <queue> > Nit 2: #include <util> > Nit 3: #include "base/callback.h" (for base::Closure) Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:52: std::vector<std::pair<base::TimeTicks, base::Closure> >, On 2013/12/17 13:21:03, bartfab wrote: > Nit: #include <vector> Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:60: MockTimeSingleThreadTaskRunner* runner); On 2013/12/17 13:21:03, bartfab wrote: > Why not use a scoped_refptr to ensure the |runner| outlives |this|? Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:95: NOTREACHED(); On 2013/12/17 13:21:03, bartfab wrote: > Nit: #include "base/logging.h" Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:165: scoped_refptr<MockTimeSingleThreadTaskRunner> runner_; On 2013/12/17 13:21:03, bartfab wrote: > Nit: #include "base/memory/ref_counted.h" Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:179: ui::TranslatedKeyEvent faked_event(false, On 2013/12/17 13:21:03, bartfab wrote: > Nit 1: #include "ui/events/event.h" > Nit 2: const Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:180: static_cast<ui::KeyboardCode>(0), 0); On 2013/12/17 13:21:03, bartfab wrote: > Nit: #include "ui/events/keycodes/keyboard_codes.h" Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:191: delegate_ = static_cast<MockLogoutConfirmationDelegate*>( On 2013/12/17 13:21:03, bartfab wrote: > Why is this necessary? The |delegate_| you get back here is the same one you set > a couple of lines above. If you remove this, you can remove the > GetConfirmationDelegateForTest() method as well. Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:195: void LogoutConfirmationDialogTest::TearDown() { On 2013/12/17 13:21:03, bartfab wrote: > Nit: If your TearDown() is empty, there is no need to override it. Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:240: EXPECT_TRUE(delegate_->WasLogoutCalled()); On 2013/12/17 13:21:03, bartfab wrote: > Nit: This is a no-op because WasLogoutCalled() returns a value that is only ever > set and never cleared. So if delegate_->WasLogoutCalled() was true above, it is > guaranteed to be true here again. Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:294: logout_button_->GetConfirmationDialogForTest()->Accept(); On 2013/12/17 13:21:03, bartfab wrote: > Nit: This is the only use of GetConfirmationDialogForTest() in the entire CL. > Since the method is used so rarely, how about you access > logout_button_->confirmation_dialog_.get() directly instead (in a helper method > provided by the LogoutConfirmationDialogTest class of course, because only this > one is a friend of LogoutButtonTray)? Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:139: // For testing purpose. On 2013/12/17 13:21:03, bartfab wrote: > Nit: Could you rephrase the comment so that it is clear which of the two > branches exists for test purposes only, and why? Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:35: virtual base::TimeDelta GetUpdateInterval() const; On 2013/12/17 13:21:03, bartfab wrote: > Nit: AFAICT, this is never overridden. Hence, it can be removed from the > Delegate. I prefer to keep GetUpdateInterval() in the delegate, otherwise I had to hard code the update interval, which looks a bit more ugly to me.
https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:93: if (owner_) { On 2013/12/17 14:58:08, bjin wrote: > On 2013/12/17 13:21:03, bartfab wrote: > > On 2013/12/17 10:29:49, bjin wrote: > > > On 2013/12/12 16:04:13, bartfab wrote: > > > > How can the owner_ ever be NULL? > > > > > > I'm not sure the reason, but OnClosed() is actually called twice during > > session > > > logout. And I believe the times OnClosed() is called is kind of undefined, > so > > I > > > added this condition check. > > > > This is a bit worrying. It would seem that OnClosed() should be called exactly > > once, when the dialog is getting closed. Could you quickly check in a debugger > > or print out a stack trace whenever OnClosed() is called to see why it gets > > called twice during logout (Philipp always remembers by heart how to print a > > stack trace, you can check with him for the right code snippet)? > > I checked the stack trace for both calls. here is the different part: > > [0x7fe15aeef893] ash::internal::LogoutConfirmationDialogView::OnClosed() > [0x7fe158c69060] views::DialogClientView::CanClose() > [0x7fe158c6ca19] views::NonClientView::CanClose() > [0x7fe158c5d8f7] views::Widget::Close() > [0x7fe15aeefb3f] > ash::internal::LogoutConfirmationDialogView::LogoutCurrentUser() > [0x7fe15aeef7de] ash::internal::LogoutConfirmationDialogView::Accept() > [0x7fe158c6ab86] views::DialogDelegate::Accept() > [0x7fe158c68c63] views::DialogClientView::AcceptWindow() > [0x7fe158c6a00a] views::DialogClientView::ButtonPressed() > vs. > [0x7fe15aeef893] ash::internal::LogoutConfirmationDialogView::OnClosed() > [0x7fe158c6a8b4] views::DialogClientView::Close() > [0x7fe158c68c8e] views::DialogClientView::AcceptWindow() > [0x7fe158c6a00a] views::DialogClientView::ButtonPressed() > > So it turns out it's unnecessary to close the dialog in > LogoutConfirmationDialogView::LogoutCurrentUser() for real browser environment > after user clicked Yes button and Accept() was called, but it's necessary for > tests with my current approach. I think it's okay and I prefer to just left > everything unchanged here. I think allowing OnClosed() to be called twice is setting up a dangerous situation that someone later may run into - it is unexpected behavior that your code handles but future code may not deal with properly. It would be best to change it so that OnClosed() is only called once in the production code path. If this means that OnClosed() is never called in testing code when a click on "OK" is simulated, that's actually OK: The core functionality we want to test is that when the user clicks "OK," a logout is initiated. Whether the dialog gets closed or not does not really matter. Once the delegate has been reset, the dialog does nothing anyway. And even if it persists on the screen for a split second longer, it will be torn down together with the browser in a moment as the logout proceeds. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:35: virtual base::TimeDelta GetUpdateInterval() const; On 2013/12/17 14:58:08, bjin wrote: > On 2013/12/17 13:21:03, bartfab wrote: > > Nit: AFAICT, this is never overridden. Hence, it can be removed from the > > Delegate. > > I prefer to keep GetUpdateInterval() in the delegate, otherwise I had to hard > code the update interval, which looks a bit more ugly to me. Remember that each method you add to the Delegate interface will increase the binary size and the runtime of the resulting Chrome binary - ever so slightly, but it will. You cannot avoid putting a constant somewhere. I see no good reason to put it in the delegate. You can move the constant away from the code though by putting it a the top of logout_confirmation_dialog_view.cc, in an anonymous namespace, e.g.: namespace { const int kCountdownUpdateIntervalMs = 1000; // 1 second. } // namespace
I made some changes, PTAL bin https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/530001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:93: if (owner_) { On 2013/12/17 15:57:46, bartfab wrote: > On 2013/12/17 14:58:08, bjin wrote: > > On 2013/12/17 13:21:03, bartfab wrote: > > > On 2013/12/17 10:29:49, bjin wrote: > > > > On 2013/12/12 16:04:13, bartfab wrote: > > > > > How can the owner_ ever be NULL? > > > > > > > > I'm not sure the reason, but OnClosed() is actually called twice during > > > session > > > > logout. And I believe the times OnClosed() is called is kind of undefined, > > so > > > I > > > > added this condition check. > > > > > > This is a bit worrying. It would seem that OnClosed() should be called > exactly > > > once, when the dialog is getting closed. Could you quickly check in a > debugger > > > or print out a stack trace whenever OnClosed() is called to see why it gets > > > called twice during logout (Philipp always remembers by heart how to print a > > > stack trace, you can check with him for the right code snippet)? > > > > I checked the stack trace for both calls. here is the different part: > > > > [0x7fe15aeef893] ash::internal::LogoutConfirmationDialogView::OnClosed() > > [0x7fe158c69060] views::DialogClientView::CanClose() > > [0x7fe158c6ca19] views::NonClientView::CanClose() > > [0x7fe158c5d8f7] views::Widget::Close() > > [0x7fe15aeefb3f] > > ash::internal::LogoutConfirmationDialogView::LogoutCurrentUser() > > [0x7fe15aeef7de] ash::internal::LogoutConfirmationDialogView::Accept() > > [0x7fe158c6ab86] views::DialogDelegate::Accept() > > [0x7fe158c68c63] views::DialogClientView::AcceptWindow() > > [0x7fe158c6a00a] views::DialogClientView::ButtonPressed() > > vs. > > [0x7fe15aeef893] ash::internal::LogoutConfirmationDialogView::OnClosed() > > [0x7fe158c6a8b4] views::DialogClientView::Close() > > [0x7fe158c68c8e] views::DialogClientView::AcceptWindow() > > [0x7fe158c6a00a] views::DialogClientView::ButtonPressed() > > > > So it turns out it's unnecessary to close the dialog in > > LogoutConfirmationDialogView::LogoutCurrentUser() for real browser environment > > after user clicked Yes button and Accept() was called, but it's necessary for > > tests with my current approach. I think it's okay and I prefer to just left > > everything unchanged here. > > I think allowing OnClosed() to be called twice is setting up a dangerous > situation that someone later may run into - it is unexpected behavior that your > code handles but future code may not deal with properly. It would be best to > change it so that OnClosed() is only called once in the production code path. If > this means that OnClosed() is never called in testing code when a click on "OK" > is simulated, that's actually OK: The core functionality we want to test is that > when the user clicks "OK," a logout is initiated. Whether the dialog gets closed > or not does not really matter. Once the delegate has been reset, the dialog does > nothing anyway. And even if it persists on the screen for a split second longer, > it will be torn down together with the browser in a moment as the logout > proceeds. Done. https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/550001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:35: virtual base::TimeDelta GetUpdateInterval() const; On 2013/12/17 15:57:46, bartfab wrote: > On 2013/12/17 14:58:08, bjin wrote: > > On 2013/12/17 13:21:03, bartfab wrote: > > > Nit: AFAICT, this is never overridden. Hence, it can be removed from the > > > Delegate. > > > > I prefer to keep GetUpdateInterval() in the delegate, otherwise I had to hard > > code the update interval, which looks a bit more ugly to me. > > Remember that each method you add to the Delegate interface will increase the > binary size and the runtime of the resulting Chrome binary - ever so slightly, > but it will. You cannot avoid putting a constant somewhere. I see no good reason > to put it in the delegate. You can move the constant away from the code though > by putting it a the top of logout_confirmation_dialog_view.cc, in an anonymous > namespace, e.g.: > > namespace { > const int kCountdownUpdateIntervalMs = 1000; // 1 second. > } // namespace Done.
https://chromiumcodereview.appspot.com/40053002/diff/590001/ash/system/logout... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://chromiumcodereview.appspot.com/40053002/diff/590001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.cc:29: const int kCountdownUpdateIntervalMs = 1000; // 1 second. Nit: Style guide: Constants like this one should not be indented. https://chromiumcodereview.appspot.com/40053002/diff/590001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.cc:39: DCHECK(Shell::GetInstance() != NULL); Nit 1: No need for != NULL. DCHECK(Shell::GetInstance()) will do. Nit 2: DCHECK(Shell::HasInstance()) may be more elegant. Nit 3: #include "base/logging.h" Nit 4: There is no real need for this DCHECK. In production code, the shell always exists. https://chromiumcodereview.appspot.com/40053002/diff/590001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.cc:86: DCHECK(owner_ != NULL); Nit 1: No need for != NULL. DCHECK(owner_) will do. Nit 2: #include "base/logging.h" Nit 3: There is no real need for this DCHECK. If owner_ really is NULL, we will crash a line later anyway. https://chromiumcodereview.appspot.com/40053002/diff/590001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.cc:117: base::TimeDelta::FromMicroseconds(kCountdownUpdateIntervalMs), FromMilliseconds, not FromMicroseconds. https://chromiumcodereview.appspot.com/40053002/diff/590001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.cc:133: if (!ash::Shell::HasInstance()) This should be done in the testing code (in the MockDelegate), not inside the production class. https://chromiumcodereview.appspot.com/40053002/diff/590001/ash/system/logout... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://chromiumcodereview.appspot.com/40053002/diff/590001/ash/system/logout... ash/system/logout_button/logout_confirmation_dialog_view.h:49: virtual void DeleteDelegate() OVERRIDE; Nit: DeleteDelegate() was the first override before, now it is the last. Please make sure that the overrides are in the same order as DialogDelegateView declares them.
https://codereview.chromium.org/40053002/diff/590001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/590001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:29: const int kCountdownUpdateIntervalMs = 1000; // 1 second. On 2013/12/18 18:26:49, bartfab wrote: > Nit: Style guide: Constants like this one should not be indented. Done. https://codereview.chromium.org/40053002/diff/590001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:39: DCHECK(Shell::GetInstance() != NULL); On 2013/12/18 18:26:49, bartfab wrote: > Nit 1: No need for != NULL. DCHECK(Shell::GetInstance()) will do. > Nit 2: DCHECK(Shell::HasInstance()) may be more elegant. > Nit 3: #include "base/logging.h" > Nit 4: There is no real need for this DCHECK. In production code, the shell > always exists. Done. https://codereview.chromium.org/40053002/diff/590001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:86: DCHECK(owner_ != NULL); On 2013/12/18 18:26:49, bartfab wrote: > Nit 1: No need for != NULL. DCHECK(owner_) will do. > Nit 2: #include "base/logging.h" > Nit 3: There is no real need for this DCHECK. If owner_ really is NULL, we will > crash a line later anyway. Done. https://codereview.chromium.org/40053002/diff/590001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:117: base::TimeDelta::FromMicroseconds(kCountdownUpdateIntervalMs), On 2013/12/18 18:26:49, bartfab wrote: > FromMilliseconds, not FromMicroseconds. Done. https://codereview.chromium.org/40053002/diff/590001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:133: if (!ash::Shell::HasInstance()) On 2013/12/18 18:26:49, bartfab wrote: > This should be done in the testing code (in the MockDelegate), not inside the > production class. Done. https://codereview.chromium.org/40053002/diff/590001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/590001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:49: virtual void DeleteDelegate() OVERRIDE; On 2013/12/18 18:26:49, bartfab wrote: > Nit: DeleteDelegate() was the first override before, now it is the last. Please > make sure that the overrides are in the same order as DialogDelegateView > declares them. Done.
lgtm
Hi stevenjb, please review ash/* and chrome/browser/chromeos/* Hi derat, please review chrome/browser/ui/ash/* and chrome/common/* Thanks Bin
lgtm for chrome/ (not sure if i'm an owner for all of these directories, though) https://codereview.chromium.org/40053002/diff/630001/chrome/browser/ui/ash/ch... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/40053002/diff/630001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_launcher_prefs.cc:75: registry->RegisterIntegerPref( nit: not your fault, but mind alphabetizing these by pref name? the order seems arbitrary right now. https://codereview.chromium.org/40053002/diff/630001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/40053002/diff/630001/chrome/common/pref_names... chrome/common/pref_names.h:855: extern const char kLogoutDialogDurationMs[]; nit: mind alphabetizing the prefs in this section too? (and also in the .cc file)
https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:39: void EnsureConfirmationDialogIsClosed(); Do these need to be public? https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:58: void ReleaseConfirmationDialog(); Comment this. (UpdateAfterLoginStatusChange should really be commented also). https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray_unittest.cc (right): https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:63: }; It looks like this was copied from session_length_limiter? It seems like something that would be useful to extract out into its own class in base/test (e.g. see TestSimpleTaskRunner), in a separate CL. If this CL is time critical, add a TODO and create/reference an issue to do this. If you keep this class here for this CL, put it in an anonymous namespace. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:76: bool WasLogoutCalled() const; bool logout_called() const { return logout_called_; } https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:104: MockLogoutConfirmationDelegate* delegate_; nit: DISALLOW... https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:165: LogoutConfirmationDialogTest* tester) : runner_(runner), : on new line https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:167: logout_called_ = false; Set in initializer list https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:193: static_cast<ui::KeyboardCode>(0), 0); align second line with 'false' or put 'false' on second line https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:39: Shell::GetInstance()->system_tray_delegate()->SignOut(); Generally a Delegate shouldn't have a default implementation. Move this to the class(es) that implement it. In particular, this class shouldn't have knowledge of Shell or SystemTrayDelegate, that's what the Delegate is for. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:107: // ash::Shell is available. Tests shouldn't generally influence the implementation. Here, I would add a ShowDialog(DialogDelegate*) method to Delegate and call delegate_->ShowDialog(this). https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:34: virtual base::TimeTicks GetCurrentTime() const; Delegate methods should generally be pure virtuals; it usually is not valid for any of them to not be implemented. Also, no need for a constructor, and the empty destructor can be inlined. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:53: void UpdateDialogDuration(base::TimeDelta duration); Comment public methods.
hello stevenjb, I uploaded another CL, PTAL https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:39: void EnsureConfirmationDialogIsClosed(); On 2013/12/18 21:55:20, stevenjb wrote: > Do these need to be public? Done. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:58: void ReleaseConfirmationDialog(); On 2013/12/18 21:55:20, stevenjb wrote: > Comment this. (UpdateAfterLoginStatusChange should really be commented also). Done. I didn't add comment for UpdateAfterLoginStatusChange() since I have fresh eyes on this function as well. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray_unittest.cc (right): https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:63: }; On 2013/12/18 21:55:20, stevenjb wrote: > It looks like this was copied from session_length_limiter? It seems like > something that would be useful to extract out into its own class in base/test > (e.g. see TestSimpleTaskRunner), in a separate CL. If this CL is time critical, > add a TODO and create/reference an issue to do this. > > If you keep this class here for this CL, put it in an anonymous namespace. Done. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:76: bool WasLogoutCalled() const; On 2013/12/18 21:55:20, stevenjb wrote: > bool logout_called() const { return logout_called_; } Done. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:104: MockLogoutConfirmationDelegate* delegate_; On 2013/12/18 21:55:20, stevenjb wrote: > nit: DISALLOW... Done. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:165: LogoutConfirmationDialogTest* tester) : runner_(runner), On 2013/12/18 21:55:20, stevenjb wrote: > : on new line Done. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:167: logout_called_ = false; On 2013/12/18 21:55:20, stevenjb wrote: > Set in initializer list Done. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_button_tray_unittest.cc:193: static_cast<ui::KeyboardCode>(0), 0); On 2013/12/18 21:55:20, stevenjb wrote: > align second line with 'false' or put 'false' on second line Done. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.cc (right): https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:39: Shell::GetInstance()->system_tray_delegate()->SignOut(); On 2013/12/18 21:55:20, stevenjb wrote: > Generally a Delegate shouldn't have a default implementation. Move this to the > class(es) that implement it. In particular, this class shouldn't have knowledge > of Shell or SystemTrayDelegate, that's what the Delegate is for. > Done. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.cc:107: // ash::Shell is available. On 2013/12/18 21:55:20, stevenjb wrote: > Tests shouldn't generally influence the implementation. Here, I would add a > ShowDialog(DialogDelegate*) method to Delegate and call > delegate_->ShowDialog(this). > Done. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:34: virtual base::TimeTicks GetCurrentTime() const; On 2013/12/18 21:55:20, stevenjb wrote: > Delegate methods should generally be pure virtuals; it usually is not valid for > any of them to not be implemented. Also, no need for a constructor, and the > empty destructor can be inlined. Done. But constructor is needed for current approach, PTAL. https://codereview.chromium.org/40053002/diff/630001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:53: void UpdateDialogDuration(base::TimeDelta duration); On 2013/12/18 21:55:20, stevenjb wrote: > Comment public methods. Done. https://codereview.chromium.org/40053002/diff/630001/chrome/browser/ui/ash/ch... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/40053002/diff/630001/chrome/browser/ui/ash/ch... chrome/browser/ui/ash/chrome_launcher_prefs.cc:75: registry->RegisterIntegerPref( On 2013/12/18 20:25:41, Daniel Erat wrote: > nit: not your fault, but mind alphabetizing these by pref name? the order seems > arbitrary right now. Done. https://codereview.chromium.org/40053002/diff/630001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/40053002/diff/630001/chrome/common/pref_names... chrome/common/pref_names.h:855: extern const char kLogoutDialogDurationMs[]; On 2013/12/18 20:25:41, Daniel Erat wrote: > nit: mind alphabetizing the prefs in this section too? (and also in the .cc > file) Done.
lgtm w/ nits https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:79: }; WS https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:55: // when the confirmation dialog is going to be destroyed. Notice that the nit: s/Notice that/Note:/ https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:30: Delegate() {} Null constructors really shouldn't be necessary for interface classes. https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:56: // preference for confirmation dialog duration changed. nit: 'changes' or 'has changed'
https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.cc (right): https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.cc:79: }; On 2013/12/19 18:57:44, stevenjb wrote: > WS Done. https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... File ash/system/logout_button/logout_button_tray.h (right): https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... ash/system/logout_button/logout_button_tray.h:55: // when the confirmation dialog is going to be destroyed. Notice that the On 2013/12/19 18:57:44, stevenjb wrote: > nit: s/Notice that/Note:/ Done. https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... File ash/system/logout_button/logout_confirmation_dialog_view.h (right): https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:30: Delegate() {} On 2013/12/19 18:57:44, stevenjb wrote: > Null constructors really shouldn't be necessary for interface classes. Done. https://codereview.chromium.org/40053002/diff/650001/ash/system/logout_button... ash/system/logout_button/logout_confirmation_dialog_view.h:56: // preference for confirmation dialog duration changed. On 2013/12/19 18:57:44, stevenjb wrote: > nit: 'changes' or 'has changed' Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/40053002/690001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/40053002/770002
Retried try job too often on win_rel for step(s) telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/40053002/770002
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/40053002/770002
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/40053002/770002
Message was sent while issue was closed.
Change committed as 242286
hello bartfab, I made a quick fix for the memory leak in tests, PTAL bjin
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/40053002/1240001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/40053002/1240001
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/40053002/1240001
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/40053002/1240001
Failed to apply patch for
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
can't find file to patch at input line 6
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: chrome/browser/chromeos/system/ash_system_tray_delegate.cc
|diff --git a/chrome/browser/chromeos/system/ash_system_tray_delegate.cc
b/chrome/browser/chromeos/system/ash_system_tray_delegate.cc
|index
5a49a4dadfd9a04de98a879194be14a14295badd..bd0613233cb5a92e4fa86f4f9504a405630a2a88
100644
|--- a/chrome/browser/chromeos/system/ash_system_tray_delegate.cc
|+++ b/chrome/browser/chromeos/system/ash_system_tray_delegate.cc
--------------------------
No file to patch. Skipping patch.
3 out of 3 hunks ignored
Patch: chrome/browser/chromeos/system/ash_system_tray_delegate.cc
Index: chrome/browser/chromeos/system/ash_system_tray_delegate.cc
diff --git a/chrome/browser/chromeos/system/ash_system_tray_delegate.cc
b/chrome/browser/chromeos/system/ash_system_tray_delegate.cc
index
5a49a4dadfd9a04de98a879194be14a14295badd..bd0613233cb5a92e4fa86f4f9504a405630a2a88
100644
--- a/chrome/browser/chromeos/system/ash_system_tray_delegate.cc
+++ b/chrome/browser/chromeos/system/ash_system_tray_delegate.cc
@@ -909,6 +909,10 @@ class SystemTrayDelegate : public ash::SystemTrayDelegate,
base::Bind(&SystemTrayDelegate::UpdateShowLogoutButtonInTray,
base::Unretained(this)));
user_pref_registrar_->Add(
+ prefs::kLogoutDialogDurationMs,
+ base::Bind(&SystemTrayDelegate::UpdateLogoutDialogDuration,
+ base::Unretained(this)));
+ user_pref_registrar_->Add(
prefs::kLargeCursorEnabled,
base::Bind(&SystemTrayDelegate::OnAccessibilityModeChanged,
base::Unretained(this),
@@ -930,6 +934,7 @@ class SystemTrayDelegate : public ash::SystemTrayDelegate,
UpdateClockType();
UpdateShowLogoutButtonInTray();
+ UpdateLogoutDialogDuration();
UpdatePerformanceTracing();
search_key_mapped_to_ =
profile->GetPrefs()->GetInteger(prefs::kLanguageRemapSearchKeyTo);
@@ -1001,6 +1006,13 @@ class SystemTrayDelegate : public
ash::SystemTrayDelegate,
prefs::kShowLogoutButtonInTray));
}
+ void UpdateLogoutDialogDuration() {
+ const int duration_ms = user_pref_registrar_->prefs()->GetInteger(
+ prefs::kLogoutDialogDurationMs);
+ GetSystemTrayNotifier()->NotifyLogoutDialogDurationChanged(
+ base::TimeDelta::FromMilliseconds(duration_ms));
+ }
+
void UpdateSessionStartTime() {
const PrefService* local_state = local_state_registrar_->prefs();
if (local_state->HasPrefPath(prefs::kSessionStartTime)) {
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/40053002/1590001
Message was sent while issue was closed.
Change committed as 243530 |
