Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(126)

Issue 2869273007: Don't use KeyboardController::GetInstance() in KeyboardUI. (Closed)

Created:
3 years, 7 months ago by yhanada
Modified:
3 years, 7 months ago
Reviewers:
sadrul
CC:
chromium-reviews, oka+watchvk_chromium.org, blakeo+virtualkb_chromium.org, dfaden+virtualkb_google.com, groby+virtualkb_chromium.org, yhanada+watchvk_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't use KeyboardController::GetInstance() in KeyboardUI. Some tests doesn't set KeyboardController instance and it causes test crashes when enabling "--use-new-virtual-keyboard-behavior" flag. We can use keyboard_controller_ member instead. BUG=624521 TEST=Covered by unit tests. Review-Url: https://codereview.chromium.org/2869273007 Cr-Commit-Position: refs/heads/master@{#471240} Committed: https://chromium.googlesource.com/chromium/src/+/f742bd099543025c6df1f7824a40b0f40f1e7cd3

Patch Set 1 #

Total comments: 4

Patch Set 2 : add the comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+36 lines, -16 lines) Patch
M ui/keyboard/keyboard_controller_unittest.cc View 1 17 chunks +35 lines, -15 lines 0 comments Download
M ui/keyboard/keyboard_ui.cc View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 18 (13 generated)
yhanada
PTAL. Thanks!
3 years, 7 months ago (2017-05-11 08:18:23 UTC) #6
sadrul
lgtm https://codereview.chromium.org/2869273007/diff/1/ui/keyboard/keyboard_controller_unittest.cc File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/2869273007/diff/1/ui/keyboard/keyboard_controller_unittest.cc#newcode212 ui/keyboard/keyboard_controller_unittest.cc:212: class KeyboardControllerTest : public testing::TestWithParam<bool>, Document what the ...
3 years, 7 months ago (2017-05-11 17:25:06 UTC) #7
yhanada
Thanks for your review. https://codereview.chromium.org/2869273007/diff/1/ui/keyboard/keyboard_controller_unittest.cc File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/2869273007/diff/1/ui/keyboard/keyboard_controller_unittest.cc#newcode212 ui/keyboard/keyboard_controller_unittest.cc:212: class KeyboardControllerTest : public testing::TestWithParam<bool>, ...
3 years, 7 months ago (2017-05-12 04:53:54 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2869273007/20001
3 years, 7 months ago (2017-05-12 05:59:16 UTC) #15
commit-bot: I haz the power
3 years, 7 months ago (2017-05-12 07:00:39 UTC) #18
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f742bd099543025c6df1f7824a40...

Powered by Google App Engine
This is Rietveld 408576698