Bug 139466 - [iOS] Add setting to ignore viewport scaling constraints
Summary: [iOS] Add setting to ignore viewport scaling constraints
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Hock
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-09 17:24 PST by Martin Hock
Modified: 2014-12-10 15:42 PST (History)
2 users (show)

See Also:


Attachments
patch (8.06 KB, patch)
2014-12-09 17:39 PST, Martin Hock
benjamin: review-
Details | Formatted Diff | Diff
address comments (8.74 KB, patch)
2014-12-09 20:21 PST, Martin Hock
benjamin: review+
benjamin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Hock 2014-12-09 17:24:53 PST
[iOS] Add viewport override preference
Comment 1 Martin Hock 2014-12-09 17:39:16 PST
Created attachment 242982 [details]
patch
Comment 2 Benjamin Poulain 2014-12-09 17:56:00 PST
Comment on attachment 242982 [details]
patch

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

The name "Override" is too generic. What do you think of "ignoreViewportScalingConstraints"?

> Source/WebCore/page/ViewportConfiguration.cpp:241
> -    applyViewportArgument(m_configuration.minimumScale, m_viewportArguments.minZoom, minimumViewportArgumentsScaleFactor, maximumViewportArgumentsScaleFactor);
> +    applyViewportArgument(m_configuration.minimumScale, m_override ? minimumViewportArgumentsScaleFactor : m_viewportArguments.minZoom, minimumViewportArgumentsScaleFactor, maximumViewportArgumentsScaleFactor);

I think I would leave the configuration unchanged, and modify minimumScale() instead.

That will make it easier to reason about the viewport configuration. The state of m_configuration would still be correct, but we would report different values to the API.

> Source/WebCore/page/ViewportConfiguration.cpp:260
> +    if (m_override)
> +        m_configuration.allowsUserScaling = true;

ditto
Comment 3 Martin Hock 2014-12-09 20:21:03 PST
Created attachment 242985 [details]
address comments
Comment 4 Benjamin Poulain 2014-12-09 20:27:19 PST
Comment on attachment 242985 [details]
address comments

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

> Source/WebCore/page/ViewportConfiguration.h:80
> +    void setIgnoreViewportScalingConstraints(bool ignoreViewportScalingConstraints) {m_ignoreViewportScalingConstraints = ignoreViewportScalingConstraints; }

I wonder if for Viewport Configuration, this should be named "IgnoreScalingConstraints" since the Viewport is implied by the object.

I let you decide the name.

> Source/WebCore/page/ViewportConfiguration.h:111
> +    bool m_ignoreViewportScalingConstraints;

You should initialize this in the constructor.
Comment 5 Martin Hock 2014-12-10 15:42:11 PST
Committed r177110: <http://trac.webkit.org/changeset/177110>