Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2289273002:
Eraser tool type plumbing from ui/events to web events and PPAPI. (Closed)
|
Created:
4 years, 3 months ago by denniskempin Modified:
4 years, 3 months ago CC:
binji+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, bradnelson+warch_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, dtapuska+chromiumwatch_chromium.org, dtapuska+blinkwatch_chromium.org, ihf+watch_chromium.org, jam, mlamouri+watch-content_chromium.org, nzolghadr+blinkwatch_chromium.org, piman+watch_chromium.org, tdresser+watch_chromium.org, teravest+watch_chromium.org, tzik, yusukes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis passes the recently added eraser tool type from ui events into
WebInputEvents. The eraser is added as a PointerType in
WebPointerProperties, which is also shared with PlatformInputEvents.
To add support for this pointer type in the PPAPI we will be adding
a new modifier flag, since the PPAPI does not have a concept of
pointer tool types.
BUG=636458
TEST=none
Committed: https://crrev.com/3ae4a08aca4ba20838ac94a7e63e37a9e0070fd2
Cr-Commit-Position: refs/heads/master@{#416141}
Patch Set 1 #
Total comments: 1
Patch Set 2 : cleanup #
Total comments: 17
Patch Set 3 : added modifier conversion to remove modifier from WebInputEvent #Patch Set 4 : add pen modifier and version uprev of ppapi #
Total comments: 5
Patch Set 5 : fixed nits #
Total comments: 1
Patch Set 6 : switched version to M55 #Patch Set 7 : removed TODOs from idl file #Patch Set 8 : The CQ works! We were stripping the SHIFT mod from NaCL events. Fixed. #
Messages
Total messages: 49 (19 generated)
Description was changed from ========== WIP: eraser plumbing BUG= ========== to ========== This passes the recently added eraser tool type from ui events into WebInputEvents. The eraser is added as a PointerType in WebPointerProperties, which is also shared with PlatformInputEvents. To add support for this pointer type in the PPAPI we will be adding a new modifier flag, since the PPAPI does not have a concept of pointer tool types. BUG=636458 TEST=none ==========
mustaq@chromium.org changed reviewers: + mustaq@chromium.org
https://codereview.chromium.org/2289273002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2289273002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebInputEvent.h:193: IsEraser = 1 << 19 Having both |IsEraser| and |PointerType::Eraser| looks redundant. May we have only one of these?
bradnelson@chromium.org changed reviewers: + bbudge@chromium.org
bradnelson@chromium.org changed reviewers: + bradnelson@chromium.org
Looks like the right general idea. +bbudge Bill what do you think, especially about the modifier value? https://codereview.chromium.org/2289273002/diff/20001/content/renderer/pepper... File content/renderer/pepper/event_conversion.cc (right): https://codereview.chromium.org/2289273002/diff/20001/content/renderer/pepper... content/renderer/pepper/event_conversion.cc:193: if (mouse_event.pointerType == blink::WebPointerProperties::PointerType::Eraser) { Keep it under 80 columns https://codereview.chromium.org/2289273002/diff/20001/ppapi/c/ppb_input_event.h File ppapi/c/ppb_input_event.h (right): https://codereview.chromium.org/2289273002/diff/20001/ppapi/c/ppb_input_event... ppapi/c/ppb_input_event.h:233: PP_INPUTEVENT_MOUSEBUTTON_ERASER = 3 Leftover, I thought the idea was you'd use the modifier? https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right): https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.cpp:24: case WebPointerProperties::PointerType::Eraser: This meant to be the same for both ends? https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:193: IsEraser = 1 << 19 Maybe keep this contiguous with the others? So the ppapi modifiers are contiguous (these probably don't care). We should ask bbudge, will add him.
https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right): https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.cpp:24: case WebPointerProperties::PointerType::Eraser: On 2016/08/30 18:19:29, bradnelson wrote: > This meant to be the same for both ends? This is for the web apis, which do not support eraser. They will eventually convert the eraser into a button. mustaq@ is looking at that. https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:193: IsEraser = 1 << 19 On 2016/08/30 18:19:29, bradnelson wrote: > Maybe keep this contiguous with the others? So the ppapi modifiers are > contiguous (these probably don't care). We should ask bbudge, will add him. I think ideally we would not need this flag here, it's really just here to make sure the PPAPI modifiers can stay in sync with the ui modifiers and won't end up conflicting after future changes to modifiers. the ui::Events never use this flag. Any alternative suggestions how we could do this?
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:193: IsEraser = 1 << 19 On 2016/08/30 at 18:27:41, denniskempin wrote: > On 2016/08/30 18:19:29, bradnelson wrote: > > Maybe keep this contiguous with the others? So the ppapi modifiers are > > contiguous (these probably don't care). We should ask bbudge, will add him. > > I think ideally we would not need this flag here, it's really just here to make sure the PPAPI modifiers can stay in sync with the ui modifiers and won't end up conflicting after future changes to modifiers. the ui::Events never use this flag. > Any alternative suggestions how we could do this? I'd break the contract in event_conversion.cpp of ppapi modifiers matching webinputevent modifiers. Is there a consumer of the ppapi for erasers? or is this just for completeness? I thought the primary use cases of pointer events was HTML based apps and not flash.
https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right): https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.cpp:24: case WebPointerProperties::PointerType::Eraser: On 2016/08/30 18:27:41, denniskempin wrote: > On 2016/08/30 18:19:29, bradnelson wrote: > > This meant to be the same for both ends? > > This is for the web apis, which do not support eraser. They will eventually > convert the eraser into a button. mustaq@ is looking at that. Yes, I will take care of Blink event handling. In general, we should have either the modifier bit or a separate pointer_type, and not both. From Blink side, we are fine with either solution but biasing slightly towards a separate type to emphasize the diff in code. Does PPAPI have any preference?
On 2016/08/30 19:13:51, mustaq wrote: > https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right): > > https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/events/PointerEventFactory.cpp:24: case > WebPointerProperties::PointerType::Eraser: > On 2016/08/30 18:27:41, denniskempin wrote: > > On 2016/08/30 18:19:29, bradnelson wrote: > > > This meant to be the same for both ends? > > > > This is for the web apis, which do not support eraser. They will eventually > > convert the eraser into a button. mustaq@ is looking at that. > > Yes, I will take care of Blink event handling. > > In general, we should have either the modifier bit or a separate pointer_type, > and not both. From Blink side, we are fine with either solution but biasing > slightly towards a separate type to emphasize the diff in code. > > Does PPAPI have any preference? dtapuska: Breaking the 1:1 mapping contract would probably be the easiest solution here. Does anyone feel strongly against that? mustaq: The PPAPI does not have the concept of a pointer type, so we are going to convert to a modifier there. However I agree with you on the bias towards a pointer type. It's a little cleaner, but also the way an eraser is handled in the the kernel input protocol, android and exosphere/wayland. Using a pointer type this should seamlessly work in Clank as well.
A modifier seems like the cleanest way to add this to Pepper, because of the API versioning problem. https://codereview.chromium.org/2289273002/diff/20001/content/renderer/pepper... File content/renderer/pepper/event_conversion.cc (right): https://codereview.chromium.org/2289273002/diff/20001/content/renderer/pepper... content/renderer/pepper/event_conversion.cc:85: "IsEraser should match"); Eliminate. https://codereview.chromium.org/2289273002/diff/20001/ppapi/c/ppb_input_event.h File ppapi/c/ppb_input_event.h (right): https://codereview.chromium.org/2289273002/diff/20001/ppapi/c/ppb_input_event... ppapi/c/ppb_input_event.h:6: /* From ppb_input_event.idl modified Thu Apr 3 14:52:10 2014. */ This file is auto-generated. Edit the IDL file instead. https://cs.chromium.org/chromium/src/ppapi/api/ppb_input_event.idl?q=ppb_inpu... In order to use this from NaCl, you will have to either version the input API (currently there are 1.0, 1.1, and 1.2) or make sure your plugin can only be invoked on versions of Chrome where you know the newer enumeration is supported. https://codereview.chromium.org/2289273002/diff/20001/ppapi/c/ppb_input_event... ppapi/c/ppb_input_event.h:219: PP_INPUTEVENT_MODIFIER_ISERASER = 1 << 19, Just define this as bit 13, since this isn't a modifier in the Webkit world. Pepper was designed before pointer events, so we need to mark mouse events with that extra information using modifiers. Given that, and as long as you're here, why not define PP_INPUTEVENT_MODIFIER_ISPEN as well?
https://codereview.chromium.org/2289273002/diff/20001/content/renderer/pepper... File content/renderer/pepper/event_conversion.cc (right): https://codereview.chromium.org/2289273002/diff/20001/content/renderer/pepper... content/renderer/pepper/event_conversion.cc:193: if (mouse_event.pointerType == blink::WebPointerProperties::PointerType::Eraser) { On 2016/08/30 18:19:29, bradnelson wrote: > Keep it under 80 columns Done. https://codereview.chromium.org/2289273002/diff/20001/ppapi/c/ppb_input_event.h File ppapi/c/ppb_input_event.h (right): https://codereview.chromium.org/2289273002/diff/20001/ppapi/c/ppb_input_event... ppapi/c/ppb_input_event.h:233: PP_INPUTEVENT_MOUSEBUTTON_ERASER = 3 On 2016/08/30 18:19:29, bradnelson wrote: > Leftover, I thought the idea was you'd use the modifier? Done. https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2289273002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:193: IsEraser = 1 << 19 On 2016/08/30 18:37:45, dtapuska wrote: > On 2016/08/30 at 18:27:41, denniskempin wrote: > > On 2016/08/30 18:19:29, bradnelson wrote: > > > Maybe keep this contiguous with the others? So the ppapi modifiers are > > > contiguous (these probably don't care). We should ask bbudge, will add him. > > > > I think ideally we would not need this flag here, it's really just here to > make sure the PPAPI modifiers can stay in sync with the ui modifiers and won't > end up conflicting after future changes to modifiers. the ui::Events never use > this flag. > > Any alternative suggestions how we could do this? > > I'd break the contract in event_conversion.cpp of ppapi modifiers matching > webinputevent modifiers. > > Is there a consumer of the ppapi for erasers? or is this just for completeness? > I thought the primary use cases of pointer events was HTML based apps and not > flash. Done.
https://codereview.chromium.org/2289273002/diff/20001/content/renderer/pepper... File content/renderer/pepper/event_conversion.cc (right): https://codereview.chromium.org/2289273002/diff/20001/content/renderer/pepper... content/renderer/pepper/event_conversion.cc:85: "IsEraser should match"); On 2016/08/30 20:46:36, bbudge wrote: > Eliminate. Done. https://codereview.chromium.org/2289273002/diff/20001/ppapi/c/ppb_input_event.h File ppapi/c/ppb_input_event.h (right): https://codereview.chromium.org/2289273002/diff/20001/ppapi/c/ppb_input_event... ppapi/c/ppb_input_event.h:6: /* From ppb_input_event.idl modified Thu Apr 3 14:52:10 2014. */ On 2016/08/30 20:46:36, bbudge wrote: > This file is auto-generated. Edit the IDL file instead. > https://cs.chromium.org/chromium/src/ppapi/api/ppb_input_event.idl?q=ppb_inpu... > > In order to use this from NaCl, you will have to either version the input API > (currently there are 1.0, 1.1, and 1.2) or make sure your plugin can only be > invoked on versions of Chrome where you know the newer enumeration is supported. Done. https://codereview.chromium.org/2289273002/diff/20001/ppapi/c/ppb_input_event... ppapi/c/ppb_input_event.h:219: PP_INPUTEVENT_MODIFIER_ISERASER = 1 << 19, On 2016/08/30 20:46:37, bbudge wrote: > Just define this as bit 13, since this isn't a modifier in the Webkit world. > > Pepper was designed before pointer events, so we need to mark mouse events with > that extra information using modifiers. Given that, and as long as you're here, > why not define PP_INPUTEVENT_MODIFIER_ISPEN as well? Done. Added ISPEN as well.
https://codereview.chromium.org/2289273002/diff/60001/ppapi/api/ppb_input_eve... File ppapi/api/ppb_input_event.idl (right): https://codereview.chromium.org/2289273002/diff/60001/ppapi/api/ppb_input_eve... ppapi/api/ppb_input_event.idl:14: M54 = 1.3 This doesn't actually version the API I'm going to reverse myself on this. To properly version this API would be difficult, and the chance of this change breaking an existing app seems remote (basically an app would have to be depending on these modifier bits being zero.) A plugin that knows about these modifiers should run fine on older versions of Chrome - it would just receive unmodified mouse events, which it should be able to handle in any case. On versions of Chrome that do support these bits, the app would be able to detect that and handle those modified events differently. Since we don't really support versioning enum values (the code generator just ignores those) you'd have to version the PPP_InputEvent interface, which is how plugins receive events. Versioning PPP interfaces is more difficult. You would have to modify a lot of Pepper code and check for API versions in the Pepper proxy. So this is fine as it is. It would probably be a good idea to annotate the new values below with the version. See this CL which added a flag to the PPB_FileIO API (which was technically a breaking change to a Stable API): https://chromiumcodereview.appspot.com/16765005 https://codereview.chromium.org/2289273002/diff/60001/ppapi/api/ppb_input_eve... ppapi/api/ppb_input_event.idl:205: PP_INPUTEVENT_MODIFIER_ISERASER = 1 << 14 Annotate these both with [version=1.3] even though it's essentially a no-op.
PointerEventFactory.cpp and WebPointerProperties.h LGTM. https://codereview.chromium.org/2289273002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right): https://codereview.chromium.org/2289273002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.cpp:24: case WebPointerProperties::PointerType::Eraser: Please add a TODO referring to crbug.com/642455.
https://codereview.chromium.org/2289273002/diff/60001/ppapi/api/ppb_input_eve... File ppapi/api/ppb_input_event.idl (right): https://codereview.chromium.org/2289273002/diff/60001/ppapi/api/ppb_input_eve... ppapi/api/ppb_input_event.idl:14: M54 = 1.3 On 2016/08/30 22:05:54, bbudge wrote: > This doesn't actually version the API > > I'm going to reverse myself on this. To properly version this API would be > difficult, and the chance of this change breaking an existing app seems remote > (basically an app would have to be depending on these modifier bits being zero.) > > A plugin that knows about these modifiers should run fine on older versions of > Chrome - it would just receive unmodified mouse events, which it should be able > to handle in any case. On versions of Chrome that do support these bits, the app > would be able to detect that and handle those modified events differently. > > Since we don't really support versioning enum values (the code generator just > ignores those) you'd have to version the PPP_InputEvent interface, which is how > plugins receive events. Versioning PPP interfaces is more difficult. You would > have to modify a lot of Pepper code and check for API versions in the Pepper > proxy. > > So this is fine as it is. It would probably be a good idea to annotate the new > values below with the version. See this CL which added a flag to the PPB_FileIO > API (which was technically a breaking change to a Stable API): > > https://chromiumcodereview.appspot.com/16765005 There is one problem that I could see: Older versions of chrome do not filter the modifier flags. ISPEN and ISERASER will be mapped to IsTouchAccessibility or IsComposing of WebInputEvent::Modifiers. So running an app that uses the PEN/ERASER flags might break in weird ways in older versions of chrome. Can the app query this input API version and disable checking for the eraser flag? I have never used PPAPI myself. I will add the version flags to the enum values below. https://codereview.chromium.org/2289273002/diff/60001/ppapi/api/ppb_input_eve... ppapi/api/ppb_input_event.idl:205: PP_INPUTEVENT_MODIFIER_ISERASER = 1 << 14 On 2016/08/30 22:05:54, bbudge wrote: > Annotate these both with [version=1.3] even though it's essentially a no-op. Acknowledged.
denniskempin@google.com changed reviewers: + bokan@chromium.org, rbyers@google.com
rbyers@chromium.org changed reviewers: + rbyers@chromium.org
LGTM
On 2016/09/01 at 17:51:03, rbyers wrote: > LGTM lgtm
Pepper lgtm
https://codereview.chromium.org/2289273002/diff/80001/ppapi/api/ppb_input_eve... File ppapi/api/ppb_input_event.idl (right): https://codereview.chromium.org/2289273002/diff/80001/ppapi/api/ppb_input_eve... ppapi/api/ppb_input_event.idl:14: M54 = 1.3 Sorry, since M54 has already branched, this should be M55
lgtm
Thanks a lot everyone. I think we're good to go here.
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org, dtapuska@chromium.org, bbudge@chromium.org, bokan@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2289273002/#ps100001 (title: "switched version to M55")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/09/01 18:36:36, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) I think it would be OK to delete the TODOs for brettw in the IDL file so you can pass the presubmit check.
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, bbudge@chromium.org, bokan@chromium.org, rbyers@chromium.org, mustaq@chromium.org Link to the patchset: https://codereview.chromium.org/2289273002/#ps120001 (title: "removed TODOs from idl file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
failure doesn't look related. retrying.
The CQ bit was checked by denniskempin@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, bbudge@chromium.org, bokan@chromium.org, rbyers@chromium.org, mustaq@chromium.org Link to the patchset: https://codereview.chromium.org/2289273002/#ps140001 (title: "The CQ works! We were stripping the SHIFT mod from NaCL events. Fixed.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== This passes the recently added eraser tool type from ui events into WebInputEvents. The eraser is added as a PointerType in WebPointerProperties, which is also shared with PlatformInputEvents. To add support for this pointer type in the PPAPI we will be adding a new modifier flag, since the PPAPI does not have a concept of pointer tool types. BUG=636458 TEST=none ========== to ========== This passes the recently added eraser tool type from ui events into WebInputEvents. The eraser is added as a PointerType in WebPointerProperties, which is also shared with PlatformInputEvents. To add support for this pointer type in the PPAPI we will be adding a new modifier flag, since the PPAPI does not have a concept of pointer tool types. BUG=636458 TEST=none ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== This passes the recently added eraser tool type from ui events into WebInputEvents. The eraser is added as a PointerType in WebPointerProperties, which is also shared with PlatformInputEvents. To add support for this pointer type in the PPAPI we will be adding a new modifier flag, since the PPAPI does not have a concept of pointer tool types. BUG=636458 TEST=none ========== to ========== This passes the recently added eraser tool type from ui events into WebInputEvents. The eraser is added as a PointerType in WebPointerProperties, which is also shared with PlatformInputEvents. To add support for this pointer type in the PPAPI we will be adding a new modifier flag, since the PPAPI does not have a concept of pointer tool types. BUG=636458 TEST=none Committed: https://crrev.com/3ae4a08aca4ba20838ac94a7e63e37a9e0070fd2 Cr-Commit-Position: refs/heads/master@{#416141} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/3ae4a08aca4ba20838ac94a7e63e37a9e0070fd2 Cr-Commit-Position: refs/heads/master@{#416141} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
