Bug 86085 - [Qt] Implement fit-to-width behaviour
Summary: [Qt] Implement fit-to-width behaviour
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Rohde Christiansen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-10 04:08 PDT by Kenneth Rohde Christiansen
Modified: 2012-05-10 06:01 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.46 KB, patch)
2012-05-10 04:11 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (7.50 KB, patch)
2012-05-10 04:57 PDT, Kenneth Rohde Christiansen
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2012-05-10 04:08:38 PDT
SSIA
Comment 1 Kenneth Rohde Christiansen 2012-05-10 04:11:34 PDT
Created attachment 141141 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-05-10 04:17:48 PDT
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.
Comment 3 zalan 2012-05-10 04:41:23 PDT
discussed over irc. While I had some reservations about the QWebViewportInfo changes, it can't really be done cleaner at this time.
Comment 4 Kenneth Rohde Christiansen 2012-05-10 04:57:22 PDT
Created attachment 141145 [details]
Patch
Comment 5 Andras Becsi 2012-05-10 05:09:10 PDT
(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.
Comment 6 Kenneth Rohde Christiansen 2012-05-10 05:12:13 PDT
(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.
Comment 7 Kenneth Rohde Christiansen 2012-05-10 05:19:19 PDT
Landed in r116632
Comment 8 Andras Becsi 2012-05-10 05:45:43 PDT
(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?
Comment 9 Kenneth Rohde Christiansen 2012-05-10 05:51:27 PDT
> 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.
Comment 10 Andras Becsi 2012-05-10 06:01:46 PDT
(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 :)