Bug 87415 – [chromium] Reset minimum/maximum page scale factors when page-scale-factor limits are unset

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[Sadrul Habib Chowdhury]

Adding a flag to enable page-scale-factor even when device-scale-factor is enabled will allow developers to find and fix various bugs that show up.

[Sadrul Habib Chowdhury]

Created attachment 143872 [details]
Patch

[Dana Jansens]

nit: The title should indicate this for when device-scale factor is being used in the compositor. It can be used in other ways and this does not affect them. 

Otherwise, LGTM.

[Dana Jansens]

You'll need to rebase also to get EWS to check it out.

[Sadrul Habib Chowdhury]

Created attachment 143890 [details]
Patch

[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.

[James Robinson]

Comment on attachment 143890 [details]
Patch

We have WebKit API for setting the page scale limits already, so it's weird to me that we have to add additional public API to do this again.  Why is that?

[Dana Jansens]

Oh true! Sadrul, can we just call WebView()->setPageScaleFactorLimits() from somewhere in chromium?

[Sadrul Habib Chowdhury]

(In reply to comment #7)
> Oh true! Sadrul, can we just call WebView()->setPageScaleFactorLimits() from somewhere in chromium?

That works! Yay! But looks like we need the following change (the change restores m_minimumPageScaleFactor and m_maximumPageScaleFactor to minPageScaleFactor and maxPageScaleFactor when setPageScaleFactorLimits is called with -1 for one of the params):


diff --git a/Source/WebKit/chromium/src/WebViewImpl.cpp b/Source/WebKit/chromium/src/WebViewImpl.cpp
index a47cb28..5bcfb73 100644
--- a/Source/WebKit/chromium/src/WebViewImpl.cpp
+++ b/Source/WebKit/chromium/src/WebViewImpl.cpp
@@ -2511,14 +2511,19 @@ void WebViewImpl::setPageScaleFactorLimits(float minPageScale, float maxPageScal
 
 bool WebViewImpl::computePageScaleFactorLimits()
 {
-    if (m_pageDefinedMinimumPageScaleFactor == -1 || m_pageDefinedMaximumPageScaleFactor == -1)
+    if ((m_pageDefinedMinimumPageScaleFactor == -1 || m_pageDefinedMaximumPageScaleFactor == -1) && m_minimumPageScaleFactor == minPageScaleFactor && m_maximumPageScaleFactor == maxPageScaleFactor)
         return false;
 
     if (!mainFrame() || !page() || !page()->mainFrame() || !page()->mainFrame()->view())
         return false;
 
-    m_minimumPageScaleFactor = min(max(m_pageDefinedMinimumPageScaleFactor, minPageScaleFactor), maxPageScaleFactor) * (deviceScaleFactor() / m_deviceScaleInCompositor);
-    m_maximumPageScaleFactor = max(min(m_pageDefinedMaximumPageScaleFactor, maxPageScaleFactor), minPageScaleFactor) * (deviceScaleFactor() / m_deviceScaleInCompositor);
+    if (m_pageDefinedMinimumPageScaleFactor == -1 || m_pageDefinedMaximumPageScaleFactor == -1) {
+        m_minimumPageScaleFactor = minPageScaleFactor;
+        m_maximumPageScaleFactor = maxPageScaleFactor;
+    } else  {
+        m_minimumPageScaleFactor = min(max(m_pageDefinedMinimumPageScaleFactor, minPageScaleFactor), maxPageScaleFactor) * (deviceScaleFactor() / m_deviceScaleInCompositor);
+        m_maximumPageScaleFactor = max(min(m_pageDefinedMaximumPageScaleFactor, maxPageScaleFactor), minPageScaleFactor) * (deviceScaleFactor() / m_deviceScaleInCompositor);
+    }
 
     int viewWidthNotIncludingScrollbars = page()->mainFrame()->view()->visibleContentRect(false).width();
     int contentsWidth = mainFrame()->contentsSize().width;


Does this look like a reasonable change? If so, I will put it up in a patch and upload for proper review. (I am unsure if I should close this bug and open a new one or just update this bug with the revised patch/title)

[Sadrul Habib Chowdhury]

Created attachment 143903 [details]
Patch

[Sadrul Habib Chowdhury]

(In reply to comment #8)
> (In reply to comment #7)
> > Oh true! Sadrul, can we just call WebView()->setPageScaleFactorLimits() from somewhere in chromium?
> 
> That works! Yay! But looks like we need the following change (the change restores m_minimumPageScaleFactor and m_maximumPageScaleFactor to minPageScaleFactor and maxPageScaleFactor when setPageScaleFactorLimits is called with -1 for one of the params):
> 
> 
> diff --git a/Source/WebKit/chromium/src/WebViewImpl.cpp b/Source/WebKit/chromium/src/WebViewImpl.cpp
> index a47cb28..5bcfb73 100644
> --- a/Source/WebKit/chromium/src/WebViewImpl.cpp
> +++ b/Source/WebKit/chromium/src/WebViewImpl.cpp
> @@ -2511,14 +2511,19 @@ void WebViewImpl::setPageScaleFactorLimits(float minPageScale, float maxPageScal
> 
>  bool WebViewImpl::computePageScaleFactorLimits()
>  {
> -    if (m_pageDefinedMinimumPageScaleFactor == -1 || m_pageDefinedMaximumPageScaleFactor == -1)
> +    if ((m_pageDefinedMinimumPageScaleFactor == -1 || m_pageDefinedMaximumPageScaleFactor == -1) && m_minimumPageScaleFactor == minPageScaleFactor && m_maximumPageScaleFactor == maxPageScaleFactor)
>          return false;
> 
>      if (!mainFrame() || !page() || !page()->mainFrame() || !page()->mainFrame()->view())
>          return false;
> 
> -    m_minimumPageScaleFactor = min(max(m_pageDefinedMinimumPageScaleFactor, minPageScaleFactor), maxPageScaleFactor) * (deviceScaleFactor() / m_deviceScaleInCompositor);
> -    m_maximumPageScaleFactor = max(min(m_pageDefinedMaximumPageScaleFactor, maxPageScaleFactor), minPageScaleFactor) * (deviceScaleFactor() / m_deviceScaleInCompositor);
> +    if (m_pageDefinedMinimumPageScaleFactor == -1 || m_pageDefinedMaximumPageScaleFactor == -1) {
> +        m_minimumPageScaleFactor = minPageScaleFactor;
> +        m_maximumPageScaleFactor = maxPageScaleFactor;
> +    } else  {
> +        m_minimumPageScaleFactor = min(max(m_pageDefinedMinimumPageScaleFactor, minPageScaleFactor), maxPageScaleFactor) * (deviceScaleFactor() / m_deviceScaleInCompositor);
> +        m_maximumPageScaleFactor = max(min(m_pageDefinedMaximumPageScaleFactor, maxPageScaleFactor), minPageScaleFactor) * (deviceScaleFactor() / m_deviceScaleInCompositor);
> +    }
> 
>      int viewWidthNotIncludingScrollbars = page()->mainFrame()->view()->visibleContentRect(false).width();
>      int contentsWidth = mainFrame()->contentsSize().width;
> 
> 
> Does this look like a reasonable change? If so, I will put it up in a patch and upload for proper review. (I am unsure if I should close this bug and open a new one or just update this bug with the revised patch/title)

I have gone ahead and put this patch up for review here. PTAL

[Dana Jansens]

Comment on attachment 143903 [details]
Patch

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

Oh I think I see, you want to be able to set the page defined scale factor limits back to -1, and then have the min/max adjusted back to the defaults?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2514
> +    if ((m_pageDefinedMinimumPageScaleFactor == -1 || m_pageDefinedMaximumPageScaleFactor == -1) && m_minimumPageScaleFactor == minPageScaleFactor && m_maximumPageScaleFactor == maxPageScaleFactor)

Use some temp var to make this more clear? e.g.
bool currentLimitsAreDefaults = m_minimumPageScaleFactor == minPageScaleFactor && m_maximumPageScaleFactor == maxPageScaleFactor;

since we're doing this check for -1s twice, maybe that would make a nice temp var too, pageDefinedLimitsAreDefaults?

[James Robinson]

Comment on attachment 143903 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2514
>> +    if ((m_pageDefinedMinimumPageScaleFactor == -1 || m_pageDefinedMaximumPageScaleFactor == -1) && m_minimumPageScaleFactor == minPageScaleFactor && m_maximumPageScaleFactor == maxPageScaleFactor)
> 
> Use some temp var to make this more clear? e.g.
> bool currentLimitsAreDefaults = m_minimumPageScaleFactor == minPageScaleFactor && m_maximumPageScaleFactor == maxPageScaleFactor;
> 
> since we're doing this check for -1s twice, maybe that would make a nice temp var too, pageDefinedLimitsAreDefaults?

so -1,-1 means "use the default"? What about calling setPageScaleLimits(1,1) when we want to override the defaults and doing nothing otherwise?

[Sadrul Habib Chowdhury]

(In reply to comment #12)
> (From update of attachment 143903 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143903&action=review
> 
> >> Source/WebKit/chromium/src/WebViewImpl.cpp:2514
> >> +    if ((m_pageDefinedMinimumPageScaleFactor == -1 || m_pageDefinedMaximumPageScaleFactor == -1) && m_minimumPageScaleFactor == minPageScaleFactor && m_maximumPageScaleFactor == maxPageScaleFactor)
> > 
> > Use some temp var to make this more clear? e.g.
> > bool currentLimitsAreDefaults = m_minimumPageScaleFactor == minPageScaleFactor && m_maximumPageScaleFactor == maxPageScaleFactor;
> > 
> > since we're doing this check for -1s twice, maybe that would make a nice temp var too, pageDefinedLimitsAreDefaults?
> 
> so -1,-1 means "use the default"? What about calling setPageScaleLimits(1,1) when we want to override the defaults and doing nothing otherwise?

If we wanted to do that, we would need to know about it here, and if we wanted to know about it here, then we need to send this info via settings  (which the original patchset did) or some other mechanism.

Or do you mean we just shouldn't call setPageScaleFactorLimits from here (i.e. in WebViewImpl::setIsAcceleratedCompositingActive), and do it from where we can make this decision (i.e. in http://codereview.chromium.org/10454019/diff/6002/content/renderer/render_view_impl.cc#newcode2781)?

[Sadrul Habib Chowdhury]

Created attachment 143913 [details]
Patch

[James Robinson]

Comment on attachment 143913 [details]
Patch

It looks like we don't need this based on the most recent chromium-side patch.