New issue
Advanced search Search tips

Issue 887042 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

TabletModeController refactoring tracking bug

Project Member Reported by x...@chromium.org, Sep 19

Issue description

OS: Chrome

This is a tracking bug for the refactoring of TabletModeController. The logic in TabletModeController is very complicated, which makes it hard to understand and bug-prone. I'm trying to clean it up a little bit and fix potential bugs (hopefully) along the way.


 
The first CL has been landed https://chromium-review.googlesource.com/c/chromium/src/+/1234354. I forgot to link the CL to this bug. Will do it for the following CLs.
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/43b3f48dfdf7c77f1b75285bff576341286fedf0

commit 43b3f48dfdf7c77f1b75285bff576341286fedf0
Author: Xiaoqian Dai <xdai@chromium.org>
Date: Mon Sep 24 20:56:43 2018

TabletModeController refactoring II.

Refactoring of the internal input events blocker in
TabletModeController.

The input events should only be blocked if 1) we're currently in tablet
mode or 2) we're currently in laptop mode but the lid is flipped over,
i.e., we are in laptop mode because of an attached external mouse.

Bug: 887042
Change-Id: I8be03c684265d79ccc3499362a7154eb44c4dec0
Reviewed-on: https://chromium-review.googlesource.com/1237407
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593671}
[modify] https://crrev.com/43b3f48dfdf7c77f1b75285bff576341286fedf0/ash/BUILD.gn
[modify] https://crrev.com/43b3f48dfdf7c77f1b75285bff576341286fedf0/ash/keyboard/virtual_keyboard_controller_unittest.cc
[modify] https://crrev.com/43b3f48dfdf7c77f1b75285bff576341286fedf0/ash/system/overview/overview_button_tray_unittest.cc
[modify] https://crrev.com/43b3f48dfdf7c77f1b75285bff576341286fedf0/ash/wm/tablet_mode/tablet_mode_controller.cc
[modify] https://crrev.com/43b3f48dfdf7c77f1b75285bff576341286fedf0/ash/wm/tablet_mode/tablet_mode_controller.h
[modify] https://crrev.com/43b3f48dfdf7c77f1b75285bff576341286fedf0/ash/wm/tablet_mode/tablet_mode_controller_test_api.cc
[modify] https://crrev.com/43b3f48dfdf7c77f1b75285bff576341286fedf0/ash/wm/tablet_mode/tablet_mode_controller_test_api.h
[modify] https://crrev.com/43b3f48dfdf7c77f1b75285bff576341286fedf0/ash/wm/tablet_mode/tablet_mode_controller_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/95f93d44d3caf29694533328e2162c360ed795c2

commit 95f93d44d3caf29694533328e2162c360ed795c2
Author: Xiaoqian Dai <xdai@chromium.org>
Date: Wed Sep 26 18:39:30 2018

TabletModeController refactoring III.

Changes in this CL:
- Remove have_seen_accelerometer_data_ variable. As this variable is not
  set if the device doesn't have kAshEnableTabletMode flag.
- Only observe the display/lid/input device events if the ui mode is
  allowed to be changed.

No functionality is changed in this CL.

Bug: 887042
Change-Id: Ic42d4cc0c25667e7aa67efcf26f65032082db126
Reviewed-on: https://chromium-review.googlesource.com/1244289
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594407}
[modify] https://crrev.com/95f93d44d3caf29694533328e2162c360ed795c2/ash/public/cpp/ash_switches.cc
[modify] https://crrev.com/95f93d44d3caf29694533328e2162c360ed795c2/ash/wm/tablet_mode/tablet_mode_controller.cc
[modify] https://crrev.com/95f93d44d3caf29694533328e2162c360ed795c2/ash/wm/tablet_mode/tablet_mode_controller.h
[modify] https://crrev.com/95f93d44d3caf29694533328e2162c360ed795c2/ash/wm/tablet_mode/tablet_mode_controller_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5b0a138d4ce2b8890dd858ca67cdb61878752c42

commit 5b0a138d4ce2b8890dd858ca67cdb61878752c42
Author: Xiaoqian Dai <xdai@chromium.org>
Date: Wed Sep 26 22:53:56 2018

Fix the regression caused by the refactoring.

I accidentally delete one line code that should not be deleted. Bring it
back.

Bug: 887042
Change-Id: I9a6c4131eb2003888e0edd987ae7362ad847c568
Reviewed-on: https://chromium-review.googlesource.com/1247182
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594508}
[modify] https://crrev.com/5b0a138d4ce2b8890dd858ca67cdb61878752c42/ash/wm/tablet_mode/tablet_mode_controller.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8242c9ca8df2ed946fdf1e7f8c8d47a5997e513e

commit 8242c9ca8df2ed946fdf1e7f8c8d47a5997e513e
Author: Xiaoqian Dai <xdai@chromium.org>
Date: Fri Oct 05 23:07:15 2018

Revert TabletModeController refactoring cls.

The refacotring CLs exposed an underlying issue that caused b/117075672.
I need to revert all the refactoring for now, and fix the underlying issue
first and then reland these refactoring cls.

Basically the assumption of "--enable-touchview" USE flag is not longer true,
thus Chrome can't reply on this flag to decide whether a device is capable of
entering tablet mode. We need to find other ways to do so.

Since it's a pure revert, TBR xiyuan@.

Revert "TabletModeController refactoring II."

This reverts commit 43b3f48dfdf7c77f1b75285bff576341286fedf0.

Revert "TabletModeController refactoring III."

This reverts commit 95f93d44d3caf29694533328e2162c360ed795c2.

Revert "Fix the regression caused by the refactoring."

This reverts commit 5b0a138d4ce2b8890dd858ca67cdb61878752c42.

Bug:887042, b/117075672
TBR=xiyuan@chromium.org

Change-Id: I494a8d05be1fb07bea404cd714b0b1d9689e46d4
Reviewed-on: https://chromium-review.googlesource.com/c/1265996
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597356}
[modify] https://crrev.com/8242c9ca8df2ed946fdf1e7f8c8d47a5997e513e/ash/keyboard/virtual_keyboard_controller_unittest.cc
[modify] https://crrev.com/8242c9ca8df2ed946fdf1e7f8c8d47a5997e513e/ash/system/overview/overview_button_tray_unittest.cc
[modify] https://crrev.com/8242c9ca8df2ed946fdf1e7f8c8d47a5997e513e/ash/wm/tablet_mode/tablet_mode_controller.cc
[modify] https://crrev.com/8242c9ca8df2ed946fdf1e7f8c8d47a5997e513e/ash/wm/tablet_mode/tablet_mode_controller.h
[modify] https://crrev.com/8242c9ca8df2ed946fdf1e7f8c8d47a5997e513e/ash/wm/tablet_mode/tablet_mode_controller_test_api.cc
[modify] https://crrev.com/8242c9ca8df2ed946fdf1e7f8c8d47a5997e513e/ash/wm/tablet_mode/tablet_mode_controller_test_api.h

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/20a76ff5b7298d2e97f9dcb9faee7915677dda57

commit 20a76ff5b7298d2e97f9dcb9faee7915677dda57
Author: Xiaoqian Dai <xdai@chromium.org>
Date: Wed Oct 24 16:48:26 2018

Fix the issue that whiskers keyboard is disabled incorrectly.

Refactoring of the internal input events blocker in
TabletModeController.

The input events should only be blocked if 1) we're currently in tablet
mode or 2) we're currently in clamshell mode but the lid is flipped over,
i.e., we are in laptop mode because of an attached external mouse.

Bug: b/118049922, 887042
Test: Newly added test and existing tests passed.
      Also manually tested on convertible/clamshell/tablet devices

Change-Id: I93d963cfab2ae14bd1cad19675f0ed4fed88c1b0
Reviewed-on: https://chromium-review.googlesource.com/c/1297577
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602370}
[modify] https://crrev.com/20a76ff5b7298d2e97f9dcb9faee7915677dda57/ash/keyboard/virtual_keyboard_controller_unittest.cc
[modify] https://crrev.com/20a76ff5b7298d2e97f9dcb9faee7915677dda57/ash/system/overview/overview_button_tray_unittest.cc
[modify] https://crrev.com/20a76ff5b7298d2e97f9dcb9faee7915677dda57/ash/wm/tablet_mode/tablet_mode_controller.cc
[modify] https://crrev.com/20a76ff5b7298d2e97f9dcb9faee7915677dda57/ash/wm/tablet_mode/tablet_mode_controller.h
[modify] https://crrev.com/20a76ff5b7298d2e97f9dcb9faee7915677dda57/ash/wm/tablet_mode/tablet_mode_controller_test_api.cc
[modify] https://crrev.com/20a76ff5b7298d2e97f9dcb9faee7915677dda57/ash/wm/tablet_mode/tablet_mode_controller_test_api.h
[modify] https://crrev.com/20a76ff5b7298d2e97f9dcb9faee7915677dda57/ash/wm/tablet_mode/tablet_mode_controller_unittest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 25

Labels: merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/68bd491df494b88a7b8a81fc3b4a470bfc432aa9

commit 68bd491df494b88a7b8a81fc3b4a470bfc432aa9
Author: Xiaoqian Dai <xdai@chromium.org>
Date: Thu Oct 25 18:14:34 2018

[Merge to M71] Fix the issue that whiskers keyboard is disabled incorrectly.

Refactoring of the internal input events blocker in
TabletModeController.

The input events should only be blocked if 1) we're currently in tablet
mode or 2) we're currently in clamshell mode but the lid is flipped over,
i.e., we are in laptop mode because of an attached external mouse.

Bug: b/118049922, 887042
Test: Newly added test and existing tests passed.
      Also manually tested on convertible/clamshell/tablet devices

TBR=xiyuan@chromium.org

(cherry picked from commit 20a76ff5b7298d2e97f9dcb9faee7915677dda57)

Change-Id: I93d963cfab2ae14bd1cad19675f0ed4fed88c1b0
Reviewed-on: https://chromium-review.googlesource.com/c/1297577
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#602370}
Reviewed-on: https://chromium-review.googlesource.com/c/1299777
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#328}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/68bd491df494b88a7b8a81fc3b4a470bfc432aa9/ash/keyboard/virtual_keyboard_controller_unittest.cc
[modify] https://crrev.com/68bd491df494b88a7b8a81fc3b4a470bfc432aa9/ash/system/overview/overview_button_tray_unittest.cc
[modify] https://crrev.com/68bd491df494b88a7b8a81fc3b4a470bfc432aa9/ash/wm/tablet_mode/tablet_mode_controller.cc
[modify] https://crrev.com/68bd491df494b88a7b8a81fc3b4a470bfc432aa9/ash/wm/tablet_mode/tablet_mode_controller.h
[modify] https://crrev.com/68bd491df494b88a7b8a81fc3b4a470bfc432aa9/ash/wm/tablet_mode/tablet_mode_controller_test_api.cc
[modify] https://crrev.com/68bd491df494b88a7b8a81fc3b4a470bfc432aa9/ash/wm/tablet_mode/tablet_mode_controller_test_api.h
[modify] https://crrev.com/68bd491df494b88a7b8a81fc3b4a470bfc432aa9/ash/wm/tablet_mode/tablet_mode_controller_unittest.cc

Labels: CommitLog-Audit-Violation Merge-Without-Approval
Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision 68bd491df494b88a7b8a81fc3b4a470bfc432aa9 was merged to refs/branch-heads/3578 branch with no merge approval from a TPM! 
Please explain why this change was merged to the branch!
 - AcknowledgeMerge: Notification Required -- 
Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/68bd491df494b88a7b8a81fc3b4a470bfc432aa9

Commit: 68bd491df494b88a7b8a81fc3b4a470bfc432aa9
Author: xdai@chromium.org
Commiter: xdai@chromium.org
Date: 2018-10-25 18:14:34 +0000 UTC

[Merge to M71] Fix the issue that whiskers keyboard is disabled incorrectly.

Refactoring of the internal input events blocker in
TabletModeController.

The input events should only be blocked if 1) we're currently in tablet
mode or 2) we're currently in clamshell mode but the lid is flipped over,
i.e., we are in laptop mode because of an attached external mouse.

Bug: b/118049922, 887042
Test: Newly added test and existing tests passed.
      Also manually tested on convertible/clamshell/tablet devices

TBR=xiyuan@chromium.org

(cherry picked from commit 20a76ff5b7298d2e97f9dcb9faee7915677dda57)

Change-Id: I93d963cfab2ae14bd1cad19675f0ed4fed88c1b0
Reviewed-on: https://chromium-review.googlesource.com/c/1297577
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#602370}
Reviewed-on: https://chromium-review.googlesource.com/c/1299777
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#328}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}

Sign in to add a comment