Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1212113007:
Add WebPointerProperties type. (Closed)
|
Created:
5 years, 5 months ago by USE eero AT chromium.org Modified:
5 years, 5 months ago CC:
blink-reviews, dglazkov+blink, mustaq, jdduke (slow), tdresser Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd WebPointerProperties type.
This adds WebPointerProperties types as common types which is inherited by
WebMouseEvent and WebTouchPoint.
BUG=508283
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199091
Patch Set 1 #
Total comments: 11
Patch Set 2 : Do not rename fields, drop WebMouseProperties and WebPointerEvent #
Total comments: 13
Patch Set 3 : Comments, do not touch radiusX/Y, relocate WebPointerProperties.h #
Messages
Total messages: 38 (11 generated)
e.hakkinen@samsung.com changed reviewers: + rbyers@chromium.org
Is this the kind of solution you were thinking in https://codereview.chromium.org/1192463008/#msg7? The idea is to define common properties needed in web mouse and pointer events and (temporary also in) web touch points in one place and to include them though inheritance into web pointer events and (temporary also into) web touch points.
This should of course be rebased on top of https://codereview.chromium.org/1216053014/ once it is reviewed.
On 2015/07/06 16:40:50, e_hakkinen wrote: > Is this the kind of solution you were thinking in > https://codereview.chromium.org/1192463008/#msg7? > > The idea is to define common properties needed in web mouse and pointer events > and (temporary also in) web touch points in one place and to include them though > inheritance into web pointer events and (temporary also into) web touch points. Interesting. I was thinking we'd do refactoring into common types only after we identified places it would actually let us simplify things. But I'm not opposed to do doing some upfront where we think it'll be helpful. I think it can be (in some cases needs to be) simpler than this (at least to start) though - I'll add some specific comments...
This is an interesting approach, thanks. Thinking about it further I think we can make your life easier (avoid a 3-sided patch) while still getting most of the benefit. The key things you need are: 1. A common place to define 'button' across WebMouseEvent and WebTouchPoint 2. A place to add your new tilt and pointerType properties 3. A plan for how we'll add stylus capabilities to WebMouseEvent in the future. I think a simple WebPointerProperties with button, tilt and pointerType can address this most simply. In the future we can move the co-ordinate fields, radius, pressure and even rotation angle there so that WebTouchPoint becomes a nearly empty shell. But unless you're actually planning on working on the desktop stylus scenarios now, there's no rush to do that. WDYT? https://codereview.chromium.org/1212113007/diff/1/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/1212113007/diff/1/public/web/WebInputEvent.h#... public/web/WebInputEvent.h:424: class WebPointerEvent : public WebMouseEvent, public WebPointerProperties { Let's hold off adding WebPointerEvent until we have a concrete use for it. I suspect we'll find it simpler/easier to rename WebMouseEvent to WebPointerEvent - we probably don't need both at any given time (once the blink and chromium repositories have merged at least). https://codereview.chromium.org/1212113007/diff/1/public/web/WebMouseProperti... File public/web/WebMouseProperties.h (right): https://codereview.chromium.org/1212113007/diff/1/public/web/WebMouseProperti... public/web/WebMouseProperties.h:27: Button button; It feels a little odd to have WebMouseProperties in WebTouchPoint. The only reason you've separated this out from WebPointerProperties was to avoid bloating WebMouseEvent with a bunch of normally unused properties, right? I wouldn't worry about that. I've done performance tests on the size of WebTouchEvent and it took a LOT of additional space to have any impact on performance. The objects are very short lived and there tends to only be a few alive at any time, and usually no more than one created per frame. So the perf impact of extra fields is low in practice. So how about we simplify all of this by having only one new class 'WebPointerProperties' - the stuff that we've generalized from mouse events and touch points along the path to having a unified pointer event. https://codereview.chromium.org/1212113007/diff/1/public/web/WebPointerProper... File public/web/WebPointerProperties.h (right): https://codereview.chromium.org/1212113007/diff/1/public/web/WebPointerProper... public/web/WebPointerProperties.h:13: class WebPointerProperties { I like the idea of a common class for the properties in common between WebMouseEvent (in the future WebPointerEvent) and WebTouchPoint. This doesn't need to match Pointer Event semantics/names exactly though - whatever is most convenient for blink/chromium is fine (it can be up to blink to do any necessary conversions). https://codereview.chromium.org/1212113007/diff/1/public/web/WebTouchPoint.h File public/web/WebTouchPoint.h (left): https://codereview.chromium.org/1212113007/diff/1/public/web/WebTouchPoint.h#... public/web/WebTouchPoint.h:66: float radiusY; You can't just replace radiusX/radiusY with width/height - they have different semantics in the case of rotation. For now why not just keep them called radiusX/radiusY? In fact, it might be simplest if the first CL doesn't change the name of ANY fields, then we don't need to change chromium at all. We can always do specific renames later if we want to change semantics, but it's not clear to me when we'll actually need that. https://codereview.chromium.org/1212113007/diff/1/public/web/WebTouchPoint.h File public/web/WebTouchPoint.h (right): https://codereview.chromium.org/1212113007/diff/1/public/web/WebTouchPoint.h#... public/web/WebTouchPoint.h:43: class WebTouchPoint : public WebMouseProperties, public WebPointerProperties { I was originally thinking we'd use aggregation (i.e. a "pointerProperties field) instead of inheritance since the use of multiple inheritance is a little unusual. But I like the idea of using inheritance to minimize the changes necessary to the code. As long as we're careful it should be fine. https://codereview.chromium.org/1212113007/diff/1/public/web/WebTouchPoint.h#... public/web/WebTouchPoint.h:64: WebFloatPoint position; These co-ordinate properties could eventually be moved into WebPointerProperties too. Don't try to do that here - there is some complexity (eg. point vs. separate x/y members, floats vs. ints) but I don't expect it would be that hard to do in a later CL. Maybe add a TODO saying they should move?
By the way, one of my motivations for simplifying this as much as possible is that it's not just this Web layer we need to worry about. It's also the platform layer (eg. PlatformTouchPoint) and the DOM layer (blink::Touch) we need to worry about. If we focus on the few things you need now, it'll make it easier to update all those layers in follow-up CLs without changing a ton of code.
PTAL. https://codereview.chromium.org/1212113007/diff/1/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/1212113007/diff/1/public/web/WebInputEvent.h#... public/web/WebInputEvent.h:424: class WebPointerEvent : public WebMouseEvent, public WebPointerProperties { On 2015/07/06 17:22:38, Rick Byers wrote: > Let's hold off adding WebPointerEvent until we have a concrete use for it. Done. https://codereview.chromium.org/1212113007/diff/1/public/web/WebMouseProperti... File public/web/WebMouseProperties.h (right): https://codereview.chromium.org/1212113007/diff/1/public/web/WebMouseProperti... public/web/WebMouseProperties.h:27: Button button; On 2015/07/06 17:22:38, Rick Byers wrote: > It feels a little odd to have WebMouseProperties in WebTouchPoint. The only > reason you've separated this out from WebPointerProperties was to avoid bloating > WebMouseEvent with a bunch of normally unused properties, right? Yes. > I wouldn't worry about that. I've done performance tests on the size of > WebTouchEvent and it took a LOT of additional space to have any impact on > performance. The objects are very short lived and there tends to only be a few > alive at any time, and usually no more than one created per frame. So the perf > impact of extra fields is low in practice. > > So how about we simplify all of this by having only one new class > 'WebPointerProperties' - the stuff that we've generalized from mouse events and > touch points along the path to having a unified pointer event. Done. https://codereview.chromium.org/1212113007/diff/1/public/web/WebPointerProper... File public/web/WebPointerProperties.h (right): https://codereview.chromium.org/1212113007/diff/1/public/web/WebPointerProper... public/web/WebPointerProperties.h:13: class WebPointerProperties { On 2015/07/06 17:22:38, Rick Byers wrote: > I like the idea of a common class for the properties in common between > WebMouseEvent (in the future WebPointerEvent) and WebTouchPoint. This doesn't > need to match Pointer Event semantics/names exactly though - whatever is most > convenient for blink/chromium is fine (it can be up to blink to do any necessary > conversions). Acknowledged. https://codereview.chromium.org/1212113007/diff/1/public/web/WebTouchPoint.h File public/web/WebTouchPoint.h (left): https://codereview.chromium.org/1212113007/diff/1/public/web/WebTouchPoint.h#... public/web/WebTouchPoint.h:66: float radiusY; On 2015/07/06 17:22:39, Rick Byers wrote: > You can't just replace radiusX/radiusY with width/height - they have different > semantics in the case of rotation. For now why not just keep them called > radiusX/radiusY? > > In fact, it might be simplest if the first CL doesn't change the name of ANY > fields, then we don't need to change chromium at all. We can always do specific > renames later if we want to change semantics, but it's not clear to me when > we'll actually need that. Done. https://codereview.chromium.org/1212113007/diff/1/public/web/WebTouchPoint.h File public/web/WebTouchPoint.h (right): https://codereview.chromium.org/1212113007/diff/1/public/web/WebTouchPoint.h#... public/web/WebTouchPoint.h:64: WebFloatPoint position; On 2015/07/06 17:22:39, Rick Byers wrote: > These co-ordinate properties could eventually be moved into WebPointerProperties > too. Don't try to do that here - there is some complexity (eg. point vs. > separate x/y members, floats vs. ints) but I don't expect it would be that hard > to do in a later CL. Maybe add a TODO saying they should move? Done.
mustaq@chromium.org changed reviewers: + mustaq@chromium.org
https://codereview.chromium.org/1212113007/diff/20001/public/web/WebTouchPoint.h File public/web/WebTouchPoint.h (right): https://codereview.chromium.org/1212113007/diff/20001/public/web/WebTouchPoin... public/web/WebTouchPoint.h:42: class WebTouchPoint : public WebPointerProperties { May be I am missing something, but I don't see how this inheritance could be helpful in incremental implementation. Could you please explain why you prefer this over a field of type WebPointerProperties here (and in WebMouseEvent)?
On 2015/07/07 18:23:11, mustaq wrote: > https://codereview.chromium.org/1212113007/diff/20001/public/web/WebTouchPoint.h > File public/web/WebTouchPoint.h (right): > > https://codereview.chromium.org/1212113007/diff/20001/public/web/WebTouchPoin... > public/web/WebTouchPoint.h:42: class WebTouchPoint : public WebPointerProperties > { > May be I am missing something, but I don't see how this inheritance could be > helpful in incremental implementation. Could you please explain why you prefer > this over a field of type WebPointerProperties here (and in WebMouseEvent)? To me the primary benefit is in reducing the amount of code that needs to change for this (in particular, avoiding changes to chromium and 3-sided patches). Aggregation would probably be a little cleaner, but probably not worth the much larger effort unless there's a more concrete benefit to be had.
LGTM with nits. Since this affects the IPC types, let's make sure both blink and chromium try jobs pass before attempting to land (instead of the default of running only blink tests). I'll keep them off now. https://codereview.chromium.org/1212113007/diff/20001/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/1212113007/diff/20001/public/web/WebInputEven... public/web/WebInputEvent.h:518: // TODO(e_hakkinen): Replace with WebPointerEvent. nit: Reference crbug.com/508283 https://codereview.chromium.org/1212113007/diff/20001/public/web/WebPointerPr... File public/web/WebPointerProperties.h (right): https://codereview.chromium.org/1212113007/diff/20001/public/web/WebPointerPr... public/web/WebPointerProperties.h:11: // WebTouchEvent and WebTouchPoint and merge this into nit: don't add extra space at the beginning of these two lines. https://codereview.chromium.org/1212113007/diff/20001/public/web/WebPointerPr... public/web/WebPointerProperties.h:13: class WebPointerProperties { Please add a high level description that describes the motivation here. Eg. "This class encapsulates the properties that are common between mouse events and touch points as we transition towards the unified pointer event model.".
The CQ bit was checked by rbyers@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/1212113007/20001
Also please reference a bug in your CL description - probably one for adding stylus support (blocking mustaq's master pointer events bug).
The CQ bit was unchecked by rbyers@chromium.org
https://codereview.chromium.org/1212113007/diff/20001/Source/web/WebInputEven... File Source/web/WebInputEvent.cpp (right): https://codereview.chromium.org/1212113007/diff/20001/Source/web/WebInputEven... Source/web/WebInputEvent.cpp:51: int pointerPropertiesData[4]; nit: I think it's preferable to just add 4 to the above. We don't care here where the size comes from, just the absolute value.
Will do soon. I am on the road without my git repos this week, but after that.
rbyers@chromium.org changed reviewers: + bokan@chromium.org
On 2015/07/09 08:02:58, e_hakkinen wrote: > Will do soon. I am on the road without my git repos this week, but after that. Sure, no rush. +bokan@ (who will also be back next week) for OWNERS in Source/web
https://codereview.chromium.org/1212113007/diff/20001/public/web/WebPointerPr... File public/web/WebPointerProperties.h (right): https://codereview.chromium.org/1212113007/diff/20001/public/web/WebPointerPr... public/web/WebPointerProperties.h:34: float radiusX; A quick suggestion around the touch orientation properties (radiusX + radiusY + rotationAngle): I think it makes the most sense to keep all three of these together because the angle is meaningless when we are unaware of non-uniform radii. We have two ways to fix this: (a) leave radiusX and radiusY in WebTouchPoint, or (b) move even rotationAngle from WebTouchPoint to here. I am slightly biased towards Option(b) because I believe we will ultimately reach there. But Option(a) also makes sense for our immediate goal (stylus support). Thoughts?
I have created crbug.com/508283 to track our progress towards our final goal. Please add the BUG reference in the CL description.
On 2015/07/09 17:47:01, mustaq wrote: > https://codereview.chromium.org/1212113007/diff/20001/public/web/WebPointerPr... > File public/web/WebPointerProperties.h (right): > > https://codereview.chromium.org/1212113007/diff/20001/public/web/WebPointerPr... > public/web/WebPointerProperties.h:34: float radiusX; > A quick suggestion around the touch orientation properties (radiusX + radiusY + > rotationAngle): I think it makes the most sense to keep all three of these > together because the angle is meaningless when we are unaware of non-uniform > radii. > > We have two ways to fix this: > (a) leave radiusX and radiusY in WebTouchPoint, or > (b) move even rotationAngle from WebTouchPoint to here. > > I am slightly biased towards Option(b) because I believe we will ultimately > reach there. But Option(a) also makes sense for our immediate goal (stylus > support). > > Thoughts? That's a good point. Stylus doesn't use radius / contact geometry at all, right? Then I'd say we leave it out of WebPointerProperties for now - no point needlessly bloating WebMouseEvent if we're not actually expecting to use it for stylus. Next step is probably to add pointerType and tilt to WebPointerProperties though, right?
Source/web lgtm Please link the patch to a bug by adding to the description: BUG=123456
https://codereview.chromium.org/1212113007/diff/20001/Source/web/WebInputEven... File Source/web/WebInputEvent.cpp (right): https://codereview.chromium.org/1212113007/diff/20001/Source/web/WebInputEven... Source/web/WebInputEvent.cpp:51: int pointerPropertiesData[4]; On 2015/07/09 00:50:23, Rick Byers (Out until 7-23) wrote: > nit: I think it's preferable to just add 4 to the above. We don't care here > where the size comes from, just the absolute value. Agreed
https://codereview.chromium.org/1212113007/diff/20001/Source/web/WebInputEven... File Source/web/WebInputEvent.cpp (right): https://codereview.chromium.org/1212113007/diff/20001/Source/web/WebInputEven... Source/web/WebInputEvent.cpp:50: int mouseData[10]; Actually, shouldn't this have shrunk? Since you removed the 'button' field?
I also moved WebPointerProperties.h from public/web/ to public/platform/ as that seems to be required so that I can later include that header file in PlatformTouch.h and reuse WebPointerProperties without violating include rules. Please correct me, if I am mistaken. https://codereview.chromium.org/1212113007/diff/20001/Source/web/WebInputEven... File Source/web/WebInputEvent.cpp (right): https://codereview.chromium.org/1212113007/diff/20001/Source/web/WebInputEven... Source/web/WebInputEvent.cpp:50: int mouseData[10]; On 2015/07/13 18:45:18, bokan wrote: > Actually, shouldn't this have shrunk? Since you removed the 'button' field? Yes, mouseData size should be 9 ints, mousePropertiesData size should be 1 int and pointerPropertiesData size should be 4 ints. But let's just merge all of them together. https://codereview.chromium.org/1212113007/diff/20001/Source/web/WebInputEven... Source/web/WebInputEvent.cpp:51: int pointerPropertiesData[4]; On 2015/07/09 00:50:23, Rick Byers (Out until 7-23) wrote: > nit: I think it's preferable to just add 4 to the above. We don't care here > where the size comes from, just the absolute value. Done. https://codereview.chromium.org/1212113007/diff/20001/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/1212113007/diff/20001/public/web/WebInputEven... public/web/WebInputEvent.h:518: // TODO(e_hakkinen): Replace with WebPointerEvent. On 2015/07/09 00:44:26, Rick Byers (Out until 7-23) wrote: > nit: Reference crbug.com/508283 Done. https://codereview.chromium.org/1212113007/diff/20001/public/web/WebPointerPr... File public/web/WebPointerProperties.h (right): https://codereview.chromium.org/1212113007/diff/20001/public/web/WebPointerPr... public/web/WebPointerProperties.h:11: // WebTouchEvent and WebTouchPoint and merge this into On 2015/07/09 00:44:26, Rick Byers (Out until 7-23) wrote: > nit: don't add extra space at the beginning of these two lines. Done. https://codereview.chromium.org/1212113007/diff/20001/public/web/WebPointerPr... public/web/WebPointerProperties.h:13: class WebPointerProperties { On 2015/07/09 00:44:27, Rick Byers (Out until 7-23) wrote: > Please add a high level description that describes the motivation here. Eg. > "This class encapsulates the properties that are common between mouse events and > touch points as we transition towards the unified pointer event model.". Done.
The CQ bit was checked by e.hakkinen@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1212113007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)
The CQ bit was checked by e.hakkinen@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1212113007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by e.hakkinen@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1212113007/#ps40001 (title: "Comments, do not touch radiusX/Y, relocate WebPointerProperties.h")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1212113007/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199091 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
