Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2226933004:
Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events (Closed)
|
Created:
4 years, 4 months ago by tapted Modified:
4 years, 4 months ago Reviewers:
Avi (use Gerrit) CC:
chromium-reviews, jam, darin-cc_chromium.org, dtapuska+chromiumwatch_chromium.org, tdresser+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events
Currently scrolling in MacViews with a "real" mouse wheel is too fast. A
multiplier is applied that maps the values used for
kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the
one used for MacViews was too big. Share the constant that content::
uses by moving it to ui/events/cocoa/cocoa_event_utils.h
Add a test to ensure the event types agree. Problem: the initializers on
NSEvent for creating test events are too limited. We also want to beef
up testing of scroll event streams for elastic scrolling and whatnot, so
move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for
creating test scroll events.
BUG=615948
Committed: https://crrev.com/315a111b6cdd928cbd0862dd97f5a98482dd0261
Committed: https://crrev.com/a3d3da13da743ee2b63e5e0fd45d899e45ce2979
Cr-Original-Commit-Position: refs/heads/master@{#411216}
Cr-Commit-Position: refs/heads/master@{#411275}
Patch Set 1 #Patch Set 2 : Nits, and just share the constant whynot #Patch Set 3 : Consolidate! #Patch Set 4 : nit diff #Patch Set 5 : Unconsolidate! NSApp sendEvent is too magical #
Total comments: 4
Patch Set 6 : Rebase for r411201 #
Messages
Total messages: 60 (44 generated)
The CQ bit was checked by tapted@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.
The CQ bit was checked by tapted@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.
The CQ bit was checked by tapted@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 tapted@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 ========== Use the same tick delta blah blah cl format works. BUG= ========== to ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. Then consolidate a bunch of stuff. E.g. another limitation was that NSEvent can't make middle-click events, so there was a (now redundant) codepath that used CGEvents for this, but it was unable to set an NSWindow. So, that's now resolved. BUG=615948 ==========
tapted@chromium.org changed reviewers: + avi@chromium.org
Description was changed from ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. Then consolidate a bunch of stuff. E.g. another limitation was that NSEvent can't make middle-click events, so there was a (now redundant) codepath that used CGEvents for this, but it was unable to set an NSWindow. So, that's now resolved. BUG=615948 ========== to ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant via cocoa_event_utils.h. Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. Then consolidate a bunch of stuff. E.g. another limitation was that NSEvent can't make middle-click events, so there was a (now redundant) codepath that used CGEvents for this, but it was unable to set an NSWindow. So, that's now resolved. BUG=615948 ==========
Patchset #5 (id:80001) has been deleted
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant via cocoa_event_utils.h. Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. Then consolidate a bunch of stuff. E.g. another limitation was that NSEvent can't make middle-click events, so there was a (now redundant) codepath that used CGEvents for this, but it was unable to set an NSWindow. So, that's now resolved. BUG=615948 ========== to ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant that content:: uses by moving it to ui/events/cocoa/cocoa_event_utils.h Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. Then consolidate a bunch of stuff. E.g. another limitation was that NSEvent can't make middle-click events, so there was a (now redundant) codepath that used CGEvents for this, but it was unable to set an NSWindow. So, that's now resolved. BUG=615948 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tapted@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 tapted@chromium.org to run a CQ dry run
Description was changed from ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant that content:: uses by moving it to ui/events/cocoa/cocoa_event_utils.h Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. Then consolidate a bunch of stuff. E.g. another limitation was that NSEvent can't make middle-click events, so there was a (now redundant) codepath that used CGEvents for this, but it was unable to set an NSWindow. So, that's now resolved. BUG=615948 ========== to ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant that content:: uses by moving it to ui/events/cocoa/cocoa_event_utils.h Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. BUG=615948 ==========
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Patchset #5 (id:120001) 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.
Hi Avi, please take a look
lgtm https://codereview.chromium.org/2226933004/diff/140001/ui/events/cocoa/cocoa_... File ui/events/cocoa/cocoa_event_utils.h (right): https://codereview.chromium.org/2226933004/diff/140001/ui/events/cocoa/cocoa_... ui/events/cocoa/cocoa_event_utils.h:15: constexpr double kScrollbarPixelsPerCocoaTick = 40.0; Is that the new preferred style? I thought enum or exported constant was the style. https://codereview.chromium.org/2226933004/diff/140001/ui/events/cocoa/events... File ui/events/cocoa/events_mac.mm (left): https://codereview.chromium.org/2226933004/diff/140001/ui/events/cocoa/events... ui/events/cocoa/events_mac.mm:144: [event hasPreciseScrollingDeltas]) { Yay 10.7!
Thanks avi! https://codereview.chromium.org/2226933004/diff/140001/ui/events/cocoa/cocoa_... File ui/events/cocoa/cocoa_event_utils.h (right): https://codereview.chromium.org/2226933004/diff/140001/ui/events/cocoa/cocoa_... ui/events/cocoa/cocoa_event_utils.h:15: constexpr double kScrollbarPixelsPerCocoaTick = 40.0; On 2016/08/10 15:17:29, Avi wrote: > Is that the new preferred style? I thought enum or exported constant was the > style. http://go/cppguide#Use_of_constexpr is pretty much "constexpr whenever you can" (also, e.g, http://go/cppguide#Static_and_Global_Variables ). E.g. it prevents the compiler slipping in a static initializer you don't want. Exporting is somewhat orthogonal -- it's still required if something takes the address -- constexpr doesn't prevent that but `enum` would. However, I avoided enum since I would have to change it to an integral constant (which I guess it could be... but wasn't before).
The CQ bit was checked by tapted@chromium.org
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 ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant that content:: uses by moving it to ui/events/cocoa/cocoa_event_utils.h Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. BUG=615948 ========== to ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant that content:: uses by moving it to ui/events/cocoa/cocoa_event_utils.h Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. BUG=615948 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant that content:: uses by moving it to ui/events/cocoa/cocoa_event_utils.h Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. BUG=615948 ========== to ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant that content:: uses by moving it to ui/events/cocoa/cocoa_event_utils.h Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. BUG=615948 Committed: https://crrev.com/315a111b6cdd928cbd0862dd97f5a98482dd0261 Cr-Commit-Position: refs/heads/master@{#411216} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/315a111b6cdd928cbd0862dd97f5a98482dd0261 Cr-Commit-Position: refs/heads/master@{#411216}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:140001) has been created in https://codereview.chromium.org/2234143003/ by tapted@chromium.org. The reason for reverting is: Somehow... this slipped through the trybots - web_input_event_builders_mac_unittest.mm:586:49:error: too many arguments to function call, expected 2, have 4.
Message was sent while issue was closed.
FYI: Findit try jobs (rerunning failed compile or tests) identified this CL at revision 411216 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
Description was changed from ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant that content:: uses by moving it to ui/events/cocoa/cocoa_event_utils.h Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. BUG=615948 Committed: https://crrev.com/315a111b6cdd928cbd0862dd97f5a98482dd0261 Cr-Commit-Position: refs/heads/master@{#411216} ========== to ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant that content:: uses by moving it to ui/events/cocoa/cocoa_event_utils.h Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. BUG=615948 Committed: https://crrev.com/315a111b6cdd928cbd0862dd97f5a98482dd0261 Cr-Commit-Position: refs/heads/master@{#411216} ==========
The CQ bit was checked by tapted@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...
https://codereview.chromium.org/2226933004/diff/140001/content/browser/render... File content/browser/renderer_host/input/web_input_event_builders_mac_unittest.mm (right): https://codereview.chromium.org/2226933004/diff/140001/content/browser/render... content/browser/renderer_host/input/web_input_event_builders_mac_unittest.mm:586: false, false); bleh - web_input_event_builders_mac.h doesn't change for year, but r411201 removed these can_rubberband args mere minutes before this landed :p. Fixed.
The CQ bit was unchecked by tapted@chromium.org
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org Link to the patchset: https://codereview.chromium.org/2226933004/#ps160001 (title: "Rebase for r411201")
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...)
The CQ bit was checked by tapted@chromium.org
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...)
The CQ bit was checked by tapted@chromium.org
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 ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant that content:: uses by moving it to ui/events/cocoa/cocoa_event_utils.h Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. BUG=615948 Committed: https://crrev.com/315a111b6cdd928cbd0862dd97f5a98482dd0261 Cr-Commit-Position: refs/heads/master@{#411216} ========== to ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant that content:: uses by moving it to ui/events/cocoa/cocoa_event_utils.h Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. BUG=615948 Committed: https://crrev.com/315a111b6cdd928cbd0862dd97f5a98482dd0261 Cr-Commit-Position: refs/heads/master@{#411216} ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant that content:: uses by moving it to ui/events/cocoa/cocoa_event_utils.h Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. BUG=615948 Committed: https://crrev.com/315a111b6cdd928cbd0862dd97f5a98482dd0261 Cr-Commit-Position: refs/heads/master@{#411216} ========== to ========== Mac: Share kScrollbarPixelsPerCocoaTick between ui:: and blink:: events Currently scrolling in MacViews with a "real" mouse wheel is too fast. A multiplier is applied that maps the values used for kCGScrollEventUnitLine to (effectively) kCGScrollEventUnitPixel, and the one used for MacViews was too big. Share the constant that content:: uses by moving it to ui/events/cocoa/cocoa_event_utils.h Add a test to ensure the event types agree. Problem: the initializers on NSEvent for creating test events are too limited. We also want to beef up testing of scroll event streams for elastic scrolling and whatnot, so move the stuff in events_mac_unittest.mm to cocoa_test_event_utils:: for creating test scroll events. BUG=615948 Committed: https://crrev.com/315a111b6cdd928cbd0862dd97f5a98482dd0261 Committed: https://crrev.com/a3d3da13da743ee2b63e5e0fd45d899e45ce2979 Cr-Original-Commit-Position: refs/heads/master@{#411216} Cr-Commit-Position: refs/heads/master@{#411275} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a3d3da13da743ee2b63e5e0fd45d899e45ce2979 Cr-Commit-Position: refs/heads/master@{#411275} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
