Collapse the API surface on WebInputEvent via accessor functions.

Collapse the API surface on WebInputEvent via accessor functions.

Move type, size, timeStampSeconds and modifiers to be private members.
This change will help harden the API so that we can place DCHECKS on
certain states.

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

BUG=625684
TBR=jochen@chromium.org, wez@chromium.org, thestig@chromium.org, sadrul@chromium.org, rockot@chromium.org, dcheng@chromium.org, girard@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2573073003
Cr-Commit-Position: refs/heads/master@{#443407}
Committed: https://chromium.googlesource.com/chromium/src/+/8c4dae156ba6495186bebe76aa4be4dc76bd7cd5

[dtapuska, 2016-12-14 17:01:14]

Description was changed from

==========
Collapse the API surface on WebInputEvent via accessor functions.

Move type, size, timeStampSeconds and modifiers to be private members.
This change will help harden the API so that we can place DCHECKS on
certain states.

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

BUG=625684
==========

to

==========
Collapse the API surface on WebInputEvent via accessor functions.

Move type, size, timeStampSeconds and modifiers to be private members.
This change will help harden the API so that we can place DCHECKS on
certain states.

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

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

[dtapuska, 2016-12-14 17:03:56]

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

[commit-bot: I haz the power, 2016-12-14 17:04:34]

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:08:07]

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

[commit-bot: I haz the power, 2016-12-14 17:08:07]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

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

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

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

https://codereview.chromium.org/2573073003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebInputEvent.h (right):

https://codereview.chromium.org/2573073003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebInputEvent.h:280: memset(this, 0,
sizeParam);
It's very weird to memset yourself once you're a real class with members, can we
add actual init for the members instead?

https://codereview.chromium.org/2573073003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebInputEvent.h:289: m_timeStampSeconds =
0.0;
this is a duplicate of the memset?

[dtapuska, 2017-01-12 16:45:13]

https://codereview.chromium.org/2573073003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebInputEvent.h (right):

https://codereview.chromium.org/2573073003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebInputEvent.h:280: memset(this, 0,
sizeParam);
On 2016/12/14 19:03:05, esprehn wrote:
> It's very weird to memset yourself once you're a real class with members, can
we
> add actual init for the members instead?

I've added a TODO. I think the memset is necessary because this object is sent
across the IPC boundary. Once we have fully moved to mojo we remove it to ensure
we don't leak process memory from the browser process.

https://codereview.chromium.org/2573073003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebInputEvent.h:289: m_timeStampSeconds =
0.0;
On 2016/12/14 19:03:05, esprehn wrote:
> this is a duplicate of the memset?

The code was that way before and I presume someone didn't want to assume the
float representation of 0.0 was all zeros in memory. But I can remove it if you
have a big concern.

[dtapuska, 2017-01-12 16:45:39]

Description was changed from

==========
Collapse the API surface on WebInputEvent via accessor functions.

Move type, size, timeStampSeconds and modifiers to be private members.
This change will help harden the API so that we can place DCHECKS on
certain states.

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

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

to

==========
Collapse the API surface on WebInputEvent via accessor functions.

Move type, size, timeStampSeconds and modifiers to be private members.
This change will help harden the API so that we can place DCHECKS on
certain states.

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

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

[dtapuska, 2017-01-12 16:45:39]

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

[dtapuska, 2017-01-12 16:46:13]

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

[commit-bot: I haz the power, 2017-01-12 16:46:39]

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, 2017-01-12 18:47:33]

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

[commit-bot: I haz the power, 2017-01-12 18:47:34]

Dry run: This issue passed the CQ dry run.

[esprehn, 2017-01-12 19:53:17]

On 2017/01/12 at 16:45:13, dtapuska wrote:
>
https://codereview.chromium.org/2573073003/diff/1/third_party/WebKit/public/p...
> File third_party/WebKit/public/platform/WebInputEvent.h (right):
> 
>
https://codereview.chromium.org/2573073003/diff/1/third_party/WebKit/public/p...
> third_party/WebKit/public/platform/WebInputEvent.h:280: memset(this, 0,
sizeParam);
> On 2016/12/14 19:03:05, esprehn wrote:
> > It's very weird to memset yourself once you're a real class with members,
can we
> > add actual init for the members instead?
> 
> I've added a TODO. I think the memset is necessary because this object is sent
across the IPC boundary. Once we have fully moved to mojo we remove it to ensure
we don't leak process memory from the browser process.

Hmm okay, hopefully we're moving to mojo soon!

> 
>
https://codereview.chromium.org/2573073003/diff/1/third_party/WebKit/public/p...
> third_party/WebKit/public/platform/WebInputEvent.h:289: m_timeStampSeconds =
0.0;
> On 2016/12/14 19:03:05, esprehn wrote:
> > this is a duplicate of the memset?
> 
> The code was that way before and I presume someone didn't want to assume the
float representation of 0.0 was all zeros in memory. But I can remove it if you
have a big concern.

It's just weird, but it's okay to leave it. Maybe add a comment about the memset
being related to IPC?

lgtm, this patch is so massive, I'm sort of assuming it's got no behavior
changes. :)

[dtapuska, 2017-01-12 21:09:55]

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

[commit-bot: I haz the power, 2017-01-12 21:10:50]

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

[majidvp, 2017-01-12 21:58:07]

mega patch lgtm

https://codereview.chromium.org/2573073003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/2573073003/diff/40001/content/browser/rendere...
content/browser/renderer_host/overscroll_controller.cc:74: }
nit: The automated indentation seems to have a bug here.
In particular the } should be aligned with return statement
in line below.

https://codereview.chromium.org/2573073003/diff/40001/ui/events/blink/web_inp...
File ui/events/blink/web_input_event_builders_win.cc (right):

https://codereview.chromium.org/2573073003/diff/40001/ui/events/blink/web_inp...
ui/events/blink/web_input_event_builders_win.cc:309:
result.setModifiers(result.modifiers() | WebInputEvent::ShiftKey);
nit: seems more efficient to just have a local modifiers value and update the
event modifiers at the end.

[dtapuska, 2017-01-12 22:01:30]

dtapuska@chromium.org changed reviewers:
+ dcheng@chromium.org, girard@chromium.org, jochen@chromium.org,
rockot@chromium.org, sadrul@chromium.org, thestig@chromium.org, wez@chromium.org

[dtapuska, 2017-01-12 22:01:32]

rockot@chromium.org: Please review changes in 
extensions/*

girard@chromium.org: Please review changes in 
vr_shell/*

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

thestig@chromium.org: Please review changes in 
chrome/*

sadrul@chromium.org: Please review changes in
ui/* 

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

dcheng@chromium.org: Please review changes in
*param_traits_unittest.cc

[dtapuska, 2017-01-12 22:08:36]

Description was changed from

==========
Collapse the API surface on WebInputEvent via accessor functions.

Move type, size, timeStampSeconds and modifiers to be private members.
This change will help harden the API so that we can place DCHECKS on
certain states.

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

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

to

==========
Collapse the API surface on WebInputEvent via accessor functions.

Move type, size, timeStampSeconds and modifiers to be private members.
This change will help harden the API so that we can place DCHECKS on
certain states.

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

BUG=625684
TBR=jochen@chromium.org, wez@chromium.org, thestig@chromium.org,
sadrul@chromium.org, rockot@chromium.org, dcheng@chromium.org,
girard@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dtapuska, 2017-01-12 22:09:34]

https://codereview.chromium.org/2573073003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/2573073003/diff/40001/content/browser/rendere...
content/browser/renderer_host/overscroll_controller.cc:74: }
On 2017/01/12 21:58:07, majidvp wrote:
> nit: The automated indentation seems to have a bug here.
> In particular the } should be aligned with return statement
> in line below.

Done.

https://codereview.chromium.org/2573073003/diff/40001/ui/events/blink/web_inp...
File ui/events/blink/web_input_event_builders_win.cc (right):

https://codereview.chromium.org/2573073003/diff/40001/ui/events/blink/web_inp...
ui/events/blink/web_input_event_builders_win.cc:309:
result.setModifiers(result.modifiers() | WebInputEvent::ShiftKey);
On 2017/01/12 21:58:07, majidvp wrote:
> nit: seems more efficient to just have a local modifiers value and update the
> event modifiers at the end.

Done.

[dtapuska, 2017-01-12 22:10:05]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2017-01-12 22:10:06]

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/2573073003/#ps60001
(title: "Fix nits")

[commit-bot: I haz the power, 2017-01-12 22:10:58]

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

[dcheng, 2017-01-12 22:12:07]

LGTM for:
content/common/content_param_traits.cc
content/common/input/input_param_traits_unittest.cc

[Wez, 2017-01-12 22:35:19]

blimp/ LGTM

[commit-bot: I haz the power, 2017-01-13 00:23:12]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484259005297880,
"parent_rev": "b0cb5ce73c6171b11cee16e6f512a4e0ca6a07eb", "commit_rev":
"8c4dae156ba6495186bebe76aa4be4dc76bd7cd5"}

[commit-bot: I haz the power, 2017-01-13 00:23:40]

Description was changed from

==========
Collapse the API surface on WebInputEvent via accessor functions.

Move type, size, timeStampSeconds and modifiers to be private members.
This change will help harden the API so that we can place DCHECKS on
certain states.

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

BUG=625684
TBR=jochen@chromium.org, wez@chromium.org, thestig@chromium.org,
sadrul@chromium.org, rockot@chromium.org, dcheng@chromium.org,
girard@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Collapse the API surface on WebInputEvent via accessor functions.

Move type, size, timeStampSeconds and modifiers to be private members.
This change will help harden the API so that we can place DCHECKS on
certain states.

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

BUG=625684
TBR=jochen@chromium.org, wez@chromium.org, thestig@chromium.org,
sadrul@chromium.org, rockot@chromium.org, dcheng@chromium.org,
girard@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2573073003
Cr-Commit-Position: refs/heads/master@{#443407}
Committed:
https://chromium.googlesource.com/chromium/src/+/8c4dae156ba6495186bebe76aa4b...
==========

[commit-bot: I haz the power, 2017-01-13 00:23:43]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8c4dae156ba6495186bebe76aa4b...

[sadrul, 2017-01-13 02:16:29]

lgtm

[girard, 2017-01-13 05:58:26]

lgtm (vr)

[Lei Zhang, 2017-02-24 08:24:19]

chrome/ and components/printing lgtm