Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1426993008:
Serialize a subset of WebInputEvents to protobufs. (Closed)
|
Created:
5 years, 1 month ago by David Trainor- moved to gerrit Modified:
5 years ago CC:
chromium-reviews, dtrainor+watch-blimp_chromium.org, esprehn, haibinlu, kmarshall+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSerialize a subset of WebInputEvents to protobufs.
- Add (de)serialization methods for WebGestureEvents (a subset of
WebInputEvents).
- Add unit tests to validate that moving to and from these events works
properly.
BUG=548806
Committed: https://crrev.com/6086a92c956e7bd81aa402b2e4dc4ec6a91c8585
Cr-Commit-Position: refs/heads/master@{#362089}
Patch Set 1 #Patch Set 2 : Minor cleanup #Patch Set 3 : Added some comments #Patch Set 4 : Rebased #
Total comments: 16
Patch Set 5 : Addressed comments. #
Total comments: 5
Patch Set 6 : Fixed unittest file #Patch Set 7 : Rebased #
Total comments: 2
Patch Set 8 : Split serialization and deserialization to message generators/processors. #
Total comments: 35
Patch Set 9 : Removed some gesture events, cleaned up a bit #Patch Set 10 : Removed unused type enum #
Total comments: 27
Patch Set 11 : Addressed more comments #
Total comments: 2
Patch Set 12 : Updated comment. #Patch Set 13 : Remove commented out old messages in input.proto #
Total comments: 4
Patch Set 14 : Addressed nits #Patch Set 15 : Re-add fatal log. #
Dependent Patchsets: Messages
Total messages: 58 (13 generated)
dtrainor@chromium.org changed reviewers: + kmarshall@chromium.org, nyquist@chromium.org, pdr@chromium.org, wez@chromium.org
pdr@: Can I get an owners review for including third_party/WebKit/public in our DEPS? nyquist@/wez@/kmarshall@: Can I get a blimp/ review? Thanks! General to/from protobuf for WebInputEvents. Doesn't tie into anything yet, but this should be easy to hook in once we start needing to serialize these. ptal thanks!
https://codereview.chromium.org/1426993008/diff/60001/blimp/common/input/inpu... File blimp/common/input/input_event_conversions.cc (right): https://codereview.chromium.org/1426993008/diff/60001/blimp/common/input/inpu... blimp/common/input/input_event_conversions.cc:17: const InputMessage::Type& type) { No need to take a const ref to a value type https://codereview.chromium.org/1426993008/diff/60001/blimp/common/input/inpu... blimp/common/input/input_event_conversions.cc:53: case InputMessage::Type_Undefined: Redundant with "default" https://codereview.chromium.org/1426993008/diff/60001/blimp/common/input/inpu... blimp/common/input/input_event_conversions.cc:61: const blink::WebInputEvent::Type& type) { ditto on const ref input https://codereview.chromium.org/1426993008/diff/60001/blimp/common/input/inpu... blimp/common/input/input_event_conversions.cc:97: case blink::WebInputEvent::Type::Undefined: unnecessary https://codereview.chromium.org/1426993008/diff/60001/blimp/common/input/inpu... blimp/common/input/input_event_conversions.cc:141: GestureArgs::Tap* args = proto->mutable_tap(); Indented 2 spcs too many https://codereview.chromium.org/1426993008/diff/60001/blimp/common/input/inpu... File blimp/common/input/input_event_conversions_unittest.cc (right): https://codereview.chromium.org/1426993008/diff/60001/blimp/common/input/inpu... blimp/common/input/input_event_conversions_unittest.cc:17: void ValidateWebInputEventSerialization(const blink::WebInputEvent& event) { This does more than just serialization... Maybe you can say that it verifies roundtripping? https://codereview.chromium.org/1426993008/diff/60001/blimp/common/input/inpu... blimp/common/input/input_event_conversions_unittest.cc:72: { I recommend just creating a WebGestureEvent array and use the braced initializer syntax to specify each of the test cases. Then you can just go plow through those cases with a simple loop. E.Z. https://codereview.chromium.org/1426993008/diff/60001/blimp/common/proto/inpu... File blimp/common/proto/input.proto (right): https://codereview.chromium.org/1426993008/diff/60001/blimp/common/proto/inpu... blimp/common/proto/input.proto:23: It would be nice to reference the Blink analogues to these messages
addressed comments. ptal thanks! https://chromiumcodereview.appspot.com/1426993008/diff/60001/blimp/common/inp... File blimp/common/input/input_event_conversions.cc (right): https://chromiumcodereview.appspot.com/1426993008/diff/60001/blimp/common/inp... blimp/common/input/input_event_conversions.cc:17: const InputMessage::Type& type) { On 2015/11/12 00:53:49, Kevin M wrote: > No need to take a const ref to a value type ah good point thanks! habit. https://chromiumcodereview.appspot.com/1426993008/diff/60001/blimp/common/inp... blimp/common/input/input_event_conversions.cc:53: case InputMessage::Type_Undefined: On 2015/11/12 00:53:50, Kevin M wrote: > Redundant with "default" Done. https://chromiumcodereview.appspot.com/1426993008/diff/60001/blimp/common/inp... blimp/common/input/input_event_conversions.cc:61: const blink::WebInputEvent::Type& type) { On 2015/11/12 00:53:50, Kevin M wrote: > ditto on const ref input Done. https://chromiumcodereview.appspot.com/1426993008/diff/60001/blimp/common/inp... blimp/common/input/input_event_conversions.cc:97: case blink::WebInputEvent::Type::Undefined: On 2015/11/12 00:53:49, Kevin M wrote: > unnecessary Done. https://chromiumcodereview.appspot.com/1426993008/diff/60001/blimp/common/inp... blimp/common/input/input_event_conversions.cc:141: GestureArgs::Tap* args = proto->mutable_tap(); On 2015/11/12 00:53:50, Kevin M wrote: > Indented 2 spcs too many Done. https://chromiumcodereview.appspot.com/1426993008/diff/60001/blimp/common/inp... File blimp/common/input/input_event_conversions_unittest.cc (right): https://chromiumcodereview.appspot.com/1426993008/diff/60001/blimp/common/inp... blimp/common/input/input_event_conversions_unittest.cc:17: void ValidateWebInputEventSerialization(const blink::WebInputEvent& event) { On 2015/11/12 00:53:50, Kevin M wrote: > This does more than just serialization... Maybe you can say that it verifies > roundtripping? good idea https://chromiumcodereview.appspot.com/1426993008/diff/60001/blimp/common/inp... blimp/common/input/input_event_conversions_unittest.cc:72: { On 2015/11/12 00:53:50, Kevin M wrote: > I recommend just creating a WebGestureEvent array and use the braced initializer > syntax to specify each of the test cases. Then you can just go plow through > those cases with a simple loop. E.Z. Yeah this looks cleaner :). https://chromiumcodereview.appspot.com/1426993008/diff/60001/blimp/common/pro... File blimp/common/proto/input.proto (right): https://chromiumcodereview.appspot.com/1426993008/diff/60001/blimp/common/pro... blimp/common/proto/input.proto:23: On 2015/11/12 00:53:50, Kevin M wrote: > It would be nice to reference the Blink analogues to these messages Done.
lgtm w/little nits https://chromiumcodereview.appspot.com/1426993008/diff/80001/blimp/common/inp... File blimp/common/input/input_event_conversions_unittest.cc (right): https://chromiumcodereview.appspot.com/1426993008/diff/80001/blimp/common/inp... blimp/common/input/input_event_conversions_unittest.cc:67: { Curly braces aren't needed since you aren't reusing a variable name anymore https://chromiumcodereview.appspot.com/1426993008/diff/80001/blimp/common/inp... blimp/common/input/input_event_conversions_unittest.cc:169: EXPECT_EQ(17U, kWebGestureEventCount); If you want to be extra safe, what about accumulating the events in a vector and then checking that its .size() == kWebGestureEventCount? That would ensure that you have added event structs for the # of events type.
https://chromiumcodereview.appspot.com/1426993008/diff/80001/blimp/common/inp... File blimp/common/input/input_event_conversions_unittest.cc (right): https://chromiumcodereview.appspot.com/1426993008/diff/80001/blimp/common/inp... blimp/common/input/input_event_conversions_unittest.cc:67: { On 2015/11/12 21:20:05, Kevin M wrote: > Curly braces aren't needed since you aren't reusing a variable name anymore Gah good point! If I end up pushing these to a vector I'll use a temp though. Just makes it a bit cleaner. https://chromiumcodereview.appspot.com/1426993008/diff/80001/blimp/common/inp... blimp/common/input/input_event_conversions_unittest.cc:67: { On 2015/11/12 21:20:05, Kevin M wrote: > Curly braces aren't needed since you aren't reusing a variable name anymore Gah good point! Because I changed this to a vector I'm using a temp variable though. Just made it a bit cleaner. https://chromiumcodereview.appspot.com/1426993008/diff/80001/blimp/common/inp... blimp/common/input/input_event_conversions_unittest.cc:169: EXPECT_EQ(17U, kWebGestureEventCount); On 2015/11/12 21:20:05, Kevin M wrote: > If you want to be extra safe, what about accumulating the events in a vector and > then checking that its .size() == kWebGestureEventCount? > > That would ensure that you have added event structs for the # of events type. Done.
lgtm
The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1426993008/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1426993008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)
The CQ bit was checked by dtrainor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1426993008/#ps120001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1426993008/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1426993008/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dtrainor@chromium.org changed reviewers: + esprehn@chromium.org - pdr@chromium.org
Hi esprehn@! pdr@ recommended I get your review on this. Could you do an owners review of the DEPS change here? Thanks! :)
https://codereview.chromium.org/1426993008/diff/120001/blimp/common/input/inp... File blimp/common/input/input_event_conversions.h (right): https://codereview.chromium.org/1426993008/diff/120001/blimp/common/input/inp... blimp/common/input/input_event_conversions.h:21: InputMessage* proto); In general there won't be a 1:1 correspondence between InputMessage contents and WebInputEvent contents, e.g. we will likely streamline events on-the-wire my removing state that doesn't change across sequences of events, such as flags. I'd recommend having an InputMessageProcessor class in blimp/net (rather than blimp/common) that does the InputMessage->WebInputEvent conversion, and an e.g. InputMessageGenerator that does the corresponding WebInputEvent->InputMessage conversion. Those can then hold state across events.
https://chromiumcodereview.appspot.com/1426993008/diff/120001/blimp/common/in... File blimp/common/input/input_event_conversions.h (right): https://chromiumcodereview.appspot.com/1426993008/diff/120001/blimp/common/in... blimp/common/input/input_event_conversions.h:21: InputMessage* proto); On 2015/11/16 21:25:47, Wez wrote: > In general there won't be a 1:1 correspondence between InputMessage contents and > WebInputEvent contents, e.g. we will likely streamline events on-the-wire my > removing state that doesn't change across sequences of events, such as flags. > > I'd recommend having an InputMessageProcessor class in blimp/net (rather than > blimp/common) that does the InputMessage->WebInputEvent conversion, and an e.g. > InputMessageGenerator that does the corresponding WebInputEvent->InputMessage > conversion. Those can then hold state across events. Ah good idea.
wez@: Let me know if this is more along the lines of what you want. Thanks!
Yes, that's the sort of structure I have in mind, though see my note re using MessageProcessor to receive the output of the generator; let me know if you'd like me to review in more detail. Thanks! https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... File blimp/net/input_message_generator.h (right): https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... blimp/net/input_message_generator.h:26: virtual ~InputMessageGenerator(); nit: Does this need to be virtual? Will we ever sub-class? https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... blimp/net/input_message_generator.h:31: InputMessage* message); Kevin's latest MessageProcessor API takes BlimpMessages via scoped_ptr, so you could have this API return the message that way, as well. nit: Given that part of making this an object rather than static methods is to decouple the two forms of input stream, it would be more flexible to have this explicitly supplied w/ the MessageProcessor to which the InputMessage should be sent - up to you, though; we can always try it this way and change that later if need be.
esprehn@chromium.org changed reviewers: + rbyers@chromium.org
That deps seems okay I guess, but I think you want rbyers to tell you if what you're doing here with the WebInputEvent makes sense. Where is this code going to get used btw? I kind of wonder if we want to move this POD struct out of blink entirely and just share it across the whole codebase. Right now having the IPC object live in blink is strange, since stuff far away wants to do things like serialize to protobufs. Maybe this object should be defined in a common/ dir?
On 2015/11/17 01:43:14, esprehn wrote: > That deps seems okay I guess, but I think you want rbyers to tell you if what > you're doing here with the WebInputEvent makes sense. > > Where is this code going to get used btw? I kind of wonder if we want to move > this POD struct out of blink entirely and just share it across the whole > codebase. Right now having the IPC object live in blink is strange, since stuff > far away wants to do things like serialize to protobufs. Maybe this object > should be defined in a common/ dir? It's going to be used in blimp/ (specifically somewhere around blimp_view and blimp_engine_session). Basically we're going to have a small client APK that can serialize and send input events to an engine that will dispatch them to the render process and blink. However on the client they also have to go through the compositor, so all of the code we'll be using (both cc/ on the client and content/, cc/, and Blink on the engine) use WebInputEvents which is why it seemed to make sense to serialize these for us in blimp/ :).
https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... File blimp/net/input_message_generator.h (right): https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... blimp/net/input_message_generator.h:26: virtual ~InputMessageGenerator(); On 2015/11/17 01:37:40, Wez wrote: > nit: Does this need to be virtual? Will we ever sub-class? Good point it doesn't. I should also add DISALLOW_COPY_AND_ASSIGN. https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... blimp/net/input_message_generator.h:31: InputMessage* message); On 2015/11/17 01:37:40, Wez wrote: > Kevin's latest MessageProcessor API takes BlimpMessages via scoped_ptr, so you > could have this API return the message that way, as well. > > nit: Given that part of making this an object rather than static methods is to > decouple the two forms of input stream, it would be more flexible to have this > explicitly supplied w/ the MessageProcessor to which the InputMessage should be > sent - up to you, though; we can always try it this way and change that later if > need be. Ah yeah I saw that. I wasn't quite sure I could do it because if I have to join Input and Compositor message types they'd need to share a processor right? I sent an email about it. We can discuss it there then I can make the relevant changes here :).
On 2015/11/17 05:32:44, David Trainor wrote: > On 2015/11/17 01:43:14, esprehn wrote: > > That deps seems okay I guess, but I think you want rbyers to tell you if what > > you're doing here with the WebInputEvent makes sense. > > > > Where is this code going to get used btw? I kind of wonder if we want to move > > this POD struct out of blink entirely and just share it across the whole > > codebase. Right now having the IPC object live in blink is strange, since > stuff > > far away wants to do things like serialize to protobufs. Maybe this object > > should be defined in a common/ dir? > > It's going to be used in blimp/ (specifically somewhere around blimp_view and > blimp_engine_session). Basically we're going to have a small client APK that > can serialize and send input events to an engine that will dispatch them to the > render process and blink. However on the client they also have to go through > the compositor, so all of the code we'll be using (both cc/ on the client and > content/, cc/, and Blink on the engine) use WebInputEvents which is why it > seemed to make sense to serialize these for us in blimp/ :). I thought about this a bit more. Currently we can't put the proto serialization code along side the actual code it serializes. There's a bug with how Chromium compiles protos that means you can't have a .proto file reference a message in a .proto file in another directory. I have a bug to look into fixing that, but it might require changes to all protos in Chromium, so it's not necessarily a quick fix. So right now the protos have to live in blimp/ :(. Also, we're planning on doing some optimizations that take advantage of the typical sequence of input events, so we're hoping we won't have to actually send the entire WebInputEvent protobuf over the network in the future.
rbyers@: Could I get an owners review for the DEPS change? Thanks! :)
https://codereview.chromium.org/1426993008/diff/140001/blimp/common/proto/inp... File blimp/common/proto/input.proto (right): https://codereview.chromium.org/1426993008/diff/140001/blimp/common/proto/inp... blimp/common/proto/input.proto:8: // blink::WebInputEvent POD struct. nit: That's an implementation detail of this early implementation, so perhaps make clear that in general InputMessage will carry web input events, and that currently we just serialize WebInputEvent data? https://codereview.chromium.org/1426993008/diff/140001/blimp/common/proto/inp... blimp/common/proto/input.proto:22: WebGestureDevice_Touchpad = 1; nit: TouchPad https://codereview.chromium.org/1426993008/diff/140001/blimp/common/proto/inp... blimp/common/proto/input.proto:23: WebGestureDevice_Touchscreen = 2; nit: TouchScreen https://codereview.chromium.org/1426993008/diff/140001/blimp/common/proto/inp... blimp/common/proto/input.proto:40: message ShowPress { nit: As below; what does this event actually mean - do we really need it? https://codereview.chromium.org/1426993008/diff/140001/blimp/common/proto/inp... blimp/common/proto/input.proto:45: message LongPress { nit: Where/when is this used? We omitted it from the protocol design because it wasn't clear. https://codereview.chromium.org/1426993008/diff/140001/blimp/common/proto/inp... blimp/common/proto/input.proto:133: optional double timestamp_seconds = 2; What is this relative to..? https://codereview.chromium.org/1426993008/diff/140001/blimp/common/proto/inp... blimp/common/proto/input.proto:135: optional GestureArgs gesture = 1000; Why give this such a high index? That just means it'll take more space to encode on the wire, for no reason! The original idea was to have the message-specific args as discrete optional sub-messages within the top-level message, e.g: optional GestureTapDown optional GestureTap etc https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... File blimp/net/input_message_generator.cc (right): https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... blimp/net/input_message_generator.cc:142: message->set_type(WebInputEventTypeToProto(event.type)); Check that the type is not "undefined" here? https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... blimp/net/input_message_generator.cc:147: event.type <= blink::WebInputEvent::Type::GestureTypeLast) { Wouldn't this work better as a switch that off-loads type-specific serializations to sub-routines? The if...else if... etc you have in the WebGestureEventToProto function basically does exactly that, but with a potentially-less-efficient form. https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... File blimp/net/input_message_generator.h (right): https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... blimp/net/input_message_generator.h:31: InputMessage* message); On 2015/11/17 at 05:33:14, David Trainor wrote: > On 2015/11/17 01:37:40, Wez wrote: > > Kevin's latest MessageProcessor API takes BlimpMessages via scoped_ptr, so you > > could have this API return the message that way, as well. > > > > nit: Given that part of making this an object rather than static methods is to > > decouple the two forms of input stream, it would be more flexible to have this > > explicitly supplied w/ the MessageProcessor to which the InputMessage should be > > sent - up to you, though; we can always try it this way and change that later if > > need be. > > Ah yeah I saw that. I wasn't quite sure I could do it because if I have to join Input and Compositor message types they'd need to share a processor right? I sent an email about it. We can discuss it there then I can make the relevant changes here :). It's feasible to have two distinct MessageProcessors, for different types (or even for the same type), writing to a single underlying channel. https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... File blimp/net/input_message_processor.cc (right): https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... blimp/net/input_message_processor.cc:143: type <= blink::WebInputEvent::Type::GestureTypeLast) { See comments in the generator - could we replace this and the if's in ProtoToWebGestureEvent w/ a switch and a more fine-grained set of ProtoToWeb sub-routines, for clarity? https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... File blimp/net/input_message_processor.h (right): https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... blimp/net/input_message_processor.h:30: }; Same comments re virtual & DISALLOW_COPY_AND_ASSIGN apply here. :)
https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/common/pr... File blimp/common/proto/input.proto (right): https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/common/pr... blimp/common/proto/input.proto:8: // blink::WebInputEvent POD struct. On 2015/11/18 02:18:32, Wez wrote: > nit: That's an implementation detail of this early implementation, so perhaps > make clear that in general InputMessage will carry web input events, and that > currently we just serialize WebInputEvent data? Done. https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/common/pr... blimp/common/proto/input.proto:22: WebGestureDevice_Touchpad = 1; On 2015/11/18 02:18:32, Wez wrote: > nit: TouchPad oddly they aren't capitalized in the WebKit file. I prefer capitalizing them too though :). https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/common/pr... blimp/common/proto/input.proto:23: WebGestureDevice_Touchscreen = 2; On 2015/11/18 02:18:32, Wez wrote: > nit: TouchScreen Done. https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/common/pr... blimp/common/proto/input.proto:40: message ShowPress { On 2015/11/18 02:18:32, Wez wrote: > nit: As below; what does this event actually mean - do we really need it? Mystery! So we rely on a Chromium class to build WebGestureEvents from ui::MotionEvents. I didn't want to make any assumptions on what that could kick out so I just supported serializing the whole WebGestureEvent class. I can prune this down to a smaller subset of WebGestureEvents though. https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/common/pr... blimp/common/proto/input.proto:45: message LongPress { On 2015/11/18 02:18:32, Wez wrote: > nit: Where/when is this used? We omitted it from the protocol design because it > wasn't clear. I don't know what Blink does with it (although this is a Blink class so I assume - maybe naively - they support it to some extent!). I can remove it if we don't want it for now. https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/common/pr... blimp/common/proto/input.proto:133: optional double timestamp_seconds = 2; On 2015/11/18 02:18:32, Wez wrote: > What is this relative to..? Currently it is the time the event happened relative to the amount time the device has been running. I'll add a comment. https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/common/pr... blimp/common/proto/input.proto:135: optional GestureArgs gesture = 1000; On 2015/11/18 02:18:32, Wez wrote: > Why give this such a high index? That just means it'll take more space to encode > on the wire, for no reason! That seemed to be the standard we've been using across most of the blimp protos I've seen. I can drop this though. That's a good point. > > The original idea was to have the message-specific args as discrete optional > sub-messages within the top-level message, e.g: > > optional GestureTapDown > optional GestureTap > > etc All of the gestures share some extra data (see GestureArgs common fields). But each gesture type has some additional data it adds. I split this out because if we ever support touch events or key events we'd have TouchArgs and KeyArgs which would have whatever those need as well. https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... File blimp/net/input_message_generator.cc (right): https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... blimp/net/input_message_generator.cc:147: event.type <= blink::WebInputEvent::Type::GestureTypeLast) { On 2015/11/18 02:18:32, Wez wrote: > Wouldn't this work better as a switch that off-loads type-specific > serializations to sub-routines? The if...else if... etc you have in the > WebGestureEventToProto function basically does exactly that, but with a > potentially-less-efficient form. Sounds good. That also lets me more explicitly not support certain WebGestureEvents that we don't want. Will make these changes. https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... File blimp/net/input_message_generator.h (right): https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... blimp/net/input_message_generator.h:31: InputMessage* message); On 2015/11/18 02:18:32, Wez wrote: > On 2015/11/17 at 05:33:14, David Trainor wrote: > > On 2015/11/17 01:37:40, Wez wrote: > > > Kevin's latest MessageProcessor API takes BlimpMessages via scoped_ptr, so > you > > > could have this API return the message that way, as well. > > > > > > nit: Given that part of making this an object rather than static methods is > to > > > decouple the two forms of input stream, it would be more flexible to have > this > > > explicitly supplied w/ the MessageProcessor to which the InputMessage should > be > > > sent - up to you, though; we can always try it this way and change that > later if > > > need be. > > > > Ah yeah I saw that. I wasn't quite sure I could do it because if I have to > join Input and Compositor message types they'd need to share a processor right? > I sent an email about it. We can discuss it there then I can make the relevant > changes here :). > > It's feasible to have two distinct MessageProcessors, for different types (or > even for the same type), writing to a single underlying channel. Oh sorry I misread your comment! Will add. https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... File blimp/net/input_message_processor.h (right): https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... blimp/net/input_message_processor.h:30: }; On 2015/11/18 02:18:32, Wez wrote: > Same comments re virtual & DISALLOW_COPY_AND_ASSIGN apply here. :) Done.
Thanks for the review Wez. I think this is ready for another round! ptal. https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/common/pr... File blimp/common/proto/input.proto (right): https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/common/pr... blimp/common/proto/input.proto:20: enum WebGestureDevice { Ended up removing this. We'll only be using Touchscreens for now... no point in adding the extra field. https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/common/pr... blimp/common/proto/input.proto:40: message ShowPress { On 2015/11/18 08:26:30, David Trainor wrote: > On 2015/11/18 02:18:32, Wez wrote: > > nit: As below; what does this event actually mean - do we really need it? > > Mystery! So we rely on a Chromium class to build WebGestureEvents from > ui::MotionEvents. I didn't want to make any assumptions on what that could kick > out so I just supported serializing the whole WebGestureEvent class. I can > prune this down to a smaller subset of WebGestureEvents though. Got rid of all of the events we didn't explicitly mention we wanted to support :). https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... File blimp/net/input_message_generator.cc (right): https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... blimp/net/input_message_generator.cc:142: message->set_type(WebInputEventTypeToProto(event.type)); On 2015/11/18 02:18:32, Wez wrote: > Check that the type is not "undefined" here? Removed a lot of this stuff. Don't need undefined anymore :). https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... blimp/net/input_message_generator.cc:147: event.type <= blink::WebInputEvent::Type::GestureTypeLast) { On 2015/11/18 08:26:30, David Trainor wrote: > On 2015/11/18 02:18:32, Wez wrote: > > Wouldn't this work better as a switch that off-loads type-specific > > serializations to sub-routines? The if...else if... etc you have in the > > WebGestureEventToProto function basically does exactly that, but with a > > potentially-less-efficient form. > > Sounds good. That also lets me more explicitly not support certain > WebGestureEvents that we don't want. Will make these changes. Done. https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... File blimp/net/input_message_generator.h (right): https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... blimp/net/input_message_generator.h:31: InputMessage* message); On 2015/11/18 08:26:30, David Trainor wrote: > On 2015/11/18 02:18:32, Wez wrote: > > On 2015/11/17 at 05:33:14, David Trainor wrote: > > > On 2015/11/17 01:37:40, Wez wrote: > > > > Kevin's latest MessageProcessor API takes BlimpMessages via scoped_ptr, so > > you > > > > could have this API return the message that way, as well. > > > > > > > > nit: Given that part of making this an object rather than static methods > is > > to > > > > decouple the two forms of input stream, it would be more flexible to have > > this > > > > explicitly supplied w/ the MessageProcessor to which the InputMessage > should > > be > > > > sent - up to you, though; we can always try it this way and change that > > later if > > > > need be. > > > > > > Ah yeah I saw that. I wasn't quite sure I could do it because if I have to > > join Input and Compositor message types they'd need to share a processor > right? > > I sent an email about it. We can discuss it there then I can make the > relevant > > changes here :). > > > > It's feasible to have two distinct MessageProcessors, for different types (or > > even for the same type), writing to a single underlying channel. > > Oh sorry I misread your comment! Will add. Looked into adding it and I think it would be cleaner without it (when I merge it with the other CL that builds the whole BlimpMessage I'll see if it could be added). The issue is it would probably be cleaner if this class didn't care about things like tab_id, render_widget_id, session_id, etc. that are required to actually build a BlimpMessage. The higher level object could manage that. I think this will fit really nicely into the serialization flow though. https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... File blimp/net/input_message_processor.cc (right): https://chromiumcodereview.appspot.com/1426993008/diff/140001/blimp/net/input... blimp/net/input_message_processor.cc:143: type <= blink::WebInputEvent::Type::GestureTypeLast) { On 2015/11/18 02:18:32, Wez wrote: > See comments in the generator - could we replace this and the if's in > ProtoToWebGestureEvent w/ a switch and a more fine-grained set of ProtoToWeb > sub-routines, for clarity? Done.
On 2015/11/17 21:10:03, David Trainor wrote: > rbyers@: Could I get an owners review for the DEPS change? Thanks! :) Sorry for the delay (travelling). Yeah this seems fine to me. LGTM I worry though that it'll drift out of sync (WebInputEvent changes fairly frequently). Do you have any tooling to warn you when new fields are added? We've seen other cases (eg. browser plugin) where code that copies all properties gets stale and causes subtle bugs to that feature (it's not reasonable to expect everyone that adds a field to WebInputEvent to know to find all the various places that have hard-coded the list of fields). Also, I assume you have some system for ensuring the two ends are always the exact same version, right? I.e. having a protobuf for this isn't going to impose any new versioning requirements on the type.
https://codereview.chromium.org/1426993008/diff/140001/blimp/common/proto/inp... File blimp/common/proto/input.proto (right): https://codereview.chromium.org/1426993008/diff/140001/blimp/common/proto/inp... blimp/common/proto/input.proto:40: message ShowPress { On 2015/11/19 00:49:43, David Trainor wrote: > On 2015/11/18 08:26:30, David Trainor wrote: > > On 2015/11/18 02:18:32, Wez wrote: > > > nit: As below; what does this event actually mean - do we really need it? > > > > Mystery! So we rely on a Chromium class to build WebGestureEvents from > > ui::MotionEvents. I didn't want to make any assumptions on what that could > kick > > out so I just supported serializing the whole WebGestureEvent class. I can > > prune this down to a smaller subset of WebGestureEvents though. > > Got rid of all of the events we didn't explicitly mention we wanted to support > :). ShowPress it the event that triggers link highlighting and the CSS :active state (shortly after TapDown when there has been no movement, which is when the finger actually goes down). https://codereview.chromium.org/1426993008/diff/140001/blimp/common/proto/inp... blimp/common/proto/input.proto:45: message LongPress { On 2015/11/18 08:26:30, David Trainor wrote: > On 2015/11/18 02:18:32, Wez wrote: > > nit: Where/when is this used? We omitted it from the protocol design because > it > > wasn't clear. > > I don't know what Blink does with it (although this is a Blink class so I assume > - maybe naively - they support it to some extent!). I can remove it if we don't > want it for now. LongPress is used to trigger text selection and context menus
[Sorry for the additional stuff, I initially did a quick skim to validate the DEPS, now I'm thinking more deeply about your scenario]. But DEPS still LGTM (and the rest is up to you <grin>). https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... File blimp/common/proto/input.proto (right): https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... blimp/common/proto/input.proto:83: // of these with the existing protocol. Blink may make assumptions that this subset violates. For example that a GestureTap is always preceeded by a GestureTapDown (and a GestureTapDown is always followed by either a GestureTap or GestureTapCancel, possibly with an intervening GestureShowPress). I forget the details of exactly how forgiving we are here but we'd like to eventually start validating more (at least in DEBUG). Anyway, not blocking from my perspective as it doesn't directly affect blink, but this may cause subtle bugs for you...
lgtm
On 2015/11/20 03:09:39, Rick Byers (slow until Nov 23) wrote: > On 2015/11/17 21:10:03, David Trainor wrote: > > rbyers@: Could I get an owners review for the DEPS change? Thanks! :) > > Sorry for the delay (travelling). > Yeah this seems fine to me. LGTM > > I worry though that it'll drift out of sync (WebInputEvent changes fairly > frequently). Do you have any tooling to warn you when new fields are added? > We've seen other cases (eg. browser plugin) where code that copies all > properties gets stale and causes subtle bugs to that feature (it's not > reasonable to expect everyone that adds a field to WebInputEvent to know to find > all the various places that have hard-coded the list of fields). > > Also, I assume you have some system for ensuring the two ends are always the > exact same version, right? I.e. having a protobuf for this isn't going to > impose any new versioning requirements on the type. Not yet, but once we get something up and running we're going to split our time between optimizations and versioning and have a solution in place before shipping. We're probably going to move the protocol away from WebInputEvent (and we'll just synthesize them on the engine), so there should be no versioning requirements on WebInputEvent.
On 2015/11/20 03:17:55, Rick Byers (slow until Nov 23) wrote: > [Sorry for the additional stuff, I initially did a quick skim to validate the > DEPS, now I'm thinking more deeply about your scenario]. > > But DEPS still LGTM (and the rest is up to you <grin>). > > https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... > File blimp/common/proto/input.proto (right): > > https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... > blimp/common/proto/input.proto:83: // of these with the existing protocol. > Blink may make assumptions that this subset violates. For example that a > GestureTap is always preceeded by a GestureTapDown (and a GestureTapDown is > always followed by either a GestureTap or GestureTapCancel, possibly with an > intervening GestureShowPress). I forget the details of exactly how forgiving we > are here but we'd like to eventually start validating more (at least in DEBUG). > Anyway, not blocking from my perspective as it doesn't directly affect blink, > but this may cause subtle bugs for you... That's a good point... this might actually impact us. We can probably synthesize these or send them if we find we have bugs. Thanks! Is there a place where we can see the current expected behavior/sequence of input events that Blink expects?
Apologies for the delay! https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... File blimp/net/input_message_generator.h (right): https://codereview.chromium.org/1426993008/diff/140001/blimp/net/input_messag... blimp/net/input_message_generator.h:31: InputMessage* message); On 2015/11/19 at 00:49:43, David Trainor wrote: > On 2015/11/18 08:26:30, David Trainor wrote: > > On 2015/11/18 02:18:32, Wez wrote: > > > On 2015/11/17 at 05:33:14, David Trainor wrote: > > > > On 2015/11/17 01:37:40, Wez wrote: > > > > > Kevin's latest MessageProcessor API takes BlimpMessages via scoped_ptr, so > > > you > > > > > could have this API return the message that way, as well. > > > > > > > > > > nit: Given that part of making this an object rather than static methods > > is > > > to > > > > > decouple the two forms of input stream, it would be more flexible to have > > > this > > > > > explicitly supplied w/ the MessageProcessor to which the InputMessage > > should > > > be > > > > > sent - up to you, though; we can always try it this way and change that > > > later if > > > > > need be. > > > > > > > > Ah yeah I saw that. I wasn't quite sure I could do it because if I have to > > > join Input and Compositor message types they'd need to share a processor > > right? > > > I sent an email about it. We can discuss it there then I can make the > > relevant > > > changes here :). > > > > > > It's feasible to have two distinct MessageProcessors, for different types (or > > > even for the same type), writing to a single underlying channel. > > > > Oh sorry I misread your comment! Will add. > > Looked into adding it and I think it would be cleaner without it (when I merge it with the other CL that builds the whole BlimpMessage I'll see if it could be added). The issue is it would probably be cleaner if this class didn't care about things like tab_id, render_widget_id, session_id, etc. that are required to actually build a BlimpMessage. The higher level object could manage that. I think this will fit really nicely into the serialization flow though. There's no requirement for this object to set those fields; you'd simply have it take the target MessageProcessor and document that it, or some other MessageProcessor in the pipeline, it responsible for setting those fields appropriately - so you're just using the interface to decouple the WebInputEvent:BlimpMessages from a 1:1 relationship. https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... File blimp/common/proto/input.proto (right): https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... blimp/common/proto/input.proto:18: message GestureBaseProperties { nit: I'd rename this GestureBase given the context; each message is inherently a list of event-specific properties. However, it would simplify some of the marshalling code if you renamed it GestureCommon and placed it alongside the Gesture-specific sub-messages - WDYT? https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... blimp/common/proto/input.proto:98: // started. nit: Is this to when the _device_ started, or when the client _app_ was started? Whichever it is, we're going to need a way of re-basing the timestamp e.g. when re-connecting to the engine after a device reboot. https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... blimp/common/proto/input.proto:102: // Only one of these fields may be set per InputMessage. nit: Add a TODO() to make these a oneof if Chromium's protobuf impl ever supports that? https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... File blimp/net/input_message_generator.cc (right): https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... blimp/net/input_message_generator.cc:15: void WebGestureEventBaseToProto(const blink::WebGestureEvent* event, const WebGestureEvent& here and below? https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... blimp/net/input_message_generator.cc:177: // Unsupported WebInputEvent. Could we instead list the events that we ignore explicitly, and perhaps NOTIMPLEMENTED() them? That way the switch will catch addition of new events that we should perhaps care about. https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... File blimp/net/input_message_generator.h (right): https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... blimp/net/input_message_generator.h:32: bool GenerateMessage(const blink::WebInputEvent* event, InputMessage* proto); nit: I think this should be const blink::WebInputEvent& - null input messages don't make any sense. Suggest returning scoped_ptr<InputMessage> - the MessageProcessor interface is now passed the BlimpMessage to process, so creating a new message object at this level doesn't really add any overhead. https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... File blimp/net/input_message_processor.cc (right): https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... blimp/net/input_message_processor.cc:191: // Unsupported WebInputEvent. Now that the UNKNOWN enum value CL has landed, let's rebase and get rid of the default: case, and have UNKNOWN DLOG(FATAL) and return false? https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... File blimp/net/input_message_unittest.cc (right): https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... blimp/net/input_message_unittest.cc:36: TEST(InputMessageTest, TestWebGestureEvents) { nit: I'd recommend breaking this into individual tests for each type, e.g: InputMessageTest.ScrollBeginRoundTrip etc, that construct the event and pass it to Validate...() directly rather than to push_back().
https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... File blimp/common/proto/input.proto (right): https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... blimp/common/proto/input.proto:98: // started. On 2015/11/23 22:41:41, Wez wrote: > nit: Is this to when the _device_ started, or when the client _app_ was started? > > Whichever it is, we're going to need a way of re-basing the timestamp e.g. when > re-connecting to the engine after a device reboot. In fact, given all the different ways to measure time, it'd be helpful to specify which method you're using, like for example http://developer.android.com/reference/android/os/SystemClock.html#elapsedRea... (or hopefully not http://developer.android.com/reference/android/os/SystemClock.html#uptimeMill... ), or if it's some other source of time, mention that instead. It seems like the definition is here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... The explanatory comment says: "Seconds since platform start with microsecond resolution." https://codereview.chromium.org/1426993008/diff/180001/blimp/net/DEPS File blimp/net/DEPS (right): https://codereview.chromium.org/1426993008/diff/180001/blimp/net/DEPS#newcode5 blimp/net/DEPS:5: "+third_party/WebKit/public/platform/WebGestureDevice.h", Nit: alphabetical order https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... File blimp/net/input_message_generator.h (right): https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... blimp/net/input_message_generator.h:30: // it to the |message_processor|. This might make use of state from previous Where is |message_processor|? https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... File blimp/net/input_message_processor.cc (right): https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... blimp/net/input_message_processor.cc:26: scoped_ptr<blink::WebGestureEvent> event(new blink::WebGestureEvent); Optional nit: Did you investigate if the code was easier to read if you extracted these three lines out (passing in the blink::WIE::Type of course) to a new function? https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... File blimp/net/input_message_processor.h (right): https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... blimp/net/input_message_processor.h:22: // data. See InputMessageProcessor for the deserialize code. Did you mean to refer the other way? Maybe "See InputMessageGenerator for the serialize code." https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... File blimp/net/input_message_unittest.cc (right): https://codereview.chromium.org/1426993008/diff/180001/blimp/net/input_messag... blimp/net/input_message_unittest.cc:36: TEST(InputMessageTest, TestWebGestureEvents) { On 2015/11/23 22:41:41, Wez wrote: > nit: I'd recommend breaking this into individual tests for each type, e.g: > InputMessageTest.ScrollBeginRoundTrip etc, that construct the event and pass it > to Validate...() directly rather than to push_back(). Agreed. That way you can even more easily identify the failing type since it'll be in the name of the test.
Description was changed from ========== Serialize a subset of WebInputEvents to protobufs. - Add (de)serialization methods for WebGestureEvents (a subset of WebInputEvents). - Add unit tests to validate that moving to and from these events works properly. BUG=548806 ========== to ========== Serialize a subset of WebInputEvents to protobufs. - Add (de)serialization methods for WebGestureEvents (a subset of WebInputEvents). - Add unit tests to validate that moving to and from these events works properly. BUG=548806 ==========
On 2015/11/20 18:07:25, David Trainor wrote: > On 2015/11/20 03:17:55, Rick Byers (slow until Nov 23) wrote: > > [Sorry for the additional stuff, I initially did a quick skim to validate the > > DEPS, now I'm thinking more deeply about your scenario]. > > > > But DEPS still LGTM (and the rest is up to you <grin>). > > > > > https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... > > File blimp/common/proto/input.proto (right): > > > > > https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... > > blimp/common/proto/input.proto:83: // of these with the existing protocol. > > Blink may make assumptions that this subset violates. For example that a > > GestureTap is always preceeded by a GestureTapDown (and a GestureTapDown is > > always followed by either a GestureTap or GestureTapCancel, possibly with an > > intervening GestureShowPress). I forget the details of exactly how forgiving > we > > are here but we'd like to eventually start validating more (at least in > DEBUG). > > Anyway, not blocking from my perspective as it doesn't directly affect blink, > > but this may cause subtle bugs for you... > > That's a good point... this might actually impact us. We can probably > synthesize these or send them if we find we have bugs. Thanks! Is there a > place where we can see the current expected behavior/sequence of input events > that Blink expects? We're in the middle of trying to be more precise about this, but it's on the back burner - turns out we have a lot of tests that may subtly violate the expectations :-) I'm not sure what the best resources is ATM, /cc tdresser@ who will know. At a minimum we have some details at https://www.chromium.org/developers/design-documents/aura/gesture-recognizer, and see related http://crbug.com/345372
On 2015/11/24 19:10:43, Rick Byers wrote: > On 2015/11/20 18:07:25, David Trainor wrote: > > On 2015/11/20 03:17:55, Rick Byers (slow until Nov 23) wrote: > > > [Sorry for the additional stuff, I initially did a quick skim to validate > the > > > DEPS, now I'm thinking more deeply about your scenario]. > > > > > > But DEPS still LGTM (and the rest is up to you <grin>). > > > > > > > > > https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... > > > File blimp/common/proto/input.proto (right): > > > > > > > > > https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... > > > blimp/common/proto/input.proto:83: // of these with the existing protocol. > > > Blink may make assumptions that this subset violates. For example that a > > > GestureTap is always preceeded by a GestureTapDown (and a GestureTapDown is > > > always followed by either a GestureTap or GestureTapCancel, possibly with an > > > intervening GestureShowPress). I forget the details of exactly how > forgiving > > we > > > are here but we'd like to eventually start validating more (at least in > > DEBUG). > > > Anyway, not blocking from my perspective as it doesn't directly affect > blink, > > > but this may cause subtle bugs for you... > > > > That's a good point... this might actually impact us. We can probably > > synthesize these or send them if we find we have bugs. Thanks! Is there a > > place where we can see the current expected behavior/sequence of input events > > that Blink expects? > > We're in the middle of trying to be more precise about this, but it's on the > back burner - turns out we have a lot of tests that may subtly violate the > expectations :-) > > I'm not sure what the best resources is ATM, /cc tdresser@ who will know. At a > minimum we have some details at > https://www.chromium.org/developers/design-documents/aura/gesture-recognizer, > and see related http://crbug.com/345372 The closest thing I'm aware of is this list that lanwei@ wrote up: https://docs.google.com/document/d/1mQFx99xz5jVUHQk2jZTjHPgMjRQd4EUDfdLG0XKOl... It's referring to the ui types, but there's a fairly simple mapping to the blink event types.
On 2015/11/24 19:52:23, tdresser wrote: > On 2015/11/24 19:10:43, Rick Byers wrote: > > On 2015/11/20 18:07:25, David Trainor wrote: > > > On 2015/11/20 03:17:55, Rick Byers (slow until Nov 23) wrote: > > > > [Sorry for the additional stuff, I initially did a quick skim to validate > > the > > > > DEPS, now I'm thinking more deeply about your scenario]. > > > > > > > > But DEPS still LGTM (and the rest is up to you <grin>). > > > > > > > > > > > > > > https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... > > > > File blimp/common/proto/input.proto (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... > > > > blimp/common/proto/input.proto:83: // of these with the existing protocol. > > > > Blink may make assumptions that this subset violates. For example that a > > > > GestureTap is always preceeded by a GestureTapDown (and a GestureTapDown > is > > > > always followed by either a GestureTap or GestureTapCancel, possibly with > an > > > > intervening GestureShowPress). I forget the details of exactly how > > forgiving > > > we > > > > are here but we'd like to eventually start validating more (at least in > > > DEBUG). > > > > Anyway, not blocking from my perspective as it doesn't directly affect > > blink, > > > > but this may cause subtle bugs for you... > > > > > > That's a good point... this might actually impact us. We can probably > > > synthesize these or send them if we find we have bugs. Thanks! Is there a > > > place where we can see the current expected behavior/sequence of input > events > > > that Blink expects? > > > > We're in the middle of trying to be more precise about this, but it's on the > > back burner - turns out we have a lot of tests that may subtly violate the > > expectations :-) > > > > I'm not sure what the best resources is ATM, /cc tdresser@ who will know. At > a > > minimum we have some details at > > https://www.chromium.org/developers/design-documents/aura/gesture-recognizer, > > and see related http://crbug.com/345372 > > The closest thing I'm aware of is this list that lanwei@ wrote up: > https://docs.google.com/document/d/1mQFx99xz5jVUHQk2jZTjHPgMjRQd4EUDfdLG0XKOl... > > It's referring to the ui types, but there's a fairly simple mapping to the blink > event types. Thanks that's a great reference. I appreciate it!
addressed more comments. ptal https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/common/pr... File blimp/common/proto/input.proto (right): https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/common/pr... blimp/common/proto/input.proto:18: message GestureBaseProperties { On 2015/11/23 22:41:41, Wez wrote: > nit: I'd rename this GestureBase given the context; each message is inherently a > list of event-specific properties. > > However, it would simplify some of the marshalling code if you renamed it > GestureCommon and placed it alongside the Gesture-specific sub-messages - WDYT? Yeah I think that's a good idea. Will change. https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/common/pr... blimp/common/proto/input.proto:98: // started. On 2015/11/24 08:12:43, nyquist wrote: > On 2015/11/23 22:41:41, Wez wrote: > > nit: Is this to when the _device_ started, or when the client _app_ was > started? > > > > Whichever it is, we're going to need a way of re-basing the timestamp e.g. > when > > re-connecting to the engine after a device reboot. > > In fact, given all the different ways to measure time, it'd be helpful to > specify which method you're using, like for example > http://developer.android.com/reference/android/os/SystemClock.html#elapsedRea... > (or hopefully not > http://developer.android.com/reference/android/os/SystemClock.html#uptimeMill... > ), or if it's some other source of time, mention that instead. > > It seems like the definition is here: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > The explanatory comment says: "Seconds since platform start with microsecond > resolution." Sadly it looks like it is SystemCLock#uptimeMillis(), so the resolution for Android touch events doesn't go to microseconds. https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/common/pr... blimp/common/proto/input.proto:102: // Only one of these fields may be set per InputMessage. On 2015/11/23 22:41:41, Wez wrote: > nit: Add a TODO() to make these a oneof if Chromium's protobuf impl ever > supports that? Done. https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/net/DEPS File blimp/net/DEPS (right): https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/net/DEPS#... blimp/net/DEPS:5: "+third_party/WebKit/public/platform/WebGestureDevice.h", On 2015/11/24 08:12:43, nyquist wrote: > Nit: alphabetical order Done. https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/net/input... File blimp/net/input_message_generator.cc (right): https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/net/input... blimp/net/input_message_generator.cc:15: void WebGestureEventBaseToProto(const blink::WebGestureEvent* event, On 2015/11/23 22:41:41, Wez wrote: > const WebGestureEvent& here and below? Done. https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/net/input... blimp/net/input_message_generator.cc:177: // Unsupported WebInputEvent. On 2015/11/23 22:41:41, Wez wrote: > Could we instead list the events that we ignore explicitly, and perhaps > NOTIMPLEMENTED() them? > > That way the switch will catch addition of new events that we should perhaps > care about. Done. https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/net/input... File blimp/net/input_message_generator.h (right): https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/net/input... blimp/net/input_message_generator.h:30: // it to the |message_processor|. This might make use of state from previous On 2015/11/24 08:12:43, nyquist wrote: > Where is |message_processor|? Secret! https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/net/input... blimp/net/input_message_generator.h:32: bool GenerateMessage(const blink::WebInputEvent* event, InputMessage* proto); On 2015/11/23 22:41:41, Wez wrote: > nit: I think this should be const blink::WebInputEvent& - null input messages > don't make any sense. > > Suggest returning scoped_ptr<InputMessage> - the MessageProcessor interface is > now passed the BlimpMessage to process, so creating a new message object at this > level doesn't really add any overhead. Won't we have to copy the message into the BlimpMessage object we end up sending? I could return a scoped_ptr<BlimpMessage> object which would avoid copying the proto though. https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/net/input... File blimp/net/input_message_processor.cc (right): https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/net/input... blimp/net/input_message_processor.cc:26: scoped_ptr<blink::WebGestureEvent> event(new blink::WebGestureEvent); On 2015/11/24 08:12:43, nyquist wrote: > Optional nit: Did you investigate if the code was easier to read if you > extracted these three lines out (passing in the blink::WIE::Type of course) to a > new function? Done. https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/net/input... blimp/net/input_message_processor.cc:191: // Unsupported WebInputEvent. On 2015/11/23 22:41:41, Wez wrote: > Now that the UNKNOWN enum value CL has landed, let's rebase and get rid of the > default: case, and have UNKNOWN DLOG(FATAL) and return false? Done. https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/net/input... File blimp/net/input_message_unittest.cc (right): https://chromiumcodereview.appspot.com/1426993008/diff/180001/blimp/net/input... blimp/net/input_message_unittest.cc:36: TEST(InputMessageTest, TestWebGestureEvents) { On 2015/11/24 08:12:43, nyquist wrote: > On 2015/11/23 22:41:41, Wez wrote: > > nit: I'd recommend breaking this into individual tests for each type, e.g: > > InputMessageTest.ScrollBeginRoundTrip etc, that construct the event and pass > it > > to Validate...() directly rather than to push_back(). > > Agreed. That way you can even more easily identify the failing type since it'll > be in the name of the test. Done.
lgtm https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... File blimp/common/proto/input.proto (right): https://codereview.chromium.org/1426993008/diff/180001/blimp/common/proto/inp... blimp/common/proto/input.proto:98: // started. On 2015/11/24 20:08:27, David Trainor wrote: > On 2015/11/24 08:12:43, nyquist wrote: > > On 2015/11/23 22:41:41, Wez wrote: > > > nit: Is this to when the _device_ started, or when the client _app_ was > > started? > > > > > > Whichever it is, we're going to need a way of re-basing the timestamp e.g. > > when > > > re-connecting to the engine after a device reboot. > > > > In fact, given all the different ways to measure time, it'd be helpful to > > specify which method you're using, like for example > > > http://developer.android.com/reference/android/os/SystemClock.html#elapsedRea... > > (or hopefully not > > > http://developer.android.com/reference/android/os/SystemClock.html#uptimeMill... > > ), or if it's some other source of time, mention that instead. > > > > It seems like the definition is here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > The explanatory comment says: "Seconds since platform start with microsecond > > resolution." > > Sadly it looks like it is SystemCLock#uptimeMillis(), so the resolution for > Android touch events doesn't go to microseconds. Oh; that might be unfortunate. That clock is also stopped when the device is idle, which might potentially screw things up when we are later communicating with an engine that has stayed alive since last time. I guess we just have a safe way to rebase the timeline even if the device has not been rebooted, since being idle also messes it up. https://codereview.chromium.org/1426993008/diff/200001/blimp/common/proto/inp... File blimp/common/proto/input.proto (right): https://codereview.chromium.org/1426993008/diff/200001/blimp/common/proto/inp... blimp/common/proto/input.proto:81: // This is based off of the client's SystemClock#uptimeMillis(). Optional nit: I just realized this comment is now Android specific. Maybe prefix that last sentence with "On Android, this is ..."?
https://chromiumcodereview.appspot.com/1426993008/diff/200001/blimp/common/pr... File blimp/common/proto/input.proto (right): https://chromiumcodereview.appspot.com/1426993008/diff/200001/blimp/common/pr... blimp/common/proto/input.proto:81: // This is based off of the client's SystemClock#uptimeMillis(). On 2015/11/24 20:33:28, nyquist wrote: > Optional nit: I just realized this comment is now Android specific. Maybe prefix > that last sentence with "On Android, this is ..."? Done.
On 2015/11/24 21:34:00, David Trainor wrote: > https://chromiumcodereview.appspot.com/1426993008/diff/200001/blimp/common/pr... > File blimp/common/proto/input.proto (right): > > https://chromiumcodereview.appspot.com/1426993008/diff/200001/blimp/common/pr... > blimp/common/proto/input.proto:81: // This is based off of the client's > SystemClock#uptimeMillis(). > On 2015/11/24 20:33:28, nyquist wrote: > > Optional nit: I just realized this comment is now Android specific. Maybe > prefix > > that last sentence with "On Android, this is ..."? > > Done. Wez can I get one more pass? :)
Yup - will take a look! On 24 November 2015 at 13:34, <dtrainor@chromium.org> wrote: > On 2015/11/24 21:34:00, David Trainor wrote: > > > https://chromiumcodereview.appspot.com/1426993008/diff/200001/blimp/common/pr... > >> File blimp/common/proto/input.proto (right): >> > > > > https://chromiumcodereview.appspot.com/1426993008/diff/200001/blimp/common/pr... > >> blimp/common/proto/input.proto:81: // This is based off of the client's >> SystemClock#uptimeMillis(). >> On 2015/11/24 20:33:28, nyquist wrote: >> > Optional nit: I just realized this comment is now Android specific. >> Maybe >> prefix >> > that last sentence with "On Android, this is ..."? >> > > Done. >> > > Wez can I get one more pass? :) > > https://chromiumcodereview.appspot.com/1426993008/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1426993008/diff/240001/blimp/net/input_messag... File blimp/net/input_message_generator.h (right): https://codereview.chromium.org/1426993008/diff/240001/blimp/net/input_messag... blimp/net/input_message_generator.h:34: // from previous messages. nit: You want the "This might make use..." to follow on from the "basic input event fields populated." - as written it sounds like populating "the rest of the required fields..." makes use of state from previous messages. https://codereview.chromium.org/1426993008/diff/240001/blimp/net/input_messag... File blimp/net/input_message_unittest.cc (right): https://codereview.chromium.org/1426993008/diff/240001/blimp/net/input_messag... blimp/net/input_message_unittest.cc:124: } // namespace blimp nit: You might add a test that validates that one of the un-handled types causes a nullptr result, rather than round-tripping?
lgtm
https://chromiumcodereview.appspot.com/1426993008/diff/240001/blimp/net/input... File blimp/net/input_message_generator.h (right): https://chromiumcodereview.appspot.com/1426993008/diff/240001/blimp/net/input... blimp/net/input_message_generator.h:34: // from previous messages. On 2015/11/25 23:52:02, Wez wrote: > nit: You want the "This might make use..." to follow on from the "basic input > event fields populated." - as written it sounds like populating "the rest of the > required fields..." makes use of state from previous messages. Done. https://chromiumcodereview.appspot.com/1426993008/diff/240001/blimp/net/input... File blimp/net/input_message_unittest.cc (right): https://chromiumcodereview.appspot.com/1426993008/diff/240001/blimp/net/input... blimp/net/input_message_unittest.cc:124: } // namespace blimp On 2015/11/25 23:52:02, Wez wrote: > nit: You might add a test that validates that one of the un-handled types causes > a nullptr result, rather than round-tripping? Good idea. Done.
The CQ bit was checked by dtrainor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org, rbyers@chromium.org, nyquist@chromium.org, wez@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1426993008/#ps280001 (title: "Re-add fatal log.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1426993008/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1426993008/280001
Message was sent while issue was closed.
Description was changed from ========== Serialize a subset of WebInputEvents to protobufs. - Add (de)serialization methods for WebGestureEvents (a subset of WebInputEvents). - Add unit tests to validate that moving to and from these events works properly. BUG=548806 ========== to ========== Serialize a subset of WebInputEvents to protobufs. - Add (de)serialization methods for WebGestureEvents (a subset of WebInputEvents). - Add unit tests to validate that moving to and from these events works properly. BUG=548806 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Serialize a subset of WebInputEvents to protobufs. - Add (de)serialization methods for WebGestureEvents (a subset of WebInputEvents). - Add unit tests to validate that moving to and from these events works properly. BUG=548806 ========== to ========== Serialize a subset of WebInputEvents to protobufs. - Add (de)serialization methods for WebGestureEvents (a subset of WebInputEvents). - Add unit tests to validate that moving to and from these events works properly. BUG=548806 Committed: https://crrev.com/6086a92c956e7bd81aa402b2e4dc4ec6a91c8585 Cr-Commit-Position: refs/heads/master@{#362089} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/6086a92c956e7bd81aa402b2e4dc4ec6a91c8585 Cr-Commit-Position: refs/heads/master@{#362089} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
