feat: add touchpad scroll speed settings slider#338
Conversation
|
Reviews (1): Last reviewed commit: "feat: add touchpad scroll speed settings..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| + if (event->type() == ui::EventType::kScroll) { | ||
| + double multiplier = GetContentClient()->browser()->GetTouchpadScrollSpeedMultiplier( | ||
| + host()->GetProcess()->GetBrowserContext()); | ||
| + if (multiplier != 1.0) { | ||
| + event->Scale(static_cast<float>(multiplier)); | ||
| + } | ||
| + } |
There was a problem hiding this comment.
kScroll intercept is not limited to touchpad input
ui::EventType::kScroll covers both touchpad two-finger scroll events and, on Wayland, mouse wheel axis events (wl_pointer::axis). The Ozone/Wayland backend converts both sources to ui::ScrollEvent with type kScroll, so users with a physical mouse attached will find their wheel scroll speed scaled by this preference as well, contradicting the "Touchpad scroll speed" label. A finger_count() > 0 guard (which libinput/Ozone sets for genuine touchpad contact) would restrict scaling to touchpad input only.
| +#include "chrome/common/pref_names.h" | ||
| +#include "components/prefs/pref_service.h" |
There was a problem hiding this comment.
#include directives placed mid-file
chrome/common/pref_names.h and components/prefs/pref_service.h are inserted at source line 3723 of chrome_content_browser_client.cc, after the body of an existing function. Chromium's style guide and clang-format require all #include directives to appear at the top of the translation unit. Because this file heavily uses Profile and PrefService, these headers are almost certainly already present via transitive includes, but the mid-file placement will be flagged by Chrome's PRESUBMIT checks and is confusing to readers. They should be added in the file's existing include block at the top.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| + double multiplier = GetContentClient()->browser()->GetTouchpadScrollSpeedMultiplier( | ||
| + host()->GetProcess()->GetBrowserContext()); |
There was a problem hiding this comment.
GetTouchpadScrollSpeedMultiplier is called on every single kScroll event, incurring two virtual dispatches (GetContentClient()->browser() and GetTouchpadScrollSpeedMultiplier()) plus a PrefService map lookup. While the pref lookup is O(1), touchpad scroll events fire continuously at the OS report rate (often 60–240 Hz). Caching the multiplier value (e.g. via a PrefChangeRegistrar observer on RenderWidgetHostViewAura or a process-level cache) would eliminate this overhead on the hot path.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
For your pull request to not get closed without review, please confirm that:
Resolves [FR]: Customizable (Touchpad) Scroll Speed on Linux #307.
otherwise I have marked my PR as draft.
organization if I lied by checking any of these checkboxes.
no way be applied to other platforms.
This PR adds a "Touchpad scroll speed" settings slider (ranging from 5% to 300%, defaulting to 100%) in
chrome://settings-> "Appearance and behavior".Implementation:
helium.browser.touchpad_scroll_speed_multiplier.<settings-slider>in the settings WebUI.ContentBrowserClient.RenderWidgetHostViewAura::OnScrollEvent).Why this approach:
Scaling the resolved logical scroll delta inside Aura ensures that both main-thread and compositor-thread (smooth) touchpad scrolling are correctly adjusted. This avoids having to intercept events inside Blink's main thread (which bypasses smooth compositor scrolls) or low-level Ozone-Wayland event pollution (which risks breaking absolute hit-testing and pinch-to-zoom gestures).
Testing:
(Note: I used an AI coding assistant to help generate the boilerplate C++/WebUI preference registrations, but verified and tested the changes manually).