Bug 78816 - [Qt] Previous web page appears outside content rect
Summary: [Qt] Previous web page appears outside content rect
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Rohde Christiansen
Depends on:
Reported: 2012-02-16 08:01 PST by Kenneth Rohde Christiansen
Modified: 2012-03-13 05:37 PDT (History)
4 users (show)

See Also:

Patch (3.02 KB, patch)
2012-02-16 08:08 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (3.50 KB, patch)
2012-02-21 10:54 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Test page exposing what happens when clipping is done with no respect for 3D transforms. (2.42 KB, text/html)
2012-03-13 03:20 PDT, Lars Knudsen
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2012-02-16 08:01:04 PST
Comment 1 Kenneth Rohde Christiansen 2012-02-16 08:08:59 PST
Created attachment 127382 [details]
Comment 2 Jocelyn Turcotte 2012-02-20 04:32:40 PST
Comment on attachment 127382 [details]

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

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:450
> +    HashSet<WebCore::WebGraphicsLayer*>::iterator end = m_registeredLayers.end();
> +    for (HashSet<WebCore::WebGraphicsLayer*>::iterator it = m_registeredLayers.begin(); it != end; ++it)
> +        (*it)->didRenderFrame();

This assumes that all the tile creation (which happen one tile at a time on a 0-timer) and rendering is going to be completed within one rendering loop to and from the UI process. It might work since this delay can be long, but it might break and cause white-flashing while committing the scale in cases where this delay isn't long enough and the removeTile calls for the previous backing store get to the UI process before the createTile of the new backing store. There would need some weird situation for this to happen though.

One way this could work is if the tiledbackingstore became completely synchronous (which might not be a bad thing since we don't need it to be async in WebKit2 AFAIK), or I think we should rely on a trigger to destroy the previous tiles that will work 100% of the time.

Maybe for now a reliable way could be to check the main and previous backing store at the end of updateContentBuffers for TiledBackingStore::coverageRatio of the visible rect and decide if it is alright to clear the previous backing store.
Comment 3 Kenneth Rohde Christiansen 2012-02-20 05:05:31 PST
How can this coverageRatio work? You want to check whether it is more than 1.0 (100%) meaning that the previous buffer is larger than the new one. But even if we are loading a new page which is smaller than the previous one we might not always get a coverageRatio larger than 1.0 - or am I missing something?
Comment 4 Jocelyn Turcotte 2012-02-20 05:33:01 PST
(In reply to comment #3)

In reference to http://trac.webkit.org/changeset/94678
I was using "coverageRatio >= 1.0f" mainly because I've always had this weird fear of floats slipping through my fingers, but it should just be "== 1.0f". The method outputs a float between 0 and 1 being "how much percentage of the rect you gave me is covered by visible tiles in this backing store".

The ratio was always calculated on the visible rect since this is where the user would see the empty-content-flashing after committing a scale.
So in that case, the moment that "mainBackingStore.coverageRatio(visibleRect) == 1.0" I would clear the previous tiles.
There was also a special case to handle where doing two quick scale commit would replace a full previousBackingStore with an almost empty mainBackingStore.
Comment 5 Noam Rosenthal 2012-02-21 10:54:04 PST
Created attachment 127999 [details]
Comment 6 WebKit Review Bot 2012-02-21 15:30:36 PST
Comment on attachment 127999 [details]

Clearing flags on attachment: 127999

Committed r108407: <http://trac.webkit.org/changeset/108407>
Comment 7 WebKit Review Bot 2012-02-21 15:30:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Lars Knudsen 2012-03-13 03:20:57 PDT
Created attachment 131581 [details]
Test page exposing what happens when clipping is done with no respect for 3D transforms.

To me, it seems wrong to do the clipping here - at least when it's done with no respect for 3D transforms.  I might be wrong though as I am just getting into this part of the code.
Comment 9 Jocelyn Turcotte 2012-03-13 05:37:36 PDT
(In reply to comment #8)
Could you add a screenshot or explain the result?
It seems to work fine on my build.