Bug 110183 - [chromium] Fix races in double-tap zoom minimum scale policy
Summary: [chromium] Fix races in double-tap zoom minimum scale policy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandre Elias
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-18 23:04 PST by Alexandre Elias
Modified: 2013-02-19 10:44 PST (History)
5 users (show)

See Also:


Attachments
Patch (14.57 KB, patch)
2013-02-18 23:10 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Elias 2013-02-18 23:04:42 PST
[chromium] Fix races in double-tap zoom minimum scale policy
Comment 1 Alexandre Elias 2013-02-18 23:10:11 PST
Created attachment 188997 [details]
Patch
Comment 2 Adam Barth 2013-02-19 10:10:20 PST
Comment on attachment 188997 [details]
Patch

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

This code really needs to be refactored.  All the page scaling state should be moved into a separate object that can be unit tested without introducing a bunch of "for testing" state.  One way to do that is to have the object call back through a virtual interface that actually operators on the WebView.  For testing, we can supply a mock implementation of the interface (e.g., so we don't need a "for testing" branch to avoid m_layerTreeView->startPageScaleAnimation).

We can land this patch as-is, but please consider a followup patch to refactor this code.

> Source/WebKit/chromium/src/WebViewImpl.cpp:406
> +    , m_doubleTapZoomPageScaleFactor(0)
> +    , m_doubleTapZoomPending(false)
> +    , m_enableFakeDoubleTapAnimationForTesting(false)
> +    , m_fakeDoubleTapPageScaleFactor(0)
> +    , m_fakeDoubleTapUseAnchor(false)

So much state!  We really should move all this viewport related state out into its own object.
Comment 3 Alexandre Elias 2013-02-19 10:39:48 PST
Comment on attachment 188997 [details]
Patch

Agreed completely, filed http://crbug.com/176998 to track.
Comment 4 WebKit Review Bot 2013-02-19 10:44:10 PST
Comment on attachment 188997 [details]
Patch

Clearing flags on attachment: 188997

Committed r143355: <http://trac.webkit.org/changeset/143355>
Comment 5 WebKit Review Bot 2013-02-19 10:44:13 PST
All reviewed patches have been landed.  Closing bug.