Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
256593013:
Fail if key synthesized events are off screen. (Closed)
|
Created:
6 years, 7 months ago by Dominik Grewe Modified:
6 years, 7 months ago CC:
chromium-reviews, jam, jdduke+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFail if key synthesized events are off screen.
Enforce that certain key input events (see list below) are on screen. This
helps detecting bugs where we're accidentally starting a gesture that's off
screen and therefore doesn't have an effect.
BUG=363125
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266762
Patch Set 1 #
Total comments: 10
Patch Set 2 : comments #
Messages
Total messages: 11 (0 generated)
PTAL. With this implementation we simply crash if the events are off screen. I thought about pushing the failure all the way back to Telemetry, but it would require quite a bit of plumbing everywhere (each time we dispatch an event we need to check if it was successful). Since the main purpose is to detect bugs, I think this is sufficient. Wdyt?
Looks pretty reasonable to me. rbyers@, is this about what you had in mind? https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/synthetic_gesture_target_base.cc (right): https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/synthetic_gesture_target_base.cc:58: for (unsigned i = 0; i < web_touch.touchesLength; i++) It's probably only necessary to check touch positions for which |web_touch.touches[i].state == WebTouchPoint::StatePressed|. https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/synthetic_gesture_target_base.cc:124: bounds -= bounds.OffsetFromOrigin(); // Translate the bounds to (0,0). Are the bounds in device pixels or DIPs?
https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/synthetic_gesture_target_base.cc (right): https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/synthetic_gesture_target_base.cc:124: bounds -= bounds.OffsetFromOrigin(); // Translate the bounds to (0,0). On 2014/04/28 16:30:23, jdduke wrote: > Are the bounds in device pixels or DIPs? Good question... In the comments it says "screen coordinates", not sure if that's device or DIPs.
Thanks! I'm fine with crashing assuming we see the details of the crash in the logs somewhere if this is ever hit on the bots (and if not that's probably an independent issue that needs fixing). Since the co-ordinate space conversion can be a little tricky, please validate that it starts failing at the exact expected boundary on both Android and Aura (a one-off manual test is fine I think). https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/synthetic_gesture_target_base.cc (right): https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/synthetic_gesture_target_base.cc:59: CHECK(PointIsOnScreen(web_touch.touches[i].screenPosition.x, The fact that you're using screenPosition (instead of position) here seems inconsistent with the OffsetFromOrigin call below (if these are really scree co-ordinates then you should be able to treat them as such instead of treating them like window co-ordiates). I don't think we're consistent with how we use screenPosition for synthetic events - we're probably sometimes actually storing window co-ords here (at least SyntheticGestureTargetAura seems to assume that in case cases). I suggest you validate the window-relative co-ords in 'position' instead. https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/synthetic_gesture_target_base.cc:122: bool SyntheticGestureTargetBase::PointIsOnScreen(int x, int y) const { You mean "within contents" not "on screen" I think (this distinction is probably less relevant on Android, but on Aura Screen != Window != contents area). https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/synthetic_gesture_target_base.cc:123: gfx::Rect bounds = host_->GetView()->GetViewBounds(); GetViewBounds says it's relative to the parent, I think you want GetContainerBounds instead (which are in screen co-ordinates). Or if you're validating window-relative co-ordinates then you can jus use GetContainerSize.
https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/synthetic_gesture_target_base.cc (right): https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/synthetic_gesture_target_base.cc:123: gfx::Rect bounds = host_->GetView()->GetViewBounds(); On 2014/04/28 17:01:22, Rick Byers wrote: > GetViewBounds says it's relative to the parent, I think you want > GetContainerBounds instead (which are in screen co-ordinates). Or if you're > validating window-relative co-ordinates then you can jus use GetContainerSize. Is GetViewBounds().size() equivalent to GetContainerSize()?
I'm not sure how to get a WebContents object so I can call GetContainerBounds/Size. I did some manual testing and it seems to work with the current implementation: It crashes if X or Y is negative or if X >= window.innerWidth or if Y >= window.innerHeight. https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/synthetic_gesture_target_base.cc (right): https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/synthetic_gesture_target_base.cc:58: for (unsigned i = 0; i < web_touch.touchesLength; i++) On 2014/04/28 16:30:23, jdduke wrote: > It's probably only necessary to check touch positions for which > |web_touch.touches[i].state == WebTouchPoint::StatePressed|. Done. https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/synthetic_gesture_target_base.cc:59: CHECK(PointIsOnScreen(web_touch.touches[i].screenPosition.x, On 2014/04/28 17:01:22, Rick Byers wrote: > The fact that you're using screenPosition (instead of position) here seems > inconsistent with the OffsetFromOrigin call below (if these are really scree > co-ordinates then you should be able to treat them as such instead of treating > them like window co-ordiates). I don't think we're consistent with how we use > screenPosition for synthetic events - we're probably sometimes actually storing > window co-ords here (at least SyntheticGestureTargetAura seems to assume that in > case cases). > > I suggest you validate the window-relative co-ords in 'position' instead. Done. https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/synthetic_gesture_target_base.cc:122: bool SyntheticGestureTargetBase::PointIsOnScreen(int x, int y) const { On 2014/04/28 17:01:22, Rick Byers wrote: > You mean "within contents" not "on screen" I think (this distinction is probably > less relevant on Android, but on Aura Screen != Window != contents area). Done.
lgtm, but please double check with rbyers@, thanks.
LGTM On 2014/04/28 17:32:24, Dominik Grewe wrote: > I'm not sure how to get a WebContents object so I can call > GetContainerBounds/Size. I did some manual testing and it seems to work with the > current implementation: It crashes if X or Y is negative or if X >= > window.innerWidth or if Y >= window.innerHeight. Yeah sorry, I was assuming you had a WebContents here. This seems fine. Thanks for testing! > https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... > File content/browser/renderer_host/input/synthetic_gesture_target_base.cc > (right): > > https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/input/synthetic_gesture_target_base.cc:58: for > (unsigned i = 0; i < web_touch.touchesLength; i++) > On 2014/04/28 16:30:23, jdduke wrote: > > It's probably only necessary to check touch positions for which > > |web_touch.touches[i].state == WebTouchPoint::StatePressed|. > > Done. > > https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/input/synthetic_gesture_target_base.cc:59: > CHECK(PointIsOnScreen(web_touch.touches[i].screenPosition.x, > On 2014/04/28 17:01:22, Rick Byers wrote: > > The fact that you're using screenPosition (instead of position) here seems > > inconsistent with the OffsetFromOrigin call below (if these are really scree > > co-ordinates then you should be able to treat them as such instead of treating > > them like window co-ordiates). I don't think we're consistent with how we use > > screenPosition for synthetic events - we're probably sometimes actually > storing > > window co-ords here (at least SyntheticGestureTargetAura seems to assume that > in > > case cases). > > > > I suggest you validate the window-relative co-ords in 'position' instead. > > Done. > > https://codereview.chromium.org/256593013/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/input/synthetic_gesture_target_base.cc:122: bool > SyntheticGestureTargetBase::PointIsOnScreen(int x, int y) const { > On 2014/04/28 17:01:22, Rick Byers wrote: > > You mean "within contents" not "on screen" I think (this distinction is > probably > > less relevant on Android, but on Aura Screen != Window != contents area). > > Done.
The CQ bit was checked by dominikg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dominikg@chromium.org/256593013/20001
Message was sent while issue was closed.
Change committed as 266762 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
