Bug 65707 – Chromium Mac: Enable rubber banding when scrolling

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

This bug is to track the work to enable rubber banding when scrolling.

Created attachment 102950 [details]
Patch to enable rubber banding on Mac Chromium.

[Nico Weber]

Comment on attachment 102950 [details]
Patch to enable rubber banding on Mac Chromium.

View in context: https://bugs.webkit.org/attachment.cgi?id=102950&action=review

fishd@ wants to review all CLs that touch the chromium webkit api.

> Source/WebKit/chromium/features.gypi:156
> +	  'ENABLE_RUBBER_BANDING=1',

indent looks funny

> Source/WebKit/chromium/public/WebInputEvent.h:328
> +    int x;

why not float?

> > Source/WebKit/chromium/public/WebInputEvent.h:328
> > +    int x;
> 
> why not float?

I've used the same types as WebMouseEvent.

Webkit2's counterpart to this also uses ints, see: Source/WebKit2/Shared/mac/WebEventFactory.mm

[Sailesh Agrawal]

Comment on attachment 102950 [details]
Patch to enable rubber banding on Mac Chromium.

View in context: https://bugs.webkit.org/attachment.cgi?id=102950&action=review

> Source/WebCore/platform/PlatformWheelEvent.h:218
> +#if PLATFORM(MAC) || PLATFORM(CHROMIUM)

Does using PLATFORM(CHROMIUM) mean that this is enabled on Chromium Windows as well? Do we want that? Should we just replace the entire #if statement with #if OS(MAC) ?

Created attachment 102953 [details]
Patch to enable rubber banding on Mac Chromium (v2)

Fixed indentation in .gypi file. Added links to bug in ChangeLogs.

Created attachment 102960 [details]
Patch

Updated patch to fix some style problems and changed ifdefs PLATFORM(MAC) || PLATFORM(CHROMIUM) to OS(MAC).

[WebKit Review Bot]

Attachment 102960 [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/WebKit/chromium/src/WebInputEventConversion.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 102960 [details]
Patch

Attachment 102960 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9304594

Created attachment 102968 [details]
Patch

[WebKit Review Bot]

Comment on attachment 102968 [details]
Patch

Attachment 102968 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9317060

[Darin Fisher (:fishd, Google)]

Comment on attachment 102968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102968&action=review

looks pretty good to me!

> Source/WebKit/chromium/public/WebInputEvent.h:328
> +    int x;

hmm... WebMouseEvent has these fields too.  maybe a WebMouseEvent should extend from WebGestureEvent, or maybe they should share some common base class?  or, maybe this is not worth it :)

Created attachment 102976 [details]
Patch

[WebKit Review Bot]

Comment on attachment 102976 [details]
Patch

Attachment 102976 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9306563

Created attachment 102980 [details]
Patch

[Mark Rowe (bdash)]

Comment on attachment 102980 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102980&action=review

> Source/WebCore/platform/PlatformWheelEvent.h:76
> +#if OS(DARWIN)

OS(DARWIN) is obviously not right here (and elsewhere).  Darwin is much lower level than mouse wheel events and the like.

Created attachment 103067 [details]
Patch

(In reply to comment #16)
> (From update of attachment 102980 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102980&action=review
> 
> > Source/WebCore/platform/PlatformWheelEvent.h:76
> > +#if OS(DARWIN)
> 
> OS(DARWIN) is obviously not right here (and elsewhere).  Darwin is much lower level than mouse wheel events and the like.

Changed to PLATFORM(MAC) || (PLATFORM(CHROMIUM) && OS(DARWIN)) since there's no OS(MAC).

[Dimitri Glazkov (Google)]

Comment on attachment 103067 [details]
Patch

ok.

[WebKit Review Bot]

Comment on attachment 103067 [details]
Patch

Clearing flags on attachment: 103067

Committed r92607: <http://trac.webkit.org/changeset/92607>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

Created attachment 103273 [details]
Patch

Created attachment 103275 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 103275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103275&action=review

> Source/WebKit/chromium/src/mac/WebInputEventFactory.mm:37
> +#if !defined(MAC_OS_X_VERSION_10_7) || (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7)

Shouldn't this be BUILDING_ON_LEOPARD, BUILDING_ON_SNOW_LEOPARD, et al. as defined in Platform.h?

[Sailesh Agrawal]

Comment on attachment 103275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103275&action=review

>> Source/WebKit/chromium/src/mac/WebInputEventFactory.mm:37
>> +#if !defined(MAC_OS_X_VERSION_10_7) || (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7)
> 
> Shouldn't this be BUILDING_ON_LEOPARD, BUILDING_ON_SNOW_LEOPARD, et al. as defined in Platform.h?

I think we did that back when 10.7 wasn't out yet. Now that 10.7 is out it's probably better to check for that explicitly.

(In reply to comment #24)
> (From update of attachment 103275 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103275&action=review
> 
> > Source/WebKit/chromium/src/mac/WebInputEventFactory.mm:37
> > +#if !defined(MAC_OS_X_VERSION_10_7) || (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7)
> 
> Shouldn't this be BUILDING_ON_LEOPARD, BUILDING_ON_SNOW_LEOPARD, et al. as defined in Platform.h?

I'm not sure what those are supposed to mean, but if I read them literally, then the answer is no.

The #ifdef should be based on which SDK is being targeted, rather than what system it's being built on.

For example, the patch failed to build on "Webkit Mac10.6" bot, but actually it failed because that bot was targeting the 10.5 SDK.

So assuming "BUILDING_ON_*" refers to the OS of the system doing the compile, then those macros are not appropriate...

Actually, I see that we do have TARGETING_LEOPARD/TARGETING_SNOW_LEOPARD but not TARGETING_LION macros, but there's a FIXME in Platform.h mentioning that they should be folded into OS().

Do you want me to change my patch to use those?

Created attachment 103290 [details]
Patch

Latest patch uses the TARGETTING_* macros, since they seemed to be preferred in WebKit.

[Nico Weber]

Comment on attachment 103290 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103290&action=review

> Source/WebKit/chromium/src/mac/WebInputEventFactory.mm:64
> +};

Hm, I'm getting build errors when compiling this with the 10.6 SDK. Do I need to do anything to make this work? (duplicate definition of NSEventTypeBeginGesture; prev definition in some system header)

[Nico Weber]

Comment on attachment 103275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103275&action=review

>>>> Source/WebKit/chromium/src/mac/WebInputEventFactory.mm:37
>>>> +#if !defined(MAC_OS_X_VERSION_10_7) || (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7)
>>> 
>>> Shouldn't this be BUILDING_ON_LEOPARD, BUILDING_ON_SNOW_LEOPARD, et al. as defined in Platform.h?
>> 
>> I think we did that back when 10.7 wasn't out yet. Now that 10.7 is out it's probably better to check for that explicitly.
> 
> I'm not sure what those are supposed to mean, but if I read them literally, then the answer is no.
> 
> The #ifdef should be based on which SDK is being targeted, rather than what system it's being built on.
> 
> For example, the patch failed to build on "Webkit Mac10.6" bot, but actually it failed because that bot was targeting the 10.5 SDK.
> 
> So assuming "BUILDING_ON_*" refers to the OS of the system doing the compile, then those macros are not appropriate...

The BUILDING_ON_* macros are defined here: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/JavaScriptCore/wtf/Platform.h&exact_package=chromium&q=TARGETING_LEOPARD&type=cs&l=390

I think this needs to be 

#if BUILDING_ON_LEOPARD || BUILDING_ON_SNOW_LEOPARD

here and

BUILDING_ON_LEOPARD

below.

[Nico Weber]

I just saw that this patch got reverted in https://bugs.webkit.org/show_bug.cgi?id=65865 – maybe because of this issue?

[Mark Mentovai]

Nico is correct. In WebKit, the BUILDING_ON macros refer to the SDK, the set of headers in use, and are set on the basis of MAC_OS_X_VERSION_MAX_ALLOWED. The TARGETING_ macros refer to the deployment target, the minimum supported runtime version, and are set on the basis of MAC_OS_X_VERSION_MIN_REQUIRED. The deployment target may identify an older release than the SDK or the same release as the SDK, but may never identify a newer release.

There isn’t a macro to identify the OS release of the system performing the build. In practice, this is wholly irrelevant. If you’re using the “system” SDK (as I think Apple does in their WebKit builds), the SDK version and the OS release are one and the same. For Chrome release builds, we use the 10.5 SDK and set a 10.5 deployment target, but don’t support 10.5 as a build platform and do our builds on 10.6.

Based on a quick look at Source/WebKit/chromium/src/mac/WebInputEventFactory.mm as in the proposed patch, the first preprocessor conditional should be BUILDING_ON_LEOPARD || BUILDING_ON_SNOW_LEOPARD, and the second should be BUILDING_ON_LEOPARD.

Created attachment 103353 [details]
Patch

One more try, now using BUILDING_ON_* macros.

[Mark Mentovai]

Comment on attachment 103353 [details]
Patch

LGTM

[Dimitri Glazkov (Google)]

Comment on attachment 103353 [details]
Patch

rs=me.

[Nico Weber]

(reopening for commit-bot)

[WebKit Review Bot]

Comment on attachment 103353 [details]
Patch

Clearing flags on attachment: 103353

Committed r92699: <http://trac.webkit.org/changeset/92699>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.