Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2968593002:
exo: Drop window on touch release (Closed)
|
Created:
3 years, 5 months ago by Dominik Laskowski Modified:
3 years, 5 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionexo: Drop window on touch release
Windows were not dropped after dragging using touch, resulting in broken
input due to unreleased capture. Phantom windows in extended desktop
mode would also stick around.
BUG=738200
TEST=Phantom windows disappear on touch release.
Review-Url: https://codereview.chromium.org/2968593002
Cr-Commit-Position: refs/heads/master@{#483870}
Committed: https://chromium.googlesource.com/chromium/src/+/d60a4c6e0cbe7ed72a141f61b74c7dd10a927212
Patch Set 1 #
Total comments: 4
Patch Set 2 : DCHECK events #
Total comments: 4
Patch Set 3 : Address nits #
Total comments: 2
Patch Set 4 : Link to bug in TODO #
Messages
Total messages: 29 (18 generated)
The CQ bit was checked by domlaskowski@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...
domlaskowski@chromium.org changed reviewers: + oshima@chromium.org, reveman@chromium.org
PTAL.
lgtm with a nit. One q though. I had the same (capture) problem even when I used track pad (tested on both cave and caroline) as well. Removing SetCapture on exo side did fix the problem so I'm wondering if there is a scenario that leads to the same situation. https://codereview.chromium.org/2968593002/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2968593002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1138: case ui::ET_GESTURE_END: { if (type == ET_GESTURE_END) { ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The broken dragging on ToT is an unrelated regression. I'll upload a fix soon. https://codereview.chromium.org/2968593002/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2968593002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1138: case ui::ET_GESTURE_END: { On 2017/06/29 22:52:34, oshima wrote: > if (type == ET_GESTURE_END) { ? XDG clients can't be dragged/resized using touch, so we will need more cases here.
lgtm with nit https://codereview.chromium.org/2968593002/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2968593002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1144: default: break; nit: NOTREACHED() here and add other GESTURE events to switch statement so it's clear that we're not failing to handle some event that we should be handling.
The CQ bit was checked by domlaskowski@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...
https://codereview.chromium.org/2968593002/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2968593002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:1144: default: break; On 2017/06/30 19:50:15, reveman wrote: > nit: NOTREACHED() here and add other GESTURE events to switch statement so it's > clear that we're not failing to handle some event that we should be handling. Checked the range instead to avoid 20+ cases.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2968593002/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2968593002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1136: nit: remove this blank line https://codereview.chromium.org/2968593002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1138: DCHECK_GE(event->type(), ui::ET_GESTURE_TYPE_START); either add all enum values above even if there's a lot of them or switch to a simple "if (event->type() == ui::ET_GESTURE_END)", the switch statement doesn't provide any value otherwise imo
https://codereview.chromium.org/2968593002/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2968593002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1136: On 2017/06/30 21:48:16, reveman wrote: > nit: remove this blank line Done. https://codereview.chromium.org/2968593002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1138: DCHECK_GE(event->type(), ui::ET_GESTURE_TYPE_START); On 2017/06/30 21:48:16, reveman wrote: > either add all enum values above even if there's a lot of them or switch to a > simple "if (event->type() == ui::ET_GESTURE_END)", the switch statement doesn't > provide any value otherwise imo Done.
lgtm with nit https://codereview.chromium.org/2968593002/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2968593002/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1130: // TODO(domlaskowski): Handle touch dragging/resizing for BoundsMode::SHELL. nit: please add bug number
The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run
https://codereview.chromium.org/2968593002/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2968593002/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1130: // TODO(domlaskowski): Handle touch dragging/resizing for BoundsMode::SHELL. On 2017/06/30 22:30:07, reveman wrote: > nit: please add bug number Done.
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 domlaskowski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2968593002/#ps60001 (title: "Link to bug in TODO")
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": 1498867403717590,
"parent_rev": "eab1ead9ca6429ab9c7f6614f3f97780857a43d5", "commit_rev":
"6ace7febbca840b02926e2a75c9c0cbe72cb6419"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498867403717590,
"parent_rev": "3c8b34fa54528562f759b6c98de2b3dce51d6c6a", "commit_rev":
"d60a4c6e0cbe7ed72a141f61b74c7dd10a927212"}
Message was sent while issue was closed.
Description was changed from ========== exo: Drop window on touch release Windows were not dropped after dragging using touch, resulting in broken input due to unreleased capture. Phantom windows in extended desktop mode would also stick around. BUG=738200 TEST=Phantom windows disappear on touch release. ========== to ========== exo: Drop window on touch release Windows were not dropped after dragging using touch, resulting in broken input due to unreleased capture. Phantom windows in extended desktop mode would also stick around. BUG=738200 TEST=Phantom windows disappear on touch release. Review-Url: https://codereview.chromium.org/2968593002 Cr-Commit-Position: refs/heads/master@{#483870} Committed: https://chromium.googlesource.com/chromium/src/+/d60a4c6e0cbe7ed72a141f61b74c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d60a4c6e0cbe7ed72a141f61b74c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
