Bug 102534 – [chromium] Add flag to enable/disable touch adjustment at runtime.

• Component: New Bugs
• Status: RESOLVED FIXED

[Kevin Ellis]

[chromium] Add flag to enable/disable touch adjustment at runtime.

[Kevin Ellis]

Created attachment 174717 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

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?

[Kevin Ellis]

(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.

[Kevin Ellis]

Created attachment 175237 [details]
Patch

[Allan Sandfeld Jensen]

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.

[Adam Barth]

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.

[Kevin Ellis]

Created attachment 175256 [details]
Patch

[Kevin Ellis]

Created attachment 175267 [details]
Patch

[Kevin Ellis]

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.

[WebKit Review Bot]

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.

[Kevin Ellis]

Created attachment 175274 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 175274 [details]
Patch

LGTM, but not a reviewer.

[Antonio Gomes]

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 ...

[Kevin Ellis]

Created attachment 175425 [details]
Patch

[Kevin Ellis]

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.

[Antonio Gomes]

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.

[Adam Barth]

Comment on attachment 175425 [details]
Patch

API change LGTM

[WebKit Review Bot]

Comment on attachment 175425 [details]
Patch

Clearing flags on attachment: 175425

Committed r135414: <http://trac.webkit.org/changeset/135414>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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).