Bug 127855 - WebKit2 View Gestures (Swipe): Update rubberBandsAt{Left,Right} when WebKit swipe is enabled
Summary: WebKit2 View Gestures (Swipe): Update rubberBandsAt{Left,Right} when WebKit s...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-29 13:57 PST by Tim Horton
Modified: 2014-01-29 14:56 PST (History)
2 users (show)

See Also:


Attachments
patch (7.89 KB, patch)
2014-01-29 14:27 PST, Tim Horton
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2014-01-29 13:57:21 PST
In http://trac.webkit.org/changeset/156924, the implicit updating of rubberBandsAt{Left,Right} upon navigation became restricted to only apps that don't directly link against WebKit2 (a mistake), or ones that do and link against an old version (intentional).

We should bring back this behavior if WebKit2 swipe is enabled, and fix the accident where we use the old behavior for apps that don't directly link against WebKit2.

<rdar://problem/15933878>
Comment 1 Tim Horton 2014-01-29 14:27:39 PST
Created attachment 222594 [details]
patch
Comment 2 Darin Adler 2014-01-29 14:47:37 PST
Comment on attachment 222594 [details]
patch

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

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:80
> -static bool shouldUseLegacyImplicitRubberBandControl()
> +static bool expectsLegacyImplicitRubberBandControl()
>  {
> -    static bool linkedAgainstExecutableExpectingImplicitRubberBandControl = NSVersionOfLinkTimeLibrary("WebKit2") < 0x021A0200 /* 538.2.0 */;
> +    if (applicationIsSafari()) {
> +        static const int32_t firstVersionOfSafariNotExpectingImplicitRubberBandControl = 0x021A0F00; // 538.15.0
> +        static bool linkedAgainstSafariExpectingImplicitRubberBandControl = NSVersionOfLinkTimeLibrary("Safari") < firstVersionOfSafariNotExpectingImplicitRubberBandControl;
> +        return linkedAgainstSafariExpectingImplicitRubberBandControl;
> +    }
> +
> +    static const int32_t firstVersionOfWebKit2WithNoImplicitRubberBandControl = 0x021A0200; // 538.2.0
> +    static bool linkedAgainstExecutableExpectingImplicitRubberBandControl = NSVersionOfLinkTimeLibrary("WebKit2") < firstVersionOfWebKit2WithNoImplicitRubberBandControl;
> +    static bool notDirectlyLinkedAgainstExecutable = NSVersionOfLinkTimeLibrary("WebKit2") == -1;
> +
> +    if (notDirectlyLinkedAgainstExecutable)
> +        return false;
> +
>      return linkedAgainstExecutableExpectingImplicitRubberBandControl;
>  }

We should not store these four different global booleans. We should compute this whole thing once and then just cache that boolean result in a global. In other words, write the whole function without any “static bool” and then use static bool in an outer function that calls the inner one:

    static inline bool expectsLegacyImplicitRubberBandControl()
    {
        static bool cachedAnswer = computeExpectsLegacyImplicitRubberBandControl();
        return cachedAnswer;
    }

I suggest marking the function inline because it’s short and only called in one place. Another way to do it is to actually put the caching outside the function:

    void WebPageProxy::platformInitialize()
    {
        static bool cachedAnswer = expectsLegacyImplicitRubberBandControl();
        setShouldUseImplicitRubberBandControl(cachedAnswer);
    }

The downside of that approach is that someone might later decide to call expectsLegacyImplicitRubberBandControl elsewhere.

I also think it’s confusing to have “linkedAgainstExecutableExpectingImplicitRubberBandControl” be true when we’re not directly linked against any executable at all. So I would write that differently. More like this.

    int32_t linkedWebKit2Version = NSVersionOfLinkTimeLibrary("WebKit2");
    return linkedWebKit2Version != -1 && linkedWebKit2Version < firstVersionOfWebKit2WithNoImplicitRubberBandControl;
Comment 3 Tim Horton 2014-01-29 14:56:53 PST
(In reply to comment #2)
> (From update of attachment 222594 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222594&action=review
> 

All good points!

> The downside of that approach is that someone might later decide to call expectsLegacyImplicitRubberBandControl elsewhere.

I don't think that's going to happen, so I'll do this.