Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2521103002:
Ignore OnKeyEvent for VKEY_POWER because it is handled by PowerButtonEvent (Closed)
|
Created:
4 years, 1 month ago by Qiang(Joe) Xu Modified:
4 years, 1 month ago Reviewers:
Daniel Erat CC:
chromium-reviews, kalyank, sadrul, oshima+watch_chromium.org, derat+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIgnore OnKeyEvent for VKEY_POWER because it is handled by PowerButtonEvent
Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. It is coming from VKEY_POWER keyevent timing issue. The right design should also avoid this key event.
BUG=633304
TEST=cannot see the bug for many taps; add unittest.
Committed: https://crrev.com/93aefeb6fd6780215aa647dfb16e840d22ef0388
Cr-Commit-Position: refs/heads/master@{#433954}
Patch Set 1 #Patch Set 2 : add tests #
Total comments: 9
Patch Set 3 : based on comments #Patch Set 4 : tests #
Messages
Total messages: 23 (12 generated)
Description was changed from ========== Ignore OnKeyEvent for VKEY_POWER because it is processed by PowerButtonEvent BUG= ========== to ========== Ignore OnKeyEvent for VKEY_POWER because it is processed by PowerButtonEvent Before this change, today I met the issue that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. Suspect it is coming from VKEY_POWER keyevent. The right design should also avoid this key event. BUG=633304 ==========
warx@chromium.org changed reviewers: + derat@chromium.org
Description was changed from ========== Ignore OnKeyEvent for VKEY_POWER because it is processed by PowerButtonEvent Before this change, today I met the issue that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. Suspect it is coming from VKEY_POWER keyevent. The right design should also avoid this key event. BUG=633304 ========== to ========== Ignore OnKeyEvent for VKEY_POWER because it is processed by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. Suspect it is coming from VKEY_POWER keyevent. The right design should also avoid this key event. BUG=633304 ==========
Description was changed from ========== Ignore OnKeyEvent for VKEY_POWER because it is processed by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. Suspect it is coming from VKEY_POWER keyevent. The right design should also avoid this key event. BUG=633304 ========== to ========== Ignore OnKeyEvent for VKEY_POWER because it is processed by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. Suspect it is coming from VKEY_POWER keyevent. The right design should also avoid this key event. BUG=633304 TEST=cannot see the bug for many taps ==========
Description was changed from ========== Ignore OnKeyEvent for VKEY_POWER because it is processed by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. Suspect it is coming from VKEY_POWER keyevent. The right design should also avoid this key event. BUG=633304 TEST=cannot see the bug for many taps ========== to ========== Ignore OnKeyEvent for VKEY_POWER because it is processed by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. Suspect it is coming from VKEY_POWER keyevent (somewhat be verified that I cannot see it on tablet mode for many taps). The right design should also avoid this key event. BUG=633304 TEST=cannot see the bug for many taps ==========
Hi Daniel, ptal, thanks!
can you add a test for this as well?
Hi Daniel, test added, ptal. https://codereview.chromium.org/2521103002/diff/20001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.cc (left): https://codereview.chromium.org/2521103002/diff/20001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.cc:29: } changed by git cl format
Description was changed from ========== Ignore OnKeyEvent for VKEY_POWER because it is processed by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. Suspect it is coming from VKEY_POWER keyevent (somewhat be verified that I cannot see it on tablet mode for many taps). The right design should also avoid this key event. BUG=633304 TEST=cannot see the bug for many taps ========== to ========== Ignore OnKeyEvent for VKEY_POWER because it is processed by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. Suspect it is coming from VKEY_POWER keyevent (somewhat be verified that I cannot see it on tablet mode for many taps). The right design should also avoid this key event. BUG=633304 TEST=cannot see the bug for many taps; add unittest. ==========
https://codereview.chromium.org/2521103002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2521103002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:141: if (event->key_code() == ui::VKEY_POWER) please also document why these are ignored. something like this is fine: // Ignore key events generated by the power button since // power button activity is already handled by OnPowerButtonEvent(). https://codereview.chromium.org/2521103002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2521103002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:312: power_manager_client_->num_set_backlights_forced_off_calls() + 1; does this count really not start out at zero? it looks like you're creating a new FakePowerManagerClient in SetUp (which is run for each test), so this is surprising. https://codereview.chromium.org/2521103002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:319: tablet_controller_->OnKeyEvent(&power_key_released); are there really multiple release events? i'd expect there to be multiple press events (for autorepeat) but only one release event. https://codereview.chromium.org/2521103002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:322: EXPECT_EQ(power_manager_client_->num_set_backlights_forced_off_calls(), nit: the order should be EXPECT_EQ(expected, actual) in order for the failure messages to make sense
Description was changed from ========== Ignore OnKeyEvent for VKEY_POWER because it is processed by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. Suspect it is coming from VKEY_POWER keyevent (somewhat be verified that I cannot see it on tablet mode for many taps). The right design should also avoid this key event. BUG=633304 TEST=cannot see the bug for many taps; add unittest. ========== to ========== Ignore OnKeyEvent for VKEY_POWER because it is handled by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. Suspect it is coming from VKEY_POWER keyevent (somewhat be verified that I cannot see it on tablet mode for many taps). The right design should also avoid this key event. BUG=633304 TEST=cannot see the bug for many taps; add unittest. ==========
Hi Daniel, ptal, thanks! https://codereview.chromium.org/2521103002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2521103002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:141: if (event->key_code() == ui::VKEY_POWER) On 2016/11/22 00:48:19, Daniel Erat wrote: > please also document why these are ignored. something like this is fine: > > // Ignore key events generated by the power button since > // power button activity is already handled by OnPowerButtonEvent(). Done. https://codereview.chromium.org/2521103002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2521103002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:312: power_manager_client_->num_set_backlights_forced_off_calls() + 1; On 2016/11/22 00:48:19, Daniel Erat wrote: > does this count really not start out at zero? it looks like you're creating a > new FakePowerManagerClient in SetUp (which is run for each test), so this is > surprising. It should start with zero. Thanks for pointing out. https://codereview.chromium.org/2521103002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:319: tablet_controller_->OnKeyEvent(&power_key_released); On 2016/11/22 00:48:19, Daniel Erat wrote: > are there really multiple release events? i'd expect there to be multiple press > events (for autorepeat) but only one release event. Yes, there are multiple release events. I made the logs before I wrote the tests. Please see https://paste.googleplex.com/6168565862367232. https://codereview.chromium.org/2521103002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:322: EXPECT_EQ(power_manager_client_->num_set_backlights_forced_off_calls(), On 2016/11/22 00:48:19, Daniel Erat wrote: > nit: the order should be EXPECT_EQ(expected, actual) in order for the failure > messages to make sense Done.
lgtm thanks! re duplicate key events, that's really strange. just out of curiosity, if it's still easy for you to log these, can you see if the events have e.g. different flags set on them?
On 2016/11/22 16:00:09, Daniel Erat wrote: > lgtm > > thanks! re duplicate key events, that's really strange. just out of curiosity, > if it's still easy for you to log these, can you see if the events have e.g. > different flags set on them? Yes, first for quick tap no holding: [13705:13705:1122/103954:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 0 [13705:13705:1122/103954:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 0 [13705:13705:1122/103954:ERROR:tablet_power_button_controller.cc(105)] OnPowerButtonEvent: down [13705:13705:1122/103955:ERROR:tablet_power_button_controller.cc(152)] power key event: released with flags: 0 [13705:13705:1122/103955:ERROR:tablet_power_button_controller.cc(152)] power key event: released with flags: 0 [13705:13705:1122/103955:ERROR:tablet_power_button_controller.cc(107)] OnPowerButtonEvent: up ------- With no is_repeated flags, there are always two events generated, not one. Is it a bug that needs investigation? Second, try to holding for a while: [13705:13705:1122/102041:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 0 [13705:13705:1122/102041:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 0 [13705:13705:1122/102041:ERROR:tablet_power_button_controller.cc(105)] OnPowerButtonEvent: down [13705:13705:1122/102042:ERROR:event_rewriter.cc(418)] Device ID 4 is unknown. [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:event_rewriter.cc(418)] Device ID 4 is unknown. [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:event_rewriter.cc(418)] Device ID 4 is unknown. [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:event_rewriter.cc(418)] Device ID 4 is unknown. [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:event_rewriter.cc(418)] Device ID 4 is unknown. [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:event_rewriter.cc(418)] Device ID 4 is unknown. [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:event_rewriter.cc(418)] Device ID 4 is unknown. [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(152)] power key event: released with flags: 0 [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(152)] power key event: released with flags: 0 [13705:13705:1122/102042:ERROR:tablet_power_button_controller.cc(107)] OnPowerButtonEvent: up ---------- Repeated flags are only on pressed events. Third, when reported bug happens: [13705:13705:1122/104011:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 0 [13705:13705:1122/104011:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 0 [13705:13705:1122/104011:ERROR:tablet_power_button_controller.cc(105)] OnPowerButtonEvent: down [13705:13705:1122/104012:ERROR:event_rewriter.cc(418)] Device ID 4 is unknown. [13705:13705:1122/104012:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/104012:ERROR:tablet_power_button_controller.cc(150)] power key event: pressed with flags: 65536 [13705:13705:1122/104012:ERROR:tablet_power_button_controller.cc(107)] OnPowerButtonEvent: up [13705:13705:1122/104012:ERROR:tablet_power_button_controller.cc(152)] power key event: released with flags: 0 [13705:13705:1122/104012:ERROR:tablet_power_button_controller.cc(152)] power key event: released with flags: 0 ---------- power key released events are received after OnPowerButtonEvent:up. This could explain why the bug is not frequently shown. It is a timing issue. Changing the test a little bit to represent this. Please take a look. Thanks!
Description was changed from ========== Ignore OnKeyEvent for VKEY_POWER because it is handled by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. Suspect it is coming from VKEY_POWER keyevent (somewhat be verified that I cannot see it on tablet mode for many taps). The right design should also avoid this key event. BUG=633304 TEST=cannot see the bug for many taps; add unittest. ========== to ========== Ignore OnKeyEvent for VKEY_POWER because it is handled by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. It is coming from VKEY_POWER keyevent timing issue. The right design should also avoid this key event. BUG=633304 TEST=cannot see the bug for many taps; add unittest. ==========
lgtm thanks! if this only happens for the power button, it's probably not a big deal. i don't think we use power button key events anywhere in chrome; powerd watches the button itself and reports presses and releases to chrome via d-bus.
The CQ bit was checked by warx@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1479841618142730,
"parent_rev": "b1ed57ee30777f09f897ae92dc3ce8850f50897d", "commit_rev":
"b06c995101189ad3aac0bf3f29fb3b477586d44d"}
Message was sent while issue was closed.
Description was changed from ========== Ignore OnKeyEvent for VKEY_POWER because it is handled by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. It is coming from VKEY_POWER keyevent timing issue. The right design should also avoid this key event. BUG=633304 TEST=cannot see the bug for many taps; add unittest. ========== to ========== Ignore OnKeyEvent for VKEY_POWER because it is handled by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. It is coming from VKEY_POWER keyevent timing issue. The right design should also avoid this key event. BUG=633304 TEST=cannot see the bug for many taps; add unittest. ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Ignore OnKeyEvent for VKEY_POWER because it is handled by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. It is coming from VKEY_POWER keyevent timing issue. The right design should also avoid this key event. BUG=633304 TEST=cannot see the bug for many taps; add unittest. ========== to ========== Ignore OnKeyEvent for VKEY_POWER because it is handled by PowerButtonEvent Before this change, today I meet the issue (it doesn't happen frequently) that pressing-and-releasing power button should SetBacklightsForcedOff(true). However, screen is turned off and then turned on. Checking powerd log that, there is Stop SetBacklightsForcedOff request. It is coming from VKEY_POWER keyevent timing issue. The right design should also avoid this key event. BUG=633304 TEST=cannot see the bug for many taps; add unittest. Committed: https://crrev.com/93aefeb6fd6780215aa647dfb16e840d22ef0388 Cr-Commit-Position: refs/heads/master@{#433954} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/93aefeb6fd6780215aa647dfb16e840d22ef0388 Cr-Commit-Position: refs/heads/master@{#433954} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
