Bug 86085 – [Qt] Implement fit-to-width behaviour

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Kenneth Rohde Christiansen]

SSIA

[Kenneth Rohde Christiansen]

Created attachment 141141 [details]
Patch

[Kenneth Rohde Christiansen]

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.

[zalan]

discussed over irc. While I had some reservations about the QWebViewportInfo changes, it can't really be done cleaner at this time.

[Kenneth Rohde Christiansen]

Created attachment 141145 [details]
Patch

[Andras Becsi]

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

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

Landed in r116632

[Andras Becsi]

(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?

[Kenneth Rohde Christiansen]

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

[Andras Becsi]

(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 :)