WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
89406
[Chromium] Let the embedder override the max page scale factor set by the page
https://bugs.webkit.org/show_bug.cgi?id=89406
Summary
[Chromium] Let the embedder override the max page scale factor set by the page
Adam Barth
Reported
2012-06-18 17:53:45 PDT
[Chromium] Let the embedder override the max page scale factor set by the page
Attachments
Patch
(7.71 KB, patch)
2012-06-18 17:56 PDT
,
Adam Barth
no flags
Details
Formatted Diff
Diff
work in progress
(5.53 KB, patch)
2012-06-25 15:20 PDT
,
Adam Barth
no flags
Details
Formatted Diff
Diff
Patch
(7.15 KB, patch)
2012-06-25 15:59 PDT
,
Adam Barth
no flags
Details
Formatted Diff
Diff
Patch
(7.58 KB, patch)
2012-06-25 16:07 PDT
,
Adam Barth
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
James Robinson
Comment 1
2012-06-18 17:55:17 PDT
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
Comment 2
2012-06-18 17:56:01 PDT
Created
attachment 148217
[details]
Patch
Adam Barth
Comment 3
2012-06-18 17:56:39 PDT
> 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
Comment 4
2012-06-18 17:58:45 PDT
Please wait for approval from
abarth@webkit.org
,
dglazkov@chromium.org
,
fishd@chromium.org
,
jamesr@chromium.org
or
tkent@chromium.org
before submitting, as this patch contains changes to the Chromium public API. See also
https://trac.webkit.org/wiki/ChromiumWebKitAPI
.
Adam Barth
Comment 5
2012-06-24 01:58:32 PDT
@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
Comment 6
2012-06-25 14:22:24 PDT
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
Comment 7
2012-06-25 14:31:45 PDT
(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
Comment 8
2012-06-25 14:35:23 PDT
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 9
2012-06-25 14:37:20 PDT
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
Comment 10
2012-06-25 15:20:35 PDT
Created
attachment 149367
[details]
work in progress
Adam Barth
Comment 11
2012-06-25 15:59:55 PDT
Created
attachment 149379
[details]
Patch
Adam Barth
Comment 12
2012-06-25 16:05:16 PDT
> > 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
Comment 13
2012-06-25 16:07:51 PDT
Created
attachment 149381
[details]
Patch
James Robinson
Comment 14
2012-06-25 16:49:55 PDT
Comment on
attachment 149381
[details]
Patch R=me, looks fine.
Adam Barth
Comment 15
2012-06-25 16:53:18 PDT
Comment on
attachment 149381
[details]
Patch Thanks!
WebKit Review Bot
Comment 16
2012-06-25 19:00:12 PDT
Comment on
attachment 149381
[details]
Patch Clearing flags on attachment: 149381 Committed
r121213
: <
http://trac.webkit.org/changeset/121213
>
WebKit Review Bot
Comment 17
2012-06-25 19:00:18 PDT
All reviewed patches have been landed. Closing bug.
Alexandre Elias
Comment 18
2012-06-25 19:01:36 PDT
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug