Bug 65707

Summary: Chromium Mac: Enable rubber banding when scrolling
Product: WebKit Reporter: asvitkine
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fishd, mark, sail, thakis, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: Unspecified   
Bug Depends on: 65865    
Bug Blocks:    
Attachments:
Description Flags
Patch to enable rubber banding on Mac Chromium.
none
Patch to enable rubber banding on Mac Chromium (v2)
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description asvitkine 2011-08-04 11:24:24 PDT
This bug is to track the work to enable rubber banding when scrolling.
Comment 1 asvitkine 2011-08-04 11:37:29 PDT
Created attachment 102950 [details]
Patch to enable rubber banding on Mac Chromium.
Comment 2 Nico Weber 2011-08-04 11:41:35 PDT
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?
Comment 3 asvitkine 2011-08-04 11:49:43 PDT
> > 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
Comment 4 Sailesh Agrawal 2011-08-04 11:52:35 PDT
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) ?
Comment 5 asvitkine 2011-08-04 11:53:13 PDT
Created attachment 102953 [details]
Patch to enable rubber banding on Mac Chromium (v2)

Fixed indentation in .gypi file. Added links to bug in ChangeLogs.
Comment 6 asvitkine 2011-08-04 12:49:49 PDT
Created attachment 102960 [details]
Patch
Comment 7 asvitkine 2011-08-04 12:51:04 PDT
Updated patch to fix some style problems and changed ifdefs PLATFORM(MAC) || PLATFORM(CHROMIUM) to OS(MAC).
Comment 8 WebKit Review Bot 2011-08-04 12:52:02 PDT
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.
Comment 9 WebKit Review Bot 2011-08-04 13:10:49 PDT
Comment on attachment 102960 [details]
Patch

Attachment 102960 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9304594
Comment 10 asvitkine 2011-08-04 13:28:55 PDT
Created attachment 102968 [details]
Patch
Comment 11 WebKit Review Bot 2011-08-04 13:54:11 PDT
Comment on attachment 102968 [details]
Patch

Attachment 102968 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9317060
Comment 12 Darin Fisher (:fishd, Google) 2011-08-04 14:00:58 PDT
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 :)
Comment 13 asvitkine 2011-08-04 14:02:57 PDT
Created attachment 102976 [details]
Patch
Comment 14 WebKit Review Bot 2011-08-04 14:10:36 PDT
Comment on attachment 102976 [details]
Patch

Attachment 102976 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9306563
Comment 15 asvitkine 2011-08-04 14:25:16 PDT
Created attachment 102980 [details]
Patch
Comment 16 Mark Rowe (bdash) 2011-08-04 15:43:22 PDT
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.
Comment 17 asvitkine 2011-08-05 06:44:35 PDT
Created attachment 103067 [details]
Patch
Comment 18 asvitkine 2011-08-05 06:48:41 PDT
(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).
Comment 19 Dimitri Glazkov (Google) 2011-08-05 14:06:10 PDT
Comment on attachment 103067 [details]
Patch

ok.
Comment 20 WebKit Review Bot 2011-08-08 10:56:53 PDT
Comment on attachment 103067 [details]
Patch

Clearing flags on attachment: 103067

Committed r92607: <http://trac.webkit.org/changeset/92607>
Comment 21 WebKit Review Bot 2011-08-08 10:56:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 asvitkine 2011-08-08 12:03:19 PDT
Created attachment 103273 [details]
Patch
Comment 23 asvitkine 2011-08-08 12:22:34 PDT
Created attachment 103275 [details]
Patch
Comment 24 Dimitri Glazkov (Google) 2011-08-08 12:23:33 PDT
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?
Comment 25 Sailesh Agrawal 2011-08-08 12:25:50 PDT
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.
Comment 26 asvitkine 2011-08-08 12:29:21 PDT
(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...
Comment 27 asvitkine 2011-08-08 12:44:58 PDT
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?
Comment 28 asvitkine 2011-08-08 14:12:00 PDT
Created attachment 103290 [details]
Patch
Comment 29 asvitkine 2011-08-08 14:22:59 PDT
Latest patch uses the TARGETTING_* macros, since they seemed to be preferred in WebKit.
Comment 30 Nico Weber 2011-08-08 17:53:22 PDT
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)
Comment 31 Nico Weber 2011-08-08 18:51:26 PDT
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.
Comment 32 Nico Weber 2011-08-08 18:58:03 PDT
I just saw that this patch got reverted in https://bugs.webkit.org/show_bug.cgi?id=65865 – maybe because of this issue?
Comment 33 Mark Mentovai 2011-08-08 20:52:50 PDT
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.
Comment 34 asvitkine 2011-08-09 06:34:15 PDT
Created attachment 103353 [details]
Patch
Comment 35 asvitkine 2011-08-09 06:37:58 PDT
One more try, now using BUILDING_ON_* macros.
Comment 36 Mark Mentovai 2011-08-09 08:22:54 PDT
Comment on attachment 103353 [details]
Patch

LGTM
Comment 37 Dimitri Glazkov (Google) 2011-08-09 08:24:21 PDT
Comment on attachment 103353 [details]
Patch

rs=me.
Comment 38 Nico Weber 2011-08-09 11:42:39 PDT
(reopening for commit-bot)
Comment 39 WebKit Review Bot 2011-08-09 12:42:39 PDT
Comment on attachment 103353 [details]
Patch

Clearing flags on attachment: 103353

Committed r92699: <http://trac.webkit.org/changeset/92699>
Comment 40 WebKit Review Bot 2011-08-09 12:42:46 PDT
All reviewed patches have been landed.  Closing bug.