Getting RenderWidgetHostViewWin ready for a GestureRecognizer.

Adding RenderWidgetHostViewWin::touch_event_ (still not in use)


BUG=124935
TEST=

[girard, 2012-05-03 21:39:23]

First CL in a series.  This adds a GestureRecognizer, and adds support classes
to initialize it.

[sky, 2012-05-03 22:39:19]

http://codereview.chromium.org/10134045/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/10134045/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_view_win.cc:338:
gesture_recognizer_(ui::GestureRecognizer::Create(this))),
If gesture_recognizer_ is going to created in the constructor, no point in
placing in a scoped_ptr.

http://codereview.chromium.org/10134045/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/10134045/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_view_win.h:475: // Overridden
from ui::GestureEventHelper.
Move these right beneath
virtual void OnSysColorChange() OVERRIDE

http://codereview.chromium.org/10134045/diff/1/ui/views/events/event.cc
File ui/views/events/event.cc (right):

http://codereview.chromium.org/10134045/diff/1/ui/views/events/event.cc#newco...
ui/views/events/event.cc:283: while (!(1 << ++i & touch_ids_bitfield_));
nit: params around (1 << ++i)

http://codereview.chromium.org/10134045/diff/1/ui/views/events/event.h
File ui/views/events/event.h (right):

http://codereview.chromium.org/10134045/diff/1/ui/views/events/event.h#newcod...
ui/views/events/event.h:454: // touch_ids_bitfield_ is the set of touch_ids
associate with this gesture.
Place |s variable names in comments, eg |touch_ids_bitfield_|.

[girard, 2012-05-03 23:07:08]

Updated as per review.
First CL in a series.  This adds a GestureRecognizer, and adds support classes
to initialize it.

[sky, 2012-05-03 23:22:38]

http://codereview.chromium.org/10134045/diff/6005/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/10134045/diff/6005/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.h:491:
ui::GestureRecognizer * gesture_recognizer_;
This leaks now. I'm saying declare it by value:
ui::GestureRecognizer gesture_recognizer_;

That said, you're not even using gesture_recognizer_ in this patch. Hold off on
adding it until you actually use it.

[tfarina, 2012-05-03 23:25:26]

http://codereview.chromium.org/10134045/diff/6005/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/10134045/diff/6005/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.h:491:
ui::GestureRecognizer * gesture_recognizer_;
If you use scoped_ptr here, you can forward declare GestireRecognizer. You don't
need to include gesture_recognizer.h. And gesture_types.h is only needed to
inherit from ui::GestureEventHelper.

[girard, 2012-05-03 23:44:42]

On 2012/05/03 23:25:26, tfarina wrote:
>
http://codereview.chromium.org/10134045/diff/6005/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host_view_win.h (right):
> 
>
http://codereview.chromium.org/10134045/diff/6005/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_view_win.h:491:
> ui::GestureRecognizer * gesture_recognizer_;
> If you use scoped_ptr here, you can forward declare GestireRecognizer. You
don't
> need to include gesture_recognizer.h. And gesture_types.h is only needed to
> inherit from ui::GestureEventHelper.

+1 on reducing the headers.  Good tip!

[girard, 2012-05-04 00:10:51]

Corrected headers, removed leak.

[tfarina, 2012-05-04 00:15:31]

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:1008: bool
RenderWidgetHostViewWin::DispatchLongPressGestureEvent(
these new methods here are not in the same order they are declared in the header
file. The order should match.

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:1038: param_first,
param_second, touch_id_bitfield);
fix indentation here, it should be 4 spaces only. Or if it fits in 80 chars,
align param_first with type.

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.h:255: virtual
ui::GestureEvent* CreateGestureEvent(ui::EventType type,
move to the next line.

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.h:258: const
base::Time time,
const base::Time& ?

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.h:261: unsigned int
touch_id_bitfield) OVERRIDE;
uint16?

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.h:262: virtual
ui::TouchEvent* CreateTouchEvent(ui::EventType type,
move to the next line.

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.h:265: base::TimeDelta
time_stamp) OVERRIDE;
does time_stamp needs to be passed by value? can it be passed by
const-reference?

[girard, 2012-05-04 00:58:30]

Thanks for the feedback.

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:1008: bool
RenderWidgetHostViewWin::DispatchLongPressGestureEvent(
On 2012/05/04 00:15:32, tfarina wrote:
> these new methods here are not in the same order they are declared in the
header
> file. The order should match.

Done.

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:1038: param_first,
param_second, touch_id_bitfield);
On 2012/05/04 00:15:32, tfarina wrote:
> fix indentation here, it should be 4 spaces only. Or if it fits in 80 chars,
> align param_first with type.

Done.

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.h:255: virtual
ui::GestureEvent* CreateGestureEvent(ui::EventType type,
On 2012/05/04 00:15:32, tfarina wrote:
> move to the next line.

Done.

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.h:258: const
base::Time time,
On 2012/05/04 00:15:32, tfarina wrote:
> const base::Time& ?
Agreed.  This is a virtual function, so the change will impact many other
files;(

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.h:262: virtual
ui::TouchEvent* CreateTouchEvent(ui::EventType type,
On 2012/05/04 00:15:32, tfarina wrote:
> move to the next line.

Done.

http://codereview.chromium.org/10134045/diff/11006/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.h:265: base::TimeDelta
time_stamp) OVERRIDE;
On 2012/05/04 00:15:32, tfarina wrote:
> does time_stamp needs to be passed by value? can it be passed by
> const-reference?
Another virtual function changes signature;)  Done.

[girard, 2012-05-04 01:00:34]

Some interface cleanup as per tfarina's suggestions.  This change impacts aura
code and the base classes (so the changes are expanding!)
Not sure what to do about a long line in root_window.cc.  Can I break OVERRIDE
onto its own line?

[sky, 2012-05-04 03:51:02]

On Thu, May 3, 2012 at 4:44 PM,  <girard@chromium.org> wrote:
> On 2012/05/03 23:25:26, tfarina wrote:
>
>
http://codereview.chromium.org/10134045/diff/6005/content/browser/renderer_ho...
>>
>> File content/browser/renderer_host/render_widget_host_view_win.h (right):
>
>
>
>
http://codereview.chromium.org/10134045/diff/6005/content/browser/renderer_ho...
>>
>> content/browser/renderer_host/render_widget_host_view_win.h:491:
>> ui::GestureRecognizer * gesture_recognizer_;
>> If you use scoped_ptr here, you can forward declare GestireRecognizer. You
>
> don't
>>
>> need to include gesture_recognizer.h. And gesture_types.h is only needed
>> to
>> inherit from ui::GestureEventHelper.
>
>
> +1 on reducing the headers.  Good tip!

The style guide says no to this, which is why I didn't suggest it.
Specifically:

"Sometimes it makes sense to have pointer (or better, scoped_ptr)
members instead of object members. However, this complicates code
readability and imposes a performance penalty, so avoid doing this
transformation if the only purpose is to minimize includes in header
files."

  -Scott

>
>
> http://codereview.chromium.org/10134045/

[girard, 2012-05-04 04:18:51]

General preparation for RenderWidgetHostViewWin, so that we can implement a
GestureRecognizer to it.  (That part will be a follow-on CL.)

[girard, 2012-05-04 04:26:56]

General preparation for RenderWidgetHostViewWin, so that we can implement a
GestureRecognizer to it.  (That part will be a follow-on CL.)

[sky, 2012-05-04 15:36:52]

Remove the const Time& and LGTM

http://codereview.chromium.org/10134045/diff/6006/ui/aura/root_window.h
File ui/aura/root_window.h (right):

http://codereview.chromium.org/10134045/diff/6006/ui/aura/root_window.h#newco...
ui/aura/root_window.h:300: const base::Time& time,
As Time and TimeDelta are primitive types we don't pass them by reference.

[tfarina, 2012-05-04 16:01:10]

On Fri, May 4, 2012 at 12:36 PM,  <sky@chromium.org> wrote:
> Remove the const Time& and LGTM
>
>
> http://codereview.chromium.org/10134045/diff/6006/ui/aura/root_window.h
> File ui/aura/root_window.h (right):
>
>
http://codereview.chromium.org/10134045/diff/6006/ui/aura/root_window.h#newco...
> ui/aura/root_window.h:300: const base::Time& time,
> As Time and TimeDelta are primitive types we don't pass them by
> reference.
>
Yeah, sorry for this. It's fine to pass base::Time by value as it's
represented as an int64 internally. Although we violate that in a
bunch of places, passing it by const-reference.

> http://codereview.chromium.org/10134045/

-- 
Thiago

[commit-bot: I haz the power, 2012-05-04 16:08:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/girard@chromium.org/10134045/13008

[commit-bot: I haz the power, 2012-05-04 16:26:11]

Try job failure for 10134045-13008 (retry) on win_rel for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sky, 2012-05-04 16:34:36]

http://codereview.chromium.org/10134045/diff/13008/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/10134045/diff/13008/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:61: #include
"ui/views/events/event.h"
As Rob pointed out, this include isn't allowed from content.