Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
267743010:
Suppressed screen rotation notifications triggeres by the accelerometer (Closed)
|
Created:
6 years, 7 months ago by bruthig Modified:
6 years, 7 months ago CC:
chromium-reviews, kalyank, sadrul, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSuppressed screen rotation notifications triggeres by the accelerometer
BUG=364949
Test=MaximizeModeControllerTest.BlockRotationNotifications
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270746
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed Rob's comments from patch set 1 #
Total comments: 18
Patch Set 3 : Addressed Rob's comments from patch set 2 #
Total comments: 5
Patch Set 4 : Reworked NotificationBlocker to not block all events #Patch Set 5 : Fixed MaximizeModeNotifcationBlocker to only filter notifcations from "ash.display" #
Total comments: 14
Patch Set 6 : Changed MaximizeModeController to inherit NotificationBlocker #Patch Set 7 : Minor fix to some whitespace #
Total comments: 22
Patch Set 8 : Moved notification blocking in to the TrayDisplay class. #Patch Set 9 : Merged with master #Patch Set 10 : Fixed maximize_mode_controller to use it's own SetDisplayRotation method. #
Total comments: 18
Patch Set 11 : Updated to not add display notifications when screen rotations are being changed by the MaximizeMod… #
Total comments: 8
Patch Set 12 : Addressed Rob's comments from patch set 11. #
Total comments: 6
Patch Set 13 : Fixed minor nits. #
Total comments: 1
Patch Set 14 : Made TrayDisplay::CreateOrUpdateNotification short circuit. #
Messages
Total messages: 28 (0 generated)
https://codereview.chromium.org/267743010/diff/1/ash/wm/maximize_mode/maximiz... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:63: class ScreenRotationNotificationBlocker nit: Just call this ScopedNotificationBlocker. It doesn't discriminate by type of notification :-). https://codereview.chromium.org/267743010/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:66: ScreenRotationNotificationBlocker() nit: I don't think any of these methods should be inlined: http://www.chromium.org/developers/coding-style#TOC-Inline-functions https://codereview.chromium.org/267743010/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:146: ScreenRotationNotificationBlocker notification_blocker; This method (OnAccelerometerUpdated) is called fairly frequently. I think it might be worth just blocking before we actually call SetRotation rather than everytime HandleScreenRotation is called just not knowing the performance cost of this. You can separate out SetRotation into a method on MaximizeModeController to avoid any code duplication.
https://codereview.chromium.org/267743010/diff/1/ash/wm/maximize_mode/maximiz... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:63: class ScreenRotationNotificationBlocker On 2014/05/05 19:06:00, flackr wrote: > nit: Just call this ScopedNotificationBlocker. It doesn't discriminate by type > of notification :-). Done. https://codereview.chromium.org/267743010/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:66: ScreenRotationNotificationBlocker() On 2014/05/05 19:06:00, flackr wrote: > nit: I don't think any of these methods should be inlined: > http://www.chromium.org/developers/coding-style#TOC-Inline-functions Done. https://codereview.chromium.org/267743010/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:146: ScreenRotationNotificationBlocker notification_blocker; On 2014/05/05 19:06:00, flackr wrote: > This method (OnAccelerometerUpdated) is called fairly frequently. I think it > might be worth just blocking before we actually call SetRotation rather than > everytime HandleScreenRotation is called just not knowing the performance cost > of this. You can separate out SetRotation into a method on > MaximizeModeController to avoid any code duplication. Done.
https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:61: // A message centre notification blocker used to suppress screen rotation nit: s/centre/center https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:67: nit: no newline needed between constructor and destructor. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:84: message_center::MessageCenter::Get()) { nit: indent 4 more to show it's part of the previous line's constructor call. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:10: #include "ash/display/display_manager.h" Forward declare DisplayManager class, or perhaps not necessary in header at all. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:53: void SetDisplayRotation(DisplayManager* display_manager, Since this doesn't need anything from the class, perhaps put this in the anonymous namespace. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:421: TEST_F(MaximizeModeControllerTest, BlockRotationNotifications) { nit: Add a comment line before this to say what this tests. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:430: message_center::MessageCenter::Get(); nit: indent 4. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:436: // adjusting the screen rotation directly nit: Punctuation. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:446: // nit: Remove empty comment line.
https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:61: // A message centre notification blocker used to suppress screen rotation On 2014/05/05 20:26:54, flackr wrote: > nit: s/centre/center Done. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:67: On 2014/05/05 20:26:54, flackr wrote: > nit: no newline needed between constructor and destructor. Done. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:84: message_center::MessageCenter::Get()) { On 2014/05/05 20:26:54, flackr wrote: > nit: indent 4 more to show it's part of the previous line's constructor call. Done. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:10: #include "ash/display/display_manager.h" On 2014/05/05 20:26:54, flackr wrote: > Forward declare DisplayManager class, or perhaps not necessary in header at all. Done. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:53: void SetDisplayRotation(DisplayManager* display_manager, On 2014/05/05 20:26:54, flackr wrote: > Since this doesn't need anything from the class, perhaps put this in the > anonymous namespace. Done. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:421: TEST_F(MaximizeModeControllerTest, BlockRotationNotifications) { On 2014/05/05 20:26:54, flackr wrote: > nit: Add a comment line before this to say what this tests. Done. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:430: message_center::MessageCenter::Get(); On 2014/05/05 20:26:54, flackr wrote: > nit: indent 4. Done. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:436: // adjusting the screen rotation directly On 2014/05/05 20:26:54, flackr wrote: > nit: Punctuation. Done. https://codereview.chromium.org/267743010/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:446: // On 2014/05/05 20:26:54, flackr wrote: > nit: Remove empty comment line. Done.
https://codereview.chromium.org/267743010/diff/40001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:62: // events caused by accelerometer events nit: Punctuation. https://codereview.chromium.org/267743010/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:84: NotifyBlockingStateChanged(); Will this cause us to remove other currently active notifications?
https://codereview.chromium.org/267743010/diff/40001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:62: // events caused by accelerometer events On 2014/05/05 21:12:12, flackr wrote: > nit: Punctuation. Done. https://codereview.chromium.org/267743010/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:84: NotifyBlockingStateChanged(); On 2014/05/05 21:12:12, flackr wrote: > Will this cause us to remove other currently active notifications? Nope https://codereview.chromium.org/267743010/diff/40001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:84: NotifyBlockingStateChanged(); On 2014/05/05 21:12:12, flackr wrote: > Will this cause us to remove other currently active notifications? Done.
https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:88: MaximizeModeController::MaximizeModeNotificationBlocker:: Perhaps this should be called DisplayNotificationBlocker https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:90: : message_center::NotificationBlocker( nit: I think the : should align with the M in Maximize above since it's at the same syntactic/lexical depth as that. https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:91: message_center::MessageCenter::Get()), nit: this should be indented 4 from the m in "message_center" above. https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:109: const message_center::NotifierId& notifier_id) const { nit: should be indented 4 from the Should above. https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:112: return true; Why does this one not defer to the base class where the other one does? https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:116: ShouldShowNotificati truncated line? https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:46: class MaximizeModeNotificationBlocker Can you forward declare the class here, and put full declaration in the .cc? Actually, after looking at what this does, I think MaximizeModeController should just be a message_center::NotificationBlocker rather than having one. https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:52: virtual void SetShouldShowNotification(bool should_show); nit: Short comment.
https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:270: message_center->MarkSinglePopupAsShown(TrayDisplay::kNotificationId, true); Since TrayDisplay is responsible for the creation and display of all notifications related to display configuration, I would rather see a method added to it for clearing. It already is working directly with the MessageCenter for injecting events. This would allow us to not expose the Id.
sadrul@chromium.org: Please review changes in
https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:88: MaximizeModeController::MaximizeModeNotificationBlocker:: On 2014/05/08 18:04:00, flackr wrote: > Perhaps this should be called DisplayNotificationBlocker Done. https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:112: return true; On 2014/05/08 18:04:00, flackr wrote: > Why does this one not defer to the base class where the other one does? It is a pure virtual function in the base class. https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:116: ShouldShowNotificati On 2014/05/08 18:04:00, flackr wrote: > truncated line? Done. https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:46: class MaximizeModeNotificationBlocker On 2014/05/08 18:04:00, flackr wrote: > Can you forward declare the class here, and put full declaration in the .cc? > > Actually, after looking at what this does, I think MaximizeModeController should > just be a message_center::NotificationBlocker rather than having one. Done. https://codereview.chromium.org/267743010/diff/80001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:52: virtual void SetShouldShowNotification(bool should_show); On 2014/05/08 18:04:00, flackr wrote: > nit: Short comment. Done.
https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:89: : message_center::NotificationBlocker(message_center::MessageCenter::Get()), Are there any potential lifetime issues here? Does the MessageCenter for sure exist before and outlive this class? https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:254: set_show_notifications(false); It still seems like it would be nicer to scope the NotificationBlocker to this method. That being said I'll defer to sadrul. https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:258: if (message_center->HasNotification(TrayDisplay::kNotificationId)) This is a bit hacky, this could probably use a todo, perhaps the different return values from notification blockers I suggested. i.e. TODO(bruthig): Instead of inspecting the current notifications, we should use an enum return from NotificationBlocker which allows permanently dismissing a notification rather than only delaying it. https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:66: void set_show_notifications(bool should_show); non-trivial methods should use TitleCase. I.e. anything that's not just a setter or getter.
https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:130: return true; on 137 you fall back to checking with the message center. Is that also needed here? https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:49: const message_center::NotifierId& notifier_id) const OVERRIDE; Nit: empty line after a function and a new scope delimiter like "private:" https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:61: void SetDisplayRotation(DisplayManager* display_manager, Please forward declare DisplayManager https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:13: #include "ui/aura/test/event_generator.h" unused? https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:433: aura::Window* root = Shell::GetPrimaryRootWindow(); only used for the event_generator https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:434: aura::test::EventGenerator event_generator(root, root); unused? https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:442: EXPECT_FALSE(message_center->HasPopupNotifications()); nit: use ASSERTs before causing changes, to confirm test prerequisites. use EXPECTs afterwards to confirm behaviours
https://codereview.chromium.org/267743010/diff/120001/ash/system/chromeos/tra... File ash/system/chromeos/tray_display.h (right): https://codereview.chromium.org/267743010/diff/120001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.h:27: // Identifier for all message center notifications. Why would the identifer for message-center notifications live in here?
https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:89: : message_center::NotificationBlocker(message_center::MessageCenter::Get()), On 2014/05/08 21:56:15, flackr wrote: > Are there any potential lifetime issues here? Does the MessageCenter for sure > exist before and outlive this class? Done. https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:130: return true; On 2014/05/09 14:57:33, jonross wrote: > on 137 you fall back to checking with the message center. Is that also needed > here? For some reason ShouldShowNotificationAsPopup is a pure virtual function in the parent class. https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:254: set_show_notifications(false); On 2014/05/08 21:56:15, flackr wrote: > It still seems like it would be nicer to scope the NotificationBlocker to this > method. That being said I'll defer to sadrul. Done. https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:258: if (message_center->HasNotification(TrayDisplay::kNotificationId)) On 2014/05/08 21:56:15, flackr wrote: > This is a bit hacky, this could probably use a todo, perhaps the different > return values from notification blockers I suggested. i.e. > TODO(bruthig): Instead of inspecting the current notifications, we should use an > enum return from NotificationBlocker which allows permanently dismissing a > notification rather than only delaying it. Done. https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:49: const message_center::NotifierId& notifier_id) const OVERRIDE; On 2014/05/09 14:57:33, jonross wrote: > Nit: empty line after a function and a new scope delimiter like "private:" Done. https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:61: void SetDisplayRotation(DisplayManager* display_manager, On 2014/05/09 14:57:33, jonross wrote: > Please forward declare DisplayManager gfx::Display::Rotation also comes from display_manager.h https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:66: void set_show_notifications(bool should_show); On 2014/05/08 21:56:15, flackr wrote: > non-trivial methods should use TitleCase. I.e. anything that's not just a setter > or getter. Done. https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:433: aura::Window* root = Shell::GetPrimaryRootWindow(); On 2014/05/09 14:57:33, jonross wrote: > only used for the event_generator Done. https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:434: aura::test::EventGenerator event_generator(root, root); On 2014/05/09 14:57:33, jonross wrote: > unused? Done. https://codereview.chromium.org/267743010/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:442: EXPECT_FALSE(message_center->HasPopupNotifications()); On 2014/05/09 14:57:33, jonross wrote: > nit: use ASSERTs before causing changes, to confirm test prerequisites. use > EXPECTs afterwards to confirm behaviours Done.
https://codereview.chromium.org/267743010/diff/180001/ash/system/chromeos/tra... File ash/system/chromeos/tray_display.cc (right): https://codereview.chromium.org/267743010/diff/180001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:138: // Overrides from message_center::NotificationBlocker: nit: use the newer style // message_center::NotificationBlocker: https://codereview.chromium.org/267743010/diff/180001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:160: NotifyBlockingStateChanged(); Does this trigger calls to the subsequent methods? If not there is no need for should_show_notifications. https://codereview.chromium.org/267743010/diff/180001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:444: ash::MaximizeModeController* maximize_mode_controller = nit: This is in the namespace of ash, don't need to specify it. https://codereview.chromium.org/267743010/diff/180001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:453: message_center->MarkSinglePopupAsShown(kNotificationId, false); nit: comment on the meaning of false. In general for ambiguous cases like this an enum is preferred. However I don't see it in the scope of this change to update that method. https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:307: int64 display_id, We only ever operate on the internal display, this parameter can be removed. https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:514: maximize_mode_controller()->set_rotation_locked(false); Is this being triggered to require disabling?
https://codereview.chromium.org/267743010/diff/180001/ash/system/chromeos/tra... File ash/system/chromeos/tray_display.cc (right): https://codereview.chromium.org/267743010/diff/180001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:453: message_center->MarkSinglePopupAsShown(kNotificationId, false); On 2014/05/14 01:00:39, jonross wrote: > nit: comment on the meaning of false. In general for ambiguous cases like this > an enum is preferred. However I don't see it in the scope of this change to > update that method. Not sure that we prefer enums for this on or off only two state kind of member. The comment style is typically /* mark_notification_as_read */ false or the other way around. Example: https://code.google.com/p/chromium/codesearch#chromium/src/ui/message_center/... That being said, can we just remove the current display notification if it's there and not add any notifications when they're accelerometer triggered? https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:312: show_display_notifications_ = false; nit: Use base::AutoReset. https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:85: bool show_display_notifications_; nit: this name describes the side effects of this variable, but not what it's value means. Prefer something like in_set_display_rotation_. https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:514: maximize_mode_controller()->set_rotation_locked(false); On 2014/05/14 01:00:39, jonross wrote: > Is this being triggered to require disabling? A SetInternalDisplayRotation(ROTATE_0...) would probably fix that.
https://codereview.chromium.org/267743010/diff/180001/ash/system/chromeos/tra... File ash/system/chromeos/tray_display.cc (right): https://codereview.chromium.org/267743010/diff/180001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:138: // Overrides from message_center::NotificationBlocker: On 2014/05/14 01:00:39, jonross wrote: > nit: use the newer style > > // message_center::NotificationBlocker: Done. https://codereview.chromium.org/267743010/diff/180001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:160: NotifyBlockingStateChanged(); On 2014/05/14 01:00:39, jonross wrote: > Does this trigger calls to the subsequent methods? > > If not there is no need for should_show_notifications. Yes it does. NotifyBlockingStateChanged will cause ScopedNotificationBlocker::ShouldShowNotificationAsPopup to be called. https://codereview.chromium.org/267743010/diff/180001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:444: ash::MaximizeModeController* maximize_mode_controller = On 2014/05/14 01:00:39, jonross wrote: > nit: This is in the namespace of ash, don't need to specify it. Done. https://codereview.chromium.org/267743010/diff/180001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:453: message_center->MarkSinglePopupAsShown(kNotificationId, false); On 2014/05/14 01:48:33, flackr wrote: > On 2014/05/14 01:00:39, jonross wrote: > > nit: comment on the meaning of false. In general for ambiguous cases like this > > an enum is preferred. However I don't see it in the scope of this change to > > update that method. > > Not sure that we prefer enums for this on or off only two state kind of member. > The comment style is typically /* mark_notification_as_read */ false or the > other way around. Example: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/message_center/... > > That being said, can we just remove the current display notification if it's > there and not add any notifications when they're accelerometer triggered? Done. https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:307: int64 display_id, On 2014/05/14 01:00:39, jonross wrote: > We only ever operate on the internal display, this parameter can be removed. Done. https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:312: show_display_notifications_ = false; On 2014/05/14 01:48:33, flackr wrote: > nit: Use base::AutoReset. Done. https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:85: bool show_display_notifications_; On 2014/05/14 01:48:33, flackr wrote: > nit: this name describes the side effects of this variable, but not what it's > value means. Prefer something like in_set_display_rotation_. Done. https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/267743010/diff/180001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:514: maximize_mode_controller()->set_rotation_locked(false); On 2014/05/14 01:48:33, flackr wrote: > On 2014/05/14 01:00:39, jonross wrote: > > Is this being triggered to require disabling? > > A SetInternalDisplayRotation(ROTATE_0...) would probably fix that. Done.
Looks good, a few comments. nit: In CL description s/triggeres/triggered https://codereview.chromium.org/267743010/diff/200001/ash/system/chromeos/tra... File ash/system/chromeos/tray_display.cc (right): https://codereview.chromium.org/267743010/diff/200001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:398: // would be annoying and not useful to the user. Probably simply state that we don't display notifications for accelerometer triggered screen rotations, can reference the bug for detailed rationale. https://codereview.chromium.org/267743010/diff/200001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:399: if (maximize_mode_controller && Are there cases where this is called and MaximizeModeController hasn't been instantiated yet? In general if something shouldn't happen (like MaximizeModeController is always instantiated) we prefer to have it crash rather than fail silently. https://codereview.chromium.org/267743010/diff/200001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:402: } else dismiss an active notification if it exists? https://codereview.chromium.org/267743010/diff/200001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/200001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:307: // Suppress message centre notifications for screen rotations caused This isn't actually suppressing the notifications, it's used to enable/allow suppressing notifications ... update comment.
https://codereview.chromium.org/267743010/diff/200001/ash/system/chromeos/tra... File ash/system/chromeos/tray_display.cc (right): https://codereview.chromium.org/267743010/diff/200001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:398: // would be annoying and not useful to the user. On 2014/05/14 20:12:41, flackr wrote: > Probably simply state that we don't display notifications for accelerometer > triggered screen rotations, can reference the bug for detailed rationale. Done. https://codereview.chromium.org/267743010/diff/200001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:399: if (maximize_mode_controller && On 2014/05/14 20:12:41, flackr wrote: > Are there cases where this is called and MaximizeModeController hasn't been > instantiated yet? In general if something shouldn't happen (like > MaximizeModeController is always instantiated) we prefer to have it crash rather > than fail silently. Done. https://codereview.chromium.org/267743010/diff/200001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:402: } On 2014/05/14 20:12:41, flackr wrote: > else dismiss an active notification if it exists? Previous notifications are already dismissed above at the start of this method. https://codereview.chromium.org/267743010/diff/200001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/200001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:307: // Suppress message centre notifications for screen rotations caused On 2014/05/14 20:12:41, flackr wrote: > This isn't actually suppressing the notifications, it's used to enable/allow > suppressing notifications ... update comment. Done.
LGTM with nits https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:307: // Update state so screen rotation notifications can be suppressed. Actually, it's evident from the code what it's doing, I think the comment about the variable in the header is sufficient. https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:30: // If true the screen's rotation is currently being changed. Probably only need the comment on the variable. https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:83: // When true the screen's orientation is being changed. s/When true/True when
On 2014/05/14 21:13:23, flackr wrote: > LGTM with nits > > https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... > File ash/wm/maximize_mode/maximize_mode_controller.cc (right): > > https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... > ash/wm/maximize_mode/maximize_mode_controller.cc:307: // Update state so screen > rotation notifications can be suppressed. > Actually, it's evident from the code what it's doing, I think the comment about > the variable in the header is sufficient. > > https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... > File ash/wm/maximize_mode/maximize_mode_controller.h (right): > > https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... > ash/wm/maximize_mode/maximize_mode_controller.h:30: // If true the screen's > rotation is currently being changed. > Probably only need the comment on the variable. > > https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... > ash/wm/maximize_mode/maximize_mode_controller.h:83: // When true the screen's > orientation is being changed. > s/When true/True when LGTM
https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:307: // Update state so screen rotation notifications can be suppressed. On 2014/05/14 21:13:23, flackr wrote: > Actually, it's evident from the code what it's doing, I think the comment about > the variable in the header is sufficient. Done. https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:30: // If true the screen's rotation is currently being changed. On 2014/05/14 21:13:23, flackr wrote: > Probably only need the comment on the variable. Done. https://codereview.chromium.org/267743010/diff/220001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:83: // When true the screen's orientation is being changed. On 2014/05/14 21:13:23, flackr wrote: > s/When true/True when Done.
lgtm https://codereview.chromium.org/267743010/diff/240001/ash/system/chromeos/tra... File ash/system/chromeos/tray_display.cc (right): https://codereview.chromium.org/267743010/diff/240001/ash/system/chromeos/tra... ash/system/chromeos/tray_display.cc:396: in_set_screen_rotation()) { Early out above in line 377 instead? So you don't have to create the Notification and then destroy it ... or are there some other side-effects of creating a Notification/clicked-delegate that we depend on?
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bruthig@chromium.org/267743010/260001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Message was sent while issue was closed.
Change committed as 270746 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
