|
|
Created:
3 years, 9 months ago by lanwei Modified:
3 years, 9 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, dtapuska+chromiumwatch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, mac-reviews_chromium.org, James Su Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake stylus's eraser button work on Mac
In order to make stylus's eraser button work on Mac, I set the pointer type to
blink::WebPointerProperties::PointerType::Eraser when we use the eraser button on the tablet.
BUG=685252
Review-Url: https://codereview.chromium.org/2739773004
Cr-Commit-Position: refs/heads/master@{#459166}
Committed: https://chromium.googlesource.com/chromium/src/+/45def6b3f18b803e4ea050a84ba8fecbc786b6df
Patch Set 1 #
Total comments: 5
Patch Set 2 : eraser button #Patch Set 3 : Add unittests #
Total comments: 4
Patch Set 4 : refactor tests #
Total comments: 2
Messages
Total messages: 53 (34 generated)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== eraser button BUG= ========== to ========== Make stylus's eraser button work on Mac In order to make stylus's eraser button work on Mac, I set the pointer type to blink::WebPointerProperties::PointerType::Eraser when we use the eraser button on the tablet. BUG=685252 ==========
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org
lanwei@chromium.org changed reviewers: + nzolghadr@chromium.org - dtapuska@chromium.org
Description was changed from ========== Make stylus's eraser button work on Mac In order to make stylus's eraser button work on Mac, I set the pointer type to blink::WebPointerProperties::PointerType::Eraser when we use the eraser button on the tablet. BUG=685252 ========== to ========== Make stylus's eraser button work on Mac In order to make stylus's eraser button work on Mac, I set the pointer type to blink::WebPointerProperties::PointerType::Eraser when we use the eraser button on the tablet. BUG=685252 ==========
Description was changed from ========== Make stylus's eraser button work on Mac In order to make stylus's eraser button work on Mac, I set the pointer type to blink::WebPointerProperties::PointerType::Eraser when we use the eraser button on the tablet. BUG=685252 ========== to ========== Make stylus's eraser button work on Mac In order to make stylus's eraser button work on Mac, I set the pointer type to blink::WebPointerProperties::PointerType::Eraser when we use the eraser button on the tablet. BUG=685252 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2739773004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2739773004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:1915: pointerType_ = isStylusEnteringProximity_ Can we just use an if statement here and not set pointerType_ if stylus was not in proximity? https://codereview.chromium.org/2739773004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:1922: pointerType_ = blink::WebPointerProperties::PointerType::Mouse; Why did we make this change here along with the eraser pointertype? Is that related?
https://codereview.chromium.org/2739773004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2739773004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:1915: pointerType_ = isStylusEnteringProximity_ On 2017/03/08 19:30:27, Navid Zolghadr wrote: > Can we just use an if statement here and not set pointerType_ if stylus was not > in proximity? I thought it was clearer to set the pointer type for all cases in this class, so we do not need to worry about it anymore. https://codereview.chromium.org/2739773004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:1922: pointerType_ = blink::WebPointerProperties::PointerType::Mouse; On 2017/03/08 19:30:27, Navid Zolghadr wrote: > Why did we make this change here along with the eraser pointertype? Is that > related? I moved all the logic of setting the pointer type here in this class.
lgtm
https://codereview.chromium.org/2739773004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/web_input_event_builders_mac.mm (right): https://codereview.chromium.org/2739773004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/web_input_event_builders_mac.mm:325: // For NSMouseExited and NSMouseEntered events, they do not have a subtype. This comment and the comment in the next block no longer applies here. - I think the other file covers these comments now but not sure. Please double-check. - Combine the two |if|s below into one: return early if NSMouseExited || NSMouseEntered || (not NSTabletPointEventSubtype && !NSTabletProximityEventSubtype).
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
lgtm
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org
lanwei@chromium.org changed reviewers: - dtapuska@chromium.org
lanwei@chromium.org changed reviewers: + tdresser@chromium.org
Tim, could you please take a look, thank you!
Can we write tests for this?
On 2017/03/14 14:54:59, tdresser wrote: > Can we write tests for this? I searched that there is no test for NSEvent of mouse event. From Mac documentation, they do not provide a constructor for tablet events, and the mouse event's constructor does not have subtype, so we cannot create the NSEvent we want to test.
The approach described here should work: http://stackoverflow.com/questions/6126226/how-to-create-an-nsevent-of-type-n...
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Patchset #3 (id:60001) has been deleted
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2739773004/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm (right): https://codereview.chromium.org/2739773004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:154: result += " "; This method appears to never output multiple pointer types currently. Do we need this logic? Do we need to return strings? Could we just return the WebPointerProperties::PointerType? https://codereview.chromium.org/2739773004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1068: CFRelease(cg_event); Could we wrap the logic for creating these events in a helper function?
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2739773004/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm (right): https://codereview.chromium.org/2739773004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:154: result += " "; On 2017/03/22 15:13:26, tdresser wrote: > This method appears to never output multiple pointer types currently. Do we need > this logic? > > Do we need to return strings? > > Could we just return the WebPointerProperties::PointerType? Done. https://codereview.chromium.org/2739773004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1068: CFRelease(cg_event); On 2017/03/22 15:13:26, tdresser wrote: > Could we wrap the logic for creating these events in a helper function? Done.
Thanks for the tests. LGTM with question. https://codereview.chromium.org/2739773004/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm (right): https://codereview.chromium.org/2739773004/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:155: NSEvent* event = [NSEvent eventWithCGEvent:cg_event]; Pardon my not knowing much about Mac development. How is the memory for this managed?
https://codereview.chromium.org/2739773004/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm (right): https://codereview.chromium.org/2739773004/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:155: NSEvent* event = [NSEvent eventWithCGEvent:cg_event]; On 2017/03/23 12:16:54, tdresser wrote: > Pardon my not knowing much about Mac development. How is the memory for this > managed? Actually, I am not sure. I saw other tests all did at this way. I thought the ScopedNSAutoreleasePool inside RenderWidgetHostViewMacTest will clean up the pointers. Or we leak them. https://codesearch.chromium.org/chromium/src/content/browser/renderer_host/re...
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nzolghadr@chromium.org, mustaq@chromium.org Link to the patchset: https://codereview.chromium.org/2739773004/#ps100001 (title: "refactor tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/23 18:55:16, lanwei wrote: > https://codereview.chromium.org/2739773004/diff/100001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm > (right): > > https://codereview.chromium.org/2739773004/diff/100001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:155: > NSEvent* event = [NSEvent eventWithCGEvent:cg_event]; > On 2017/03/23 12:16:54, tdresser wrote: > > Pardon my not knowing much about Mac development. How is the memory for this > > managed? > > Actually, I am not sure. I saw other tests all did at this way. I thought the > ScopedNSAutoreleasePool inside RenderWidgetHostViewMacTest will clean up the > pointers. Or we leak them. > https://codesearch.chromium.org/chromium/src/content/browser/renderer_host/re... Thanks, I think you're right that the ScopedNSAutoreleasePool deals with this. Still LGTM.
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490295327158560, "parent_rev": "4752ee127654fcb5683a285f0990a47f4d8c8397", "commit_rev": "45def6b3f18b803e4ea050a84ba8fecbc786b6df"}
Message was sent while issue was closed.
Description was changed from ========== Make stylus's eraser button work on Mac In order to make stylus's eraser button work on Mac, I set the pointer type to blink::WebPointerProperties::PointerType::Eraser when we use the eraser button on the tablet. BUG=685252 ========== to ========== Make stylus's eraser button work on Mac In order to make stylus's eraser button work on Mac, I set the pointer type to blink::WebPointerProperties::PointerType::Eraser when we use the eraser button on the tablet. BUG=685252 Review-Url: https://codereview.chromium.org/2739773004 Cr-Commit-Position: refs/heads/master@{#459166} Committed: https://chromium.googlesource.com/chromium/src/+/45def6b3f18b803e4ea050a84ba8... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/45def6b3f18b803e4ea050a84ba8... |