Add constructors to WebInputEvents and setters so we can work at cleaning up these public structs.

Add constructors to WebInputEvents and setters so we can work at
cleaning up these public structs.

Adding constructors allow us to ensure that certain fields such as the
timestamp are initialized correctly without having to default initialize
it with a slow API call like querying the current time.

Design: https://docs.google.com/document/d/1s4Lfy22CNU1OZ5Rec6Oano_5BvIhdK6uFVsVe7FphKI/edit

BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=wez@chromium.org, thestig@chromium.org, dcheng@chromium.org, clamy@chromium.org

Committed: https://crrev.com/899ac22255a8d7856d70e41425469e977e0d9f61
Cr-Commit-Position: refs/heads/master@{#441153}

[dtapuska, 2016-12-13 18:45:24]

Description was changed from

==========
Add constructors to WebInputEvents and setters so we can work at cleaning up
these public structs.

BUG=
==========

to

==========
Add constructors to WebInputEvents and setters so we can work at cleaning up
these public structs.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dtapuska, 2016-12-13 18:45:29]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 18:46:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 19:10:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 19:10:52]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[dtapuska, 2016-12-13 21:03:11]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 21:03:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 21:49:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 21:50:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dtapuska, 2016-12-13 22:08:07]

Description was changed from

==========
Add constructors to WebInputEvents and setters so we can work at cleaning up
these public structs.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add constructors to WebInputEvents and setters so we can work at
cleaning up these public structs.
    
Adding constructors allow us to ensure that certain fields such as the
timestamp are initialized correctly without having to default initialize
it with a slow API call like querying the current time.
    
Design:
https://docs.google.com/document/d/1s4Lfy22CNU1OZ5Rec6Oano_5BvIhdK6uFVsVe7Fph...
    
BUG=625684
==========

[dtapuska, 2016-12-13 22:09:36]

Description was changed from

==========
Add constructors to WebInputEvents and setters so we can work at
cleaning up these public structs.
    
Adding constructors allow us to ensure that certain fields such as the
timestamp are initialized correctly without having to default initialize
it with a slow API call like querying the current time.
    
Design:
https://docs.google.com/document/d/1s4Lfy22CNU1OZ5Rec6Oano_5BvIhdK6uFVsVe7Fph...
    
BUG=625684
==========

to

==========
Add constructors to WebInputEvents and setters so we can work at
cleaning up these public structs.
    
Adding constructors allow us to ensure that certain fields such as the
timestamp are initialized correctly without having to default initialize
it with a slow API call like querying the current time.
    
Design:
https://docs.google.com/document/d/1s4Lfy22CNU1OZ5Rec6Oano_5BvIhdK6uFVsVe7Fph...
    
BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dtapuska, 2016-12-13 22:09:40]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 22:10:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 00:10:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 00:10:33]

Dry run: 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_...)

[dtapuska, 2016-12-14 16:34:36]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 16:35:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 17:45:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 17:45:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dtapuska, 2016-12-14 18:50:28]

dtapuska@chromium.org changed reviewers:
+ esprehn@chromium.org, majidvp@chromium.org

[dtapuska, 2016-12-14 18:50:29]

esprehn@ can you provide feedback on this. The goal is to move WebInputEvent*
away from a POD and to a C++ API.

[esprehn, 2016-12-14 19:03:50]

This seems reasonable to me, though I thought we used to use memset for speed
here and also directly wrote these structs through IPC somehow? We might want to
move this struct to mojom instead and use it on both sides of the IPC?

[dtapuska, 2016-12-14 19:05:55]

On 2016/12/14 19:03:50, esprehn wrote:
> This seems reasonable to me, though I thought we used to use memset for speed
> here and also directly wrote these structs through IPC somehow? We might want
to
> move this struct to mojom instead and use it on both sides of the IPC?

Yup and I'd keep this class as essentially a POD for now. But once we do that
then we can add more virtual functions to clean the code up. I presume we would
define a mojom type that would populate this object to be consumed by the web
platform.

[esprehn, 2016-12-14 19:07:47]

On 2016/12/14 at 19:05:55, dtapuska wrote:
> On 2016/12/14 19:03:50, esprehn wrote:
> > This seems reasonable to me, though I thought we used to use memset for
speed
> > here and also directly wrote these structs through IPC somehow? We might
want to
> > move this struct to mojom instead and use it on both sides of the IPC?
> 
> Yup and I'd keep this class as essentially a POD for now. But once we do that
then we can add more virtual functions to clean the code up. I presume we would
define a mojom type that would populate this object to be consumed by the web
platform.

You could even use the mojom type directly inside the web platform code if you
wanted, it depends on how low/high level it ends up being.

[dtapuska, 2016-12-15 16:18:59]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 16:19:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 18:15:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 18:16:01]

Dry run: This issue passed the CQ dry run.

[majidvp, 2016-12-19 20:09:53]

I definitely support the idea of cleaning up the initialization
by requiring the fields in the constructor. I can see
that you have already found and fixed a few cases
where we were not initializing the timestamp correctly. YAY.

https://codereview.chromium.org/2569273002/diff/80001/blimp/client/core/input...
File blimp/client/core/input/blimp_input_manager.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/blimp/client/core/input...
blimp/client/core/input/blimp_input_manager.cc:84: web_gesture.setModifiers(0);
s/0/NoModifiers/

https://codereview.chromium.org/2569273002/diff/80001/blimp/net/input_message...
File blimp/net/input_message_unittest.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/blimp/net/input_message...
blimp/net/input_message_unittest.cc:38: blink::WebGestureEvent event(type,
blink::WebInputEvent::NoModifiers, 1.23);
nit: s/1.23/TimeStampForTesting/

https://codereview.chromium.org/2569273002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_generation_interactive_uitest.cc
(right):

https://codereview.chromium.org/2569273002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_generation_interactive_uitest.cc:122:
ui::EventTimeForNow());
nit: can this just be TimeStampForTesting?

https://codereview.chromium.org/2569273002/diff/80001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/fullscreen_controller_test.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/fullscreen_controller_test.cc:52:
ui::EventTimeForNow());
nit: can this be TimeStampForTesting

https://codereview.chromium.org/2569273002/diff/80001/components/test_runner/...
File components/test_runner/event_sender.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/components/test_runner/...
components/test_runner/event_sender.cc:2133: if
(HandleInputEventOnViewOrPopup(wheel_event) ==
hmm, I think there is a pre-existing bug here where we dispatch the generated
wheel event even if args are incorrect and we should be throwing an error.

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:191:
ui::EventTimeStampToSeconds(ui::EventTimeForNow()), ui::LatencyInfo());
Any idea why the original logic was doing rather complicated dance of creating a
web keyboard event and using it to initialize a native event using memcpy? I
just want to make sure we are not missing some subtle 
behavior here.

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:510:
ui::EventTimeStampToSeconds(base::TimeTicks::Now()), LatencyInfo());
nit: s/base::TimeTicks::Now()/ui::EventTimeForNow()/

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/native_web_keyboard_event_android.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
content/browser/renderer_host/native_web_keyboard_event_android.cc:39:
os_event(NULL),
s/NULL/nullptr/

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/native_web_keyboard_event_aura.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
content/browser/renderer_host/native_web_keyboard_event_aura.cc:39:
os_event(NULL),
s/NULL/nullptr/

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_unittest.cc:549:
NativeWebKeyboardEvent native_event(type, modifiers, ui::EventTimeForNow());
Shouldn't this be the simulate time i.e., GetNextSimulatedEventTimeSeconds?

https://codereview.chromium.org/2569273002/diff/80001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:1609:
mouse_event.setModifiers(0);
s/0/NoModifiers/

https://codereview.chromium.org/2569273002/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:7698:
ui::EventTimeStampFromSeconds(100));
nit: 100 feels arbitrary here and could be replace with EventTimeForTesting.

https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebInputEvent.h (left):

https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebInputEvent.h:273: modifiers = 0;
why do you no longer not need to initialize modifiers? Is it because it is being
zeroed by memset?

https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebInputEvent.h (right):

https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebInputEvent.h:197: NoModifiers = 0,
nit: Can we just have None = 0 and put it at the top and use
WebInputEvent::Modifiers::None and usage location? I feel this is cleaner and
does not lose any or the readability.

https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebInputEvent.h:229: 
Great idea to have a constant for testing.

[dtapuska, 2016-12-20 19:27:23]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 19:28:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dtapuska, 2016-12-20 19:49:21]

https://codereview.chromium.org/2569273002/diff/80001/blimp/client/core/input...
File blimp/client/core/input/blimp_input_manager.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/blimp/client/core/input...
blimp/client/core/input/blimp_input_manager.cc:84: web_gesture.setModifiers(0);
On 2016/12/19 20:09:52, majidvp wrote:
> s/0/NoModifiers/

Done.

https://codereview.chromium.org/2569273002/diff/80001/blimp/net/input_message...
File blimp/net/input_message_unittest.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/blimp/net/input_message...
blimp/net/input_message_unittest.cc:38: blink::WebGestureEvent event(type,
blink::WebInputEvent::NoModifiers, 1.23);
On 2016/12/19 20:09:52, majidvp wrote:
> nit: s/1.23/TimeStampForTesting/

done; but I was trying to keep the original code but I agree this is better.

https://codereview.chromium.org/2569273002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_generation_interactive_uitest.cc
(right):

https://codereview.chromium.org/2569273002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_generation_interactive_uitest.cc:122:
ui::EventTimeForNow());
On 2016/12/19 20:09:52, majidvp wrote:
> nit: can this just be TimeStampForTesting?

Done.

https://codereview.chromium.org/2569273002/diff/80001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/fullscreen_controller_test.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/fullscreen_controller_test.cc:52:
ui::EventTimeForNow());
On 2016/12/19 20:09:52, majidvp wrote:
> nit: can this be TimeStampForTesting

Done.

https://codereview.chromium.org/2569273002/diff/80001/components/test_runner/...
File components/test_runner/event_sender.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/components/test_runner/...
components/test_runner/event_sender.cc:2133: if
(HandleInputEventOnViewOrPopup(wheel_event) ==
On 2016/12/19 20:09:52, majidvp wrote:
> hmm, I think there is a pre-existing bug here where we dispatch the generated
> wheel event even if args are incorrect and we should be throwing an error. 

Done.

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:191:
ui::EventTimeStampToSeconds(ui::EventTimeForNow()), ui::LatencyInfo());
On 2016/12/19 20:09:52, majidvp wrote:
> Any idea why the original logic was doing rather complicated dance of creating
a
> web keyboard event and using it to initialize a native event using memcpy? I
> just want to make sure we are not missing some subtle 
> behavior here. 

Because there wasn't a constructor on the NativeWebKeyboardEvent that took a
type in.

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:510:
ui::EventTimeStampToSeconds(base::TimeTicks::Now()), LatencyInfo());
On 2016/12/19 20:09:52, majidvp wrote:
> nit: s/base::TimeTicks::Now()/ui::EventTimeForNow()/

Done.

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/native_web_keyboard_event_android.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
content/browser/renderer_host/native_web_keyboard_event_android.cc:39:
os_event(NULL),
On 2016/12/19 20:09:52, majidvp wrote:
> s/NULL/nullptr/

Done.

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/native_web_keyboard_event_aura.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
content/browser/renderer_host/native_web_keyboard_event_aura.cc:39:
os_event(NULL),
On 2016/12/19 20:09:52, majidvp wrote:
> s/NULL/nullptr/

Done.

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_unittest.cc:549:
NativeWebKeyboardEvent native_event(type, modifiers, ui::EventTimeForNow());
On 2016/12/19 20:09:52, majidvp wrote:
> Shouldn't this be the simulate time i.e., GetNextSimulatedEventTimeSeconds?

Done.

https://codereview.chromium.org/2569273002/diff/80001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2569273002/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:1609:
mouse_event.setModifiers(0);
On 2016/12/19 20:09:52, majidvp wrote:
> s/0/NoModifiers/

Done.

https://codereview.chromium.org/2569273002/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:7698:
ui::EventTimeStampFromSeconds(100));
On 2016/12/19 20:09:52, majidvp wrote:
> nit: 100 feels arbitrary here and could be replace with EventTimeForTesting.

Done.

https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebInputEvent.h (left):

https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebInputEvent.h:273: modifiers = 0;
On 2016/12/19 20:09:53, majidvp wrote:
> why do you no longer not need to initialize modifiers? Is it because it is
being
> zeroed by memset?

yes it is an int and zeroed in the memset. I left the assignment for the others
because Undefined could change.

https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebInputEvent.h (right):

https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebInputEvent.h:197: NoModifiers = 0,
On 2016/12/19 20:09:52, majidvp wrote:
> nit: Can we just have None = 0 and put it at the top and use
> WebInputEvent::Modifiers::None and usage location? I feel this is cleaner and
> does not lose any or the readability.

Done.

https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebInputEvent.h:229: 
On 2016/12/19 20:09:52, majidvp wrote:
> Great idea to have a constant for testing.

Done.

[dtapuska, 2016-12-20 20:39:27]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 20:40:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 23:57:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 23:57:19]

Dry run: This issue passed the CQ dry run.

[majidvp, 2016-12-21 15:49:45]

lgtm.

I should note that the patch also catches and fixes some pre-existing issues
where the timestamp
was not initialized. This is exactly why we need this constructors as previously
it was very easy
to not initialize the struct field correctly.

Although this changes the behavior but I think the fixes are safe enough to
bundle with this
patch.

https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebInputEvent.h (right):

https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebInputEvent.h:197: NoModifiers = 0,
On 2016/12/19 20:09:52, majidvp wrote:
> nit: Can we just have None = 0 and put it at the top and use
> WebInputEvent::Modifiers::None and usage location? I feel this is cleaner and
> does not lose any or the readability.

I prefer these bitset values to be defined as const int and live 
outside of the enum. The enum is better be used to declare constant
bit flags instead. There is already InputModifiers that does this.

But this is not really related to this patch so lgtm.

[dtapuska, 2016-12-22 00:05:54]

On 2016/12/21 15:49:45, majidvp wrote:
> lgtm.
> 
> I should note that the patch also catches and fixes some pre-existing issues
> where the timestamp
> was not initialized. This is exactly why we need this constructors as
previously
> it was very easy
> to not initialize the struct field correctly.
> 
> Although this changes the behavior but I think the fixes are safe enough to
> bundle with this
> patch.
> 
>
https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
> File third_party/WebKit/public/platform/WebInputEvent.h (right):
> 
>
https://codereview.chromium.org/2569273002/diff/80001/third_party/WebKit/publ...
> third_party/WebKit/public/platform/WebInputEvent.h:197: NoModifiers = 0,
> On 2016/12/19 20:09:52, majidvp wrote:
> > nit: Can we just have None = 0 and put it at the top and use
> > WebInputEvent::Modifiers::None and usage location? I feel this is cleaner
and
> > does not lose any or the readability.
> 
> I prefer these bitset values to be defined as const int and live 
> outside of the enum. The enum is better be used to declare constant
> bit flags instead. There is already InputModifiers that does this.
> 
> But this is not really related to this patch so lgtm.

Esprehn@ are you able to look at this?

[esprehn, 2016-12-22 02:33:13]

This patch is so incredibly massive it's very hard to review. Does it change
behavior? You said it fixes some time things we forgot to set, how would that
manifest before?

Rs lgtm, but in the future it'd be nice to keep it as mechanical as possible and
not change or fix anything so it's more clear when we can stamp things. :)

[dtapuska, 2017-01-03 14:02:30]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 14:02:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dtapuska, 2017-01-03 14:58:50]

dtapuska@chromium.org changed reviewers:
+ rbyers@chromium.org

[dtapuska, 2017-01-03 14:58:51]

rbyers@chromium.org: Please review changes in 

event_sender.[cc|h]

[Rick Byers, 2017-01-03 15:39:25]

event_sender LGTM

[dtapuska, 2017-01-03 15:48:02]

dtapuska@chromium.org changed reviewers:
+ clamy@chromium.org, dcheng@chromium.org, thestig@chromium.org,
wez@chromium.org

[dtapuska, 2017-01-03 15:48:03]

wez@chromium.org: Please review changes in 
blimp/*

clamy@chromium.org: Please review changes in 
content/*

thestig@chromium.org: Please review changes in 
components/printing/test/print_web_view_helper_browsertest.cc
chrome/*

dcheng@chromium.org: Please review changes in
content/common/input/input_param_traits_unittest.cc

[commit-bot: I haz the power, 2017-01-03 16:19:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 16:19:03]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2017-01-03 18:01:18]

Description was changed from

==========
Add constructors to WebInputEvents and setters so we can work at
cleaning up these public structs.
    
Adding constructors allow us to ensure that certain fields such as the
timestamp are initialized correctly without having to default initialize
it with a slow API call like querying the current time.
    
Design:
https://docs.google.com/document/d/1s4Lfy22CNU1OZ5Rec6Oano_5BvIhdK6uFVsVe7Fph...
    
BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add constructors to WebInputEvents and setters so we can work at
cleaning up these public structs.
    
Adding constructors allow us to ensure that certain fields such as the
timestamp are initialized correctly without having to default initialize
it with a slow API call like querying the current time.
    
Design:
https://docs.google.com/document/d/1s4Lfy22CNU1OZ5Rec6Oano_5BvIhdK6uFVsVe7Fph...
    
BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=wez@chromium.org, thestig@chromium.org, dcheng@chromium.org,
clamy@chromium.org
==========

[dtapuska, 2017-01-03 18:02:17]

On 2017/01/03 15:48:03, dtapuska wrote:
> mailto:wez@chromium.org: Please review changes in 
> blimp/*
> 
> mailto:clamy@chromium.org: Please review changes in 
> content/*
> 
> mailto:thestig@chromium.org: Please review changes in 
> components/printing/test/print_web_view_helper_browsertest.cc
> chrome/*
> 
> mailto:dcheng@chromium.org: Please review changes in
> content/common/input/input_param_traits_unittest.cc

TBR top level owners.

[dtapuska, 2017-01-03 18:02:34]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2017-01-03 18:02:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, majidvp@chromium.org
Link to the patchset: https://codereview.chromium.org/2569273002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-03 18:03:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 18:09:21]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483466554843910,
"parent_rev": "5ce64346c4f8d3f27a2021a23d10c1d1fb825239", "commit_rev":
"e2642286e03404f81e33ebb7604dbec3e804c3c8"}

[commit-bot: I haz the power, 2017-01-03 18:09:52]

Description was changed from

==========
Add constructors to WebInputEvents and setters so we can work at
cleaning up these public structs.
    
Adding constructors allow us to ensure that certain fields such as the
timestamp are initialized correctly without having to default initialize
it with a slow API call like querying the current time.
    
Design:
https://docs.google.com/document/d/1s4Lfy22CNU1OZ5Rec6Oano_5BvIhdK6uFVsVe7Fph...
    
BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=wez@chromium.org, thestig@chromium.org, dcheng@chromium.org,
clamy@chromium.org
==========

to

==========
Add constructors to WebInputEvents and setters so we can work at
cleaning up these public structs.
    
Adding constructors allow us to ensure that certain fields such as the
timestamp are initialized correctly without having to default initialize
it with a slow API call like querying the current time.
    
Design:
https://docs.google.com/document/d/1s4Lfy22CNU1OZ5Rec6Oano_5BvIhdK6uFVsVe7Fph...
    
BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=wez@chromium.org, thestig@chromium.org, dcheng@chromium.org,
clamy@chromium.org

Review-Url: https://codereview.chromium.org/2569273002
==========

[commit-bot: I haz the power, 2017-01-03 18:09:56]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2017-01-03 18:12:24]

Description was changed from

==========
Add constructors to WebInputEvents and setters so we can work at
cleaning up these public structs.
    
Adding constructors allow us to ensure that certain fields such as the
timestamp are initialized correctly without having to default initialize
it with a slow API call like querying the current time.
    
Design:
https://docs.google.com/document/d/1s4Lfy22CNU1OZ5Rec6Oano_5BvIhdK6uFVsVe7Fph...
    
BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=wez@chromium.org, thestig@chromium.org, dcheng@chromium.org,
clamy@chromium.org

Review-Url: https://codereview.chromium.org/2569273002
==========

to

==========
Add constructors to WebInputEvents and setters so we can work at
cleaning up these public structs.

Adding constructors allow us to ensure that certain fields such as the
timestamp are initialized correctly without having to default initialize
it with a slow API call like querying the current time.

Design:
https://docs.google.com/document/d/1s4Lfy22CNU1OZ5Rec6Oano_5BvIhdK6uFVsVe7Fph...

BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=wez@chromium.org, thestig@chromium.org, dcheng@chromium.org,
clamy@chromium.org

Committed: https://crrev.com/899ac22255a8d7856d70e41425469e977e0d9f61
Cr-Commit-Position: refs/heads/master@{#441153}
==========

[commit-bot: I haz the power, 2017-01-03 18:12:28]

Patchset 8 (id:??) landed as
https://crrev.com/899ac22255a8d7856d70e41425469e977e0d9f61
Cr-Commit-Position: refs/heads/master@{#441153}

[Wez, 2017-01-04 00:02:27]

blimp/ LGTM

[dcheng, 2017-01-04 00:34:52]

content/common/input/input_param_traits_unittest.cc lgtm

[Lei Zhang, 2017-02-24 08:43:23]

components/printing/test/print_web_view_helper_browsertest.cc
and chrome/ lgtm

https://codereview.chromium.org/2569273002/diff/140001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/user_input_tracker_unittest.cc (right):

https://codereview.chromium.org/2569273002/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/user_input_tracker_unittest.cc:25:
FakeInputEvent(blink::WebInputEvent::Type type = blink::WebInputEvent::Char,
I'm personally biased again default arguments. You could have made this without
default arguments and added a default ctor that delegates to this one. That
would cover everything except the IgnoredEventType case.

But this is also a test, and it's not my code. So I'm meh more about it. Maybe
mention this to bmcquade and see if there's a stronger preference?