SSIA
Created attachment 141141 [details] Patch
Comment on attachment 141141 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141141&action=review > Source/WebCore/ChangeLog:9 > + fully. 9 without showing any chrome background.
discussed over irc. While I had some reservations about the QWebViewportInfo changes, it can't really be done cleaner at this time.
Created attachment 141145 [details] Patch
(In reply to comment #4) > Created an attachment (id=141145) [details] > Patch I have a pending two-liner fix for Flickable to address the float comparison issue which prevents Flickable from automatically locking the content if it fits into the visual viewport but the content width is larger than the viewport by a fraction of a pixel. This bug tries to fix a problem which is the result of the same rounding issue. I'm not sure if copying and slightly modifying the restrictMinimumScaleFactorToViewportSize function from ViewportArguments.cpp is the correct solution here, since this is a rounding issue and computeViewportAttributes in ViewportArguments.cpp should already calculate the initial scale that fits into view.
(In reply to comment #5) > (In reply to comment #4) > > Created an attachment (id=141145) [details] [details] > > Patch > > I have a pending two-liner fix for Flickable to address the float comparison issue which prevents Flickable from automatically locking the content if it fits into the visual viewport but the content width is larger than the viewport by a fraction of a pixel. > > This bug tries to fix a problem which is the result of the same rounding issue. No it does not :-) This bug is fixing the minimum bound which has nothing directly to do with the axis locking. > I'm not sure if copying and slightly modifying the restrictMinimumScaleFactorToViewportSize function from ViewportArguments.cpp is the correct solution here, since this is a rounding issue and computeViewportAttributes in ViewportArguments.cpp should already calculate the initial scale that fits into view. That does consider that the size might be bigger than the actual layout viewport.
Landed in r116632
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > Created an attachment (id=141145) [details] [details] [details] > > > Patch > > > > I have a pending two-liner fix for Flickable to address the float comparison issue which prevents Flickable from automatically locking the content if it fits into the visual viewport but the content width is larger than the viewport by a fraction of a pixel. > > > > This bug tries to fix a problem which is the result of the same rounding issue. > > No it does not :-) This bug is fixing the minimum bound which has nothing directly to do with the axis locking. The minimum scale (which should fit into view) should have the result that Flickable disables horizontal content movement, so this patch has an impact on axis locking, and it actually fixes some cases where Flickable did not lock :) > > > I'm not sure if copying and slightly modifying the restrictMinimumScaleFactorToViewportSize function from ViewportArguments.cpp is the correct solution here, since this is a rounding issue and computeViewportAttributes in ViewportArguments.cpp should already calculate the initial scale that fits into view. > > That does consider that the size might be bigger than the actual layout viewport. I see. What I do not understand is why we needed to duplicate the computeMinimumScaleFactorForContentContained function here if we could use the exact same one in ViewportArguments.cpp (WebCore::computeMinimumScaleFactorForContentContained) as we did with the restrictMinimumScaleFactorToViewportSize?
> What I do not understand is why we needed to duplicate the computeMinimumScaleFactorForContentContained function here if we could use the exact same one in ViewportArguments.cpp (WebCore::computeMinimumScaleFactorForContentContained) as we did with the restrictMinimumScaleFactorToViewportSize? I am using the one from ViewportArguments.cpp, there is no duplication here. The restrict* method is still there for those not implementing proper fit-to-view behavior like us now and Chromium. It is also used for teh testing system.
(In reply to comment #9) > > What I do not understand is why we needed to duplicate the computeMinimumScaleFactorForContentContained function here if we could use the exact same one in ViewportArguments.cpp (WebCore::computeMinimumScaleFactorForContentContained) as we did with the restrictMinimumScaleFactorToViewportSize? > > I am using the one from ViewportArguments.cpp, there is no duplication here. The restrict* method is still there for those not implementing proper fit-to-view behavior like us now and Chromium. It is also used for teh testing system. Right, my bad, I looked at the wrong working copy :)