Bug 130555 - [iOS][WK2] Reduce the tiling coverage to the current rect and 1 tile size ahead
Summary: [iOS][WK2] Reduce the tiling coverage to the current rect and 1 tile size ahead
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-20 20:06 PDT by Benjamin Poulain
Modified: 2014-03-21 19:41 PDT (History)
8 users (show)

See Also:


Attachments
Patch (19.06 KB, patch)
2014-03-20 20:07 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (18.17 KB, patch)
2014-03-20 21:17 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (18.47 KB, patch)
2014-03-21 17:26 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (483.28 KB, application/zip)
2014-03-21 18:35 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2014-03-20 20:06:27 PDT
[iOS][WK2] Reduce the tiling coverage to the current rect and 1 tile size ahead
Comment 1 Benjamin Poulain 2014-03-20 20:07:28 PDT
Created attachment 227371 [details]
Patch
Comment 2 Benjamin Poulain 2014-03-20 20:14:13 PDT
I dropped the acceleration. Taking into account acceleration gives better approximation of futureRect but with the tiling-ahead, that made almost no difference.

Tiling ahead only goes for one tile size. Anything bigger was slowing down rendering so much that the result would be useless by the time it comes to the UIProcess. This version is also much simpler.

The way I push the coverage rect is currently done by pushing is as the clip rect in flushCompositingState(). I am not sure what would be the right way to do that. Pulling the transforms from TileController seems just as bad.
Comment 3 Benjamin Poulain 2014-03-20 21:17:02 PDT
Created attachment 227379 [details]
Patch
Comment 4 Simon Fraser (smfr) 2014-03-21 12:34:14 PDT
Comment on attachment 227379 [details]
Patch

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

> Source/WebCore/platform/ScrollView.h:188
> +    void setScrollVelocityInformation(double horizontalVelocity, double verticalVelocity, double timestamp);

FloatSize?
"information" seems redundant.

> Source/WebCore/platform/ScrollView.h:189
> +    FloatRect computeCoverageRect(double horizontalMargin, double verticalMargin);

Are these margins on both sides?
Function should be const.

> Source/WebCore/platform/ScrollView.h:418
> +    double m_horizontalVelocity;
> +    double m_verticalVelocity;

FloatSize?

> Source/WebCore/platform/ScrollView.h:419
> +    double m_velocityUpdateTimestamp;

I'd call this m_lastVelocityUpdateTime

> Source/WebCore/platform/ios/ScrollViewIOS.mm:149
> +        futureRect.setWidth(futureRect.width() + horizontalMargin);
> +        if (m_horizontalVelocity < 0)
> +            futureRect.setX(std::max(futureRect.x() - horizontalMargin, 0.));
> +    }
> +
> +    if (m_verticalVelocity) {
> +        futureRect.setHeight(futureRect.height() + verticalMargin);
> +        if (m_verticalVelocity < 0)
> +            futureRect.setY(std::max(futureRect.y() - verticalMargin, 0.));
> +    }

I think we need to clamp when velocity is really high, to avoid making too many tiles.

> Source/WebKit2/Shared/VisibleContentRectUpdateInfo.h:80
> +    double m_horizontalVelocity;
> +    double m_verticalVelocity;

FloatSize?

> Source/WebKit2/UIProcess/ios/WKContentView.mm:66
> +class HistoricalKinematicData {

VelocityData maybe? Kinematic sounds like we're doing multi-body simulation.

> Source/WebKit2/UIProcess/ios/WKContentView.mm:75
> +    CGPoint velocityForNewData(CGPoint newPosition, double timestamp)

I think a CGSize would make more sense as a return value.
Comment 5 Benjamin Poulain 2014-03-21 17:26:40 PDT
Created attachment 227515 [details]
Patch
Comment 6 Build Bot 2014-03-21 18:35:40 PDT
Comment on attachment 227515 [details]
Patch

Attachment 227515 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4648652651888640

New failing tests:
media/W3C/audio/canPlayType/canPlayType_application_octet_stream.html
Comment 7 Build Bot 2014-03-21 18:35:43 PDT
Created attachment 227521 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 8 Benjamin Poulain 2014-03-21 19:40:55 PDT
Comment on attachment 227515 [details]
Patch

Clearing flags on attachment: 227515

Committed r166110: <http://trac.webkit.org/changeset/166110>
Comment 9 Benjamin Poulain 2014-03-21 19:41:00 PDT
All reviewed patches have been landed.  Closing bug.