Bug 127855 – WebKit2 View Gestures (Swipe): Update rubberBandsAt{Left,Right} when WebKit swipe is enabled

• Component: WebKit2
• Status: NEW

[Tim Horton]

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>

[Tim Horton]

Created attachment 222594 [details]
patch

[Darin Adler]

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;

[Tim Horton]

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