Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2287613002:
Count Touch events via WebTouchPoint::State values instead. (Closed)
DescriptionCount Touch events via WebTouchPoint::State values instead.
Don't assume that there is one WebTouchEvent for each
ui::ET_TOUCH_PRESSED/RELEASED/CANCELLED event. Instead,
check the states of the WebTouchPoints in each
WebTouchEvent to determine how many ui-Touch events it
represents.
A temporary UMA stat is added to allow detection of empty
gesture queues. When it drops to zero we'll know the
problem has been solved (this will create a lot less noise
than DumpWithoutCrashing). https://crbug.com/642008 tracks
the removal/disabling of this UMA stat when it's no longer
needed.
BUG=595422
Committed: https://crrev.com/266191e27be990d0e26b12e0dd43f830ad7d31ad
Cr-Commit-Position: refs/heads/master@{#415279}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address comments, add UMA histogram. #
Total comments: 2
Patch Set 3 : Add DCHECK, bug reference. #
Total comments: 4
Patch Set 4 : Address suggestions. #
Messages
Total messages: 31 (19 generated)
The CQ bit was checked by wjmaclean@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...
Description was changed from ========== Count Touch events via WebTouchPoint::State values instead. Don't assume that there is one WebTouchEvent for each ui::ET_TOUCH_PRESSED/RELEASED/CANCELLED event. Instead, check the states of the WebTouchPoints in each WebTouchEvent to determine how many ui-Touch events it represents. BUG=595422 ========== to ========== Count Touch events via WebTouchPoint::State values instead. Don't assume that there is one WebTouchEvent for each ui::ET_TOUCH_PRESSED/RELEASED/CANCELLED event. Instead, check the states of the WebTouchPoints in each WebTouchEvent to determine how many ui-Touch events it represents. BUG=595422 ==========
wjmaclean@chromium.org changed reviewers: + tdresser@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think this might be a solution to our missing gesture-target bug ... could you please give it an initial sanity check? The missing gesture target can be explained if multiple ui Touch End/Cancel events are combined into a single WebTouchEvent ... in this case RWHIER incorrectly keeps track of current active touches. If active_touches_ > 0 after a touch sequence ends, then any subsequent touch sequence will fail to add a target to the gesture queue, but may still send a gesture sequence, triggering the bug.
https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_input_event_router.cc (left): https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:216: return; We could move modifying active_touches_ up, and then keep the early return, right? https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:193: unsigned CountChangedTouchPoints(const blink::WebTouchEvent* event) { Can we take a const reference here instead of a pointer? https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:213: if (event->touches[i].state == required_state ) Remove extra space "required_state )" https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:217: return changed_count; In what circumstances is this not 1? Is it only for cancels? https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:255: if (touch_target_.target) { If we keep the early return, we can get rid of this conditional. https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:274: active_touches_ -= CountChangedTouchPoints(event); I think again, we can move this earlier and keep the early return.
wjmaclean@chromium.org changed reviewers: + mpearson@google.com
tdresser@ - I've addressed your comments, please let me know what you think. I've also added a UMA mettric to track how often we hit an empty gesture queue ... + mpearson@ for histograms.xml. https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_input_event_router.cc (left): https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:216: return; On 2016/08/29 14:36:34, tdresser wrote: > We could move modifying active_touches_ up, and then keep the early return, > right? We could move it before the start of the if-statement, though does that really buy us that much? E.g. then we compare active_touches_ == 1, which I suspect requires loading 1 into a reg, etc. https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:193: unsigned CountChangedTouchPoints(const blink::WebTouchEvent* event) { On 2016/08/29 14:36:34, tdresser wrote: > Can we take a const reference here instead of a pointer? Done. https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:213: if (event->touches[i].state == required_state ) On 2016/08/29 14:36:34, tdresser wrote: > Remove extra space "required_state )" Done. https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:217: return changed_count; On 2016/08/29 14:36:34, tdresser wrote: > In what circumstances is this not 1? > > Is it only for cancels? I suspect it is only for cancels, but I thought it would be worth including it for TouchStart/End as insurance. Of course, in those cases we could DCHECK or CHECK the one-ness. This CL is sort of an experiment. I've added a UMA histogram to the second patch-set so we can track if it makes the problem go away or not. https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:255: if (touch_target_.target) { On 2016/08/29 14:36:34, tdresser wrote: > If we keep the early return, we can get rid of this conditional. I don't think so ... the early return is inside the if-statement, because it only applies on the first TouchStart. https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:274: active_touches_ -= CountChangedTouchPoints(event); On 2016/08/29 14:36:34, tdresser wrote: > I think again, we can move this earlier and keep the early return. Done.
The CQ bit was checked by wjmaclean@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...
LGTM with nits. https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:217: return changed_count; On 2016/08/29 15:40:58, wjmaclean wrote: > On 2016/08/29 14:36:34, tdresser wrote: > > In what circumstances is this not 1? > > > > Is it only for cancels? > > I suspect it is only for cancels, but I thought it would be worth including it > for TouchStart/End as insurance. Of course, in those cases we could DCHECK or > CHECK the one-ness. > > This CL is sort of an experiment. I've added a UMA histogram to the second > patch-set so we can track if it makes the problem go away or not. You have confirmed that this does happen for cancels though? https://codereview.chromium.org/2287613002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:255: if (touch_target_.target) { On 2016/08/29 15:40:58, wjmaclean wrote: > On 2016/08/29 14:36:34, tdresser wrote: > > If we keep the early return, we can get rid of this conditional. > > I don't think so ... the early return is inside the if-statement, because it > only applies on the first TouchStart. Acknowledged. https://codereview.chromium.org/2287613002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2287613002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:218: If we aren't aware of any case where this shouldn't be 1 for start/end, I'd be in favor of a DCHECK. https://codereview.chromium.org/2287613002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2287613002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12678: + routing gesture events to frames' renderers. Let's make it clear this is a temporary UMA, and file a bug to remove it (referenced here) once we've sorted this out.
Description was changed from ========== Count Touch events via WebTouchPoint::State values instead. Don't assume that there is one WebTouchEvent for each ui::ET_TOUCH_PRESSED/RELEASED/CANCELLED event. Instead, check the states of the WebTouchPoints in each WebTouchEvent to determine how many ui-Touch events it represents. BUG=595422 ========== to ========== Count Touch events via WebTouchPoint::State values instead. Don't assume that there is one WebTouchEvent for each ui::ET_TOUCH_PRESSED/RELEASED/CANCELLED event. Instead, check the states of the WebTouchPoints in each WebTouchEvent to determine how many ui-Touch events it represents. A temporary UMA stat is added to allow detection of empty gesture queues. When it drops to zero we'll know the problem has been solved (this will create a lot less noise than DumpWithoutCrashing). https://crbug.com/642008 tracks the removal/disabling of this UMA stat when it's no longer needed. BUG=595422 ==========
Description was changed from ========== Count Touch events via WebTouchPoint::State values instead. Don't assume that there is one WebTouchEvent for each ui::ET_TOUCH_PRESSED/RELEASED/CANCELLED event. Instead, check the states of the WebTouchPoints in each WebTouchEvent to determine how many ui-Touch events it represents. A temporary UMA stat is added to allow detection of empty gesture queues. When it drops to zero we'll know the problem has been solved (this will create a lot less noise than DumpWithoutCrashing). https://crbug.com/642008 tracks the removal/disabling of this UMA stat when it's no longer needed. BUG=595422 ========== to ========== Count Touch events via WebTouchPoint::State values instead. Don't assume that there is one WebTouchEvent for each ui::ET_TOUCH_PRESSED/RELEASED/CANCELLED event. Instead, check the states of the WebTouchPoints in each WebTouchEvent to determine how many ui-Touch events it represents. A temporary UMA stat is added to allow detection of empty gesture queues. When it drops to zero we'll know the problem has been solved (this will create a lot less noise than DumpWithoutCrashing). https://crbug.com/642008 tracks the removal/disabling of this UMA stat when it's no longer needed. BUG=595422 ==========
Description was changed from ========== Count Touch events via WebTouchPoint::State values instead. Don't assume that there is one WebTouchEvent for each ui::ET_TOUCH_PRESSED/RELEASED/CANCELLED event. Instead, check the states of the WebTouchPoints in each WebTouchEvent to determine how many ui-Touch events it represents. A temporary UMA stat is added to allow detection of empty gesture queues. When it drops to zero we'll know the problem has been solved (this will create a lot less noise than DumpWithoutCrashing). https://crbug.com/642008 tracks the removal/disabling of this UMA stat when it's no longer needed. BUG=595422 ========== to ========== Count Touch events via WebTouchPoint::State values instead. Don't assume that there is one WebTouchEvent for each ui::ET_TOUCH_PRESSED/RELEASED/CANCELLED event. Instead, check the states of the WebTouchPoints in each WebTouchEvent to determine how many ui-Touch events it represents. A temporary UMA stat is added to allow detection of empty gesture queues. When it drops to zero we'll know the problem has been solved (this will create a lot less noise than DumpWithoutCrashing). https://crbug.com/642008 tracks the removal/disabling of this UMA stat when it's no longer needed. BUG=595422 ==========
Done ... bug filed, DCHECK added, and issue description updated.
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
histograms.xml lgtm baring comments below https://codereview.chromium.org/2287613002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2287613002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:480: // it's puspose, namely telling us when the incidents of empty nit: it's -> its nit: puspose -> purpose https://codereview.chromium.org/2287613002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2287613002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12674: +<histogram name="Event.FrameEventRouting.NoGestureTarget"> units="" or enum=""? or put something in the description to say that true == empty; false, non-empty.
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2287613002/#ps60001 (title: "Address suggestions.")
https://codereview.chromium.org/2287613002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2287613002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:480: // it's puspose, namely telling us when the incidents of empty On 2016/08/29 19:31:33, Mark P wrote: > nit: it's -> its > nit: puspose -> purpose Done. https://codereview.chromium.org/2287613002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2287613002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12674: +<histogram name="Event.FrameEventRouting.NoGestureTarget"> On 2016/08/29 19:31:33, Mark P wrote: > units="" or enum=""? > or put something in the description to say that true == empty; false, non-empty. Done.
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by wjmaclean@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Count Touch events via WebTouchPoint::State values instead. Don't assume that there is one WebTouchEvent for each ui::ET_TOUCH_PRESSED/RELEASED/CANCELLED event. Instead, check the states of the WebTouchPoints in each WebTouchEvent to determine how many ui-Touch events it represents. A temporary UMA stat is added to allow detection of empty gesture queues. When it drops to zero we'll know the problem has been solved (this will create a lot less noise than DumpWithoutCrashing). https://crbug.com/642008 tracks the removal/disabling of this UMA stat when it's no longer needed. BUG=595422 ========== to ========== Count Touch events via WebTouchPoint::State values instead. Don't assume that there is one WebTouchEvent for each ui::ET_TOUCH_PRESSED/RELEASED/CANCELLED event. Instead, check the states of the WebTouchPoints in each WebTouchEvent to determine how many ui-Touch events it represents. A temporary UMA stat is added to allow detection of empty gesture queues. When it drops to zero we'll know the problem has been solved (this will create a lot less noise than DumpWithoutCrashing). https://crbug.com/642008 tracks the removal/disabling of this UMA stat when it's no longer needed. BUG=595422 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Count Touch events via WebTouchPoint::State values instead. Don't assume that there is one WebTouchEvent for each ui::ET_TOUCH_PRESSED/RELEASED/CANCELLED event. Instead, check the states of the WebTouchPoints in each WebTouchEvent to determine how many ui-Touch events it represents. A temporary UMA stat is added to allow detection of empty gesture queues. When it drops to zero we'll know the problem has been solved (this will create a lot less noise than DumpWithoutCrashing). https://crbug.com/642008 tracks the removal/disabling of this UMA stat when it's no longer needed. BUG=595422 ========== to ========== Count Touch events via WebTouchPoint::State values instead. Don't assume that there is one WebTouchEvent for each ui::ET_TOUCH_PRESSED/RELEASED/CANCELLED event. Instead, check the states of the WebTouchPoints in each WebTouchEvent to determine how many ui-Touch events it represents. A temporary UMA stat is added to allow detection of empty gesture queues. When it drops to zero we'll know the problem has been solved (this will create a lot less noise than DumpWithoutCrashing). https://crbug.com/642008 tracks the removal/disabling of this UMA stat when it's no longer needed. BUG=595422 Committed: https://crrev.com/266191e27be990d0e26b12e0dd43f830ad7d31ad Cr-Commit-Position: refs/heads/master@{#415279} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/266191e27be990d0e26b12e0dd43f830ad7d31ad Cr-Commit-Position: refs/heads/master@{#415279} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
