Bug 89406 – [Chromium] Let the embedder override the max page scale factor set by the page

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

[Chromium] Let the embedder override the max page scale factor set by the page

[James Robinson]

WebView.h:

    // PageScaleFactor will be force-clamped between minPageScale and maxPageScale
    // (and these values will persist until setPageScaleFactorLimits is called
    // again).
    virtual void setPageScaleFactorLimits(float minPageScale, float maxPageScale) = 0;


is that not sufficient?

[Adam Barth]

Created attachment 148217 [details]
Patch

[Adam Barth]

> is that not sufficient?

No, that's a ceiling for the max page scale factor.  This patch lets the embedder set a floor for the max page scale factor.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

@jamesr: What's the next step here?  Are you happy with the design if the API, or do we need to iterate?  I've verified that this feature is actively used in chromium-android.  (In Chrome for Android, if you go into Settings -> Accessibility, this code implements the "Force enable zoom" checkbox.)

[Alexandre Elias]

FYI, setPageScaleFactorLimits is no longer used by Chrome for Android (because it was for viewport tag logic now moved into WebViewImpl), nor are there plans to use it again in the future.  The only user is now on desktop platforms to disable pinch-zoom.  We should probably remove it, especially since the setting is liable to be unexpectedly overridden by the viewport tag anyway.

Anyway, as for this value, I think passing a boolean would be clearer, there are already confusingly many min/max values and I can't think of a reason the embedder would want to pass a specific number.  I suggest naming it "ignoreViewportTagMaximumScale".

[Adam Barth]

(In reply to comment #6)
> FYI, setPageScaleFactorLimits is no longer used by Chrome for Android (because it was for viewport tag logic now moved into WebViewImpl), nor are there plans to use it again in the future.  The only user is now on desktop platforms to disable pinch-zoom.  We should probably remove it, especially since the setting is liable to be unexpectedly overridden by the viewport tag anyway.

Ok.  I'll investigate and prepare a patch to remove it.

> Anyway, as for this value, I think passing a boolean would be clearer, there are already confusingly many min/max values and I can't think of a reason the embedder would want to pass a specific number.  I suggest naming it "ignoreViewportTagMaximumScale".

The implementation in the chromium-android branch uses a boolean, but then there's this funny 5.0f constant.  I guess that constant needs to be somewhere.

[Alexandre Elias]

For the number, we can use:

const float WebView::maxPageScaleFactor = 4.0;

which is defined in WebViewImpl and already exported in WebView.  (The exact number 5 is not important, 4 is fine.)

[Adam Barth]

Comment on attachment 148217 [details]
Patch

Ok.  Will do.  By the way, I think 4.0 was the real limit in practice in the chromium-android branch too.

[Adam Barth]

Created attachment 149367 [details]
work in progress

[Adam Barth]

Created attachment 149379 [details]
Patch

[Adam Barth]

> > FYI, setPageScaleFactorLimits is no longer used by Chrome for Android (because it was for viewport tag logic now moved into WebViewImpl), nor are there plans to use it again in the future.  The only user is now on desktop platforms to disable pinch-zoom.  We should probably remove it, especially since the setting is liable to be unexpectedly overridden by the viewport tag anyway.
> 
> Ok.  I'll investigate and prepare a patch to remove it.

Turns out setPageScaleFactorLimits is still used by WebKit internally to implement the viewport meta tag.  We might still want to remove the API given that it's not clear that the page can override it using a meta tag.

[Adam Barth]

Created attachment 149381 [details]
Patch

[James Robinson]

Comment on attachment 149381 [details]
Patch

R=me, looks fine.

[Adam Barth]

Comment on attachment 149381 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 149381 [details]
Patch

Clearing flags on attachment: 149381

Committed r121213: <http://trac.webkit.org/changeset/121213>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexandre Elias]

(In reply to comment #12)
> > > FYI, setPageScaleFactorLimits is no longer used by Chrome for Android (because it was for viewport tag logic now moved into WebViewImpl), nor are there plans to use it again in the future.  The only user is now on desktop platforms to disable pinch-zoom.  We should probably remove it, especially since the setting is liable to be unexpectedly overridden by the viewport tag anyway.
> > 
> > Ok.  I'll investigate and prepare a patch to remove it.
> 
> Turns out setPageScaleFactorLimits is still used by WebKit internally to implement the viewport meta tag.  We might still want to remove the API given that it's not clear that the page can override it using a meta tag.

Right, sorry I wasn't clear, I only meant remove the API from public WebView.