Bug 74720 - [Qt] Handle the layers visible rect calculation on the web process.
Summary: [Qt] Handle the layers visible rect calculation on the web process.
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: Jocelyn Turcotte
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-16 08:45 PST by Jocelyn Turcotte
Modified: 2012-01-20 01:28 PST (History)
5 users (show)

See Also:


Attachments
Patch (44.70 KB, patch)
2011-12-16 08:47 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (12.63 KB, patch)
2012-01-13 05:26 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (27.14 KB, patch)
2012-01-18 06:55 PST, Jocelyn Turcotte
noam: review+
noam: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2011-12-16 08:45:36 PST
[Qt] Handle the layers visible rect calculation on the web process.
Comment 1 Jocelyn Turcotte 2011-12-16 08:47:09 PST
Created attachment 119614 [details]
Patch
Comment 2 Jocelyn Turcotte 2011-12-16 08:49:38 PST
This patch works but makes the desktop view not show any content unless we resize the view. This is caused by the layertreehost not receiving the first visible rect because AC had not been completely entered yet. I'll have to fix this after Christmas before pushing any of this.

It would work previously since painting would update the visible rect later on.
Comment 3 Noam Rosenthal 2011-12-16 09:49:03 PST
Comment on attachment 119614 [details]
Patch

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

Awesome.

> Source/WebCore/platform/graphics/texmap/LayerTransform.cpp:80
> +        .translate3d(originX + m_position.x(), originY + m_position.y(), m_anchorPoint.z() )

Space before parentheses
Comment 4 Jocelyn Turcotte 2012-01-13 05:26:45 PST
Created attachment 122414 [details]
Patch

Resubmitting after rebase and addressing comments.
Comment 5 Noam Rosenthal 2012-01-13 06:15:05 PST
Comment on attachment 122414 [details]
Patch

Wait... I think you uploaded the wrong patch to the wrong bug.
Comment 6 Noam Rosenthal 2012-01-13 10:26:11 PST
btw if you're adding files for TextureMapper, don't forget to add them to the EFL/GTK builds (just grep for TextureMapperNode.cpp)
Comment 7 Jocelyn Turcotte 2012-01-18 06:55:06 PST
Created attachment 122920 [details]
Patch

Updated patch after rebasing and splitting the LayerTransform part into bug #76291.
Another patch will follow up to do a projection of the visible rect instead of mapping it like collectVisibleContentsRects was doing.
Comment 8 Noam Rosenthal 2012-01-18 07:05:18 PST
Comment on attachment 122920 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:335
> +    webPageProxy->drawingArea()->setVisibleContentsRectAndScale(IntRect(IntPoint(), viewportSize), 1.0);

You don't need the .0 (coding style)

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:109
> +        child->setVisibleContentRectAndScale(m_visiblePageContentsRect, m_contentsScale);

I doubt this compiles... you named the variable differently in the header :)

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:119
> +    toWebGraphicsLayer(layer)->setVisibleContentRectAndScale(m_visiblePageContentsRect, m_contentsScale);

Instead of doing all this propagation, can we add a function to LayerTreeTileClient?
Comment 9 Jocelyn Turcotte 2012-01-18 08:28:31 PST
(In reply to comment #8)
> I doubt this compiles... you named the variable differently in the header :)
Oh sorry, last minute fix.

> Instead of doing all this propagation, can we add a function to LayerTreeTileClient?
Humm we would still need a method to trigger the visible rect update that would then call the getter on the interface from each layer. The root cause is that layers can be created and added to the tree without the tree host knowing about them, and this method would have to be called instead of the setter in set/addChildren. Personally I prefer a straight setter for the rect and scale.
Comment 10 Noam Rosenthal 2012-01-18 09:05:02 PST
Comment on attachment 122920 [details]
Patch

Please fix variable name (I prefer m_pageVisibleRect or m_visibleRectInPageCoordinates) and maybe merge in patch 76541 before committing.
Comment 11 Jocelyn Turcotte 2012-01-19 07:38:44 PST
Committed r105413: <http://trac.webkit.org/changeset/105413>
Comment 12 Rafael Brandao 2012-01-19 14:46:38 PST
(In reply to comment #11)
> Committed r105413: <http://trac.webkit.org/changeset/105413>

It seems that this patch has broken something. I was running a layout test with: MiniBrowser --desktop 'file:///opt/git/webkit/LayoutTests/editing/selection/regional-indicators.html', and it was visible until the patch before this one (I have a release build). When I've applied this, then MiniBrowser usually loads but shows a blank page instead, I'm not sure what is going on. Even for "http://google.com" I still get nothing there.

Running MiniBrowser without --desktop, I still shows a blank page, but it seems to show up when I refresh it. Any ideas?
Comment 13 Jocelyn Turcotte 2012-01-20 01:28:06 PST
(In reply to comment #12)
> Running MiniBrowser without --desktop, I still shows a blank page, but it seems to show up when I refresh it. Any ideas?

Yep sorry about that, I should have waited to push this patch with the one in bug #76296 which fixes this issue. I'll try to get it in today.