Bug 78008 - Investigate rubberbanding/transform-overhang-[e|se|s].html failures
Summary: Investigate rubberbanding/transform-overhang-[e|se|s].html failures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P2 Normal
Assignee: asvitkine
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-07 11:39 PST by asvitkine
Modified: 2012-02-22 12:18 PST (History)
4 users (show)

See Also:


Attachments
Patch (99.89 KB, patch)
2012-02-17 06:57 PST, asvitkine
no flags Details | Formatted Diff | Diff
Patch (100.50 KB, patch)
2012-02-22 09:32 PST, asvitkine
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description asvitkine 2012-02-07 11:39:22 PST
These are the remaining rubberbanding / compositing test failures not fixed by https://bugs.webkit.org/show_bug.cgi?id=78007
Comment 1 asvitkine 2012-02-16 14:08:47 PST
The code in ScrollView::calculateOverhangAreasForPainting() is not producing any overhang areas because  contentsWidth() and contentsHeight() are both 0.

Those checks is there to prevent a "full overhang" bug happening when the page is loading. It does make me think these are related to how the layout tests are setup for the composited case, since we're not seeing the same effect for the non-composited versions of these tests.
Comment 2 asvitkine 2012-02-16 14:42:11 PST
The issue is that in the Composited path, the overhang layer was only getting enabled/disabled in ScrollView::scrollContents().

However, the order of things was such that the scrolling took place before the size had been set, at which time overhang was rightfully calculated to not exist. However, there was no update to this when the size was finally set.

The solution is to re-calculate overhang bounds on size change in addition to scrolling.
Comment 3 asvitkine 2012-02-17 06:57:30 PST
Created attachment 127583 [details]
Patch
Comment 4 asvitkine 2012-02-17 06:59:17 PST
James, can you review?
Comment 5 asvitkine 2012-02-21 12:08:04 PST
Ping.
Comment 6 James Robinson 2012-02-21 22:44:27 PST
Comment on attachment 127583 [details]
Patch

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

Seems OK, but this weird mix of ScrollView/FrameView/RenderLayerCompositor for overhang areas is really annoying to try to understand and debug.

On mac !CHROMIUM, the ScrollbarTheme sets up the overhang area layer to just be a tiled pattern and the shadow layer to be some sort of magic path-based layer. we don't support either of those in our compositor today, but I think we could with not too much effort. I think we'd be much better off using an approach like that rather than manually repainting + reuploading this overhang layer all the time.  I suspect you won't want to work on improving this, but would you mind filing some bugs to improve this?  I'm not super confident in our ability to maintain the current code.

> Source/WebCore/platform/ScrollView.h:285
> +    void updateOverhangAreas();

this should be private

calculate...() probably should be private as well
Comment 7 asvitkine 2012-02-22 09:32:27 PST
Created attachment 128228 [details]
Patch
Comment 8 asvitkine 2012-02-22 09:37:54 PST
(In reply to comment #6)
> (From update of attachment 127583 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127583&action=review
> 
> Seems OK, but this weird mix of ScrollView/FrameView/RenderLayerCompositor for overhang areas is really annoying to try to understand and debug.
> 
> On mac !CHROMIUM, the ScrollbarTheme sets up the overhang area layer to just be a tiled pattern and the shadow layer to be some sort of magic path-based layer. we don't support either of those in our compositor today, but I think we could with not too much effort. I think we'd be much better off using an approach like that rather than manually repainting + reuploading this overhang layer all the time.  I suspect you won't want to work on improving this, but would you mind filing some bugs to improve this?  

Filed: https://bugs.webkit.org/show_bug.cgi?id=79251

> > Source/WebCore/platform/ScrollView.h:285
> > +    void updateOverhangAreas();
> 
> this should be private
> 
> calculate...() probably should be private as well

Done.
Comment 9 WebKit Review Bot 2012-02-22 12:15:31 PST
The commit-queue encountered the following flaky tests while processing attachment 128228 [details]:

storage/indexeddb/index-count.html bug 79266 (authors: hans@chromium.org and jsbell@chromium.org)
The commit-queue is continuing to process your patch.
Comment 10 WebKit Review Bot 2012-02-22 12:18:17 PST
Comment on attachment 128228 [details]
Patch

Clearing flags on attachment: 128228

Committed r108536: <http://trac.webkit.org/changeset/108536>
Comment 11 WebKit Review Bot 2012-02-22 12:18:22 PST
All reviewed patches have been landed.  Closing bug.