Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2180603002:
Virtual keyboard - do not change work area of screen if in non-sticky mode. (Closed)
|
Created:
4 years, 5 months ago by hariank Modified:
4 years, 4 months ago CC:
chromium-reviews, sadrul, dzhioev+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, kalyank, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVirtual keyboard - do not change work area of screen if in non-sticky mode.
Added switch in ash_switches to enable this behavior.
BUG=624521
Committed: https://crrev.com/3e69afa89eee8f663c90e67cea4c43495010f44f
Cr-Commit-Position: refs/heads/master@{#409273}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Virtual keyboard - do not change work area of screen if in non-sticky mode #
Total comments: 10
Patch Set 3 : Virtual keyboard - do not change work area of screen if in non-sticky mode #
Total comments: 10
Patch Set 4 : Virtual keyboard - do not change work area of screen if in non-sticky mode #Patch Set 5 : Virtual keyboard - do not change work area of screen if in non-sticky mode #
Messages
Total messages: 41 (24 generated)
Description was changed from ========== Virtual keyboard - do not change work area of screen if in non-sticky mode. Added switch in ash_switches to enable this behavior. BUG=624521 ========== to ========== Virtual keyboard - do not change work area of screen if in non-sticky mode. Added switch in ash_switches to enable this behavior. BUG=624521 ==========
hariank@google.com changed reviewers: + oshima@chromium.org
The CQ bit was checked by hariank@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
looking good. could you please add a unit test? https://codereview.chromium.org/2180603002/diff/1/ash/common/ash_switches.cc File ash/common/ash_switches.cc (right): https://codereview.chromium.org/2180603002/diff/1/ash/common/ash_switches.cc#... ash/common/ash_switches.cc:127: const char kAshUseNewVKWindowBehavior[] = "ash-use-new-vk-window-behavior"; kAshNoWorkAreaUpdateByVK (or something other person can guess that it does) https://codereview.chromium.org/2180603002/diff/1/ash/common/wm/workspace/wor... File ash/common/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/2180603002/diff/1/ash/common/wm/workspace/wor... ash/common/wm/workspace/workspace_layout_manager.cc:7: #include <algorithm> new line between c++ include and chrome includes. https://codereview.chromium.org/2180603002/diff/1/ash/common/wm/workspace/wor... ash/common/wm/workspace/workspace_layout_manager.cc:152: keyboard::KeyboardController::GetInstance()->get_lock_keyboard()); if think this method will be gone with new mode, (or does completely different thing), so you can just check it at the beginning of the method, and just return if !change_work_area. https://codereview.chromium.org/2180603002/diff/1/ui/keyboard/keyboard_contro... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/2180603002/diff/1/ui/keyboard/keyboard_contro... ui/keyboard/keyboard_controller.h:92: bool get_lock_keyboard() { return lock_keyboard_; } bool get_lock_keyboard() const { ...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2180603002/diff/1/ash/common/ash_switches.cc File ash/common/ash_switches.cc (right): https://codereview.chromium.org/2180603002/diff/1/ash/common/ash_switches.cc#... ash/common/ash_switches.cc:127: const char kAshUseNewVKWindowBehavior[] = "ash-use-new-vk-window-behavior"; On 2016/07/22 23:45:01, oshima wrote: > kAshNoWorkAreaUpdateByVK > > (or something other person can guess that it does) Going to keep this. https://codereview.chromium.org/2180603002/diff/1/ash/common/wm/workspace/wor... File ash/common/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/2180603002/diff/1/ash/common/wm/workspace/wor... ash/common/wm/workspace/workspace_layout_manager.cc:7: #include <algorithm> On 2016/07/22 23:45:01, oshima wrote: > new line between c++ include and chrome includes. > Done. https://codereview.chromium.org/2180603002/diff/1/ash/common/wm/workspace/wor... ash/common/wm/workspace/workspace_layout_manager.cc:152: keyboard::KeyboardController::GetInstance()->get_lock_keyboard()); On 2016/07/22 23:45:01, oshima wrote: > if think this method will be gone with new mode, (or does completely different > thing), so you can just check it at the beginning of the method, and just return > if !change_work_area. Done. https://codereview.chromium.org/2180603002/diff/1/ui/keyboard/keyboard_contro... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/2180603002/diff/1/ui/keyboard/keyboard_contro... ui/keyboard/keyboard_controller.h:92: bool get_lock_keyboard() { return lock_keyboard_; } On 2016/07/22 23:45:01, oshima wrote: > bool get_lock_keyboard() const { ... Done.
The CQ bit was checked by hariank@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2180603002/diff/20001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_layout_manager_unittest.cc (right): https://codereview.chromium.org/2180603002/diff/20001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1074: keyboard::SetAccessibilityKeyboardEnabled(true); Why this change here? Won't this change what other unittests test? https://codereview.chromium.org/2180603002/diff/20001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1105: void SetVKFlag() { EnableNewVKMode() would be better. https://codereview.chromium.org/2180603002/diff/20001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1232: work_area.height() - 100); can you compute this from keyboard bounds? https://codereview.chromium.org/2180603002/diff/20001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1241: if (kb_controller->ui()->GetKeyboardWindow()->bounds().height() == 0) { no conditional branch in unit test. Why do you need this? https://codereview.chromium.org/2180603002/diff/20001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1255: if (kb_controller->ui()->GetKeyboardWindow()->bounds().height() == 0) { same here.
https://codereview.chromium.org/2180603002/diff/20001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_layout_manager_unittest.cc (right): https://codereview.chromium.org/2180603002/diff/20001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1074: keyboard::SetAccessibilityKeyboardEnabled(true); On 2016/07/27 21:40:23, oshima wrote: > Why this change here? Won't this change what other unittests test? Done. https://codereview.chromium.org/2180603002/diff/20001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1105: void SetVKFlag() { On 2016/07/27 21:40:23, oshima wrote: > EnableNewVKMode() > > would be better. Done. https://codereview.chromium.org/2180603002/diff/20001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1232: work_area.height() - 100); On 2016/07/27 21:40:23, oshima wrote: > can you compute this from keyboard bounds? Done. https://codereview.chromium.org/2180603002/diff/20001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1241: if (kb_controller->ui()->GetKeyboardWindow()->bounds().height() == 0) { On 2016/07/27 21:40:23, oshima wrote: > no conditional branch in unit test. Why do you need this? Done. https://codereview.chromium.org/2180603002/diff/20001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1255: if (kb_controller->ui()->GetKeyboardWindow()->bounds().height() == 0) { On 2016/07/27 21:40:23, oshima wrote: > same here. Done.
The CQ bit was checked by hariank@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_layout_manager_unittest.cc (right): https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1235: EXPECT_EQ(orig_window_bounds.ToString(), window->bounds().ToString()); comparing with ToString() is old style. Can you just compare Rect? https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1241: Shell::GetPrimaryRootWindow()->bounds(), 100)); Did you copy this from https://cs.chromium.org/chromium/src/ash/wm/lock_layout_manager_unittest.cc?r... What it does is to use the bounds origin to see if it was set, so you can just set it once here and remove next one. https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1246: work_area.height() - 100); compure this from orig_window_bounds and Offset. https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1301: work_area.height() - 100); same here, and move this to here it is used.
https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_layout_manager_unittest.cc (right): https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1235: EXPECT_EQ(orig_window_bounds.ToString(), window->bounds().ToString()); On 2016/07/28 15:53:47, oshima wrote: > comparing with ToString() is old style. Can you just compare Rect? Done. https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1241: Shell::GetPrimaryRootWindow()->bounds(), 100)); On 2016/07/28 15:53:47, oshima wrote: > Did you copy this from > https://cs.chromium.org/chromium/src/ash/wm/lock_layout_manager_unittest.cc?r... > > What it does is to use the bounds origin to see if it was set, so you can just > set it once here and remove next one. Done. https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1246: work_area.height() - 100); On 2016/07/28 15:53:47, oshima wrote: > compure this from orig_window_bounds and Offset. Done. https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1301: work_area.height() - 100); On 2016/07/28 15:53:47, oshima wrote: > same here, and move this to here it is used. Done.
https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_layout_manager_unittest.cc (right): https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1246: work_area.height() - 100); On 2016/07/28 17:24:10, hariank wrote: > On 2016/07/28 15:53:47, oshima wrote: > > compure this from orig_window_bounds and Offset. > > Done. Sorry I wasn't clear. Here is what I meant gfx::Rect changed_window_bounds(orig_window_bounds); changedwindow_bounds.Offset(0, -shift);
https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_layout_manager_unittest.cc (right): https://codereview.chromium.org/2180603002/diff/40001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_layout_manager_unittest.cc:1246: work_area.height() - 100); On 2016/07/28 17:33:48, oshima wrote: > On 2016/07/28 17:24:10, hariank wrote: > > On 2016/07/28 15:53:47, oshima wrote: > > > compure this from orig_window_bounds and Offset. > > > > Done. > > Sorry I wasn't clear. Here is what I meant > > gfx::Rect changed_window_bounds(orig_window_bounds); > changedwindow_bounds.Offset(0, -shift); > Done.
lgtm
The CQ bit was checked by hariank@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hariank@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
hariank@google.com changed reviewers: + bryeung@chromium.org
hariank@google.com changed reviewers: + jam@chromium.org - bryeung@chromium.org
nots sure why i'm added, please add a more specific reviewer
hariank@google.com changed reviewers: + bshe@chromium.org - jam@chromium.org
Please review the changes in ui/keyboard.
On 2016/07/28 22:10:05, hariank wrote: > Please review the changes in ui/keyboard. lgtm sorry for the delay. I totally missed this review
The CQ bit was checked by hariank@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Virtual keyboard - do not change work area of screen if in non-sticky mode. Added switch in ash_switches to enable this behavior. BUG=624521 ========== to ========== Virtual keyboard - do not change work area of screen if in non-sticky mode. Added switch in ash_switches to enable this behavior. BUG=624521 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Virtual keyboard - do not change work area of screen if in non-sticky mode. Added switch in ash_switches to enable this behavior. BUG=624521 ========== to ========== Virtual keyboard - do not change work area of screen if in non-sticky mode. Added switch in ash_switches to enable this behavior. BUG=624521 Committed: https://crrev.com/3e69afa89eee8f663c90e67cea4c43495010f44f Cr-Commit-Position: refs/heads/master@{#409273} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3e69afa89eee8f663c90e67cea4c43495010f44f Cr-Commit-Position: refs/heads/master@{#409273} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
