Bug 108252 – [BlackBerry] Zooming in during page load of non-scalable webpage results in fixed magnification

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Jacky Jiang]

PR: 284828

[Jacky Jiang]

Created attachment 185541 [details]
Patch

[Yong Li]

Comment on attachment 185541 [details]
Patch

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

I don't like this confusing function, and your patch makes it even more confusing.

> Source/WebKit/blackberry/Api/WebPage.cpp:1772
> +        // We can get float layoutSize(342.284122, 521.448467) and m_maximumScale(2.243750)
> +        // after computing viewport meta and lay out the contents at IntSize(342, 521).

Can we adjust m_maximumScale to fit the integer layout size?

> Source/WebKit/blackberry/Api/WebPage.cpp:1774
> +        // Therefore, zoomToFitScale(2.245681) could be a bit larger than m_maximumScale
> +        // based on that contents size and results in maximumScale()!=minimumScale(), which

can we make all these floating comparison with something like (fabs(f1-f2) < 0.01)?

[Yong Li]

Comment on attachment 185541 [details]
Patch

how about:
1) isPageScalable = minScale < maxScale (rather than !=)
or 2) maxScale() returns std::max(maxScale, zoomToFitScale)

[Jacky Jiang]

Comment on attachment 185541 [details]
Patch

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

>> Source/WebKit/blackberry/Api/WebPage.cpp:1772
>> +        // after computing viewport meta and lay out the contents at IntSize(342, 521).
> 
> Can we adjust m_maximumScale to fit the integer layout size?

That might be best to be resolved right after viewport computing which was the first way I considered.   However, looks not good.   We have to change result.devicePixelRatio to zoomToFitScale which can not be calculated at that point and also it is not necessary to be std::max(deviceWidth/342, deviceHeight/521).
    if (result.initialScale > 0)
        setInitialScale(result.initialScale * result.devicePixelRatio);
    if (result.minimumScale > 0)
        setMinimumScale(result.minimumScale * result.devicePixelRatio);
    if (result.maximumScale > 0)
        setMaximumScale(result.maximumScale * result.devicePixelRatio);

>> Source/WebKit/blackberry/Api/WebPage.cpp:1774
>> +        // based on that contents size and results in maximumScale()!=minimumScale(), which
> 
> can we make all these floating comparison with something like (fabs(f1-f2) < 0.01)?

This is doable to make things better, but can not fix the issue.

[Jacky Jiang]

(In reply to comment #3)
> (From update of attachment 185541 [details])
> how about:
> 1) isPageScalable = minScale < maxScale (rather than !=)
May be it is OK. What if  minScale() > maxScale()?  

> or 2) maxScale() returns std::max(maxScale, zoomToFitScale)
Suppose  there is another page that m_maxScale:2.4 and zoomToFitScale: 2.5,  previously we would return 4 as maximumScaleScale().
If we return std::max(maxScale, zoomToFitScale),  we will then return 2.5 as the maximumScaleScale() and the page will be non-zoomable even if the page was supposed to be zoomable, which seems not good.

Adding Arvid to the discussion, any opinions on the bug?

[Jacky Jiang]

(In reply to comment #5)
> (In reply to comment #3)
> Adding Arvid to the discussion, any opinions on the bug?

Forgot CC.

[Jacky Jiang]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 185541 [details] [details])
> > how about:
> > 1) isPageScalable = minScale < maxScale (rather than !=)
> May be it is OK. What if  minScale() > maxScale()?  
This doesn't fix the problem by testing, and also there are some cases minScale() >= maxScale()  although m_minimumScale < m_maximumScale which expects the page to be scalable.

[Jacky Jiang]

Created attachment 186095 [details]
Patch

Updated the patch by taking Yong's suggestion.

[Yong Li]

Comment on attachment 186095 [details]
Patch

thanks. much simpler

[Jacky Jiang]

Committed r141626: <http://trac.webkit.org/changeset/141626>