Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(49)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 617423002: Make GestureTextSelector detect its own gestures (Closed)

Created:
6 years, 2 months ago by jdduke (slow)
Modified:
6 years, 2 months ago
Reviewers:
Changwan Ryu, sadrul
CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, tdresser+watch_chromium.org, jam, penghuang+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su, jdduke+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Make GestureTextSelector detect its own gestures Routing gestures from the FilteredGestureProvider to the GestureTextSelector is clumsy, confusing and unnecessary. Instead, have each GestureTextSelector lazily create its own GestureDetector, simplifying the touch and gesture event flow from the embedder. Also move all gesture detection-related listeners into their own files, decoupling include dependencies. BUG=419550 Committed: https://crrev.com/6905f2ebc9ef58535629c6e3a038c6ac875993e7 Cr-Commit-Position: refs/heads/master@{#297900}

Patch Set 1 #

Patch Set 2 : Fix GN build #

Total comments: 4

Patch Set 3 : Code review #

Patch Set 4 : Fix win builds #

Total comments: 4

Patch Set 5 : Sort gyp/gn #

Unified diffs Side-by-side diffs Delta from patch set Stats (+327 lines, -312 lines) Patch
M content/browser/renderer_host/input/gesture_text_selector.h View 1 2 2 chunks +14 lines, -10 lines 0 comments Download
M content/browser/renderer_host/input/gesture_text_selector.cc View 1 2 1 chunk +57 lines, -38 lines 0 comments Download
M content/browser/renderer_host/input/gesture_text_selector_unittest.cc View 4 chunks +10 lines, -76 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.h View 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.cc View 4 chunks +3 lines, -14 lines 0 comments Download
M ui/events/BUILD.gn View 1 2 3 4 3 chunks +5 lines, -1 line 0 comments Download
M ui/events/events.gyp View 1 2 3 4 2 chunks +4 lines, -0 lines 0 comments Download
M ui/events/gesture_detection/gesture_detector.h View 1 2 3 4 chunks +5 lines, -66 lines 0 comments Download
M ui/events/gesture_detection/gesture_detector.cc View 4 chunks +4 lines, -57 lines 0 comments Download
A ui/events/gesture_detection/gesture_listeners.h View 1 chunk +83 lines, -0 lines 0 comments Download
A ui/events/gesture_detection/gesture_listeners.cc View 1 chunk +61 lines, -0 lines 0 comments Download
M ui/events/gesture_detection/gesture_provider.cc View 5 chunks +8 lines, -7 lines 0 comments Download
M ui/events/gesture_detection/scale_gesture_detector.h View 1 2 3 2 chunks +2 lines, -29 lines 0 comments Download
M ui/events/gesture_detection/scale_gesture_detector.cc View 2 chunks +1 line, -13 lines 0 comments Download
A ui/events/gesture_detection/scale_gesture_listeners.h View 1 chunk +47 lines, -0 lines 0 comments Download
A ui/events/gesture_detection/scale_gesture_listeners.cc View 1 chunk +23 lines, -0 lines 0 comments Download

Messages

Total messages: 11 (3 generated)
jdduke (slow)
changwan@: Could you take a look at GestureTextSelector? I think the only functional difference is ...
6 years, 2 months ago (2014-10-01 21:54:59 UTC) #2
jdduke (slow)
sadrul@: PTAL for ui/events/ ownership, thanks.
6 years, 2 months ago (2014-10-01 21:58:42 UTC) #4
Changwan Ryu
gesture_text_selector* lgtm nice work! https://codereview.chromium.org/617423002/diff/20001/content/browser/renderer_host/input/gesture_text_selector.cc File content/browser/renderer_host/input/gesture_text_selector.cc (right): https://codereview.chromium.org/617423002/diff/20001/content/browser/renderer_host/input/gesture_text_selector.cc#newcode58 content/browser/renderer_host/input/gesture_text_selector.cc:58: gesture_detector_->OnTouchEvent(event); Could you add a ...
6 years, 2 months ago (2014-10-02 00:39:53 UTC) #5
sadrul
ui/events lgtm https://codereview.chromium.org/617423002/diff/60001/ui/events/BUILD.gn File ui/events/BUILD.gn (right): https://codereview.chromium.org/617423002/diff/60001/ui/events/BUILD.gn#newcode183 ui/events/BUILD.gn:183: "gesture_detection/gesture_config_helper.h", This list doesn't seem sorted? https://codereview.chromium.org/617423002/diff/60001/ui/events/events.gyp ...
6 years, 2 months ago (2014-10-02 19:14:48 UTC) #6
jdduke (slow)
https://codereview.chromium.org/617423002/diff/20001/content/browser/renderer_host/input/gesture_text_selector.cc File content/browser/renderer_host/input/gesture_text_selector.cc (right): https://codereview.chromium.org/617423002/diff/20001/content/browser/renderer_host/input/gesture_text_selector.cc#newcode58 content/browser/renderer_host/input/gesture_text_selector.cc:58: gesture_detector_->OnTouchEvent(event); On 2014/10/02 00:39:53, Changwan Ryu wrote: > Could ...
6 years, 2 months ago (2014-10-02 19:30:24 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/617423002/80001
6 years, 2 months ago (2014-10-02 19:48:19 UTC) #9
commit-bot: I haz the power
Committed patchset #5 (id:80001) as a578d3c3d7b85e2dc8d675fa511d14c8520921f6
6 years, 2 months ago (2014-10-02 20:58:40 UTC) #10
commit-bot: I haz the power
6 years, 2 months ago (2014-10-02 20:59:25 UTC) #11
Message was sent while issue was closed. 
Patchset 5 (id:??) landed as
https://crrev.com/6905f2ebc9ef58535629c6e3a038c6ac875993e7
Cr-Commit-Position: refs/heads/master@{#297900}

Powered by Google App Engine
This is Rietveld 408576698