Overhaul Mandoline event transport code

Overhaul Mandoline event transport code

In the course of its development, Mandoline's event code has diverged away
from event payloads that are a good fit with the current state of blink's
input handling logic. This CL begins a process of updating the event code
with an extended wheel interface capable of supporting the standards track
wheel event and revised pointer events that will be extended in a future
CL with support for the pointer events specification.

BUG=524828
Committed: https://crrev.com/895c6c35457b7fe13da31e862cd50eb537d5e7e6
Cr-Commit-Position: refs/heads/master@{#349015}

[rjkroege, 2015-09-08 20:28:32]

rjkroege@chromium.org changed reviewers:
+ sadrul@chromium.org

[rjkroege, 2015-09-08 20:28:33]

PTAL.

[sadrul, 2015-09-09 04:19:04]

sadrul@chromium.org changed reviewers:
+ sky@chromium.org

[sadrul, 2015-09-09 04:19:05]

+sky@

https://codereview.chromium.org/1313353010/diff/1/components/html_viewer/blin...
File components/html_viewer/blink_input_events_type_converters.cc (right):

https://codereview.chromium.org/1313353010/diff/1/components/html_viewer/blin...
components/html_viewer/blink_input_events_type_converters.cc:152: //
TODO(rjkroege): Add wheel event support when blink does.
not sure what the TODO here means ... blink already supports wheel events, no?

https://codereview.chromium.org/1313353010/diff/1/components/html_viewer/touc...
File components/html_viewer/touch_handler.cc (right):

https://codereview.chromium.org/1313353010/diff/1/components/html_viewer/touc...
components/html_viewer/touch_handler.cc:18: // TODO(rjkroege): Understand why we
need this code in html_viewer.
GR currently happens in html_viewer. So it needs to be able to create the
MotionEvents for it.

https://codereview.chromium.org/1313353010/diff/1/components/pdf_viewer/pdf_v...
File components/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1313353010/diff/1/components/pdf_viewer/pdf_v...
components/pdf_viewer/pdf_viewer.cc:380: event->pointer_data->kind ==
mojo::POINTER_KIND_WHEEL &&
It should be possible to look at event->action, instead of pointer_data->kind to
decide if this is a wheel event I think.

Unrelated to this CL: this looks ... curious. Do we switch pages in a pdf for
each mouse-wheel event?

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
File mojo/converters/input_events/input_events_type_converters.cc (right):

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
mojo/converters/input_events/input_events_type_converters.cc:323: } break;
The indent above looks broken. 'git cl format'?

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
File mojo/converters/input_events/tests/DEPS (right):

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
mojo/converters/input_events/tests/DEPS:2: "+components/html_viewer",
:(

Should the blink converters move into //mojo/converters/input_events/ instead
(or maybe //mojo/converters/input_events/blink/)? Maybe follow-up patch?

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
File mojo/converters/input_events/tests/input_events_unittest.cc (right):

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
mojo/converters/input_events/tests/input_events_unittest.cc:27: mojo::EventPtr
mojo_event(mojo::Event::From(*(ui::Event::Clone(mouseev))));
I don't think there's a reason to use Event::Clone() here? Just:

  EventPtr mojo_event(Event::From(mouseev));

(since already in mojo namespace)

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
mojo/converters/input_events/tests/input_events_unittest.cc:30:
EXPECT_TRUE(mojo_event->pointer_data->kind = POINTER_KIND_MOUSE);
EXPECT_EQ

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
mojo/converters/input_events/tests/input_events_unittest.cc:49:
gfx::Vector2d(-1.0 * 53, -2.0 * 53), gfx::PointF(1.0, 2.0),
Instead of hardcoding 53 everywhere, can we use ui::MouseWheelEvent::kWheelDelta
(which is larger on Windows, which we probably want to use)?

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_event_...
File ui/mojo/events/input_event_constants.mojom (right):

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_event_...
ui/mojo/events/input_event_constants.mojom:48: WHEEL,
Do we need 'wheel' as a separate kind? e.g. both mouse and pen can send wheel
events I think?

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
File ui/mojo/events/input_events.mojom (right):

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
ui/mojo/events/input_events.mojom:77: struct WheelData {
Would it make sense to make this ScrollData? (e.g. we could possibly reuse this
for scrolls from gestures/touchpads?)

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
ui/mojo/events/input_events.mojom:78: WheelMode mode;
As per above, ScrollMode?

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
ui/mojo/events/input_events.mojom:89: struct BrushData {
'Brush' here seems awkward, but I don't have a suggestion for a better name (I
was going to suggest PointerDetails, as we do for ui::Event, but that could be
confusing with PointerData here).

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
ui/mojo/events/input_events.mojom:94: // |pressure| range is [0,1]. It will be
set to 0.5 when the device cannot
The 'unknown' value for pressure in the spec is 0, and 0.5 only when there is an
active point (i.e. button/touch-point down). Clarify the comment here
accordingly?

[rjkroege, 2015-09-09 19:45:38]

PTAL.

https://codereview.chromium.org/1313353010/diff/1/components/html_viewer/blin...
File components/html_viewer/blink_input_events_type_converters.cc (right):

https://codereview.chromium.org/1313353010/diff/1/components/html_viewer/blin...
components/html_viewer/blink_input_events_type_converters.cc:152: //
TODO(rjkroege): Add wheel event support when blink does.
On 2015/09/09 04:19:04, sadrul wrote:
> not sure what the TODO here means ... blink already supports wheel events, no?

Per rbyers: it supports the legacy non-standard wheel events but not the new
standardized wheel event in DOM level3 events.
(http://www.w3.org/TR/DOM-Level-3-Events/#events-wheelevents)

I updated the comment.

https://codereview.chromium.org/1313353010/diff/1/components/html_viewer/touc...
File components/html_viewer/touch_handler.cc (right):

https://codereview.chromium.org/1313353010/diff/1/components/html_viewer/touc...
components/html_viewer/touch_handler.cc:18: // TODO(rjkroege): Understand why we
need this code in html_viewer.
On 2015/09/09 04:19:04, sadrul wrote:
> GR currently happens in html_viewer. So it needs to be able to create the
> MotionEvents for it.

thanks. TODO updated.

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
File mojo/converters/input_events/input_events_type_converters.cc (right):

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
mojo/converters/input_events/input_events_type_converters.cc:323: } break;
On 2015/09/09 04:19:04, sadrul wrote:
> The indent above looks broken. 'git cl format'?

Aside: I did do a 'git cl format'. And I trusted that it had done the right
thing.

I fixed this up by hand.

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
File mojo/converters/input_events/tests/DEPS (right):

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
mojo/converters/input_events/tests/DEPS:2: "+components/html_viewer",
On 2015/09/09 04:19:04, sadrul wrote:
> :(
> 
> Should the blink converters move into //mojo/converters/input_events/ instead
> (or maybe //mojo/converters/input_events/blink/)? Maybe follow-up patch?

So... I wanted to test the full event life-cycle from ui::Event to WebEvent. And
that meant this. Which is definitely unfortunate from a layering viewpoint.

I'm happy to take a TODO to move the blink converters elsewhere.
mojo/converters/input_events/blink seem reasonable to me.

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
File mojo/converters/input_events/tests/input_events_unittest.cc (right):

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
mojo/converters/input_events/tests/input_events_unittest.cc:27: mojo::EventPtr
mojo_event(mojo::Event::From(*(ui::Event::Clone(mouseev))));
On 2015/09/09 04:19:04, sadrul wrote:
> I don't think there's a reason to use Event::Clone() here? Just:
> 
>   EventPtr mojo_event(Event::From(mouseev));
> 
> (since already in mojo namespace)

no. The template specialization in the converters fails because there is no
converter for ui::MouseEvent, only ui::Event. Conceivably, we might want it. I
improved the code.

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
mojo/converters/input_events/tests/input_events_unittest.cc:30:
EXPECT_TRUE(mojo_event->pointer_data->kind = POINTER_KIND_MOUSE);
On 2015/09/09 04:19:04, sadrul wrote:
> EXPECT_EQ

Done.

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
mojo/converters/input_events/tests/input_events_unittest.cc:49:
gfx::Vector2d(-1.0 * 53, -2.0 * 53), gfx::PointF(1.0, 2.0),
On 2015/09/09 04:19:04, sadrul wrote:
> Instead of hardcoding 53 everywhere, can we use
ui::MouseWheelEvent::kWheelDelta
> (which is larger on Windows, which we probably want to use)?

Done.

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_event_...
File ui/mojo/events/input_event_constants.mojom (right):

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_event_...
ui/mojo/events/input_event_constants.mojom:48: WHEEL,
On 2015/09/09 04:19:04, sadrul wrote:
> Do we need 'wheel' as a separate kind? e.g. both mouse and pen can send wheel
> events I think?

Based on http://www.w3.org/TR/pointerevents/#pointerevent-interface and the DOM
level 3 spec, I think that it's reasonable to make them separate. But I may have
(mis)read the wrong specs.

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
File ui/mojo/events/input_events.mojom (right):

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
ui/mojo/events/input_events.mojom:77: struct WheelData {
On 2015/09/09 04:19:05, sadrul wrote:
> Would it make sense to make this ScrollData? (e.g. we could possibly reuse
this
> for scrolls from gestures/touchpads?)

I have this long frustration with the re-use of wheel events to mean scrolling.
I think we should separate scrolling from wheel events because (by spec) Wheel
events are delivered sync to JS and can be PreventDefault()-ed while scrolling
generates (async) scroll notify events that cannot. As a result, I claim that
code will end up more understandable if we keep these two concepts separate.
WDYT?

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
ui/mojo/events/input_events.mojom:78: WheelMode mode;
On 2015/09/09 04:19:05, sadrul wrote:
> As per above, ScrollMode?

see the above. I will update this after updating the name of WheelData as
appropriate

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
ui/mojo/events/input_events.mojom:89: struct BrushData {
On 2015/09/09 04:19:05, sadrul wrote:
> 'Brush' here seems awkward, but I don't have a suggestion for a better name (I
> was going to suggest PointerDetails, as we do for ui::Event, but that could be
> confusing with PointerData here).

Brush: short, analogous with paint programs. Am happy to use a different word
but Pointer is dangerously close to being as over-used as View. :-)

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
ui/mojo/events/input_events.mojom:94: // |pressure| range is [0,1]. It will be
set to 0.5 when the device cannot
On 2015/09/09 04:19:04, sadrul wrote:
> The 'unknown' value for pressure in the spec is 0, and 0.5 only when there is
an
> active point (i.e. button/touch-point down). Clarify the comment here
> accordingly?

Done

[sadrul, 2015-09-10 17:39:15]

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
File mojo/converters/input_events/tests/input_events_unittest.cc (right):

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
mojo/converters/input_events/tests/input_events_unittest.cc:27: mojo::EventPtr
mojo_event(mojo::Event::From(*(ui::Event::Clone(mouseev))));
On 2015/09/09 19:45:38, rjkroege wrote:
> On 2015/09/09 04:19:04, sadrul wrote:
> > I don't think there's a reason to use Event::Clone() here? Just:
> > 
> >   EventPtr mojo_event(Event::From(mouseev));
> > 
> > (since already in mojo namespace)
> 
> no. The template specialization in the converters fails because there is no
> converter for ui::MouseEvent, only ui::Event. Conceivably, we might want it. I
> improved the code.

That's ... weird? Does ui::MouseEvent not auto-downcast to ui::Event?

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_event_...
File ui/mojo/events/input_event_constants.mojom (right):

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_event_...
ui/mojo/events/input_event_constants.mojom:48: WHEEL,
On 2015/09/09 19:45:38, rjkroege wrote:
> On 2015/09/09 04:19:04, sadrul wrote:
> > Do we need 'wheel' as a separate kind? e.g. both mouse and pen can send
wheel
> > events I think?
> 
> Based on http://www.w3.org/TR/pointerevents/#pointerevent-interface and the
DOM
> level 3 spec, I think that it's reasonable to make them separate. But I may
have
> (mis)read the wrong specs.

I am not sure. I feel like the event-type should be sufficient here, and the
PointerKind would be more useful as an indication of what kind of device it
originated from (so it could be used to treat events coming from a normal mouse
differently than events coming from a touchpad). If we use WHEEL for both of
these kinds of events, we would lose that information.

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
File ui/mojo/events/input_events.mojom (right):

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
ui/mojo/events/input_events.mojom:77: struct WheelData {
On 2015/09/09 19:45:38, rjkroege wrote:
> On 2015/09/09 04:19:05, sadrul wrote:
> > Would it make sense to make this ScrollData? (e.g. we could possibly reuse
> this
> > for scrolls from gestures/touchpads?)
> 
> I have this long frustration with the re-use of wheel events to mean
scrolling.
> I think we should separate scrolling from wheel events because (by spec) Wheel
> events are delivered sync to JS and can be PreventDefault()-ed while scrolling
> generates (async) scroll notify events that cannot. As a result, I claim that
> code will end up more understandable if we keep these two concepts separate.
> WDYT?

Re-using wheel events to mean scrolling does sound bad. Although in this case, I
am suggesting having a more explicit Scroll* struct that the various kinds of
events could use. So a WHEEL event can include some Scroll* struct if it is
meant to trigger scrolling, while a gesture-scroll event can also include the
same Scroll* struct, thus we avoid introducing similar struct for essentially
the same purpose. Does that make sense?

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
ui/mojo/events/input_events.mojom:89: struct BrushData {
On 2015/09/09 19:45:38, rjkroege wrote:
> On 2015/09/09 04:19:05, sadrul wrote:
> > 'Brush' here seems awkward, but I don't have a suggestion for a better name
(I
> > was going to suggest PointerDetails, as we do for ui::Event, but that could
be
> > confusing with PointerData here).
> 
> Brush: short, analogous with paint programs. Am happy to use a different word
> but Pointer is dangerously close to being as over-used as View. :-)

Heh, yeah. I am OK with Brush until we can think of a better name

https://codereview.chromium.org/1313353010/diff/20001/components/html_viewer/...
File components/html_viewer/touch_handler.cc (right):

https://codereview.chromium.org/1313353010/diff/20001/components/html_viewer/...
components/html_viewer/touch_handler.cc:39: properties->pressure = 0.0;
We can look at (event.flags | EVENT_FLAGS_LEFT_MOUSE_BUTTON) here to decide
whether to set to 0.5 or not.

[rjkroege, 2015-09-12 01:29:02]

PTAL

https://codereview.chromium.org/1313353010/diff/1/components/pdf_viewer/pdf_v...
File components/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1313353010/diff/1/components/pdf_viewer/pdf_v...
components/pdf_viewer/pdf_viewer.cc:380: event->pointer_data->kind ==
mojo::POINTER_KIND_WHEEL &&
On 2015/09/09 04:19:04, sadrul wrote:
> It should be possible to look at event->action, instead of pointer_data->kind
to
> decide if this is a wheel event I think.

Done.

> 
> Unrelated to this CL: this looks ... curious. Do we switch pages in a pdf for
> each mouse-wheel event?

yes. Fixed.


Resolving this blocked on http://crbug.com/530268

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
File mojo/converters/input_events/tests/input_events_unittest.cc (right):

https://codereview.chromium.org/1313353010/diff/1/mojo/converters/input_event...
mojo/converters/input_events/tests/input_events_unittest.cc:27: mojo::EventPtr
mojo_event(mojo::Event::From(*(ui::Event::Clone(mouseev))));
On 2015/09/10 17:39:15, sadrul wrote:
> On 2015/09/09 19:45:38, rjkroege wrote:
> > On 2015/09/09 04:19:04, sadrul wrote:
> > > I don't think there's a reason to use Event::Clone() here? Just:
> > > 
> > >   EventPtr mojo_event(Event::From(mouseev));
> > > 
> > > (since already in mojo namespace)
> > 
> > no. The template specialization in the converters fails because there is no
> > converter for ui::MouseEvent, only ui::Event. Conceivably, we might want it.
I
> > improved the code.
> 
> That's ... weird? Does ui::MouseEvent not auto-downcast to ui::Event?

Nope. not in a template instantiation. It is possible to add more converters but
I'd rather not do that to (very slightly) simplify the test at the cost of more
template instantiations.

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_event_...
File ui/mojo/events/input_event_constants.mojom (right):

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_event_...
ui/mojo/events/input_event_constants.mojom:48: WHEEL,
On 2015/09/10 17:39:15, sadrul wrote:
> On 2015/09/09 19:45:38, rjkroege wrote:
> > On 2015/09/09 04:19:04, sadrul wrote:
> > > Do we need 'wheel' as a separate kind? e.g. both mouse and pen can send
> wheel
> > > events I think?
> > 
> > Based on http://www.w3.org/TR/pointerevents/#pointerevent-interface and the
> DOM
> > level 3 spec, I think that it's reasonable to make them separate. But I may
> have
> > (mis)read the wrong specs.
> 
> I am not sure. I feel like the event-type should be sufficient here, and the
> PointerKind would be more useful as an indication of what kind of device it
> originated from (so it could be used to treat events coming from a normal
mouse
> differently than events coming from a touchpad). If we use WHEEL for both of
> these kinds of events, we would lose that information.

I agree. In Web spec land, Pointer encompasses touch, pen, mouse. while Wheel is
completely separate.

I made wheel a new kind of top-level event distinct from Pointer.

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
File ui/mojo/events/input_events.mojom (right):

https://codereview.chromium.org/1313353010/diff/1/ui/mojo/events/input_events...
ui/mojo/events/input_events.mojom:77: struct WheelData {
On 2015/09/10 17:39:15, sadrul wrote:
> On 2015/09/09 19:45:38, rjkroege wrote:
> > On 2015/09/09 04:19:05, sadrul wrote:
> > > Would it make sense to make this ScrollData? (e.g. we could possibly reuse
> > this
> > > for scrolls from gestures/touchpads?)
> > 
> > I have this long frustration with the re-use of wheel events to mean
> scrolling.
> > I think we should separate scrolling from wheel events because (by spec)
Wheel
> > events are delivered sync to JS and can be PreventDefault()-ed while
scrolling
> > generates (async) scroll notify events that cannot. As a result, I claim
that
> > code will end up more understandable if we keep these two concepts separate.
> > WDYT?
> 
> Re-using wheel events to mean scrolling does sound bad. Although in this case,
I
> am suggesting having a more explicit Scroll* struct that the various kinds of
> events could use. So a WHEEL event can include some Scroll* struct if it is
> meant to trigger scrolling, while a gesture-scroll event can also include the
> same Scroll* struct, thus we avoid introducing similar struct for essentially
> the same purpose. Does that make sense?

At some level yes. But we don't know if a wheel is meant to trigger scrolling.
If we knew, MUS would have scrolled and not need to send the event at all. A
wheel event is sent to the app iff it might not mean scrolling.

I want to deal with gesture event payloads in a future CL.

https://codereview.chromium.org/1313353010/diff/20001/components/html_viewer/...
File components/html_viewer/touch_handler.cc (right):

https://codereview.chromium.org/1313353010/diff/20001/components/html_viewer/...
components/html_viewer/touch_handler.cc:39: properties->pressure = 0.0;
On 2015/09/10 17:39:15, sadrul wrote:
> We can look at (event.flags | EVENT_FLAGS_LEFT_MOUSE_BUTTON) here to decide
> whether to set to 0.5 or not.

Done.

[sadrul, 2015-09-14 18:15:48]

lgtm

[rjkroege, 2015-09-14 18:30:06]

On 2015/09/14 18:15:48, sadrul wrote:
> lgtm

sky@: please take a look?

[sky, 2015-09-14 19:27:26]

https://codereview.chromium.org/1313353010/diff/40001/mojo/converters/input_e...
File mojo/converters/input_events/tests/DEPS (right):

https://codereview.chromium.org/1313353010/diff/40001/mojo/converters/input_e...
mojo/converters/input_events/tests/DEPS:2: "+components/html_viewer",
mojo should not depend upon components/html_viewer. Maybe move the test there?

https://codereview.chromium.org/1313353010/diff/40001/mojo/converters/input_e...
File mojo/converters/input_events/tests/input_events_unittest.cc (right):

https://codereview.chromium.org/1313353010/diff/40001/mojo/converters/input_e...
mojo/converters/input_events/tests/input_events_unittest.cc:35:
EXPECT_TRUE(webevent);
ASSERT (if this is false, next line crashes, which you want to avoid).

https://codereview.chromium.org/1313353010/diff/40001/mojo/converters/input_e...
mojo/converters/input_events/tests/input_events_unittest.cc:60:
EXPECT_TRUE(webevent);
ASSERT

https://codereview.chromium.org/1313353010/diff/40001/ui/mojo/events/input_ev...
File ui/mojo/events/input_event_constants.mojom (right):

https://codereview.chromium.org/1313353010/diff/40001/ui/mojo/events/input_ev...
ui/mojo/events/input_event_constants.mojom:45: UNKNOWN,
When would the pointer kind be unknown? You should document this here as others
are bound to ask too.

https://codereview.chromium.org/1313353010/diff/40001/ui/mojo/events/input_ev...
File ui/mojo/events/input_events.mojom (right):

https://codereview.chromium.org/1313353010/diff/40001/ui/mojo/events/input_ev...
ui/mojo/events/input_events.mojom:68: LocationData where;
'where' is a weird name for this (same with line 80 below). How about location?

[rjkroege, 2015-09-14 21:07:38]

PTAL

https://codereview.chromium.org/1313353010/diff/40001/mojo/converters/input_e...
File mojo/converters/input_events/tests/DEPS (right):

https://codereview.chromium.org/1313353010/diff/40001/mojo/converters/input_e...
mojo/converters/input_events/tests/DEPS:2: "+components/html_viewer",
On 2015/09/14 19:27:25, sky wrote:
> mojo should not depend upon components/html_viewer. Maybe move the test there?

Done.

https://codereview.chromium.org/1313353010/diff/40001/mojo/converters/input_e...
File mojo/converters/input_events/tests/input_events_unittest.cc (right):

https://codereview.chromium.org/1313353010/diff/40001/mojo/converters/input_e...
mojo/converters/input_events/tests/input_events_unittest.cc:35:
EXPECT_TRUE(webevent);
On 2015/09/14 19:27:25, sky wrote:
> ASSERT (if this is false, next line crashes, which you want to avoid).

True. Done.

https://codereview.chromium.org/1313353010/diff/40001/mojo/converters/input_e...
mojo/converters/input_events/tests/input_events_unittest.cc:60:
EXPECT_TRUE(webevent);
On 2015/09/14 19:27:25, sky wrote:
> ASSERT

Done.

https://codereview.chromium.org/1313353010/diff/40001/ui/mojo/events/input_ev...
File ui/mojo/events/input_event_constants.mojom (right):

https://codereview.chromium.org/1313353010/diff/40001/ui/mojo/events/input_ev...
ui/mojo/events/input_event_constants.mojom:45: UNKNOWN,
On 2015/09/14 19:27:25, sky wrote:
> When would the pointer kind be unknown? You should document this here as
others
> are bound to ask too.

I can't think of a good reason why we need this. Removed.

https://codereview.chromium.org/1313353010/diff/40001/ui/mojo/events/input_ev...
File ui/mojo/events/input_events.mojom (right):

https://codereview.chromium.org/1313353010/diff/40001/ui/mojo/events/input_ev...
ui/mojo/events/input_events.mojom:68: LocationData where;
On 2015/09/14 19:27:26, sky wrote:
> 'where' is a weird name for this (same with line 80 below). How about
location?

Done.

[sky, 2015-09-14 21:36:28]

LGTM

[rjkroege, 2015-09-15 00:12:30]

rjkroege@chromium.org changed reviewers:
+ jam@chromium.org

[rjkroege, 2015-09-15 00:12:31]

+jam OWNERS for pdf viewer and whatever made sky's LGTM not count for
view_manager

[jam, 2015-09-15 16:27:44]

On 2015/09/15 00:12:31, rjkroege wrote:
> +jam OWNERS for pdf viewer and whatever made sky's LGTM not count for
> view_manager

lgtm

[rjkroege, 2015-09-15 17:52:45]

The CQ bit was checked by rjkroege@chromium.org

[rjkroege, 2015-09-15 17:52:46]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1313353010/#ps60001
(title: "review comments")

[commit-bot: I haz the power, 2015-09-15 17:53:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313353010/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313353010/60001

[commit-bot: I haz the power, 2015-09-15 17:57:11]

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

[commit-bot: I haz the power, 2015-09-15 17:57:12]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rjkroege, 2015-09-15 21:04:46]

The CQ bit was checked by rjkroege@chromium.org

[commit-bot: I haz the power, 2015-09-15 21:05:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313353010/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313353010/60001

[commit-bot: I haz the power, 2015-09-15 21:07:29]

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

[commit-bot: I haz the power, 2015-09-15 21:07:31]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rjkroege, 2015-09-15 21:31:52]

The CQ bit was checked by rjkroege@chromium.org

[rjkroege, 2015-09-15 21:31:53]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
jam@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1313353010/#ps80001
(title: "rebased")

[commit-bot: I haz the power, 2015-09-15 21:32:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313353010/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313353010/80001

[commit-bot: I haz the power, 2015-09-15 22:10:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-15 22:10:55]

Patchset 5 (id:??) landed as
https://crrev.com/895c6c35457b7fe13da31e862cd50eb537d5e7e6
Cr-Commit-Position: refs/heads/master@{#349015}

[Devlin, 2015-09-15 23:16:24]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1344223002/ by rdevlin.cronin@chromium.org.

The reason for reverting is: Broke GN build:

ERROR at //components/html_viewer/input_events_unittest.cc:10:11: Can't include
this header from here.
#include "ui/events/event.h"
          ^----------------
The target:
  //components/html_viewer:html_viewer_unittests
is including a file from the target:
  //ui/events:events

It's usually best to depend directly on the destination target.
In some cases, the destination target is considered a subcomponent
of an intermediate target. In this case, the intermediate target
should depend publicly on the destination to forward the ability
to include headers.

Dependency chain (there may also be others):
  //components/html_viewer:html_viewer_unittests -->
  //components/html_viewer:lib --[private]-->
  //ui/events:events

https://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32020/....

[rjkroege, 2015-09-17 00:06:54]

The CQ bit was checked by rjkroege@chromium.org

[rjkroege, 2015-09-17 00:06:54]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
jam@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1313353010/#ps100001
(title: "fixed gn check")

[commit-bot: I haz the power, 2015-09-17 00:08:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313353010/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313353010/100001

[commit-bot: I haz the power, 2015-09-17 01:08:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-17 01:08:52]

Patchset 6 (id:??) landed as
https://crrev.com/9c57b5f85a889b3511ad56003aa634f80c0e9f9a
Cr-Commit-Position: refs/heads/master@{#349303}

[msw, 2015-09-18 18:44:53]

It looks like this CL broke html_viewer_unittests
(InputEventLibTest.MouseWheelEventConversionNonPrecise in particular) on our
Chromium Mojo Windows FYI bot. Here's the first breakage and the latest bot run:
http://build.chromium.org/p/chromium.mojo/builders/Chromium%20Mojo%20Windows/...
http://build.chromium.org/p/chromium.mojo/builders/Chromium%20Mojo%20Windows/...

[commit-bot: I haz the power, 2015-09-23 12:50:33]

Patchset 6 (id:??) landed as
https://crrev.com/895c6c35457b7fe13da31e862cd50eb537d5e7e6
Cr-Commit-Position: refs/heads/master@{#349015}