Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(11)

Issue 3388013: Forward touch events to a RenderWidget's associated WebWidget (Closed)

Created:
9 years, 4 months ago by rjkroege
Modified:
7 years, 1 month ago
CC:
chromium-reviews, darin-cc_chromium.org, brettw-cc_chromium.org
Visibility:
Public.

Description

Forward 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+277 lines, -3 lines) Patch
M chrome/chrome_renderer.gypi View 2 chunks +8 lines, -0 lines 0 comments Download
A chrome/renderer/gesture_manager.h View 1 2 1 chunk +84 lines, -0 lines 0 comments Download
A chrome/renderer/gesture_manager.cc View 1 2 3 1 chunk +151 lines, -0 lines 0 comments Download
M chrome/renderer/render_widget.h View 2 chunks +9 lines, -0 lines 1 comment Download
M chrome/renderer/render_widget.cc View 5 chunks +25 lines, -3 lines 1 comment Download

Messages

Total messages: 12 (0 generated)
rjkroege
Note: depends on https://bugs.webkit.org/show_bug.cgi?id=45560.
9 years, 4 months ago (2010-09-21 21:54:38 UTC) #1
sky
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: ...
9 years, 4 months ago (2010-09-21 22:28:40 UTC) #2
rjkroege
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 ...
9 years, 4 months ago (2010-09-23 01:51:57 UTC) #3
sky
I don't think I'm really adding any value here other than style nits... http://codereview.chromium.org/3388013/diff/5001/6002 File ...
9 years, 4 months ago (2010-09-23 17:13:03 UTC) #4
rjkroege
I think I've addressed the comments. Hopefully I'll have the differences between the C++ style ...
9 years, 4 months ago (2010-09-23 21:36:55 UTC) #5
sky
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 ...
9 years, 4 months ago (2010-09-23 21:57:13 UTC) #6
rjkroege
Darin: could you have a look at the RenderWidget changes?
9 years, 4 months ago (2010-09-24 20:09:49 UTC) #7
darin (slow to review)
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 ...
9 years, 4 months ago (2010-09-25 15:21:06 UTC) #8
darin (slow to review)
On Sat, Sep 25, 2010 at 8:21 AM, <darin@chromium.org> wrote: > > http://codereview.chromium.org/3388013/diff/15001/16004 > File ...
9 years, 4 months ago (2010-09-25 15:24:29 UTC) #9
dglazkov1
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 > ...
9 years, 4 months ago (2010-09-27 02:07:12 UTC) #10
rjkroege
On Sat, Sep 25, 2010 at 11:24 AM, Darin Fisher <darin@chromium.org> wrote: > On Sat, ...
9 years, 4 months ago (2010-09-27 17:19:39 UTC) #11
darin (slow to review)
9 years, 4 months ago (2010-09-27 17:27:52 UTC) #12
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
> >
> >
>

Powered by Google App Engine
This is Rietveld 408576698