PR: 284828
Created attachment 185541 [details] Patch
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)?
Comment on attachment 185541 [details] Patch how about: 1) isPageScalable = minScale < maxScale (rather than !=) or 2) maxScale() returns std::max(maxScale, zoomToFitScale)
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.
(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?
(In reply to comment #5) > (In reply to comment #3) > Adding Arvid to the discussion, any opinions on the bug? Forgot CC.
(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.
Created attachment 186095 [details] Patch Updated the patch by taking Yong's suggestion.
Comment on attachment 186095 [details] Patch thanks. much simpler
Committed r141626: <http://trac.webkit.org/changeset/141626>