|
|
Created:
10 years, 3 months ago by rjkroege Modified:
8 years ago CC:
chromium-reviews, darin-cc_chromium.org, brettw-cc_chromium.org Visibility:
Public. |
DescriptionForward touch events to a RenderWidget's associated WebWidget
Receives WebTouchEvents over the IPC bridge and forwards
them to the RenderWidget's WebWidget. Sends each touch event
to an interim GestureManager.
BUG=
TEST=unit_tests
Patch Set 1 #
Total comments: 30
Patch Set 2 : Addressed reviewer comments. #
Total comments: 18
Patch Set 3 : addressed nits #
Total comments: 2
Patch Set 4 : removed commented-out include #
Total comments: 2
Messages
Total messages: 12 (0 generated)
Note: depends on https://bugs.webkit.org/show_bug.cgi?id=45560.
http://codereview.chromium.org/3388013/diff/1/3 File chrome/renderer/gesture_manager.cc (right): http://codereview.chromium.org/3388013/diff/1/3#newcode6 chrome/renderer/gesture_manager.cc:6: #ifndef NDEBUG newline between 5 and 6. http://codereview.chromium.org/3388013/diff/1/3#newcode22 chrome/renderer/gesture_manager.cc:22: GestureManager::~GestureManager() { Make sure the order of methods in the .cc matches that of the .h http://codereview.chromium.org/3388013/diff/1/3#newcode32 chrome/renderer/gesture_manager.cc:32: DLOG(INFO) << "ProcessTouchEventForGesture..."; We generally don't use DLOGs. See threads on chromium-dev for details. http://codereview.chromium.org/3388013/diff/1/3#newcode35 chrome/renderer/gesture_manager.cc:35: const WebTouchPoint &p = event.touchPoints[i]; Point& p http://codereview.chromium.org/3388013/diff/1/3#newcode37 chrome/renderer/gesture_manager.cc:37: case NoGesture: indenting is wrong. http://codereview.chromium.org/3388013/diff/1/3#newcode48 chrome/renderer/gesture_manager.cc:48: DLOG(INFO) << ">> error shouldn't happen\n"; NOTREACHED if it shouldn't happen. http://codereview.chromium.org/3388013/diff/1/3#newcode52 chrome/renderer/gesture_manager.cc:52: && isInClickTimeWindow(event.timeStampSeconds) indent to align with ( http://codereview.chromium.org/3388013/diff/1/3#newcode98 chrome/renderer/gesture_manager.cc:98: // Dispatch a mouseDown/mouseUp/Click sequence Descriptions should be in the header. http://codereview.chromium.org/3388013/diff/1/4 File chrome/renderer/gesture_manager.h (right): http://codereview.chromium.org/3388013/diff/1/4#newcode5 chrome/renderer/gesture_manager.h:5: remove this line http://codereview.chromium.org/3388013/diff/1/4#newcode20 chrome/renderer/gesture_manager.h:20: remove this line http://codereview.chromium.org/3388013/diff/1/4#newcode38 chrome/renderer/gesture_manager.h:38: // The states of the gesture manager state machine. Enums should appear first. http://codereview.chromium.org/3388013/diff/1/4#newcode40 chrome/renderer/gesture_manager.h:40: NoGesture, Use all caps for enums, eg NO_GESTURE. http://codereview.chromium.org/3388013/diff/1/4#newcode58 chrome/renderer/gesture_manager.h:58: bool isInClickTimeWindow(const double ts); Methods start with capital. http://codereview.chromium.org/3388013/diff/1/4#newcode69 chrome/renderer/gesture_manager.h:69: static const double MaxClickDownTime = .8; Constants are named with k, and the value assigned in the definition. http://codereview.chromium.org/3388013/diff/1/4#newcode80 chrome/renderer/gesture_manager.h:80: SyntheticCandidateGesture m_candidateGesture; Only use webkit style (m_ if you're in webkit).
http://codereview.chromium.org/3388013/diff/1/3 File chrome/renderer/gesture_manager.cc (right): http://codereview.chromium.org/3388013/diff/1/3#newcode6 chrome/renderer/gesture_manager.cc:6: #ifndef NDEBUG On 2010/09/21 22:28:40, sky wrote: > newline between 5 and 6. Done. http://codereview.chromium.org/3388013/diff/1/3#newcode22 chrome/renderer/gesture_manager.cc:22: GestureManager::~GestureManager() { On 2010/09/21 22:28:40, sky wrote: > Make sure the order of methods in the .cc matches that of the .h Done. http://codereview.chromium.org/3388013/diff/1/3#newcode32 chrome/renderer/gesture_manager.cc:32: DLOG(INFO) << "ProcessTouchEventForGesture..."; On 2010/09/21 22:28:40, sky wrote: > We generally don't use DLOGs. See threads on chromium-dev for details. I can only find a thread asking for when one should use LOG vs DLOG so remain in the dark. Can you clarify? http://codereview.chromium.org/3388013/diff/1/3#newcode35 chrome/renderer/gesture_manager.cc:35: const WebTouchPoint &p = event.touchPoints[i]; On 2010/09/21 22:28:40, sky wrote: > Point& p Done. http://codereview.chromium.org/3388013/diff/1/3#newcode37 chrome/renderer/gesture_manager.cc:37: case NoGesture: On 2010/09/21 22:28:40, sky wrote: > indenting is wrong. Done. http://codereview.chromium.org/3388013/diff/1/3#newcode48 chrome/renderer/gesture_manager.cc:48: DLOG(INFO) << ">> error shouldn't happen\n"; On 2010/09/21 22:28:40, sky wrote: > NOTREACHED if it shouldn't happen. Done. http://codereview.chromium.org/3388013/diff/1/3#newcode52 chrome/renderer/gesture_manager.cc:52: && isInClickTimeWindow(event.timeStampSeconds) On 2010/09/21 22:28:40, sky wrote: > indent to align with ( Done. http://codereview.chromium.org/3388013/diff/1/3#newcode98 chrome/renderer/gesture_manager.cc:98: // Dispatch a mouseDown/mouseUp/Click sequence On 2010/09/21 22:28:40, sky wrote: > Descriptions should be in the header. Done. http://codereview.chromium.org/3388013/diff/1/4 File chrome/renderer/gesture_manager.h (right): http://codereview.chromium.org/3388013/diff/1/4#newcode5 chrome/renderer/gesture_manager.h:5: On 2010/09/21 22:28:40, sky wrote: > remove this line Done. http://codereview.chromium.org/3388013/diff/1/4#newcode20 chrome/renderer/gesture_manager.h:20: On 2010/09/21 22:28:40, sky wrote: > remove this line Done. http://codereview.chromium.org/3388013/diff/1/4#newcode38 chrome/renderer/gesture_manager.h:38: // The states of the gesture manager state machine. On 2010/09/21 22:28:40, sky wrote: > Enums should appear first. Done. http://codereview.chromium.org/3388013/diff/1/4#newcode40 chrome/renderer/gesture_manager.h:40: NoGesture, On 2010/09/21 22:28:40, sky wrote: > Use all caps for enums, eg NO_GESTURE. Done. http://codereview.chromium.org/3388013/diff/1/4#newcode58 chrome/renderer/gesture_manager.h:58: bool isInClickTimeWindow(const double ts); On 2010/09/21 22:28:40, sky wrote: > Methods start with capital. Done. I had written all of this code originally to be in WebKit and forgot to update the coding conventions when I moved it to Chrome. Sorry. http://codereview.chromium.org/3388013/diff/1/4#newcode69 chrome/renderer/gesture_manager.h:69: static const double MaxClickDownTime = .8; On 2010/09/21 22:28:40, sky wrote: > Constants are named with k, and the value assigned in the definition. Done. http://codereview.chromium.org/3388013/diff/1/4#newcode80 chrome/renderer/gesture_manager.h:80: SyntheticCandidateGesture m_candidateGesture; On 2010/09/21 22:28:40, sky wrote: > Only use webkit style (m_ if you're in webkit). Done.
I don't think I'm really adding any value here other than style nits... http://codereview.chromium.org/3388013/diff/5001/6002 File chrome/renderer/gesture_manager.cc (right): http://codereview.chromium.org/3388013/diff/5001/6002#newcode33 chrome/renderer/gesture_manager.cc:33: DLOG(INFO) << "ProcessTouchEventForGesture"; The problem with random DLOGs like this is if every developer added what they thought was mildly interesting to DLOG then it rapidly adds up to a ton of console spam and you can never find what you really care about. Think of it this way, a year from now is this output going to be interesting to you? http://codereview.chromium.org/3388013/diff/5001/6002#newcode61 chrome/renderer/gesture_manager.cc:61: || p.state == WebTouchPoint::StateMoved) you should indent this one more space, otherwise it looks like it's at the same nesting level as the && on the next line, which isn't right. http://codereview.chromium.org/3388013/diff/5001/6003 File chrome/renderer/gesture_manager.h (right): http://codereview.chromium.org/3388013/diff/5001/6003#newcode44 chrome/renderer/gesture_manager.h:44: // be the target of any generated synthetic event. Finally, handled what is handled? previously_handled? http://codereview.chromium.org/3388013/diff/5001/6003#newcode49 chrome/renderer/gesture_manager.h:49: bool previouslyHandled); previously_handled http://codereview.chromium.org/3388013/diff/5001/6003#newcode57 chrome/renderer/gesture_manager.h:57: static const double kMaxClickDownTime = .8; nit: Values for constants are typically in the .cc, but apparently the style guide doesn't say either way so this is fine although not typical of Chrome. http://codereview.chromium.org/3388013/diff/5001/6003#newcode67 chrome/renderer/gesture_manager.h:67: void DispatchSyntheticClick(RenderWidget*, const WebKit::WebTouchEvent&, Parameters should have names, and the definition/declaration should match. Also, when you wrap put each parameter on its own line. http://codereview.chromium.org/3388013/diff/5001/6003#newcode71 chrome/renderer/gesture_manager.h:71: bool IsInClickTimeWindow(const double ts); no point in using const here. http://codereview.chromium.org/3388013/diff/5001/6003#newcode80 chrome/renderer/gesture_manager.h:80: void ScrollViaTouchMotion(const WebKit::WebTouchPoint& p, RenderWidget *w); RenderWidget* w http://codereview.chromium.org/3388013/diff/5001/6003#newcode92 chrome/renderer/gesture_manager.h:92: double firstTouchTime_; first_touch_time_
I think I've addressed the comments. Hopefully I'll have the differences between the C++ style guide and the Java/JavaScript guide internalized soon. Should someone else review the renderer changes? They're tiny and I'll make sure that that I get a passing try result before committing. http://codereview.chromium.org/3388013/diff/5001/6002 File chrome/renderer/gesture_manager.cc (right): http://codereview.chromium.org/3388013/diff/5001/6002#newcode33 chrome/renderer/gesture_manager.cc:33: DLOG(INFO) << "ProcessTouchEventForGesture"; On 2010/09/23 17:13:03, sky wrote: > The problem with random DLOGs like this is if every developer added what they > thought was mildly interesting to DLOG then it rapidly adds up to a ton of > console spam and you can never find what you really care about. Think of it this > way, a year from now is this output going to be interesting to you? True. Removed. http://codereview.chromium.org/3388013/diff/5001/6002#newcode61 chrome/renderer/gesture_manager.cc:61: || p.state == WebTouchPoint::StateMoved) On 2010/09/23 17:13:03, sky wrote: > you should indent this one more space, otherwise it looks like it's at the same > nesting level as the && on the next line, which isn't right. Done. http://codereview.chromium.org/3388013/diff/5001/6003 File chrome/renderer/gesture_manager.h (right): http://codereview.chromium.org/3388013/diff/5001/6003#newcode44 chrome/renderer/gesture_manager.h:44: // be the target of any generated synthetic event. Finally, handled On 2010/09/23 17:13:03, sky wrote: > what is handled? previously_handled? Done. http://codereview.chromium.org/3388013/diff/5001/6003#newcode49 chrome/renderer/gesture_manager.h:49: bool previouslyHandled); On 2010/09/23 17:13:03, sky wrote: > previously_handled Done. http://codereview.chromium.org/3388013/diff/5001/6003#newcode57 chrome/renderer/gesture_manager.h:57: static const double kMaxClickDownTime = .8; On 2010/09/23 17:13:03, sky wrote: > nit: Values for constants are typically in the .cc, but apparently the style > guide doesn't say either way so this is fine although not typical of Chrome. I moved them. http://codereview.chromium.org/3388013/diff/5001/6003#newcode67 chrome/renderer/gesture_manager.h:67: void DispatchSyntheticClick(RenderWidget*, const WebKit::WebTouchEvent&, On 2010/09/23 17:13:03, sky wrote: > Parameters should have names, and the definition/declaration should match. Also, > when you wrap put each parameter on its own line. Done. http://codereview.chromium.org/3388013/diff/5001/6003#newcode71 chrome/renderer/gesture_manager.h:71: bool IsInClickTimeWindow(const double ts); On 2010/09/23 17:13:03, sky wrote: > no point in using const here. Done. http://codereview.chromium.org/3388013/diff/5001/6003#newcode80 chrome/renderer/gesture_manager.h:80: void ScrollViaTouchMotion(const WebKit::WebTouchPoint& p, RenderWidget *w); On 2010/09/23 17:13:03, sky wrote: > RenderWidget* w Done. http://codereview.chromium.org/3388013/diff/5001/6003#newcode92 chrome/renderer/gesture_manager.h:92: double firstTouchTime_; On 2010/09/23 17:13:03, sky wrote: > first_touch_time_ Done. And the others members too.
LGTM Yes to someone else reviewing the renderer change. -Scott http://codereview.chromium.org/3388013/diff/11001/12002 File chrome/renderer/gesture_manager.cc (right): http://codereview.chromium.org/3388013/diff/11001/12002#newcode7 chrome/renderer/gesture_manager.cc:7: //#ifndef NDEBUG Remove this and line 11. http://codereview.chromium.org/3388013/diff/11001/12002#newcode22 chrome/renderer/gesture_manager.cc:22: remove one newline here.
Darin: could you have a look at the RenderWidget changes?
http://codereview.chromium.org/3388013/diff/15001/16004 File chrome/renderer/render_widget.cc (right): http://codereview.chromium.org/3388013/diff/15001/16004#newcode360 chrome/renderer/render_widget.cc:360: if (WebInputEvent::isTouchEventType(input_event->type)) { why is this code here instead of inside the WebViewImpl::handleInputEvent function? if you put the code in handleInputEvent, then you can write layout tests for this feature more easily by using the eventSender http://codereview.chromium.org/3388013/diff/15001/16005 File chrome/renderer/render_widget.h (right): http://codereview.chromium.org/3388013/diff/15001/16005#newcode355 chrome/renderer/render_widget.h:355: // A cached pointer to the GestureManager. why cache this here? GestureManager::Get() is fast.
On Sat, Sep 25, 2010 at 8:21 AM, <darin@chromium.org> wrote: > > http://codereview.chromium.org/3388013/diff/15001/16004 > File chrome/renderer/render_widget.cc (right): > > http://codereview.chromium.org/3388013/diff/15001/16004#newcode360 > chrome/renderer/render_widget.cc:360: if > (WebInputEvent::isTouchEventType(input_event->type)) { > why is this code here instead of inside the > WebViewImpl::handleInputEvent function? > > if you put the code in handleInputEvent, then you can write layout tests > for this feature more easily by using the eventSender > also, I recall objecting to the addition of WebWidget::gestureScroll. GestureManager, which is the caller of gestureScroll, could easily live in WebKit. -Darin > > http://codereview.chromium.org/3388013/diff/15001/16005 > File chrome/renderer/render_widget.h (right): > > http://codereview.chromium.org/3388013/diff/15001/16005#newcode355 > chrome/renderer/render_widget.h:355: // A cached pointer to the > GestureManager. > why cache this here? GestureManager::Get() is fast. > > > http://codereview.chromium.org/3388013/show >
On 2010/09/25 15:21:06, darin wrote: > http://codereview.chromium.org/3388013/diff/15001/16004 > File chrome/renderer/render_widget.cc (right): > > http://codereview.chromium.org/3388013/diff/15001/16004#newcode360 > chrome/renderer/render_widget.cc:360: if > (WebInputEvent::isTouchEventType(input_event->type)) { > why is this code here instead of inside the WebViewImpl::handleInputEvent > function? > > if you put the code in handleInputEvent, then you can write layout tests for > this feature more easily by using the eventSender I agree and I commented in https://bugs.webkit.org/show_bug.cgi?id=45560. I think the line of separation of concerns needs to move and possibly put GestureManager into WebCore/platform. Or something. > http://codereview.chromium.org/3388013/diff/15001/16005 > File chrome/renderer/render_widget.h (right): > > http://codereview.chromium.org/3388013/diff/15001/16005#newcode355 > chrome/renderer/render_widget.h:355: // A cached pointer to the GestureManager. > why cache this here? GestureManager::Get() is fast.
On Sat, Sep 25, 2010 at 11:24 AM, Darin Fisher <darin@chromium.org> wrote: > On Sat, Sep 25, 2010 at 8:21 AM, <darin@chromium.org> wrote: >> >> http://codereview.chromium.org/3388013/diff/15001/16004 >> File chrome/renderer/render_widget.cc (right): >> >> http://codereview.chromium.org/3388013/diff/15001/16004#newcode360 >> chrome/renderer/render_widget.cc:360: if >> (WebInputEvent::isTouchEventType(input_event->type)) { >> why is this code here instead of inside the >> WebViewImpl::handleInputEvent function? >> >> if you put the code in handleInputEvent, then you can write layout tests >> for this feature more easily by using the eventSender > > also, I recall objecting to the addition of WebWidget::gestureScroll. > GestureManager, which is the caller of gestureScroll, could easily live in > WebKit. Sorry for pestering you with this CL. I managed to misunderstand the degree of your objection in the bugzilla comment. I agree though: architecturally the GestureManager is much better living in WebKit. I'll revise the WebKit CL shortly. Rob. > -Darin > >> >> http://codereview.chromium.org/3388013/diff/15001/16005 >> File chrome/renderer/render_widget.h (right): >> >> http://codereview.chromium.org/3388013/diff/15001/16005#newcode355 >> chrome/renderer/render_widget.h:355: // A cached pointer to the >> GestureManager. >> why cache this here? GestureManager::Get() is fast. >> >> http://codereview.chromium.org/3388013/show > >
On Mon, Sep 27, 2010 at 10:19 AM, Robert Kroeger <rjkroege@chromium.org>wrote: > On Sat, Sep 25, 2010 at 11:24 AM, Darin Fisher <darin@chromium.org> wrote: > > On Sat, Sep 25, 2010 at 8:21 AM, <darin@chromium.org> wrote: > >> > >> http://codereview.chromium.org/3388013/diff/15001/16004 > >> File chrome/renderer/render_widget.cc (right): > >> > >> http://codereview.chromium.org/3388013/diff/15001/16004#newcode360 > >> chrome/renderer/render_widget.cc:360: if > >> (WebInputEvent::isTouchEventType(input_event->type)) { > >> why is this code here instead of inside the > >> WebViewImpl::handleInputEvent function? > >> > >> if you put the code in handleInputEvent, then you can write layout tests > >> for this feature more easily by using the eventSender > > > > also, I recall objecting to the addition of WebWidget::gestureScroll. > > GestureManager, which is the caller of gestureScroll, could easily live > in > > WebKit. > > Sorry for pestering you with this CL. I managed to misunderstand the > degree of your objection in the bugzilla comment. I agree though: > architecturally the GestureManager is much better living in WebKit. > I'll revise the WebKit CL shortly. > > Rob. > Thanks! > > > -Darin > > > >> > >> http://codereview.chromium.org/3388013/diff/15001/16005 > >> File chrome/renderer/render_widget.h (right): > >> > >> http://codereview.chromium.org/3388013/diff/15001/16005#newcode355 > >> chrome/renderer/render_widget.h:355: // A cached pointer to the > >> GestureManager. > >> why cache this here? GestureManager::Get() is fast. > >> > >> http://codereview.chromium.org/3388013/show > > > > > |