WIP Add getCoalescedEvents API using vector of WebInputEvents

Add getCoalescedEvents API using vector of WebInputEvents

This CL passes vector of coalesced events down
the path of input events.

BUG=656781

[Navid Zolghadr, 2016-11-07 15:55:08]

Description was changed from

==========
Add getCoalescedEvents API

This CL passes vector of coalesced events down
the path of input events.

BUG=656781
==========

to

==========
Add getCoalescedEvents API

This CL passes vector of coalesced events down
the path of input events.

BUG=656781
==========

[Navid Zolghadr, 2016-11-07 16:01:29]

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

[Navid Zolghadr, 2016-11-07 16:01:31]

Can you have a quick look and let me know if you have an idea about how to pass
the pointers down to blink?

https://codereview.chromium.org/2479123003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebWidget.h (right):

https://codereview.chromium.org/2479123003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebWidget.h:122: const std::vector<const
WebInputEvent*>& =
This was a very confusing part for me. I wasn't sure how I should have passed
these objects down to WebWidget. I can see inside blink we would like to use
Members to keep track of pointers and stuff. But then that structure is not
visible outside. On the other hand, we have ui::ScopedWebInputEvent which is
outside of Blink. So I was not sure what the best way of passing this would be.
Do you have any suggestions?

[Navid Zolghadr, 2016-11-07 20:13:33]

nzolghadr@chromium.org changed reviewers:
+ mustaq@chromium.org

[Navid Zolghadr, 2016-11-07 20:13:34]

Mustaq was missed the first time I sent this.

Can you have a quick look and let me know if you have an idea about how to pass
the pointers down to blink?

[Navid Zolghadr, 2016-11-16 17:12:46]

Description was changed from

==========
Add getCoalescedEvents API

This CL passes vector of coalesced events down
the path of input events.

BUG=656781
==========

to

==========
Add getCoalescedEvents API using vector of WebInputEvents

This CL passes vector of coalesced events down
the path of input events.

BUG=656781
==========

[Navid Zolghadr, 2016-11-16 17:12:46]

Description was changed from

==========
Add getCoalescedEvents API

This CL passes vector of coalesced events down
the path of input events.

BUG=656781
==========

to

==========
Add getCoalescedEvents API using vector of WebInputEvents

This CL passes vector of coalesced events down
the path of input events.

BUG=656781
==========

[Navid Zolghadr, 2016-11-16 17:17:38]

https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS#new...
content/common/DEPS:12:
"+third_party/WebKit/public/platform/CoalescedWebInputEvent.h",
Dave, is this okay to include such a object like CoalescedWebInputEvent here? It
doesn't seem to be inline with the comment above. If not I won't be able to
modify the ScopedWebInputEventWithLatencyInfo to contain a coalesced event and I
should find another place to keep that structure.

[dtapuska, 2016-11-21 18:21:18]

from a quick high level view this seems reasonable.

But we probably want a platform owner to take a look at this to make sure they
are ok with it too before you put a lot of effort into it.

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/CoalescedWebInputEvent.cpp (right):

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/CoalescedWebInputEvent.cpp:41: if
(blink::WebInputEvent::isGestureEventType(event.type)) {
What about keyboard?

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/CoalescedWebInputEvent.h (right):

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/CoalescedWebInputEvent.h:2: * Copyright (C)
2009 Google Inc. All rights reserved.
wrong copy right use the simplified one for chromium.

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/CoalescedWebInputEvent.h:57: WebInputEvent*
eventPointer();
should we have a const ptr version as well? How often do we pass the raw
WebInputEvent ptr around. Isn't it mostly const?

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/CoalescedWebInputEvent.h:60: size_t
coalescedEventSize();
const?

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/CoalescedWebInputEvent.h:62: const
std::vector<WebInputEvent*>& getCoalescedEventsPointers();
is this const function?

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/CoalescedWebInputEvent.h:65:
ScopedWebInputEvent mEvent;
These are a weird format for member definition.

[Navid Zolghadr, 2016-11-21 18:26:19]

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

[Navid Zolghadr, 2016-11-21 18:26:20]

rbyers@ can you have a look at

content/common/DEPS

and see whether that is okay to dependent on a class like CoalescedWebInputEvent
which is certainly more complicated than POD s like WebInputEvents?

[Rick Byers, 2016-11-21 19:11:57]

On 2016/11/21 18:26:20, Navid Zolghadr wrote:
> rbyers@ can you have a look at
> 
> content/common/DEPS
> 
> and see whether that is okay to dependent on a class like
CoalescedWebInputEvent
> which is certainly more complicated than POD s like WebInputEvents?

Conceptually having content depend on a blink data structure that represents a
set of WebInputEvents should be fine.  The problem (and AFAIK the reason for the
comment in DEPS) is that IPC objects need to support memcpy, and so non-POD
classes like this will likely not work correctly with our IPC system. Mojo is
changing this of course, but I'm not up-to-date on the current status.  Maybe
chat with someone on the blink architecture team (like jbroman) about the best
immediate-term design here?

[Navid Zolghadr, 2016-11-21 19:16:01]

On 2016/11/21 19:11:57, Rick Byers wrote:
> On 2016/11/21 18:26:20, Navid Zolghadr wrote:
> > rbyers@ can you have a look at
> > 
> > content/common/DEPS
> > 
> > and see whether that is okay to dependent on a class like
> CoalescedWebInputEvent
> > which is certainly more complicated than POD s like WebInputEvents?
> 
> Conceptually having content depend on a blink data structure that represents a
> set of WebInputEvents should be fine.  The problem (and AFAIK the reason for
the
> comment in DEPS) is that IPC objects need to support memcpy, and so non-POD
> classes like this will likely not work correctly with our IPC system. Mojo is
> changing this of course, but I'm not up-to-date on the current status.  Maybe
> chat with someone on the blink architecture team (like jbroman) about the best
> immediate-term design here?

I'm actually not sending this type over IPC. I'm sending the vector of
WebInputEvents and construct this object on the other side. So basically the IPC
remains to work with WebInputEvent as it is in 

content/common/input_messages.h

There was some discussion about this over in this design doc as well:
https://docs.google.com/document/d/1x-e8fH3I0DBrmrNufPQwRJHMYnW5q9CYZVEHe7Bsf...

[Navid Zolghadr, 2016-11-21 19:34:25]

nzolghadr@chromium.org changed reviewers:
+ jbroman@chromium.org

[Navid Zolghadr, 2016-11-21 19:34:26]

Jeremy, I wanted to ask your opinion about the changes in
content/common/DEPS

and the fact that CoalescedWebInputEvent is not a POD like WebInputEvent. Note
that we are not sending CoalescedWebInputEvent as is over IPC and still send a
vector of WebInputEvents in
 content/common/input_messages.h

Here is a design doc discussing about this solution.
https://docs.google.com/document/d/1x-e8fH3I0DBrmrNufPQwRJHMYnW5q9CYZVEHe7Bsf...

[Rick Byers, 2016-11-22 00:09:23]

On 2016/11/21 19:16:01, Navid Zolghadr wrote:
> On 2016/11/21 19:11:57, Rick Byers wrote:
> > On 2016/11/21 18:26:20, Navid Zolghadr wrote:
> > > rbyers@ can you have a look at
> > > 
> > > content/common/DEPS
> > > 
> > > and see whether that is okay to dependent on a class like
> > CoalescedWebInputEvent
> > > which is certainly more complicated than POD s like WebInputEvents?
> > 
> > Conceptually having content depend on a blink data structure that represents
a
> > set of WebInputEvents should be fine.  The problem (and AFAIK the reason for
> the
> > comment in DEPS) is that IPC objects need to support memcpy, and so non-POD
> > classes like this will likely not work correctly with our IPC system. Mojo
is
> > changing this of course, but I'm not up-to-date on the current status. 
Maybe
> > chat with someone on the blink architecture team (like jbroman) about the
best
> > immediate-term design here?
> 
> I'm actually not sending this type over IPC. I'm sending the vector of
> WebInputEvents and construct this object on the other side. So basically the
IPC
> remains to work with WebInputEvent as it is in 
> 
> content/common/input_messages.h
> 
> There was some discussion about this over in this design doc as well:
>
https://docs.google.com/document/d/1x-e8fH3I0DBrmrNufPQwRJHMYnW5q9CYZVEHe7Bsf...

Ah, interesting - thanks.  In cases like this in the past we'd typically end up
with two different definitions - one using chromium types and one using blink
types, with the Web* layer doing the conversion back and forth.  I think, for
example, that we don't yet want to start using std::vector in blink (since we're
already using WTF::Vector all over the place).  Maybe there's some opportunity
still to share some code though?

[Navid Zolghadr, 2016-11-22 01:03:08]

On 2016/11/22 00:09:23, Rick Byers wrote:
> On 2016/11/21 19:16:01, Navid Zolghadr wrote:
> > On 2016/11/21 19:11:57, Rick Byers wrote:
> > > On 2016/11/21 18:26:20, Navid Zolghadr wrote:
> > > > rbyers@ can you have a look at
> > > > 
> > > > content/common/DEPS
> > > > 
> > > > and see whether that is okay to dependent on a class like
> > > CoalescedWebInputEvent
> > > > which is certainly more complicated than POD s like WebInputEvents?
> > > 
> > > Conceptually having content depend on a blink data structure that
represents
> a
> > > set of WebInputEvents should be fine.  The problem (and AFAIK the reason
for
> > the
> > > comment in DEPS) is that IPC objects need to support memcpy, and so
non-POD
> > > classes like this will likely not work correctly with our IPC system. Mojo
> is
> > > changing this of course, but I'm not up-to-date on the current status. 
> Maybe
> > > chat with someone on the blink architecture team (like jbroman) about the
> best
> > > immediate-term design here?
> > 
> > I'm actually not sending this type over IPC. I'm sending the vector of
> > WebInputEvents and construct this object on the other side. So basically the
> IPC
> > remains to work with WebInputEvent as it is in 
> > 
> > content/common/input_messages.h
> > 
> > There was some discussion about this over in this design doc as well:
> >
>
https://docs.google.com/document/d/1x-e8fH3I0DBrmrNufPQwRJHMYnW5q9CYZVEHe7Bsf...
> 
> Ah, interesting - thanks.  In cases like this in the past we'd typically end
up
> with two different definitions - one using chromium types and one using blink
> types, with the Web* layer doing the conversion back and forth.  I think, for
> example, that we don't yet want to start using std::vector in blink (since
we're
> already using WTF::Vector all over the place).  Maybe there's some opportunity
> still to share some code though?

Note that the std::vector is up until PageWidgetDelegate and at that point is
changed to a WTF::Vector. Would that be acceptable? Otherwise I'm not sure how I
can share the same structure there and The only way I can imagine is to copy
this structure as you mentioned.

[jbroman, 2016-11-22 15:27:37]

https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS#new...
content/common/DEPS:12:
"+third_party/WebKit/public/platform/CoalescedWebInputEvent.h",
On 2016/11/16 at 17:17:38, Navid Zolghadr wrote:
> Dave, is this okay to include such a object like CoalescedWebInputEvent here?
It doesn't seem to be inline with the comment above. If not I won't be able to
modify the ScopedWebInputEventWithLatencyInfo to contain a coalesced event and I
should find another place to keep that structure.

Is there a lot of value to doing this, as opposed to just carrying around
std::vector until we call into Blink? Yes it's another copy, but I think we'd
rather move toward having _less_ Blink code running in the browser process, not
more. (It's tricky, because using certain Blink classes requires Blink to be
initialized, which it is not in the browser process.)

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/CoalescedWebInputEvent.cpp (right):

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/CoalescedWebInputEvent.cpp:1: /*
Blink-side implementations of classes in public/platform/ should be in
Source/platform/exported/; implementations of classes in public/web/ should be
in Source/web/.

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/CoalescedWebInputEvent.cpp:81: NOTREACHED();
This looks like it _is_ reached; there's no early return that would prevent it.

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/CoalescedWebInputEvent.h (right):

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/CoalescedWebInputEvent.h:42: class
CoalescedWebInputEvent {
nit: I'd actually prefer the words in the other order (WebCoalescedInputEvent),
so that "Web" remains a prefix of classes in public/ (there are a couple
exceptions); not sure how public/OWNERS feel.

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/CoalescedWebInputEvent.h:46: void
operator()(blink::WebInputEvent* web_event) const;
Hmm. I thought WebInputEvent (and its derived classes) was specifically designed
to be trivially copyable/destructible, in which case this deleter doesn't
actually wind up doing anything interesting. Is that so, or not?

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/CoalescedWebInputEvent.h:66:
std::vector<ScopedWebInputEvent> mCoalescedEvents;
Blink style: m_event, m_coalescedEvents

std::vector is still discouraged in public/. Prefer WebVector.

[Navid Zolghadr, 2016-11-22 15:38:04]

https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS#new...
content/common/DEPS:12:
"+third_party/WebKit/public/platform/CoalescedWebInputEvent.h",
On 2016/11/22 15:27:37, jbroman wrote:
> On 2016/11/16 at 17:17:38, Navid Zolghadr wrote:
> > Dave, is this okay to include such a object like CoalescedWebInputEvent
here?
> It doesn't seem to be inline with the comment above. If not I won't be able to
> modify the ScopedWebInputEventWithLatencyInfo to contain a coalesced event and
I
> should find another place to keep that structure.
> 
> Is there a lot of value to doing this, as opposed to just carrying around
> std::vector until we call into Blink? Yes it's another copy, but I think we'd
> rather move toward having _less_ Blink code running in the browser process,
not
> more. (It's tricky, because using certain Blink classes requires Blink to be
> initialized, which it is not in the browser process.)

So you are suggesting something like what I had in the patchset 1 right?

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/CoalescedWebInputEvent.h (right):

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/CoalescedWebInputEvent.h:46: void
operator()(blink::WebInputEvent* web_event) const;
On 2016/11/22 15:27:37, jbroman wrote:
> Hmm. I thought WebInputEvent (and its derived classes) was specifically
designed
> to be trivially copyable/destructible, in which case this deleter doesn't
> actually wind up doing anything interesting. Is that so, or not?

I believe their destructor is not virtual intentionally. So they default
destructor would not delete the child object property if the pointer is
referring to the parent class. So we had to implement a delete operator. This
very same logic also exists in ui/events/blink/web_input_event_traits.cc which
if we agree that I can move it here and use it outside of Blink I'm going to
remove.

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/CoalescedWebInputEvent.h:66:
std::vector<ScopedWebInputEvent> mCoalescedEvents;
On 2016/11/22 15:27:37, jbroman wrote:
> Blink style: m_event, m_coalescedEvents
> 
> std::vector is still discouraged in public/. Prefer WebVector.

Alright. I didn't know about WebVector. So can I use WebVector all the way in
the Blink as well or should I convert it to Vector as soon as possible?
Going back to your other point if we were to carry over a vector around instead
of this new type I believe two things will changed (similar to patchset 1).
First is that the deletion logic doesn't need to be in Blink and can live were
they are right now in ui/events/blink/web_input_event_traits.cc. Also I need to
change all those parameters in all the function along the way to include
WebVector. Is that okay?

[jbroman, 2016-11-22 16:02:39]

https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS#new...
content/common/DEPS:12:
"+third_party/WebKit/public/platform/CoalescedWebInputEvent.h",
On 2016/11/22 at 15:38:04, Navid Zolghadr wrote:
> On 2016/11/22 15:27:37, jbroman wrote:
> > On 2016/11/16 at 17:17:38, Navid Zolghadr wrote:
> > > Dave, is this okay to include such a object like CoalescedWebInputEvent
here?
> > It doesn't seem to be inline with the comment above. If not I won't be able
to
> > modify the ScopedWebInputEventWithLatencyInfo to contain a coalesced event
and I
> > should find another place to keep that structure.
> > 
> > Is there a lot of value to doing this, as opposed to just carrying around
> > std::vector until we call into Blink? Yes it's another copy, but I think
we'd
> > rather move toward having _less_ Blink code running in the browser process,
not
> > more. (It's tricky, because using certain Blink classes requires Blink to be
> > initialized, which it is not in the browser process.)
> 
> So you are suggesting something like what I had in the patchset 1 right?

Discussed offline. It'd be nice not to expand this list at all, but something
that was at least all in the header (and doesn't require linking any non-inline
Blink code) would probably be okay. Do check with esprehn@ and content/OWNERS,
though, because this is likely to be controversial.

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/CoalescedWebInputEvent.h (right):

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/CoalescedWebInputEvent.h:46: void
operator()(blink::WebInputEvent* web_event) const;
On 2016/11/22 at 15:38:04, Navid Zolghadr wrote:
> On 2016/11/22 15:27:37, jbroman wrote:
> > Hmm. I thought WebInputEvent (and its derived classes) was specifically
designed
> > to be trivially copyable/destructible, in which case this deleter doesn't
> > actually wind up doing anything interesting. Is that so, or not?
> 
> I believe their destructor is not virtual intentionally. So they default
destructor would not delete the child object property if the pointer is
referring to the parent class. So we had to implement a delete operator. This
very same logic also exists in ui/events/blink/web_input_event_traits.cc which
if we agree that I can move it here and use it outside of Blink I'm going to
remove.

OK. Strictly speaking this is the most correct thing to do, though I think we
were already relying elsewhere on it being legal to destroy these objects
without calling their destructor. (I'm not familiar enough with how
WebInputEvent is used to be confident of that, though.)

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/CoalescedWebInputEvent.h:66:
std::vector<ScopedWebInputEvent> mCoalescedEvents;
On 2016/11/22 at 15:38:04, Navid Zolghadr wrote:
> On 2016/11/22 15:27:37, jbroman wrote:
> > Blink style: m_event, m_coalescedEvents
> > 
> > std::vector is still discouraged in public/. Prefer WebVector.
> 
> Alright. I didn't know about WebVector. So can I use WebVector all the way in
the Blink as well or should I convert it to Vector as soon as possible?
> Going back to your other point if we were to carry over a vector around
instead of this new type I believe two things will changed (similar to patchset
1). First is that the deletion logic doesn't need to be in Blink and can live
were they are right now in ui/events/blink/web_input_event_traits.cc. Also I
need to change all those parameters in all the function along the way to include
WebVector. Is that okay?

WebVector is legal throughout Blink if you're dealing with an object coming from
the Blink API. WTF::Vector is preferred inside Blink's core generally.

[Navid Zolghadr, 2016-11-22 16:59:53]

nzolghadr@chromium.org changed reviewers:
+ esprehn@chromium.org, jochen@chromium.org

[Navid Zolghadr, 2016-11-22 16:59:54]

https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS#new...
content/common/DEPS:12:
"+third_party/WebKit/public/platform/CoalescedWebInputEvent.h",
On 2016/11/22 16:02:39, jbroman wrote:
> On 2016/11/22 at 15:38:04, Navid Zolghadr wrote:
> > On 2016/11/22 15:27:37, jbroman wrote:
> > > On 2016/11/16 at 17:17:38, Navid Zolghadr wrote:
> > > > Dave, is this okay to include such a object like CoalescedWebInputEvent
> here?
> > > It doesn't seem to be inline with the comment above. If not I won't be
able
> to
> > > modify the ScopedWebInputEventWithLatencyInfo to contain a coalesced event
> and I
> > > should find another place to keep that structure.
> > > 
> > > Is there a lot of value to doing this, as opposed to just carrying around
> > > std::vector until we call into Blink? Yes it's another copy, but I think
> we'd
> > > rather move toward having _less_ Blink code running in the browser
process,
> not
> > > more. (It's tricky, because using certain Blink classes requires Blink to
be
> > > initialized, which it is not in the browser process.)
> > 
> > So you are suggesting something like what I had in the patchset 1 right?
> 
> Discussed offline. It'd be nice not to expand this list at all, but something
> that was at least all in the header (and doesn't require linking any
non-inline
> Blink code) would probably be okay. Do check with esprehn@ and content/OWNERS,
> though, because this is likely to be controversial.

esprehn@, jochen@ what do you think about this?
We need a class here (in this patched it is called CoalescedWebInputEvent which
represents both the WebInputEvent and all its coalesced events. Not that the
WebInputEvent is a polymorphic class and we seem to have a custom logic for
deletion instead of using virtual destructors. Here is some context in the
design doc:
https://docs.google.com/a/chromium.org/document/d/1x-e8fH3I0DBrmrNufPQwRJHMYn...

So is it okay to have the code outside of Blink to depend on such a class? If no
what if we move the code in the CoalescedWebInputEvent.cpp file (which is mainly
a container logic) to the CoalescedWebInputEvent.h?

[jochen (gone - plz use gerrit), 2016-11-23 15:09:43]

deferring to esprehn@

[Navid Zolghadr, 2016-11-28 15:36:30]

On 2016/11/23 15:09:43, jochen wrote:
> deferring to esprehn@

esprehn@ can you have a look at let us know what you think?

[Navid Zolghadr, 2016-12-06 03:21:26]

On 2016/11/28 15:36:30, Navid Zolghadr wrote:
> On 2016/11/23 15:09:43, jochen wrote:
> > deferring to esprehn@
> 
> esprehn@ can you have a look at let us know what you think?

ping

[dglazkov, 2016-12-06 03:46:46]

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

[dglazkov, 2016-12-06 03:46:48]

How can this patch be smaller? It looks like the entire feature is implemented
in the patch -- and it's hard to make sense of what's just moving things around
and what's a genuine change.

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/CoalescedWebInputEvent.h (right):

https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/CoalescedWebInputEvent.h:31: #ifndef
CoalescedWebInputEvent_h
Nit: WebCoalescedInputEvent <-- "Web" is the prefix, signifying this is part of
the Web* API.

[Navid Zolghadr, 2016-12-06 04:21:59]

On 2016/12/06 03:46:48, dglazkov wrote:
> How can this patch be smaller? It looks like the entire feature is implemented
> in the patch -- and it's hard to make sense of what's just moving things
around
> and what's a genuine change.
> 
>
https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
> File third_party/WebKit/public/platform/CoalescedWebInputEvent.h (right):
> 
>
https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
> third_party/WebKit/public/platform/CoalescedWebInputEvent.h:31: #ifndef
> CoalescedWebInputEvent_h
> Nit: WebCoalescedInputEvent <-- "Web" is the prefix, signifying this is part
of
> the Web* API.

This whole change is just a prototype of things that are needed to be changed. I
just did it to give others an impression of the overall change. But as you said
it is hard to see the every single file as most of it is just plumbing.
I have already splitted parts of this change that was not controversial and
landed already like https://crrev.com/2510133002.

To focus more on the problem here is a little more context:
The one part that is controversial is about the CoalescedWebInputEvent (I'll
change it to WebCoalescedInputEvent later as you suggested) and whether it is
okay to expose such a class with those functions and memory managements outside
of the Blink or not. Basically the only files you might need to look will be the
CoalescedWebInputEvent.cpp and .h and the fact that it is used by content/common
and a few other places beyond blink and adding the dependencies. Basically the
comment in "content/common/DEPS" says only POD files are allowed to be included.
The question is that is it okay to expose a class that is a little more than
just a POD (like what WebInputEvent was) defined inside Blink to other
components like this or is it better to just duplicate (to some extent) the
definition of the class one for inside the Blink and the other defined outside.
Jeremy was suggesting it might be okay to have it as it but he also mentioned
Elliott may have a stronger opinions about this. Let me know if you would prefer
a small meeting and we can talk about it over GVC or if there is any other
questions I can address here. 

Here is the link to the design doc in case it is lost in all these comments:
https://docs.google.com/document/d/1x-e8fH3I0DBrmrNufPQwRJHMYnW5q9CYZVEHe7Bsf...

[dglazkov, 2016-12-06 04:57:28]

On 2016/12/06 at 04:21:59, nzolghadr wrote:
> On 2016/12/06 03:46:48, dglazkov wrote:
> > How can this patch be smaller? It looks like the entire feature is
implemented
> > in the patch -- and it's hard to make sense of what's just moving things
around
> > and what's a genuine change.
> > 
> >
https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
> > File third_party/WebKit/public/platform/CoalescedWebInputEvent.h (right):
> > 
> >
https://codereview.chromium.org/2479123003/diff/20001/third_party/WebKit/publ...
> > third_party/WebKit/public/platform/CoalescedWebInputEvent.h:31: #ifndef
> > CoalescedWebInputEvent_h
> > Nit: WebCoalescedInputEvent <-- "Web" is the prefix, signifying this is part
of
> > the Web* API.
> 
> This whole change is just a prototype of things that are needed to be changed.
I just did it to give others an impression of the overall change. But as you
said it is hard to see the every single file as most of it is just plumbing.

Oh, got it. Apologies for drive-by review. Will look at the problem you
described tomorrow.

[dglazkov, 2016-12-06 23:05:40]

On 2016/11/22 at 16:02:39, jbroman wrote:
> https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS
> File content/common/DEPS (right):
> 
>
https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS#new...
> content/common/DEPS:12:
"+third_party/WebKit/public/platform/CoalescedWebInputEvent.h",
> On 2016/11/22 at 15:38:04, Navid Zolghadr wrote:
> > On 2016/11/22 15:27:37, jbroman wrote:
> > > On 2016/11/16 at 17:17:38, Navid Zolghadr wrote:
> > > > Dave, is this okay to include such a object like CoalescedWebInputEvent
here?
> > > It doesn't seem to be inline with the comment above. If not I won't be
able to
> > > modify the ScopedWebInputEventWithLatencyInfo to contain a coalesced event
and I
> > > should find another place to keep that structure.
> > > 
> > > Is there a lot of value to doing this, as opposed to just carrying around
> > > std::vector until we call into Blink? Yes it's another copy, but I think
we'd
> > > rather move toward having _less_ Blink code running in the browser
process, not
> > > more. (It's tricky, because using certain Blink classes requires Blink to
be
> > > initialized, which it is not in the browser process.)
> > 
> > So you are suggesting something like what I had in the patchset 1 right?
> 
> Discussed offline. It'd be nice not to expand this list at all, but something
that was at least all in the header (and doesn't require linking any non-inline
Blink code) would probably be okay. Do check with esprehn@ and content/OWNERS,
though, because this is likely to be controversial.

Could we go with a combo? WebInputEvent + std::vector<WebInputEvent> outside of
content/renderer, and then immediately converted into WebCoalescedInputEvent
when in content/renderer?

[esprehn, 2016-12-07 00:18:54]

From my side this patch is so massive, I think we need to split it into the web
API and the plumbing or something?

[Navid Zolghadr, 2016-12-07 00:46:45]

On 2016/12/06 23:05:40, dglazkov wrote:
> On 2016/11/22 at 16:02:39, jbroman wrote:
> > https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS
> > File content/common/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/2479123003/diff/20001/content/common/DEPS#new...
> > content/common/DEPS:12:
> "+third_party/WebKit/public/platform/CoalescedWebInputEvent.h",
> > On 2016/11/22 at 15:38:04, Navid Zolghadr wrote:
> > > On 2016/11/22 15:27:37, jbroman wrote:
> > > > On 2016/11/16 at 17:17:38, Navid Zolghadr wrote:
> > > > > Dave, is this okay to include such a object like
CoalescedWebInputEvent
> here?
> > > > It doesn't seem to be inline with the comment above. If not I won't be
> able to
> > > > modify the ScopedWebInputEventWithLatencyInfo to contain a coalesced
event
> and I
> > > > should find another place to keep that structure.
> > > > 
> > > > Is there a lot of value to doing this, as opposed to just carrying
around
> > > > std::vector until we call into Blink? Yes it's another copy, but I think
> we'd
> > > > rather move toward having _less_ Blink code running in the browser
> process, not
> > > > more. (It's tricky, because using certain Blink classes requires Blink
to
> be
> > > > initialized, which it is not in the browser process.)
> > > 
> > > So you are suggesting something like what I had in the patchset 1 right?
> > 
> > Discussed offline. It'd be nice not to expand this list at all, but
something
> that was at least all in the header (and doesn't require linking any
non-inline
> Blink code) would probably be okay. Do check with esprehn@ and content/OWNERS,
> though, because this is likely to be controversial.
> 
> Could we go with a combo? WebInputEvent + std::vector<WebInputEvent> outside
of
> content/renderer, and then immediately converted into WebCoalescedInputEvent
> when in content/renderer?

This is what kind of the first patch in this CL looked like. I'm fine with this
if no one else has any problem. There is a little complication with this though.
WebInputEvent is a polymorphic class. So I need to keep an
std::vector<std::unique_ptr<WebInputEvent> > and pass a const & of it around.
So I need

[jochen (gone - plz use gerrit), 2016-12-07 15:34:55]

jochen@chromium.org changed reviewers:
- jochen@chromium.org