[chromium] Add flag to enable/disable touch adjustment at runtime.
Created attachment 174717 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 174717 [details] Patch I'm not terribly familiar with this feature - why does it make sense to adjust this on a per-process basis instead of something per WebView? When would an embedder want to toggle this behavior?
(In reply to comment #3) > (From update of attachment 174717 [details]) > I'm not terribly familiar with this feature - why does it make sense to adjust this on a per-process basis instead of something per WebView? When would an embedder want to toggle this behavior? The primary use of this feature is for the chrome developer to diagnose problems with touch. To this end, following the same pattern used for enabling/disabling touch events.
Created attachment 175237 [details] Patch
Comment on attachment 175237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175237&action=review Looks good overall. > Source/WebCore/page/EventHandler.cpp:2706 > +bool EventHandler::touchAdjustmentEnabled(const PlatformGestureEvent& event) > +{ > + if (m_frame->settings() && !m_frame->settings()->touchAdjustmentEnabled()) > + return false; > + return !event.area().isEmpty(); > +} With the combination of checking both the flag and if gesture events come with an area I am not sure touchAdjustmentEnabled is the right word. Perhaps shouldApplyTouchAdjustment, or something similar.
Comment on attachment 175237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175237&action=review > Source/WebCore/page/Settings.h:295 > +#if ENABLE(TOUCH_ADJUSTMENT) > + void setEnableTouchAdjustment(bool flag) { m_touchAdjustmentEnabled = flag; } > + bool touchAdjustmentEnabled() const { return m_touchAdjustmentEnabled; } > +#endif Please use Settings.in rather than creating this setting manually.
Created attachment 175256 [details] Patch
Created attachment 175267 [details] Patch
Comment on attachment 175237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175237&action=review >> Source/WebCore/page/EventHandler.cpp:2706 >> +} > > With the combination of checking both the flag and if gesture events come with an area I am not sure touchAdjustmentEnabled is the right word. Perhaps shouldApplyTouchAdjustment, or something similar. Done. >> Source/WebCore/page/Settings.h:295 >> +#endif > > Please use Settings.in rather than creating this setting manually. Done.
Attachment 175267 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:21: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 175274 [details] Patch
Comment on attachment 175274 [details] Patch LGTM, but not a reviewer.
Comment on attachment 175274 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175274&action=review Looks good. One minor issue, one request and one question. would like to see another round before it lands, then the r-. > Source/WebCore/ChangeLog:9 > + [chromium] Add flag to enable/disable touch adjustment at runtime. > + https://bugs.webkit.org/show_bug.cgi?id=102534 > + > + Reviewed by NOBODY (OOPS!). > + > + Add setting for enabling or disabling touch adjustment. > + You should mention here why the compile time only flag is not enough. Things like when compile-time would be enabled, but the run-time setting disable, etc. > Source/WebCore/page/EventHandler.cpp:2701 > +bool EventHandler::shouldApplyTouchAdjustment(const PlatformGestureEvent& event) const? > Source/WebCore/page/Settings.in:143 > +touchAdjustmentEnabled initial=true default true? How does it work when the compile flag is disabled? Do we care? Found it a bit odd ...
Created attachment 175425 [details] Patch
Comment on attachment 175274 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175274&action=review >> Source/WebCore/ChangeLog:9 >> + > > You should mention here why the compile time only flag is not enough. Things like when compile-time would be enabled, but the run-time setting disable, etc. Done. >> Source/WebCore/page/EventHandler.cpp:2701 >> +bool EventHandler::shouldApplyTouchAdjustment(const PlatformGestureEvent& event) > > const? Done. >> Source/WebCore/page/Settings.in:143 >> +touchAdjustmentEnabled initial=true > > default true? How does it work when the compile flag is disabled? Do we care? Found it a bit odd ... Added comment to indicate that both the compile option is also required to enable touch adjustment. This option provides dynamic control to disable touch adjustment as required at run-time.
Comment on attachment 175425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175425&action=review > Source/WebCore/page/EventHandler.cpp:2750 > bool EventHandler::adjustGesturePosition(const PlatformGestureEvent& gestureEvent, IntPoint& adjustedPoint) did not check all call sites, but this returned bool seems not too useful.
Comment on attachment 175425 [details] Patch API change LGTM
Comment on attachment 175425 [details] Patch Clearing flags on attachment: 175425 Committed r135414: <http://trac.webkit.org/changeset/135414>
All reviewed patches have been landed. Closing bug.
Comment on attachment 175274 [details] Patch Cleared review? from obsolete attachment 175274 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).