Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2576973003:
Fix a crash when typing in an exo window. (Closed)
DescriptionFix a crash when typing in a exo window.
The exo is used on ChromeOS, it doesn't expect char event. The
InputMethodBridgeChromeOS should filters out char events.
BUG=None
Committed: https://crrev.com/2eb89f84e4aacc8454cc4a41d108dd22f9532317
Cr-Commit-Position: refs/heads/master@{#438904}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add comment #
Total comments: 2
Messages
Total messages: 25 (14 generated)
Description was changed from ========== Fix a crash when typing in a exo window. The exo is used on ChromeOS, it doesn't expect char event. The InputMethodBridgeChromeOS should filters out char events. BUG=None ========== to ========== Fix a crash when typing in a exo window. The exo is used on ChromeOS, it doesn't expect char event. The InputMethodBridgeChromeOS should filters out char events. BUG=None ==========
penghuang@chromium.org changed reviewers: + moshayedi@google.com
Hadi, PTAL. Thanks.
moshayedi@chromium.org changed reviewers: + moshayedi@chromium.org
lgtm. https://codereview.chromium.org/2576973003/diff/1/chrome/browser/ui/views/ime... File chrome/browser/ui/views/ime_driver/input_method_bridge_chromeos.cc (right): https://codereview.chromium.org/2576973003/diff/1/chrome/browser/ui/views/ime... chrome/browser/ui/views/ime_driver/input_method_bridge_chromeos.cc:42: callback.Run(true); Can you please add some comments for why we are filtering out the character events?
https://codereview.chromium.org/2576973003/diff/1/chrome/browser/ui/views/ime... File chrome/browser/ui/views/ime_driver/input_method_bridge_chromeos.cc (right): https://codereview.chromium.org/2576973003/diff/1/chrome/browser/ui/views/ime... chrome/browser/ui/views/ime_driver/input_method_bridge_chromeos.cc:42: callback.Run(true); On 2016/12/15 15:28:58, Hadi wrote: > Can you please add some comments for why we are filtering out the character > events? Done.
penghuang@chromium.org changed reviewers: + sky@chromium.org
Hi Scott, PTAL. Thanks.
The CQ bit was checked by penghuang@chromium.org 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/2576973003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/ime_driver/input_method_bridge_chromeos.cc (right): https://codereview.chromium.org/2576973003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/ime_driver/input_method_bridge_chromeos.cc:42: // On Linux (include ChromeOS), the mus emulates the WM_CHAR generation This seems like a work around. Unnecessarily consuming events means other parts of the system might not get them. Can't exo filter the events out if it doesn't want them?
https://codereview.chromium.org/2576973003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/ime_driver/input_method_bridge_chromeos.cc (right): https://codereview.chromium.org/2576973003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/ime_driver/input_method_bridge_chromeos.cc:42: // On Linux (include ChromeOS), the mus emulates the WM_CHAR generation On 2016/12/15 16:54:14, sky wrote: > This seems like a work around. Unnecessarily consuming events means other parts > of the system might not get them. Can't exo filter the events out if it doesn't > want them? On Window, the char events are consumed by InputMethodWin[1]. So seems on other platforms, the IME should consumes those char events too. But InputMethodChromeOS doesn't expect char events, so I think the IME bridge should filter them out. And see [2] [3] [4], looks like other parts of system do not expect char events too. [1] https://cs.chromium.org/chromium/src/ui/base/ime/input_method_win.cc?sq=packa... [2] https://cs.chromium.org/chromium/src/ui/views/mus/native_widget_mus.cc?rcl=14... [3] https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_nat... [4] https://cs.chromium.org/chromium/src/ui/wm/core/accelerator_filter.cc?rcl=148...
Ok, LGTM
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
I talked to peng offline. Sounds like chrome does not receive char events on ChromeOS (non-mash). So in mash world, it makes sense to consume the char events that chrome does receive (although the ideal fix would be to not have those char events be synthesized from the ws at all) lgtm
The CQ bit was checked by penghuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from moshayedi@chromium.org Link to the patchset: https://codereview.chromium.org/2576973003/#ps20001 (title: "Add comment")
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": 20001, "attempt_start_ts": 1481832195160080,
"parent_rev": "69e394014b7e1c23f9fd41ae9bf8d5e7c1c1b51f", "commit_rev":
"40587799088bbbd066fb6add99d5d44926e05adb"}
Message was sent while issue was closed.
Description was changed from ========== Fix a crash when typing in a exo window. The exo is used on ChromeOS, it doesn't expect char event. The InputMethodBridgeChromeOS should filters out char events. BUG=None ========== to ========== Fix a crash when typing in a exo window. The exo is used on ChromeOS, it doesn't expect char event. The InputMethodBridgeChromeOS should filters out char events. BUG=None Review-Url: https://codereview.chromium.org/2576973003 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix a crash when typing in a exo window. The exo is used on ChromeOS, it doesn't expect char event. The InputMethodBridgeChromeOS should filters out char events. BUG=None Review-Url: https://codereview.chromium.org/2576973003 ========== to ========== Fix a crash when typing in a exo window. The exo is used on ChromeOS, it doesn't expect char event. The InputMethodBridgeChromeOS should filters out char events. BUG=None Committed: https://crrev.com/2eb89f84e4aacc8454cc4a41d108dd22f9532317 Cr-Commit-Position: refs/heads/master@{#438904} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2eb89f84e4aacc8454cc4a41d108dd22f9532317 Cr-Commit-Position: refs/heads/master@{#438904} |
