Summary: | WebKit2 View Gestures (Swipe): Update rubberBandsAt{Left,Right} when WebKit swipe is enabled | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||
Component: | WebKit2 | Assignee: | Tim Horton <thorton> | ||||
Status: | NEW --- | ||||||
Severity: | Normal | CC: | andersca, sam | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Tim Horton
2014-01-29 13:57:21 PST
Created attachment 222594 [details]
patch
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; (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. |