Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2020553003:
Hook up TABLET_MODE switch to maximize_mode_controller (Closed)
|
Created:
4 years, 6 months ago by jcliang Modified:
4 years, 6 months ago CC:
chromium-reviews, derat+watch_chromium.org, hashimoto+watch_chromium.org, kalyank, oshima+watch_chromium.org, sadrul, Mr4D (OOO till 08-26) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHook up TABLET_MODE switch to maximize_mode_controller
This is part of the patch set that hooks up tablet mode switch
to Chrome. If the tablet mode switch is on then Chrome can know
for sure that it needs to enable maximize mode (tablet mode).
The helps Chrome to decide whether to enable tablet mode in some
corner cases where the accelerometer readings are not stable.
BUG=chrome-os-partner:49114
TEST=unit tests
TEST=Build power_manager and chrome and deploy to my DUT. Make
sure that:
- The device enters maximize mode when tablet mode switch
is on.
- The device leaves maximize mode when tablet mode switch
is off and lid angle is less than kExitMaximizeModeAngle.
Signed-off-by: Ricky Liang <jcliang@chromium.org>;
Committed: https://crrev.com/3fb658ae4db44dfe1a1686ce0717070e11247d9c
Cr-Commit-Position: refs/heads/master@{#400076}
Patch Set 1 #
Total comments: 7
Patch Set 2 : #
Total comments: 11
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 14
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 3
Patch Set 7 : #Patch Set 8 : Hook up TABLET_MODE switch to maximize_mode_controller #
Total comments: 4
Patch Set 9 : Hook up TABLET_MODE switch to maximize_mode_controller #
Messages
Total messages: 79 (29 generated)
Description was changed from ========== Hook up TABLET_MODE switch to maximize_mode_controller This is part of the patch set that hooks up tablet mode switch to Chrome. If the tablet mode switch is on then Chrome can know for sure that it needs to enable maximize mode (tablet mode). The helps Chrome to decide whether to enable tablet mode in some corner cases where the accelerometer readings are not stable. BUG= http://crosbug.com/p/49114 TEST=Build power_manager and chrome and deploy to my DUT. Make sure that: - The device enters maximize mode when tablet mode switch is on. - The device leaves maximize mode when tablet mode switch is off and lid angle is less than kExitMaximizeModeAngle. Signed-off-by: Ricky Liang <jcliang@chromium.org> ========== to ========== Hook up TABLET_MODE switch to maximize_mode_controller This is part of the patch set that hooks up tablet mode switch to Chrome. If the tablet mode switch is on then Chrome can know for sure that it needs to enable maximize mode (tablet mode). The helps Chrome to decide whether to enable tablet mode in some corner cases where the accelerometer readings are not stable. BUG= http://crosbug.com/p/49114 TEST=Build power_manager and chrome and deploy to my DUT. Make sure that: - The device enters maximize mode when tablet mode switch is on. - The device leaves maximize mode when tablet mode switch is off and lid angle is less than kExitMaximizeModeAngle. Signed-off-by: Ricky Liang <jcliang@chromium.org> ==========
jcliang@chromium.org changed reviewers: + afakhry@chromium.org, derat@chromium.org, jonross@chromium.org, oshima@chromium.org
On 2016/05/27 14:10:56, jcliang wrote: > mailto:jcliang@chromium.org changed reviewers: > + mailto:afakhry@chromium.org, mailto:derat@chromium.org, mailto:jonross@chromium.org, > mailto:oshima@chromium.org Ping. Can someone review this CL, or loop in appropriate reviewers if I didn't invite the right people in the first place? Thanks.
On 2016/06/03 02:08:18, jcliang wrote: > On 2016/05/27 14:10:56, jcliang wrote: > > mailto:jcliang@chromium.org changed reviewers: > > + mailto:afakhry@chromium.org, mailto:derat@chromium.org, > mailto:jonross@chromium.org, > > mailto:oshima@chromium.org > > Ping. Can someone review this CL, or loop in appropriate reviewers if I didn't > invite the right people in the first place? Thanks. I'd be interested in learning how this switch functions. Is it possible for the switch to be turned off, and therefore no longer in explicit tablet mode, while the device is being held in portrait and nearly vertical? If not we could avoid the explicit forcing of maximized mode. Instead the receiving of the switch could be used to stop listening to accelerometer events in MaximizeModeController
On 2016/06/03 02:18:28, jonross wrote: > On 2016/06/03 02:08:18, jcliang wrote: > > On 2016/05/27 14:10:56, jcliang wrote: > > > mailto:jcliang@chromium.org changed reviewers: > > > + mailto:afakhry@chromium.org, mailto:derat@chromium.org, > > mailto:jonross@chromium.org, > > > mailto:oshima@chromium.org > > > > Ping. Can someone review this CL, or loop in appropriate reviewers if I didn't > > invite the right people in the first place? Thanks. > > I'd be interested in learning how this switch functions. Is it possible for the > switch to be turned off, and therefore no longer in explicit tablet mode, while > the device is being held in portrait and nearly vertical? > > If not we could avoid the explicit forcing of maximized mode. Instead the > receiving of the switch could be used to stop listening to accelerometer events > in MaximizeModeController The tablet mode switch is actually a GMR sensor. It works just like the lid-close switch but in the opposite direction. The case you described is possible and is one of the reason that we need a GMR sensor in additional to the accelerometers. The major issue that the GMR sensor solves on ChromeOS is when you hold the device vertically in portrait mode and then flip the lid to from laptop to tablet. The accelerometers can't be used to calculate the lid angle in this case.
On 2016/06/03 02:31:50, jcliang wrote: > On 2016/06/03 02:18:28, jonross wrote: > > On 2016/06/03 02:08:18, jcliang wrote: > > > On 2016/05/27 14:10:56, jcliang wrote: > > > > mailto:jcliang@chromium.org changed reviewers: > > > > + mailto:afakhry@chromium.org, mailto:derat@chromium.org, > > > mailto:jonross@chromium.org, > > > > mailto:oshima@chromium.org > > > > > > Ping. Can someone review this CL, or loop in appropriate reviewers if I > didn't > > > invite the right people in the first place? Thanks. > > > > I'd be interested in learning how this switch functions. Is it possible for > the > > switch to be turned off, and therefore no longer in explicit tablet mode, > while > > the device is being held in portrait and nearly vertical? > > > > If not we could avoid the explicit forcing of maximized mode. Instead the > > receiving of the switch could be used to stop listening to accelerometer > events > > in MaximizeModeController > > The tablet mode switch is actually a GMR sensor. It works just like the > lid-close switch but in the opposite direction. > > The case you described is possible and is one of the reason that we need a GMR > sensor in additional to the accelerometers. The major issue that the GMR sensor > solves on ChromeOS is when you hold the device vertically in portrait mode and > then flip the lid to from laptop to tablet. The accelerometers can't be used to > calculate the lid angle in this case. That is another nasty edge case. Though if combining both this sensor and the accelerometer, I wonder if we might still be able to accidentally re-enter maximize mode
https://codereview.chromium.org/2020553003/diff/1/ash/wm/maximize_mode/maximi... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/1/ash/wm/maximize_mode/maximi... ash/wm/maximize_mode/maximize_mode_controller.cc:240: lid_open_past_180_ = true; I think that this should be set when the tablet mode on event is received. Then the tablet mode early exit could be in HandleHingeRotation. https://codereview.chromium.org/2020553003/diff/1/ash/wm/maximize_mode/maximi... ash/wm/maximize_mode/maximize_mode_controller.cc:354: return; With this here it is possible for the tablet mode to be active, and for the keyboard to be reenabled on line 343 https://codereview.chromium.org/2020553003/diff/1/ash/wm/maximize_mode/maximi... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/2020553003/diff/1/ash/wm/maximize_mode/maximi... ash/wm/maximize_mode/maximize_mode_controller.h:177: bool force_maximize_mode_; We have ways to forcibly trigger maximize mode, and disregard accelerometers. See EnterMaximizeMode checking for a testing flag, and ignoring accelerometers. Can this be named after the sensor? Eg: tablet_mode_
https://codereview.chromium.org/2020553003/diff/1/ash/wm/maximize_mode/maximi... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/1/ash/wm/maximize_mode/maximi... ash/wm/maximize_mode/maximize_mode_controller.cc:240: lid_open_past_180_ = true; On 2016/06/03 03:27:19, jonross wrote: > I think that this should be set when the tablet mode on event is received. > > Then the tablet mode early exit could be in HandleHingeRotation. If lid_open_past_180_ is set when the tablet mode on event is received, there will be a race between HandleHingeRotation() and here. HandleHingeRotation() could unset lid_open_past_180_ right after we set it here. https://codereview.chromium.org/2020553003/diff/1/ash/wm/maximize_mode/maximi... ash/wm/maximize_mode/maximize_mode_controller.cc:354: return; On 2016/06/03 03:27:19, jonross wrote: > With this here it is possible for the tablet mode to be active, and for the > keyboard to be reenabled on line 343 Done. https://codereview.chromium.org/2020553003/diff/1/ash/wm/maximize_mode/maximi... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/2020553003/diff/1/ash/wm/maximize_mode/maximi... ash/wm/maximize_mode/maximize_mode_controller.h:177: bool force_maximize_mode_; On 2016/06/03 03:27:19, jonross wrote: > We have ways to forcibly trigger maximize mode, and disregard accelerometers. > See EnterMaximizeMode checking for a testing flag, and ignoring accelerometers. Do you mean switches::kAshEnableTouchViewTesting? I thought this is a command-line switch so we can't use it. > > Can this be named after the sensor? > > Eg: tablet_mode_ Done.
please be sure to add unit tests for the new behavior. https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:240: lid_open_past_180_ = true; this doesn't seem right. this member feels like it should only be updated by HandleHingeRotation(). https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:344: if (tablet_mode_) i don't think i've looked at this code before, but adding this check here, rather than in the places that call LeaveMaximizeMode(), seems a bit weird. i'd prefer that the logic to decide whether we should be in maximize mode or not didn't live inside of this method. (a more common approach might be something like an UpdateState() method that looks at various factors to decide which mode it should be in and then enters or leaves the mode.) https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:174: // Forces maximize mode. Used along with the tablet_mode switch on Chrome OS. what's "tablet_mode" referring to here? if it's a variable name, please write it as |tablet_mode|; otherwise, please use "tablet mode". (if it's referring to the kernel event code, that'd be SW_TABLET_MODE, right?) https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:177: bool tablet_mode_; how about something like "tablet_mode_switch_is_on_" instead? that makes it a bit clearer where this comes from.
https://codereview.chromium.org/2020553003/diff/1/ash/wm/maximize_mode/maximi... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/2020553003/diff/1/ash/wm/maximize_mode/maximi... ash/wm/maximize_mode/maximize_mode_controller.h:177: bool force_maximize_mode_; On 2016/06/03 05:28:49, jcliang wrote: > On 2016/06/03 03:27:19, jonross wrote: > > We have ways to forcibly trigger maximize mode, and disregard accelerometers. > > See EnterMaximizeMode checking for a testing flag, and ignoring > accelerometers. > > Do you mean switches::kAshEnableTouchViewTesting? I thought this is a > command-line switch so we can't use it. > > > > > Can this be named after the sensor? > > > > Eg: tablet_mode_ > > Done. Yeah we can't use the switch here, I was just bringing up the naming, as we already have a 'force' effectively. https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:240: lid_open_past_180_ = true; On 2016/06/03 15:00:18, Daniel Erat wrote: > this doesn't seem right. this member feels like it should only be updated by > HandleHingeRotation(). Though the lid_open_past_180_ may actually be what the sensor is telling us. https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:344: if (tablet_mode_) On 2016/06/03 15:00:18, Daniel Erat wrote: > i don't think i've looked at this code before, but adding this check here, > rather than in the places that call LeaveMaximizeMode(), seems a bit weird. i'd > prefer that the logic to decide whether we should be in maximize mode or not > didn't live inside of this method. (a more common approach might be something > like an UpdateState() method that looks at various factors to decide which mode > it should be in and then enters or leaves the mode.) The 'update state' method is HandleHingeRotation. Which could be a more appropriate place for this exit. These methods are separated out for use with a testing flag, and for unittesting.
https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:344: if (tablet_mode_) On 2016/06/03 16:27:24, jonross wrote: > On 2016/06/03 15:00:18, Daniel Erat wrote: > > i don't think i've looked at this code before, but adding this check here, > > rather than in the places that call LeaveMaximizeMode(), seems a bit weird. > i'd > > prefer that the logic to decide whether we should be in maximize mode or not > > didn't live inside of this method. (a more common approach might be something > > like an UpdateState() method that looks at various factors to decide which > mode > > it should be in and then enters or leaves the mode.) > > The 'update state' method is HandleHingeRotation. Which could be a more > appropriate place for this exit. These methods are separated out for use with a > testing flag, and for unittesting. what if the "decide what to do" code from HandleHingeRotation were separated out into a new UpdateState or SelectMode or whatever method that HandleHingeRotation, TabletModeEventReceived, etc. call after updating the lid_open_past_180_, tablet_mode_, etc. members?
What's the best way to run the unit tests? Is there a way to run the chromeos-only unit tests on the host? https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:240: lid_open_past_180_ = true; On 2016/06/03 16:27:24, jonross wrote: > On 2016/06/03 15:00:18, Daniel Erat wrote: > > this doesn't seem right. this member feels like it should only be updated by > > HandleHingeRotation(). > > Though the lid_open_past_180_ may actually be what the sensor is telling us. This is removed. I use another tablet_mode_swithc_was_on_ to achieve the same result in HandleHingeRotation. https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:344: if (tablet_mode_) On 2016/06/03 16:59:51, Daniel Erat wrote: > On 2016/06/03 16:27:24, jonross wrote: > > On 2016/06/03 15:00:18, Daniel Erat wrote: > > > i don't think i've looked at this code before, but adding this check here, > > > rather than in the places that call LeaveMaximizeMode(), seems a bit weird. > > i'd > > > prefer that the logic to decide whether we should be in maximize mode or not > > > didn't live inside of this method. (a more common approach might be > something > > > like an UpdateState() method that looks at various factors to decide which > > mode > > > it should be in and then enters or leaves the mode.) > > > > The 'update state' method is HandleHingeRotation. Which could be a more > > appropriate place for this exit. These methods are separated out for use with > a > > testing flag, and for unittesting. > > what if the "decide what to do" code from HandleHingeRotation were separated out > into a new UpdateState or SelectMode or whatever method that > HandleHingeRotation, TabletModeEventReceived, etc. call after updating the > lid_open_past_180_, tablet_mode_, etc. members? This is removed. Everything is handled in HandleHingeRotation now. https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:174: // Forces maximize mode. Used along with the tablet_mode switch on Chrome OS. On 2016/06/03 15:00:19, Daniel Erat wrote: > what's "tablet_mode" referring to here? if it's a variable name, please write it > as |tablet_mode|; otherwise, please use "tablet mode". (if it's referring to the > kernel event code, that'd be SW_TABLET_MODE, right?) Done. https://codereview.chromium.org/2020553003/diff/20001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:177: bool tablet_mode_; On 2016/06/03 15:00:18, Daniel Erat wrote: > how about something like "tablet_mode_switch_is_on_" instead? that makes it a > bit clearer where this comes from. Done.
in the past, i built ash_unittests and then ran e.g. "out/Debug/ash_unittests --gtest_filter=PowerStatus\*" to run individual suites of tests. i don't know if this code still lives in that test binary. https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:238: chromeos::AccelerometerReader::GetInstance()->AddObserver(this); this approach seems problematic. if powerd sends multiple notifications (e.g. because it crashes and restarts, or the switch is malfunctioning, or there's a kernel bug, or there's a firmware bug, or etc.), won't you double-add or double-remove the observer? i wouldn't be surprised if that causing a failed assert and crashes chrome. what's wrong with the approach that i suggested? https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:176: bool tablet_mode_switch_was_on_; it would probably be better to track "is_on" instead of "was_on". using temporarily-set member variables to pass state from one method to another is confusing: someone looking at the member has no idea *when* the switch was on.
In ash/BUILD.gn maximize_mode_controller_unittest is not built if !is_chromeose, so building against the host doesn't give me the unit test I need. I've tried building ash_unittests for the target board and then deploy ash_unittest and its required files to the DUT, but it segfaults when I run the unit test. I guess I'll try to hack BUILD.gn and make it build maximize_mode_controller_unittest on the host if there's no bettery way. In fact ash_unittests does not build cleanly. I need to comment out or hack wm/window_modality_controller_unittest.cc as it generates a warning that fails the build. This makes me wonder if we're actually running these unit tests... https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:238: chromeos::AccelerometerReader::GetInstance()->AddObserver(this); On 2016/06/06 15:24:55, Daniel Erat wrote: > this approach seems problematic. if powerd sends multiple notifications (e.g. > because it crashes and restarts, or the switch is malfunctioning, or there's a > kernel bug, or there's a firmware bug, or etc.), won't you double-add or > double-remove the observer? i wouldn't be surprised if that causing a failed > assert and crashes chrome. > > what's wrong with the approach that i suggested? I didn't realize that {Add|Remove}Observer() are not protected against double {add|remove}. The remove/add observers are not necessary and are just some enhancements suggested by Jonathan. We can factor out the state changing logic in HandleHingeRotation() to another UpdateState() function, but it doesn't really solve the issue here. The problem we have here is that we're depending on lid_open_past_180_ = true to leave maximize mode, and we need to check if we need to leave maximize mode after tablet mode switch if off. Setting lid_open_past_180_ or tablet_mode_switch_was_on_ both were for this reason. Now I think to solve this issue we should remove lid_open_past_180_, and replace it with IsMaximizeModeManagerEnabled(). https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.h (right): https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.h:176: bool tablet_mode_switch_was_on_; On 2016/06/06 15:24:55, Daniel Erat wrote: > it would probably be better to track "is_on" instead of "was_on". using > temporarily-set member variables to pass state from one method to another is > confusing: someone looking at the member has no idea *when* the switch was on. Yes I agree. This is improved after we remove lid_open_past_180_.
On 2016/06/06 16:48:37, jcliang wrote: > In ash/BUILD.gn maximize_mode_controller_unittest is not built if !is_chromeose, > so building against the host doesn't give me the unit test I need. I've tried > building ash_unittests for the target board and then deploy ash_unittest and its > required files to the DUT, but it segfaults when I run the unit test. I guess > I'll try to hack BUILD.gn and make it build maximize_mode_controller_unittest on > the host if there's no bettery way. ash_unittests builds & runs on linux desktop/chromium waterfall, not on the device. You should mock impl if needed. > > In fact ash_unittests does not build cleanly. I need to comment out or hack > wm/window_modality_controller_unittest.cc as it generates a warning that fails > the build. This makes me wonder if we're actually running these unit tests... > > https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... > File ash/wm/maximize_mode/maximize_mode_controller.cc (right): > > https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... > ash/wm/maximize_mode/maximize_mode_controller.cc:238: > chromeos::AccelerometerReader::GetInstance()->AddObserver(this); > On 2016/06/06 15:24:55, Daniel Erat wrote: > > this approach seems problematic. if powerd sends multiple notifications (e.g. > > because it crashes and restarts, or the switch is malfunctioning, or there's a > > kernel bug, or there's a firmware bug, or etc.), won't you double-add or > > double-remove the observer? i wouldn't be surprised if that causing a failed > > assert and crashes chrome. > > > > what's wrong with the approach that i suggested? > > I didn't realize that {Add|Remove}Observer() are not protected against double > {add|remove}. The remove/add observers are not necessary and are just some > enhancements suggested by Jonathan. > > We can factor out the state changing logic in HandleHingeRotation() to another > UpdateState() function, but it doesn't really solve the issue here. The problem > we have here is that we're depending on lid_open_past_180_ = true to leave > maximize mode, and we need to check if we need to leave maximize mode after > tablet mode switch if off. Setting lid_open_past_180_ or > tablet_mode_switch_was_on_ both were for this reason. > > Now I think to solve this issue we should remove lid_open_past_180_, and replace > it with IsMaximizeModeManagerEnabled(). > > https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... > File ash/wm/maximize_mode/maximize_mode_controller.h (right): > > https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... > ash/wm/maximize_mode/maximize_mode_controller.h:176: bool > tablet_mode_switch_was_on_; > On 2016/06/06 15:24:55, Daniel Erat wrote: > > it would probably be better to track "is_on" instead of "was_on". using > > temporarily-set member variables to pass state from one method to another is > > confusing: someone looking at the member has no idea *when* the switch was on. > > Yes I agree. This is improved after we remove lid_open_past_180_.
On Tue, Jun 7, 2016 at 12:52 AM, <oshima@chromium.org> wrote: > On 2016/06/06 16:48:37, jcliang wrote: >> In ash/BUILD.gn maximize_mode_controller_unittest is not built if > !is_chromeose, >> so building against the host doesn't give me the unit test I need. I've >> tried >> building ash_unittests for the target board and then deploy ash_unittest >> and > its >> required files to the DUT, but it segfaults when I run the unit test. I >> guess >> I'll try to hack BUILD.gn and make it build >> maximize_mode_controller_unittest > on >> the host if there's no bettery way. > > ash_unittests builds & runs on linux desktop/chromium waterfall, not on the > device. > You should mock impl if needed. Thanks Oshima. Do you happen to know the proper way to force is_chromeos in gn on linux desktop? > > >> >> In fact ash_unittests does not build cleanly. I need to comment out or >> hack >> wm/window_modality_controller_unittest.cc as it generates a warning that >> fails >> the build. This makes me wonder if we're actually running these unit >> tests... >> >> > https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... >> File ash/wm/maximize_mode/maximize_mode_controller.cc (right): >> >> > https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... >> ash/wm/maximize_mode/maximize_mode_controller.cc:238: >> chromeos::AccelerometerReader::GetInstance()->AddObserver(this); >> On 2016/06/06 15:24:55, Daniel Erat wrote: >> > this approach seems problematic. if powerd sends multiple notifications > (e.g. >> > because it crashes and restarts, or the switch is malfunctioning, or >> > there's > a >> > kernel bug, or there's a firmware bug, or etc.), won't you double-add or >> > double-remove the observer? i wouldn't be surprised if that causing a >> > failed >> > assert and crashes chrome. >> > >> > what's wrong with the approach that i suggested? >> >> I didn't realize that {Add|Remove}Observer() are not protected against >> double >> {add|remove}. The remove/add observers are not necessary and are just some >> enhancements suggested by Jonathan. >> >> We can factor out the state changing logic in HandleHingeRotation() to >> another >> UpdateState() function, but it doesn't really solve the issue here. The > problem >> we have here is that we're depending on lid_open_past_180_ = true to leave >> maximize mode, and we need to check if we need to leave maximize mode >> after >> tablet mode switch if off. Setting lid_open_past_180_ or >> tablet_mode_switch_was_on_ both were for this reason. >> >> Now I think to solve this issue we should remove lid_open_past_180_, and > replace >> it with IsMaximizeModeManagerEnabled(). >> >> > https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... >> File ash/wm/maximize_mode/maximize_mode_controller.h (right): >> >> > https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... >> ash/wm/maximize_mode/maximize_mode_controller.h:176: bool >> tablet_mode_switch_was_on_; >> On 2016/06/06 15:24:55, Daniel Erat wrote: >> > it would probably be better to track "is_on" instead of "was_on". using >> > temporarily-set member variables to pass state from one method to >> > another is >> > confusing: someone looking at the member has no idea *when* the switch >> > was > on. >> >> Yes I agree. This is improved after we remove lid_open_past_180_. > > > > https://codereview.chromium.org/2020553003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:214: return; i'm wondering if this early return can cause problems. i'm not familiar with this code, but HandleHingeRotation looks like it updates some other members (mostly related to smoothing readings, i think). is it really okay to skip all of that? https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:158: void ToggleTabletMode(const bool on) { nit: rename this to SetTabletMode (toggling would just reverse the current state) and remove "const" from param https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:163: bool IsTabletModeOn() { i'd delete this method and the calls to it. checking the class's internal members doesn't seem useful and makes it hard to change the class later; you really just want to check that it's setting maximized mode correctly (which you're already doing). https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:285: EXPECT_FALSE(IsMaximizeModeStarted()); why didn't this trigger maximize mode? it is because not enough time had passed? https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:300: } can you open the lid to a large angle at the end of this test while the switch is off (after advancing time, if necessary) to make sure that that still triggers maximize mode?
On Mon, Jun 6, 2016 at 10:21 AM, Ricky Liang <jcliang@chromium.org> wrote: > On Tue, Jun 7, 2016 at 12:52 AM, <oshima@chromium.org> wrote: > > On 2016/06/06 16:48:37, jcliang wrote: > >> In ash/BUILD.gn maximize_mode_controller_unittest is not built if > > !is_chromeose, > >> so building against the host doesn't give me the unit test I need. I've > >> tried > >> building ash_unittests for the target board and then deploy ash_unittest > >> and > > its > >> required files to the DUT, but it segfaults when I run the unit test. I > >> guess > >> I'll try to hack BUILD.gn and make it build > >> maximize_mode_controller_unittest > > on > >> the host if there's no bettery way. > > > > ash_unittests builds & runs on linux desktop/chromium waterfall, not on > the > > device. > > You should mock impl if needed. > > Thanks Oshima. Do you happen to know the proper way to force > is_chromeos in gn on linux desktop? > target_os = " chromeos" It *may* still build with target_os = linux, but it should fail with the message that it won't run with normal linux config. If it wasn't, please let me know. > > > > > >> > >> In fact ash_unittests does not build cleanly. I need to comment out or > >> hack > >> wm/window_modality_controller_unittest.cc as it generates a warning that > >> fails > >> the build. This makes me wonder if we're actually running these unit > >> tests... > >> > >> > > > https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... > >> File ash/wm/maximize_mode/maximize_mode_controller.cc (right): > >> > >> > > > https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... > >> ash/wm/maximize_mode/maximize_mode_controller.cc:238: > >> chromeos::AccelerometerReader::GetInstance()->AddObserver(this); > >> On 2016/06/06 15:24:55, Daniel Erat wrote: > >> > this approach seems problematic. if powerd sends multiple > notifications > > (e.g. > >> > because it crashes and restarts, or the switch is malfunctioning, or > >> > there's > > a > >> > kernel bug, or there's a firmware bug, or etc.), won't you double-add > or > >> > double-remove the observer? i wouldn't be surprised if that causing a > >> > failed > >> > assert and crashes chrome. > >> > > >> > what's wrong with the approach that i suggested? > >> > >> I didn't realize that {Add|Remove}Observer() are not protected against > >> double > >> {add|remove}. The remove/add observers are not necessary and are just > some > >> enhancements suggested by Jonathan. > >> > >> We can factor out the state changing logic in HandleHingeRotation() to > >> another > >> UpdateState() function, but it doesn't really solve the issue here. The > > problem > >> we have here is that we're depending on lid_open_past_180_ = true to > leave > >> maximize mode, and we need to check if we need to leave maximize mode > >> after > >> tablet mode switch if off. Setting lid_open_past_180_ or > >> tablet_mode_switch_was_on_ both were for this reason. > >> > >> Now I think to solve this issue we should remove lid_open_past_180_, and > > replace > >> it with IsMaximizeModeManagerEnabled(). > >> > >> > > > https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... > >> File ash/wm/maximize_mode/maximize_mode_controller.h (right): > >> > >> > > > https://codereview.chromium.org/2020553003/diff/40001/ash/wm/maximize_mode/ma... > >> ash/wm/maximize_mode/maximize_mode_controller.h:176: bool > >> tablet_mode_switch_was_on_; > >> On 2016/06/06 15:24:55, Daniel Erat wrote: > >> > it would probably be better to track "is_on" instead of "was_on". > using > >> > temporarily-set member variables to pass state from one method to > >> > another is > >> > confusing: someone looking at the member has no idea *when* the switch > >> > was > > on. > >> > >> Yes I agree. This is improved after we remove lid_open_past_180_. > > > > > > > > https://codereview.chromium.org/2020553003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:214: return; On 2016/06/06 17:23:07, Daniel Erat wrote: > i'm wondering if this early return can cause problems. i'm not familiar with > this code, but HandleHingeRotation looks like it updates some other members > (mostly related to smoothing readings, i think). is it really okay to skip all > of that? I think the {base|lid}_smoothed_ essentially act like filters to discard unstable readings, e.g. the readings got when the device is held vertical. I don't see a need to continuously compute the smoothed readings. https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right): https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:158: void ToggleTabletMode(const bool on) { On 2016/06/06 17:23:08, Daniel Erat wrote: > nit: rename this to SetTabletMode (toggling would just reverse the current > state) and remove "const" from param Done. https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:163: bool IsTabletModeOn() { On 2016/06/06 17:23:08, Daniel Erat wrote: > i'd delete this method and the calls to it. checking the class's internal > members doesn't seem useful and makes it hard to change the class later; you > really just want to check that it's setting maximized mode correctly (which > you're already doing). Done. https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:285: EXPECT_FALSE(IsMaximizeModeStarted()); On 2016/06/06 17:23:07, Daniel Erat wrote: > why didn't this trigger maximize mode? it is because not enough time had passed? I meant to trigger an unstable reading and make sure the state doesn't change. I've added a HoldDeviceVertical() to make the intent clear. https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:300: } On 2016/06/06 17:23:07, Daniel Erat wrote: > can you open the lid to a large angle at the end of this test while the switch > is off (after advancing time, if necessary) to make sure that that still > triggers maximize mode? Done.
(no more comments from me beyond these, but i'll let jon approve it since i still don't understand all of the logic in this code) https://codereview.chromium.org/2020553003/diff/80001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/80001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:234: if (on) { nit: you can simplify this to: tablet_mode_switch_is_on_ = on; if (on && !IsMaximize...) Enter... https://codereview.chromium.org/2020553003/diff/80001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:275: std::abs(base_reading.x()) > kHingeVerticalSmoothingMaximum; could you add a comment here? i don't understand exactly what this represents.
https://codereview.chromium.org/2020553003/diff/80001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/80001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:234: if (on) { On 2016/06/06 18:57:14, Daniel Erat wrote: > nit: you can simplify this to: > > tablet_mode_switch_is_on_ = on; > if (on && !IsMaximize...) > Enter... Done. https://codereview.chromium.org/2020553003/diff/80001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:275: std::abs(base_reading.x()) > kHingeVerticalSmoothingMaximum; On 2016/06/06 18:57:14, Daniel Erat wrote: > could you add a comment here? i don't understand exactly what this represents. Done.
https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:214: return; On 2016/06/06 18:19:25, jcliang wrote: > On 2016/06/06 17:23:07, Daniel Erat wrote: > > i'm wondering if this early return can cause problems. i'm not familiar with > > this code, but HandleHingeRotation looks like it updates some other members > > (mostly related to smoothing readings, i think). is it really okay to skip all > > of that? > > I think the {base|lid}_smoothed_ essentially act like filters to discard > unstable readings, e.g. the readings got when the device is held vertical. I > don't see a need to continuously compute the smoothed readings. We continuously compute them as the motion from flat to near vertical can be quite quick. We didn't want to have false exits due to accelerometer jank right after this motion. In general the smoothed accelerometer data leads to better decisions when there is no tablet switch. If the user flips the switch while nearly vertical the device may be flaky in deciding which mode to be in, without a history of accelerometer values. https://codereview.chromium.org/2020553003/diff/100001/ash/wm/maximize_mode/m... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/100001/ash/wm/maximize_mode/m... ash/wm/maximize_mode/maximize_mode_controller.cc:296: bool is_angle_stable = !is_angle_reliable && lid_angle >= kMinStableAngle && On devices without a tablet mode switch, does this prevent entering maximize mode? (Eg. on a veyron_minnie)
https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:214: return; On 2016/06/07 15:27:15, jonross wrote: > On 2016/06/06 18:19:25, jcliang wrote: > > On 2016/06/06 17:23:07, Daniel Erat wrote: > > > i'm wondering if this early return can cause problems. i'm not familiar with > > > this code, but HandleHingeRotation looks like it updates some other members > > > (mostly related to smoothing readings, i think). is it really okay to skip > all > > > of that? > > > > I think the {base|lid}_smoothed_ essentially act like filters to discard > > unstable readings, e.g. the readings got when the device is held vertical. I > > don't see a need to continuously compute the smoothed readings. > > We continuously compute them as the motion from flat to near vertical can be > quite quick. We didn't want to have false exits due to accelerometer jank right > after this motion. > > In general the smoothed accelerometer data leads to better decisions when there > is no tablet switch. > > If the user flips the switch while nearly vertical the device may be flaky in > deciding which mode to be in, without a history of accelerometer values. Should I remove the early return here? The smoothed values seem to converge to the right lid angle fairly quickly if the device is horizontal. I noticed that when the device is nearly vertical the smoothed values keep the computed lid angle stay at the last value seen before the device is close to vertical. In other words, if I open the lid to 90 degrees when the device is horizontal, flip the device vertically, and open the lid to 360 degrees, the smoothed values will give me lid_angle=90 ever since the device is vertical. In the above example if the device has a tablet mode switch, the switch is on when the lid angle is 360 degrees. If the device is still vertical when the switch is off the device will exit maximize mode since the smoothed values give lid_angle=90. So I added a is_angle_reliable check in HandleHingeRotation() to ignore any lid angles we get while the device is vertical, which keeps the device in maximize mode when switch is off until we have a reliable lid angle to decide the new state. https://codereview.chromium.org/2020553003/diff/100001/ash/wm/maximize_mode/m... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/100001/ash/wm/maximize_mode/m... ash/wm/maximize_mode/maximize_mode_controller.cc:296: bool is_angle_stable = !is_angle_reliable && lid_angle >= kMinStableAngle && On 2016/06/07 15:27:15, jonross wrote: > On devices without a tablet mode switch, does this prevent entering maximize > mode? (Eg. on a veyron_minnie) Ah, there's a typo here. I forgot to invert the condition when I renamed the variable. The boolean variable is_angle_reliable should be inverted in line 272 above and this should be bool is_angle_stable = is_angle_reliable && lid_angle >= kMinStableAngle && ... I'll fix this in the next patch roll. This is_angle_reliable only prevents state transition when the device is vertical, so devices without tablet mode switch should function just as before.
https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:214: return; On 2016/06/07 16:57:23, jcliang wrote: > On 2016/06/07 15:27:15, jonross wrote: > > On 2016/06/06 18:19:25, jcliang wrote: > > > On 2016/06/06 17:23:07, Daniel Erat wrote: > > > > i'm wondering if this early return can cause problems. i'm not familiar > with > > > > this code, but HandleHingeRotation looks like it updates some other > members > > > > (mostly related to smoothing readings, i think). is it really okay to skip > > all > > > > of that? > > > > > > I think the {base|lid}_smoothed_ essentially act like filters to discard > > > unstable readings, e.g. the readings got when the device is held vertical. I > > > don't see a need to continuously compute the smoothed readings. > > > > We continuously compute them as the motion from flat to near vertical can be > > quite quick. We didn't want to have false exits due to accelerometer jank > right > > after this motion. > > > > In general the smoothed accelerometer data leads to better decisions when > there > > is no tablet switch. > > > > If the user flips the switch while nearly vertical the device may be flaky in > > deciding which mode to be in, without a history of accelerometer values. > > Should I remove the early return here? The smoothed values seem to converge to > the right lid angle fairly quickly if the device is horizontal. > > I noticed that when the device is nearly vertical the smoothed values keep the > computed lid angle stay at the last value seen before the device is close to > vertical. In other words, if I open the lid to 90 degrees when the device is > horizontal, flip the device vertically, and open the lid to 360 degrees, the > smoothed values will give me lid_angle=90 ever since the device is vertical. > > In the above example if the device has a tablet mode switch, the switch is on > when the lid angle is 360 degrees. If the device is still vertical when the > switch is off the device will exit maximize mode since the smoothed values give > lid_angle=90. So I added a is_angle_reliable check in HandleHingeRotation() to > ignore any lid angles we get while the device is vertical, which keeps the > device in maximize mode when switch is off until we have a reliable lid angle to > decide the new state. I think I would like to see this check after the smoothing has been calculated. While you have added a is_angle_reliable, one problem that occurs is often while vertical, small movements of the device reduce the x-axis component of gravity to be less than 1g. It's during these times that the smoothing helps lock to the previous value. https://codereview.chromium.org/2020553003/diff/100001/ash/wm/maximize_mode/m... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/100001/ash/wm/maximize_mode/m... ash/wm/maximize_mode/maximize_mode_controller.cc:296: bool is_angle_stable = !is_angle_reliable && lid_angle >= kMinStableAngle && On 2016/06/07 16:57:23, jcliang wrote: > On 2016/06/07 15:27:15, jonross wrote: > > On devices without a tablet mode switch, does this prevent entering maximize > > mode? (Eg. on a veyron_minnie) > > Ah, there's a typo here. I forgot to invert the condition when I renamed the > variable. The boolean variable is_angle_reliable should be inverted in line 272 > above and this should be > > bool is_angle_stable = is_angle_reliable && lid_angle >= kMinStableAngle && > ... > > I'll fix this in the next patch roll. This is_angle_reliable only prevents state > transition when the device is vertical, so devices without tablet mode switch > should function just as before. That SGTM
Description was changed from ========== Hook up TABLET_MODE switch to maximize_mode_controller This is part of the patch set that hooks up tablet mode switch to Chrome. If the tablet mode switch is on then Chrome can know for sure that it needs to enable maximize mode (tablet mode). The helps Chrome to decide whether to enable tablet mode in some corner cases where the accelerometer readings are not stable. BUG= http://crosbug.com/p/49114 TEST=Build power_manager and chrome and deploy to my DUT. Make sure that: - The device enters maximize mode when tablet mode switch is on. - The device leaves maximize mode when tablet mode switch is off and lid angle is less than kExitMaximizeModeAngle. Signed-off-by: Ricky Liang <jcliang@chromium.org> ========== to ========== Hook up TABLET_MODE switch to maximize_mode_controller This is part of the patch set that hooks up tablet mode switch to Chrome. If the tablet mode switch is on then Chrome can know for sure that it needs to enable maximize mode (tablet mode). The helps Chrome to decide whether to enable tablet mode in some corner cases where the accelerometer readings are not stable. BUG= http://crosbug.com/p/49114 TEST=unit tests TEST=Build power_manager and chrome and deploy to my DUT. Make sure that: - The device enters maximize mode when tablet mode switch is on. - The device leaves maximize mode when tablet mode switch is off and lid angle is less than kExitMaximizeModeAngle. Signed-off-by: Ricky Liang <jcliang@chromium.org> ==========
https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/60001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:214: return; On 2016/06/07 22:04:42, jonross wrote: > On 2016/06/07 16:57:23, jcliang wrote: > > On 2016/06/07 15:27:15, jonross wrote: > > > On 2016/06/06 18:19:25, jcliang wrote: > > > > On 2016/06/06 17:23:07, Daniel Erat wrote: > > > > > i'm wondering if this early return can cause problems. i'm not familiar > > with > > > > > this code, but HandleHingeRotation looks like it updates some other > > members > > > > > (mostly related to smoothing readings, i think). is it really okay to > skip > > > all > > > > > of that? > > > > > > > > I think the {base|lid}_smoothed_ essentially act like filters to discard > > > > unstable readings, e.g. the readings got when the device is held vertical. > I > > > > don't see a need to continuously compute the smoothed readings. > > > > > > We continuously compute them as the motion from flat to near vertical can be > > > quite quick. We didn't want to have false exits due to accelerometer jank > > right > > > after this motion. > > > > > > In general the smoothed accelerometer data leads to better decisions when > > there > > > is no tablet switch. > > > > > > If the user flips the switch while nearly vertical the device may be flaky > in > > > deciding which mode to be in, without a history of accelerometer values. > > > > Should I remove the early return here? The smoothed values seem to converge to > > the right lid angle fairly quickly if the device is horizontal. > > > > I noticed that when the device is nearly vertical the smoothed values keep the > > computed lid angle stay at the last value seen before the device is close to > > vertical. In other words, if I open the lid to 90 degrees when the device is > > horizontal, flip the device vertically, and open the lid to 360 degrees, the > > smoothed values will give me lid_angle=90 ever since the device is vertical. > > > > In the above example if the device has a tablet mode switch, the switch is on > > when the lid angle is 360 degrees. If the device is still vertical when the > > switch is off the device will exit maximize mode since the smoothed values > give > > lid_angle=90. So I added a is_angle_reliable check in HandleHingeRotation() to > > ignore any lid angles we get while the device is vertical, which keeps the > > device in maximize mode when switch is off until we have a reliable lid angle > to > > decide the new state. > > I think I would like to see this check after the smoothing has been calculated. > > While you have added a is_angle_reliable, one problem that occurs is often while > vertical, small movements of the device reduce the x-axis component of gravity > to be less than 1g. It's during these times that the smoothing helps lock to the > previous value. Done.
The CQ bit was checked by jcliang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020553003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-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.
On 2016/06/08 04:13:22, commit-bot: I haz the power wrote: > Dry run: No L-G-T-M from a valid reviewer yet. > CQ run can only be started by full committers or once the patch has > received an L-G-T-M from a full committer. > Even if an L-G-T-M may have been provided, it was from a non-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. LGTM
The CQ bit was checked by jcliang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020553003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
On 2016/06/08 15:47:37, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux > (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) Looks like I need to roll cros_system_api as well...
Description was changed from ========== Hook up TABLET_MODE switch to maximize_mode_controller This is part of the patch set that hooks up tablet mode switch to Chrome. If the tablet mode switch is on then Chrome can know for sure that it needs to enable maximize mode (tablet mode). The helps Chrome to decide whether to enable tablet mode in some corner cases where the accelerometer readings are not stable. BUG= http://crosbug.com/p/49114 TEST=unit tests TEST=Build power_manager and chrome and deploy to my DUT. Make sure that: - The device enters maximize mode when tablet mode switch is on. - The device leaves maximize mode when tablet mode switch is off and lid angle is less than kExitMaximizeModeAngle. Signed-off-by: Ricky Liang <jcliang@chromium.org> ========== to ========== Hook up TABLET_MODE switch to maximize_mode_controller This is part of the patch set that hooks up tablet mode switch to Chrome. If the tablet mode switch is on then Chrome can know for sure that it needs to enable maximize mode (tablet mode). The helps Chrome to decide whether to enable tablet mode in some corner cases where the accelerometer readings are not stable. BUG=chrome-os-partner:49114 TEST=unit tests TEST=Build power_manager and chrome and deploy to my DUT. Make sure that: - The device enters maximize mode when tablet mode switch is on. - The device leaves maximize mode when tablet mode switch is off and lid angle is less than kExitMaximizeModeAngle. Signed-off-by: Ricky Liang <jcliang@chromium.org> ==========
Description was changed from
==========
Hook up TABLET_MODE switch to maximize_mode_controller
This is part of the patch set that hooks up tablet mode switch
to Chrome. If the tablet mode switch is on then Chrome can know
for sure that it needs to enable maximize mode (tablet mode).
The helps Chrome to decide whether to enable tablet mode in some
corner cases where the accelerometer readings are not stable.
BUG=chrome-os-partner:49114
TEST=unit tests
TEST=Build power_manager and chrome and deploy to my DUT. Make
sure that:
- The device enters maximize mode when tablet mode switch
is on.
- The device leaves maximize mode when tablet mode switch
is off and lid angle is less than kExitMaximizeModeAngle.
Signed-off-by: Ricky Liang <jcliang@chromium.org>
==========
to
==========
Hook up TABLET_MODE switch to maximize_mode_controller
This is part of the patch set that hooks up tablet mode switch
to Chrome. If the tablet mode switch is on then Chrome can know
for sure that it needs to enable maximize mode (tablet mode).
The helps Chrome to decide whether to enable tablet mode in some
corner cases where the accelerometer readings are not stable.
BUG=chrome-os-partner:49114
CQ-DEPEND=CL:2049793002
TEST=unit tests
TEST=Build power_manager and chrome and deploy to my DUT. Make
sure that:
- The device enters maximize mode when tablet mode switch
is on.
- The device leaves maximize mode when tablet mode switch
is off and lid angle is less than kExitMaximizeModeAngle.
Signed-off-by: Ricky Liang <jcliang@chromium.org>
==========
Description was changed from
==========
Hook up TABLET_MODE switch to maximize_mode_controller
This is part of the patch set that hooks up tablet mode switch
to Chrome. If the tablet mode switch is on then Chrome can know
for sure that it needs to enable maximize mode (tablet mode).
The helps Chrome to decide whether to enable tablet mode in some
corner cases where the accelerometer readings are not stable.
BUG=chrome-os-partner:49114
CQ-DEPEND=CL:2049793002
TEST=unit tests
TEST=Build power_manager and chrome and deploy to my DUT. Make
sure that:
- The device enters maximize mode when tablet mode switch
is on.
- The device leaves maximize mode when tablet mode switch
is off and lid angle is less than kExitMaximizeModeAngle.
Signed-off-by: Ricky Liang <jcliang@chromium.org>
==========
to
==========
Hook up TABLET_MODE switch to maximize_mode_controller
This is part of the patch set that hooks up tablet mode switch
to Chrome. If the tablet mode switch is on then Chrome can know
for sure that it needs to enable maximize mode (tablet mode).
The helps Chrome to decide whether to enable tablet mode in some
corner cases where the accelerometer readings are not stable.
BUG=chrome-os-partner:49114
TEST=unit tests
TEST=Build power_manager and chrome and deploy to my DUT. Make
sure that:
- The device enters maximize mode when tablet mode switch
is on.
- The device leaves maximize mode when tablet mode switch
is off and lid angle is less than kExitMaximizeModeAngle.
Signed-off-by: Ricky Liang <jcliang@chromium.org>
==========
The CQ bit was checked by jcliang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020553003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jcliang@chromium.org
The CQ bit was unchecked by jcliang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jonross@chromium.org Link to the patchset: https://codereview.chromium.org/2020553003/#ps140001 (title: "Hook up TABLET_MODE switch to maximize_mode_controller")
The CQ bit was checked by jcliang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020553003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from
==========
Hook up TABLET_MODE switch to maximize_mode_controller
This is part of the patch set that hooks up tablet mode switch
to Chrome. If the tablet mode switch is on then Chrome can know
for sure that it needs to enable maximize mode (tablet mode).
The helps Chrome to decide whether to enable tablet mode in some
corner cases where the accelerometer readings are not stable.
BUG=chrome-os-partner:49114
TEST=unit tests
TEST=Build power_manager and chrome and deploy to my DUT. Make
sure that:
- The device enters maximize mode when tablet mode switch
is on.
- The device leaves maximize mode when tablet mode switch
is off and lid angle is less than kExitMaximizeModeAngle.
Signed-off-by: Ricky Liang <jcliang@chromium.org>
==========
to
==========
Hook up TABLET_MODE switch to maximize_mode_controller
This is part of the patch set that hooks up tablet mode switch
to Chrome. If the tablet mode switch is on then Chrome can know
for sure that it needs to enable maximize mode (tablet mode).
The helps Chrome to decide whether to enable tablet mode in some
corner cases where the accelerometer readings are not stable.
BUG=chrome-os-partner:49114
TEST=unit tests
TEST=Build power_manager and chrome and deploy to my DUT. Make
sure that:
- The device enters maximize mode when tablet mode switch
is on.
- The device leaves maximize mode when tablet mode switch
is off and lid angle is less than kExitMaximizeModeAngle.
Signed-off-by: Ricky Liang <jcliang@chromium.org>
==========
jcliang@chromium.org changed reviewers: + skuhne@chromium.org
Looks like the presubmit check requires LGTM from owners of all the modified
files:
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
ash/wm/maximize_mode/maximize_mode_controller.cc
ash/wm/maximize_mode/maximize_mode_controller.h
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc
chromeos/dbus/power_manager_client.cc
chromeos/dbus/power_manager_client.h
@skuhne, @derat - Could you help?
lgtm
skuhne@chromium.org changed reviewers: + flackr@chromium.org
ash/wm/maximize_mode/* lgtm, but please wait for flackr to have a look about the beyond180 case
ash/wm/maximize_mode/* lgtm, but please wait for flackr to have a look about the beyond180 case
https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... ash/wm/maximize_mode/maximize_mode_controller.cc:272: std::abs(base_reading.x()) <= kHingeVerticalSmoothingMaximum; This should check largest_hinge_acceleration instead to be consistent with smoothing_ratio. Note, smoothing_ratio should also be equal to 1.0f if this is true meaning no change in computed lid angle as well. https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... ash/wm/maximize_mode/maximize_mode_controller.cc:308: if (IsMaximizeModeWindowManagerEnabled() && is_angle_stable && Unfortunately IsMaximizeModeWindowManagerEnabled is not synonymous with lid_open_past_180_. If we want to make this change we should remove EnableMaximizeModeWindowManager and force all state changes through (Enter|Exit)MaximizeMode instead. I do think this is a worthwhile cleanup - having the separate state for testing code is really messy. We can even remove the keyboard shortcut for touchview testing. What will happen is with this change, when you enable the touchview testing flag and use the keyboard shortcut to enable touchview (https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller....) we won't disable the keyboard and touchpad when the lid is opened past 180 degrees.
https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... ash/wm/maximize_mode/maximize_mode_controller.cc:272: std::abs(base_reading.x()) <= kHingeVerticalSmoothingMaximum; On 2016/06/15 07:53:24, flackr wrote: > This should check largest_hinge_acceleration instead to be consistent with > smoothing_ratio. Note, smoothing_ratio should also be equal to 1.0f if this is > true meaning no change in computed lid angle as well. Done. https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... ash/wm/maximize_mode/maximize_mode_controller.cc:308: if (IsMaximizeModeWindowManagerEnabled() && is_angle_stable && On 2016/06/15 07:53:24, flackr wrote: > Unfortunately IsMaximizeModeWindowManagerEnabled is not synonymous with > lid_open_past_180_. If we want to make this change we should remove > EnableMaximizeModeWindowManager and force all state changes through > (Enter|Exit)MaximizeMode instead. I do think this is a worthwhile cleanup - > having the separate state for testing code is really messy. We can even remove > the keyboard shortcut for touchview testing. > > What will happen is with this change, when you enable the touchview testing flag > and use the keyboard shortcut to enable touchview > (https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller....) > we won't disable the keyboard and touchpad when the lid is opened past 180 > degrees. So this change will only impact touchview testing flag + enable touchview with keyboard shortcut + on chromeos, which is limited. I'd like to merge this CL first and leave the clean-up in another follow up CL because this CL is currently blocking other people from rolling cros_system_api in DEPS. Does it sound reasonable to you?
On 2016/06/15 at 08:14:10, jcliang wrote: > https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... > File ash/wm/maximize_mode/maximize_mode_controller.cc (right): > > https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... > ash/wm/maximize_mode/maximize_mode_controller.cc:272: std::abs(base_reading.x()) <= kHingeVerticalSmoothingMaximum; > On 2016/06/15 07:53:24, flackr wrote: > > This should check largest_hinge_acceleration instead to be consistent with > > smoothing_ratio. Note, smoothing_ratio should also be equal to 1.0f if this is > > true meaning no change in computed lid angle as well. > > Done. > > https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... > ash/wm/maximize_mode/maximize_mode_controller.cc:308: if (IsMaximizeModeWindowManagerEnabled() && is_angle_stable && > On 2016/06/15 07:53:24, flackr wrote: > > Unfortunately IsMaximizeModeWindowManagerEnabled is not synonymous with > > lid_open_past_180_. If we want to make this change we should remove > > EnableMaximizeModeWindowManager and force all state changes through > > (Enter|Exit)MaximizeMode instead. I do think this is a worthwhile cleanup - > > having the separate state for testing code is really messy. We can even remove > > the keyboard shortcut for touchview testing. > > > > What will happen is with this change, when you enable the touchview testing flag > > and use the keyboard shortcut to enable touchview > > (https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller....) > > we won't disable the keyboard and touchpad when the lid is opened past 180 > > degrees. > > So this change will only impact touchview testing flag + enable touchview with keyboard shortcut + on chromeos, which is limited. I'd like to merge this CL first and leave the clean-up in another follow up CL because this CL is currently blocking other people from rolling cros_system_api in DEPS. Does it sound reasonable to you? Sure, but if we're removing lid_open_past_180_ can you add a TODO with a bug to remove / not expose EnableMaximizeModeWindowManager and expose and use Enter/Exit maximize mode instead?
On 2016/06/15 08:23:41, flackr wrote: > On 2016/06/15 at 08:14:10, jcliang wrote: > > > https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... > > File ash/wm/maximize_mode/maximize_mode_controller.cc (right): > > > > > https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... > > ash/wm/maximize_mode/maximize_mode_controller.cc:272: > std::abs(base_reading.x()) <= kHingeVerticalSmoothingMaximum; > > On 2016/06/15 07:53:24, flackr wrote: > > > This should check largest_hinge_acceleration instead to be consistent with > > > smoothing_ratio. Note, smoothing_ratio should also be equal to 1.0f if this > is > > > true meaning no change in computed lid angle as well. > > > > Done. > > > > > https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... > > ash/wm/maximize_mode/maximize_mode_controller.cc:308: if > (IsMaximizeModeWindowManagerEnabled() && is_angle_stable && > > On 2016/06/15 07:53:24, flackr wrote: > > > Unfortunately IsMaximizeModeWindowManagerEnabled is not synonymous with > > > lid_open_past_180_. If we want to make this change we should remove > > > EnableMaximizeModeWindowManager and force all state changes through > > > (Enter|Exit)MaximizeMode instead. I do think this is a worthwhile cleanup - > > > having the separate state for testing code is really messy. We can even > remove > > > the keyboard shortcut for touchview testing. > > > > > > What will happen is with this change, when you enable the touchview testing > flag > > > and use the keyboard shortcut to enable touchview > > > > (https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller....) > > > we won't disable the keyboard and touchpad when the lid is opened past 180 > > > degrees. > > > > So this change will only impact touchview testing flag + enable touchview with > keyboard shortcut + on chromeos, which is limited. I'd like to merge this CL > first and leave the clean-up in another follow up CL because this CL is > currently blocking other people from rolling cros_system_api in DEPS. Does it > sound reasonable to you? > > Sure, but if we're removing lid_open_past_180_ can you add a TODO with a bug to > remove / not expose EnableMaximizeModeWindowManager and expose and use > Enter/Exit maximize mode instead? I added a TODO above EnableMaximizeModeWindowManager. Thanks!
On 2016/06/15 at 08:53:29, jcliang wrote: > On 2016/06/15 08:23:41, flackr wrote: > > On 2016/06/15 at 08:14:10, jcliang wrote: > > > > > https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... > > > File ash/wm/maximize_mode/maximize_mode_controller.cc (right): > > > > > > > > https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... > > > ash/wm/maximize_mode/maximize_mode_controller.cc:272: > > std::abs(base_reading.x()) <= kHingeVerticalSmoothingMaximum; > > > On 2016/06/15 07:53:24, flackr wrote: > > > > This should check largest_hinge_acceleration instead to be consistent with > > > > smoothing_ratio. Note, smoothing_ratio should also be equal to 1.0f if this > > is > > > > true meaning no change in computed lid angle as well. > > > > > > Done. > > > > > > > > https://codereview.chromium.org/2020553003/diff/140001/ash/wm/maximize_mode/m... > > > ash/wm/maximize_mode/maximize_mode_controller.cc:308: if > > (IsMaximizeModeWindowManagerEnabled() && is_angle_stable && > > > On 2016/06/15 07:53:24, flackr wrote: > > > > Unfortunately IsMaximizeModeWindowManagerEnabled is not synonymous with > > > > lid_open_past_180_. If we want to make this change we should remove > > > > EnableMaximizeModeWindowManager and force all state changes through > > > > (Enter|Exit)MaximizeMode instead. I do think this is a worthwhile cleanup - > > > > having the separate state for testing code is really messy. We can even > > remove > > > > the keyboard shortcut for touchview testing. > > > > > > > > What will happen is with this change, when you enable the touchview testing > > flag > > > > and use the keyboard shortcut to enable touchview > > > > > > (https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller....) > > > > we won't disable the keyboard and touchpad when the lid is opened past 180 > > > > degrees. > > > > > > So this change will only impact touchview testing flag + enable touchview with > > keyboard shortcut + on chromeos, which is limited. I'd like to merge this CL > > first and leave the clean-up in another follow up CL because this CL is > > currently blocking other people from rolling cros_system_api in DEPS. Does it > > sound reasonable to you? > > > > Sure, but if we're removing lid_open_past_180_ can you add a TODO with a bug to > > remove / not expose EnableMaximizeModeWindowManager and expose and use > > Enter/Exit maximize mode instead? > > I added a TODO above EnableMaximizeModeWindowManager. Thanks! Thanks. LGTM.
The CQ bit was checked by jcliang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, skuhne@chromium.org, jonross@chromium.org Link to the patchset: https://codereview.chromium.org/2020553003/#ps160001 (title: "Hook up TABLET_MODE switch to maximize_mode_controller")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020553003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by jcliang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020553003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
oshima@chromium.org changed reviewers: - oshima@chromium.org
The CQ bit was checked by jcliang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020553003/160001
Message was sent while issue was closed.
Description was changed from
==========
Hook up TABLET_MODE switch to maximize_mode_controller
This is part of the patch set that hooks up tablet mode switch
to Chrome. If the tablet mode switch is on then Chrome can know
for sure that it needs to enable maximize mode (tablet mode).
The helps Chrome to decide whether to enable tablet mode in some
corner cases where the accelerometer readings are not stable.
BUG=chrome-os-partner:49114
TEST=unit tests
TEST=Build power_manager and chrome and deploy to my DUT. Make
sure that:
- The device enters maximize mode when tablet mode switch
is on.
- The device leaves maximize mode when tablet mode switch
is off and lid angle is less than kExitMaximizeModeAngle.
Signed-off-by: Ricky Liang <jcliang@chromium.org>
==========
to
==========
Hook up TABLET_MODE switch to maximize_mode_controller
This is part of the patch set that hooks up tablet mode switch
to Chrome. If the tablet mode switch is on then Chrome can know
for sure that it needs to enable maximize mode (tablet mode).
The helps Chrome to decide whether to enable tablet mode in some
corner cases where the accelerometer readings are not stable.
BUG=chrome-os-partner:49114
TEST=unit tests
TEST=Build power_manager and chrome and deploy to my DUT. Make
sure that:
- The device enters maximize mode when tablet mode switch
is on.
- The device leaves maximize mode when tablet mode switch
is off and lid angle is less than kExitMaximizeModeAngle.
Signed-off-by: Ricky Liang <jcliang@chromium.org>
==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from
==========
Hook up TABLET_MODE switch to maximize_mode_controller
This is part of the patch set that hooks up tablet mode switch
to Chrome. If the tablet mode switch is on then Chrome can know
for sure that it needs to enable maximize mode (tablet mode).
The helps Chrome to decide whether to enable tablet mode in some
corner cases where the accelerometer readings are not stable.
BUG=chrome-os-partner:49114
TEST=unit tests
TEST=Build power_manager and chrome and deploy to my DUT. Make
sure that:
- The device enters maximize mode when tablet mode switch
is on.
- The device leaves maximize mode when tablet mode switch
is off and lid angle is less than kExitMaximizeModeAngle.
Signed-off-by: Ricky Liang <jcliang@chromium.org>
==========
to
==========
Hook up TABLET_MODE switch to maximize_mode_controller
This is part of the patch set that hooks up tablet mode switch
to Chrome. If the tablet mode switch is on then Chrome can know
for sure that it needs to enable maximize mode (tablet mode).
The helps Chrome to decide whether to enable tablet mode in some
corner cases where the accelerometer readings are not stable.
BUG=chrome-os-partner:49114
TEST=unit tests
TEST=Build power_manager and chrome and deploy to my DUT. Make
sure that:
- The device enters maximize mode when tablet mode switch
is on.
- The device leaves maximize mode when tablet mode switch
is off and lid angle is less than kExitMaximizeModeAngle.
Signed-off-by: Ricky Liang <jcliang@chromium.org>
Committed: https://crrev.com/3fb658ae4db44dfe1a1686ce0717070e11247d9c
Cr-Commit-Position: refs/heads/master@{#400076}
==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/3fb658ae4db44dfe1a1686ce0717070e11247d9c Cr-Commit-Position: refs/heads/master@{#400076} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
