Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
412013002:
Prevent entering maximize mode when the lid is closed or has recently opened. (Closed)
DescriptionPrevent entering maximize mode when the lid is closed or has recently opened.
BUG=387879
TEST=MaximizeModeController.CloseLidWhileInMaximizeMode
TEST=MaximizeModeController.WasLidOpenedRecently
TEST=MaximizeModeController.HingeAnglesWithLidClosed
TEST=MaximizeModeController.StableHingeAnglesWithLidOpened
TEST=MaximizeModeController.UnstableHingeAnglesWhenLidRecentlyOpened
TEST=MaximizeModeController.UnstableHingAnglesWithLidOpened
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287625
Patch Set 1 #
Total comments: 20
Patch Set 2 : Swapped out TimeTicksProvider for TickClock #
Total comments: 20
Patch Set 3 : Simplified maximize mode enter/exit logic plus some minor style fixes. #
Total comments: 4
Patch Set 4 : Fixed minor style nits. #
Total comments: 39
Patch Set 5 : Merged with trunk #Patch Set 6 : Simplified enter/exit maximize mode logic and minor nit fixes. #
Total comments: 10
Patch Set 7 : Cleaned up MaximizeModeControllerTest fixture by removing static class variables. #
Total comments: 2
Patch Set 8 : Minor sp nit fix. #
Messages
Total messages: 19 (0 generated)
jonross@, Can you do a preliminary review on this?
https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:18: #include "chromeos/dbus/dbus_thread_manager.h" Can you confirm via gyp files that this will always compile in when MaximizeModeController does? If not we will need to separate this out. https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:51: const int kLidRecentlyOpenedTolerance = 2; For constants representing a time duration we append a representation for the time "Ms" for milliseconds, "Seconds" for seconds. Could you update this constant? https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:276: // A lid open event won't always occur when coming out of a suspend state. If it does not, is there a way to poll lid state upon resuming from suspend? https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:307: } else if (angle > kMinStableAngle && Should we exit MaximizeMode if angle <kMinStableAngle? https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:312: last_lid_open_time_ = base::TimeTicks(); This moves the lid open time to be later than when it was received. Would this not delay state change further? https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:401: if (IsMaximizeModeWindowManagerEnabled()) We should stop calling this twice. https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:428: Shell::GetInstance()->display_controller()->RemoveObserver(this); Nice fix. https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:478: if (!last_lid_open_time_.is_null()) { Quick exits are prefered. if (last_lid_open_time_.is_null()) return false; //Logic below https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.h:117: class TimeTickProvider { I dislike the idea of changing the time provider. Other classes that calculate duration all directly use base::TimeTicks::Now() directly in their code. Where the start time can be modified: https://code.google.com/p/chromium/codesearch#chromium/src/ash/system/chromeo... Or their method for processing takes base::TimeTicks as the parameter, and so it can be modified by tests: https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/animation/l... Since this class exists for testing I would prefer if you switched to an alternate method such as these. https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.h:128: class TimeTickProviderImpl : public TimeTickProvider { This class is not needed for testing, nor for any definitions below. Please move to an anonymous namespace within the .cc file.
Added some polish and some replies to comments. Can you please review :) https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:18: #include "chromeos/dbus/dbus_thread_manager.h" On 2014/07/24 14:22:00, jonross wrote: > Can you confirm via gyp files that this will always compile in when > MaximizeModeController does? > > If not we will need to separate this out. Done. https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:51: const int kLidRecentlyOpenedTolerance = 2; On 2014/07/24 14:22:00, jonross wrote: > For constants representing a time duration we append a representation for the > time "Ms" for milliseconds, "Seconds" for seconds. > > Could you update this constant? Done. https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:276: // A lid open event won't always occur when coming out of a suspend state. On 2014/07/24 14:22:00, jonross wrote: > If it does not, is there a way to poll lid state upon resuming from suspend? I'm not sure if there is a way or not but I don't think it is necessary. As far as I know you can't come out of suspend when the lid is closed and thus such a poll would always return "not open" and would be useless. Explicitly setting "lid_is_closed_ = false" here is handling the case when we are already in a maximize mode position and the user hits the power button. https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:307: } else if (angle > kMinStableAngle && On 2014/07/24 14:22:00, jonross wrote: > Should we exit MaximizeMode if angle <kMinStableAngle? No we don't want to exit MaximizeMode in this case because when the lid is fully open we get erroneous readings like 5 degrees and we don't want to flicker in and out of MaximizeMode when the lid is fully open. https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:312: last_lid_open_time_ = base::TimeTicks(); On 2014/07/24 14:22:00, jonross wrote: > This moves the lid open time to be later than when it was received. Would this > not delay state change further? I don't think this is doing what you think. It's setting the last_lid_open_time_ to be "NULL", not Now(). https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:401: if (IsMaximizeModeWindowManagerEnabled()) On 2014/07/24 14:22:00, jonross wrote: > We should stop calling this twice. In order to stop calling it twice each spot that calls it would have to include the "if (IsMaximizeModeWindowManagerEnabled()) " guard and this was starting to bloat the code. I could create a "TryEnterMaximizeMode()" method that would have the guard and then call this method, but I really don't like the idea of the code bloat with guarding every call. https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:478: if (!last_lid_open_time_.is_null()) { On 2014/07/24 14:22:00, jonross wrote: > Quick exits are prefered. > > if (last_lid_open_time_.is_null()) > return false; > > //Logic below Done.
https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:276: // A lid open event won't always occur when coming out of a suspend state. On 2014/07/24 19:57:06, bruthig wrote: > On 2014/07/24 14:22:00, jonross wrote: > > If it does not, is there a way to poll lid state upon resuming from suspend? > > I'm not sure if there is a way or not but I don't think it is necessary. As far > as I know you can't come out of suspend when the lid is closed and thus such a > poll would always return "not open" and would be useless. Explicitly setting > "lid_is_closed_ = false" here is handling the case when we are already in a > maximize mode position and the user hits the power button. > Acknowledged. https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:307: } else if (angle > kMinStableAngle && On 2014/07/24 19:57:06, bruthig wrote: > On 2014/07/24 14:22:00, jonross wrote: > > Should we exit MaximizeMode if angle <kMinStableAngle? > > No we don't want to exit MaximizeMode in this case because when the lid is fully > open we get erroneous readings like 5 degrees and we don't want to flicker in > and out of MaximizeMode when the lid is fully open. Acknowledged. https://codereview.chromium.org/412013002/diff/1/ash/wm/maximize_mode/maximiz... ash/wm/maximize_mode/maximize_mode_controller.cc:312: last_lid_open_time_ = base::TimeTicks(); On 2014/07/24 19:57:06, bruthig wrote: > On 2014/07/24 14:22:00, jonross wrote: > > This moves the lid open time to be later than when it was received. Would this > > not delay state change further? > > I don't think this is doing what you think. It's setting the > last_lid_open_time_ to be "NULL", not Now(). My misreading. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:20: #include "chromeos/dbus/dbus_thread_manager.h" #if defined this as well. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:53: base::TimeDelta kLidRecentlyOpenedTolerance = base::TimeDelta::FromSeconds(2); Make this a const. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:327: } I'd rather the new logic be introduced into the previous formats if structure. Rather than a multi-tier if. Would also remove need for added checks in Enter/Leave https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:17: #include "chromeos/dbus/power_manager_client.h" The one time we break include order is to separate out conditional includes. Can you add this below display.h? With an empty line to separate. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:27: } Keep namespace forward declarations in alphabetical order. With the namespace where the class is being defined kept to last. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:203: // Used to prevent entering maximize mode. nit: only overflow comments when at the 80 char limit. Can you also check the other comments you added above. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:166: TriggerAccelerometerUpdate(kHingeBaseVector, kHingeLidVector_90); Are we ever going to see lid vectors this high while the lid is shut? https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:215: TEST_F(MaximizeModeControllerTest, WasLidOpenedRecently) { Rename to WasLidOpenedRecentlyAffectedByTime, or something to that affect. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:243: EXPECT_FALSE(WasLidOpenedRecently()); Trivial case for not chrome_os. Don't need to test for it.
Please take another look. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:20: #include "chromeos/dbus/dbus_thread_manager.h" On 2014/07/25 13:39:06, jonross wrote: > #if defined this as well. Done. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:53: base::TimeDelta kLidRecentlyOpenedTolerance = base::TimeDelta::FromSeconds(2); On 2014/07/25 13:39:06, jonross wrote: > Make this a const. Done. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:327: } On 2014/07/25 13:39:06, jonross wrote: > I'd rather the new logic be introduced into the previous formats if structure. > Rather than a multi-tier if. > > Would also remove need for added checks in Enter/Leave Done. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:17: #include "chromeos/dbus/power_manager_client.h" On 2014/07/25 13:39:06, jonross wrote: > The one time we break include order is to separate out conditional includes. Can > you add this below display.h? With an empty line to separate. Done. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:27: } On 2014/07/25 13:39:06, jonross wrote: > Keep namespace forward declarations in alphabetical order. With the namespace > where the class is being defined kept to last. Done. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:203: // Used to prevent entering maximize mode. On 2014/07/25 13:39:06, jonross wrote: > nit: only overflow comments when at the 80 char limit. Can you also check the > other comments you added above. Done. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:166: TriggerAccelerometerUpdate(kHingeBaseVector, kHingeLidVector_90); On 2014/07/25 13:39:06, jonross wrote: > Are we ever going to see lid vectors this high while the lid is shut? We shouldn't during normal expected operation, but tests should sometimes verify reasonable behaviour in light of unexpected cases. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:215: TEST_F(MaximizeModeControllerTest, WasLidOpenedRecently) { On 2014/07/25 13:39:07, jonross wrote: > Rename to WasLidOpenedRecentlyAffectedByTime, or something to that affect. Done. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:243: EXPECT_FALSE(WasLidOpenedRecently()); On 2014/07/25 13:39:06, jonross wrote: > Trivial case for not chrome_os. Don't need to test for it. I disagree, if there is some bug in ifdefs, or a bad initialization of last_lid_open_time_, or otherwise that causes WasLidOpenedRecently() to return true then it would prevent entering maximize mode ever.
LGTM with nits. Feel free to add flackr@ https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:166: TriggerAccelerometerUpdate(kHingeBaseVector, kHingeLidVector_90); On 2014/07/25 15:20:42, bruthig wrote: > On 2014/07/25 13:39:06, jonross wrote: > > Are we ever going to see lid vectors this high while the lid is shut? > > We shouldn't during normal expected operation, but tests should sometimes verify > reasonable behaviour in light of unexpected cases. Acknowledged. https://codereview.chromium.org/412013002/diff/20001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:243: EXPECT_FALSE(WasLidOpenedRecently()); On 2014/07/25 15:20:43, bruthig wrote: > On 2014/07/25 13:39:06, jonross wrote: > > Trivial case for not chrome_os. Don't need to test for it. > > I disagree, if there is some bug in ifdefs, or a bad initialization of > last_lid_open_time_, or otherwise that causes WasLidOpenedRecently() to return > true then it would prevent entering maximize mode ever. Acknowledged. https://codereview.chromium.org/412013002/diff/30001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/412013002/diff/30001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:319: if (IsAngleStable(angle)) { nit: no '{' for single line if https://codereview.chromium.org/412013002/diff/30001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:330: (angle > kMaxStableAngle && WasLidOpenedRecently())))) { nit: for multi-line if-statements only indent wrap-around lines if a new stage of bracketed logic: if (a || b && (c || d)) {
Rob, can you please take a gander :) https://codereview.chromium.org/412013002/diff/30001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/412013002/diff/30001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:319: if (IsAngleStable(angle)) { On 2014/07/25 15:56:35, jonross wrote: > nit: no '{' for single line if Done. https://codereview.chromium.org/412013002/diff/30001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:330: (angle > kMaxStableAngle && WasLidOpenedRecently())))) { On 2014/07/25 15:56:35, jonross wrote: > nit: for multi-line if-statements only indent wrap-around lines if a new stage > of bracketed logic: > > if (a || > b && > (c || > d)) { Done.
https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (left): https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:379: RecordTouchViewStateTransition(); Was this never called before? There are no removed observers. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:56: const base::TimeDelta kLidRecentlyOpenedTolerance = how about kLidRecentlyOpenedDuration? https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:57: base::TimeDelta::FromSeconds(2); nit: indent 4. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:91: bool IsAngleStable(float angle) { I feel like this should be defined as a bool where it is used since we only call it with the current angle. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:167: last_lid_open_time_(), Default constructor is automatically called, no need to explicitly call in initializer list. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:174: GetPowerManagerClient()->AddObserver(this); nit: indent 4. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:183: GetPowerManagerClient()->RemoveObserver(this); nit: indent 4. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:288: // A lid open event won't always occur when coming out of a suspend state. This seems problematic, so we can get a lid closed event, suspend, and then wake up without getting a lid open event? If so, please file (and reference) a bug for this. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:289: lid_is_closed_ = false; If lid_is_closed_ was true we probably need to set last_lid_open_time_ to be now, right? Assuming we don't get the open event. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:326: if (lid_is_closed_ || LeaveMaximizeMode is called from LidEventReceived, why do we need to call it again here? https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:329: (angle > kMaxStableAngle && WasLidOpenedRecently())))) { If we call LeaveMaximizeMode on lid opened (which seems to be the case), and we won't re-enter maximize mode until we get a stable angle, do we need this last line of the condition? https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:331: } else if (!maximize_mode_engaged && Probably could add !lid_is_closed_ here though. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:40: class ASH_EXPORT MaximizeModeController : public AccelerometerObserver, nit: Move the : public AccelerometerObserver down to align with the rest of the base classes. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:128: void SetTickClockForTest(base::TickClock* tick_clock); nit: Pass in a scoped_ptr to ensure correct pointer management. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:194: // erroneous acceleromter readings as the lid is opened but the acceleromater nit: s/acceleromter/accelerometer nit: s/acceleromater/accelerometer https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:169: TriggerAccelerometerUpdate(kHingeBaseVector, kHingeLidVector_180); All of these values < 270 would not normally engage maximize mode, I'd remove from the test. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:198: EXPECT_FALSE(IsMaximizeModeStarted()); Why test 5 again? https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:209: EXPECT_TRUE(IsMaximizeModeStarted()); This is tested by another test isn't it? https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:233: // 2 seconds after lid open. 1 time before the threshold and 1 after is fine.
flackr@, can you take another look :) https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (left): https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:379: RecordTouchViewStateTransition(); On 2014/07/29 16:41:45, flackr wrote: > Was this never called before? There are no removed observers. No it wasn't being called. Appears to have changed with www.crbug.com/359619. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:56: const base::TimeDelta kLidRecentlyOpenedTolerance = On 2014/07/29 16:41:45, flackr wrote: > how about kLidRecentlyOpenedDuration? Done. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:57: base::TimeDelta::FromSeconds(2); On 2014/07/29 16:41:44, flackr wrote: > nit: indent 4. Done. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:91: bool IsAngleStable(float angle) { On 2014/07/29 16:41:45, flackr wrote: > I feel like this should be defined as a bool where it is used since we only call > it with the current angle. Done. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:167: last_lid_open_time_(), On 2014/07/29 16:41:44, flackr wrote: > Default constructor is automatically called, no need to explicitly call in > initializer list. Done. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:174: GetPowerManagerClient()->AddObserver(this); On 2014/07/29 16:41:44, flackr wrote: > nit: indent 4. Done. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:183: GetPowerManagerClient()->RemoveObserver(this); On 2014/07/29 16:41:44, flackr wrote: > nit: indent 4. Done. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:288: // A lid open event won't always occur when coming out of a suspend state. On 2014/07/29 16:41:45, flackr wrote: > This seems problematic, so we can get a lid closed event, suspend, and then wake > up without getting a lid open event? If so, please file (and reference) a bug > for this. This shouldn't happen, removed. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:326: if (lid_is_closed_ || On 2014/07/29 16:41:44, flackr wrote: > LeaveMaximizeMode is called from LidEventReceived, why do we need to call it > again here? Done. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:329: (angle > kMaxStableAngle && WasLidOpenedRecently())))) { On 2014/07/29 16:41:44, flackr wrote: > If we call LeaveMaximizeMode on lid opened (which seems to be the case), and we > won't re-enter maximize mode until we get a stable angle, do we need this last > line of the condition? Done. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:331: } else if (!maximize_mode_engaged && On 2014/07/29 16:41:44, flackr wrote: > Probably could add !lid_is_closed_ here though. Done. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:40: class ASH_EXPORT MaximizeModeController : public AccelerometerObserver, On 2014/07/29 16:41:45, flackr wrote: > nit: Move the : public AccelerometerObserver down to align with the rest of the > base classes. Done. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:128: void SetTickClockForTest(base::TickClock* tick_clock); On 2014/07/29 16:41:45, flackr wrote: > nit: Pass in a scoped_ptr to ensure correct pointer management. Done. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:194: // erroneous acceleromter readings as the lid is opened but the acceleromater On 2014/07/29 16:41:45, flackr wrote: > nit: s/acceleromter/accelerometer > nit: s/acceleromater/accelerometer Done. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:169: TriggerAccelerometerUpdate(kHingeBaseVector, kHingeLidVector_180); On 2014/07/29 16:41:45, flackr wrote: > All of these values < 270 would not normally engage maximize mode, I'd remove > from the test. Done. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:198: EXPECT_FALSE(IsMaximizeModeStarted()); On 2014/07/29 16:41:45, flackr wrote: > Why test 5 again? Removed. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:209: EXPECT_TRUE(IsMaximizeModeStarted()); On 2014/07/29 16:41:45, flackr wrote: > This is tested by another test isn't it? Yes, removed. https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:233: // 2 seconds after lid open. On 2014/07/29 16:41:45, flackr wrote: > 1 time before the threshold and 1 after is fine. I disagree, "2 seconds after lid open" is testing exactly on the boundary of the threshold and is the difference between a < and <=.
https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:233: // 2 seconds after lid open. On 2014/07/31 16:10:49, bruthig wrote: > On 2014/07/29 16:41:45, flackr wrote: > > 1 time before the threshold and 1 after is fine. > > I disagree, "2 seconds after lid open" is testing exactly on the boundary of the > threshold and is the difference between a < and <=. The exact boundary is really more of an implementation detail, i.e. it doesn't really matter that the threshold is exactly 2, just that there is some short time for which it will be true, and some long time for which it won't be. Having a test for the exact boundary is just another thing to change if we change the threshold. https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:325: (angle > kMaxStableAngle && !WasLidOpenedRecently()))) { this technically breaks if angle == kMaxStableAngle. Should probably be angle > kEnterMaximizeModeAngle && (is_angle_stable || !WasLidOpenRecently()) https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:41: : public AccelerometerObserver, Also, indent 4 (making all of the following indented 6). https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:53: const gfx::Vector3dF kHingeBaseVector(1.0f, 0.0f, 0.0f); Global object types are forbidden: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo... You can instead create these as statics on the class or during the test class setup, although even better would be to define a function like Vector3dF LidVector(Vector3dF base, float angle) which would let you get the corresponding lid vector for a base vector. This could be used to clean up most of these tests. https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:124: maximize_mode_controller()->tick_clock_->NowTicks()); nit: indent 4. https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:129: maximize_mode_controller()->tick_clock_->NowTicks()); nit: indent 4.
flackr@, Can you take another look? :) https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/412013002/diff/50001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:233: // 2 seconds after lid open. On 2014/07/31 20:21:41, flackr wrote: > On 2014/07/31 16:10:49, bruthig wrote: > > On 2014/07/29 16:41:45, flackr wrote: > > > 1 time before the threshold and 1 after is fine. > > > > I disagree, "2 seconds after lid open" is testing exactly on the boundary of > the > > threshold and is the difference between a < and <=. > > The exact boundary is really more of an implementation detail, i.e. it doesn't > really matter that the threshold is exactly 2, just that there is some short > time for which it will be true, and some long time for which it won't be. Having > a test for the exact boundary is just another thing to change if we change the > threshold. Done. https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.cc:325: (angle > kMaxStableAngle && !WasLidOpenedRecently()))) { On 2014/07/31 20:21:41, flackr wrote: > this technically breaks if angle == kMaxStableAngle. Should probably be angle > > kEnterMaximizeModeAngle && (is_angle_stable || !WasLidOpenRecently()) Done. https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller.h:41: : public AccelerometerObserver, On 2014/07/31 20:21:41, flackr wrote: > Also, indent 4 (making all of the following indented 6). Done. https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:53: const gfx::Vector3dF kHingeBaseVector(1.0f, 0.0f, 0.0f); On 2014/07/31 20:21:41, flackr wrote: > Global object types are forbidden: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo... > > You can instead create these as statics on the class or during the test class > setup, although even better would be to define a function like Vector3dF > LidVector(Vector3dF base, float angle) which would let you get the corresponding > lid vector for a base vector. This could be used to clean up most of these > tests. Done. https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:124: maximize_mode_controller()->tick_clock_->NowTicks()); On 2014/07/31 20:21:41, flackr wrote: > nit: indent 4. Done. https://codereview.chromium.org/412013002/diff/90001/ash/wm/maximize_mode/max... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:129: maximize_mode_controller()->tick_clock_->NowTicks()); On 2014/07/31 20:21:41, flackr wrote: > nit: indent 4. Done.
LGTM https://codereview.chromium.org/412013002/diff/110001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/412013002/diff/110001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:46: // When the lid is near open (or near closed) the acceleromter readings may be nit: s/acceleromter/accelerometer
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/412013002/110001
The CQ bit was unchecked by bruthig@chromium.org
Simple spelling fix, no need to review. https://codereview.chromium.org/412013002/diff/110001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/412013002/diff/110001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:46: // When the lid is near open (or near closed) the acceleromter readings may be On 2014/08/05 18:22:34, flackr wrote: > nit: s/acceleromter/accelerometer Done.
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/412013002/130001
Message was sent while issue was closed.
Change committed as 287625 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
