Bug 75730 - Repaint all graphics layers when their renderer offset changes
Summary: Repaint all graphics layers when their renderer offset changes
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: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-06 12:35 PST by Adrienne Walker
Modified: 2012-01-11 19:47 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.68 KB, patch)
2012-01-06 13:04 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Test case, move offset handling (9.32 KB, patch)
2012-01-11 17:35 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Add include for EFL, no review needed (9.35 KB, patch)
2012-01-11 18:44 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2012-01-06 12:35:15 PST
Repaint all graphics layers when their renderer offset changes
Comment 1 Adrienne Walker 2012-01-06 13:04:24 PST
Created attachment 121478 [details]
Patch
Comment 2 Adrienne Walker 2012-01-06 13:08:20 PST
This bug is intended to fix http://crbug.com/107769.

As the map scrolls left to right, different elements end up going into the compositing layer, changing its bounds and its offset from the renderer.  It looks like the layer in question is the foreground layer, which doesn't have the same logic as the main graphics layer to invalidate itself when the offset changes.

I'm working on reducing a test case for this.
Comment 3 James Robinson 2012-01-06 13:38:15 PST
Comment on attachment 121478 [details]
Patch

Looks reasonable, but I think we'll need some tests
Comment 4 Simon Fraser (smfr) 2012-01-06 13:43:46 PST
Comment on attachment 121478 [details]
Patch

I think I'd prefer that the responsibility for calling setNeedsDisplay remains with the GraphicsLayer client, and is not pushed down to GraphicsLayer. My justification is that GraphicsLayer doesn't actually use m_offsetFromRenderer in its painting code at all (but perhaps it should?)
Comment 5 Adrienne Walker 2012-01-06 13:52:45 PST
(In reply to comment #4)
> (From update of attachment 121478 [details])
> I think I'd prefer that the responsibility for calling setNeedsDisplay remains with the GraphicsLayer client, and is not pushed down to GraphicsLayer. My justification is that GraphicsLayer doesn't actually use m_offsetFromRenderer in its painting code at all (but perhaps it should?)

I'm happy to change this.  However, I'm curious; is there a use case where a GraphicsLayerClient would not want to call setNeedsDisplay when the offset from the renderer changed?
Comment 6 Simon Fraser (smfr) 2012-01-06 13:55:49 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 121478 [details] [details])
> > I think I'd prefer that the responsibility for calling setNeedsDisplay remains with the GraphicsLayer client, and is not pushed down to GraphicsLayer. My justification is that GraphicsLayer doesn't actually use m_offsetFromRenderer in its painting code at all (but perhaps it should?)
> 
> I'm happy to change this.  However, I'm curious; is there a use case where a GraphicsLayerClient would not want to call setNeedsDisplay when the offset from the renderer changed?

No, I think that's a sensible thing to do (at least for layers where drawsContent() == true).

The first thing that RenderLayerBacking::paintContents() does is 

        LayoutSize offset = graphicsLayer->offsetFromRenderer();
        context.translate(-offset);

so that could move into the caller, in which case it makes sense for GraphicsLayer to invalidate when offsetFromRenderer changes.
Comment 7 Adrienne Walker 2012-01-11 17:35:48 PST
Created attachment 122143 [details]
Test case, move offset handling
Comment 8 Simon Fraser (smfr) 2012-01-11 17:40:54 PST
Comment on attachment 122143 [details]
Test case, move offset handling

Nice!
Comment 9 Gyuyoung Kim 2012-01-11 18:24:47 PST
Comment on attachment 122143 [details]
Test case, move offset handling

Attachment 122143 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11118219
Comment 10 Adrienne Walker 2012-01-11 18:44:27 PST
Created attachment 122150 [details]
Add include for EFL, no review needed
Comment 11 Adrienne Walker 2012-01-11 19:10:13 PST
(In reply to comment #8)
> (From update of attachment 122143 [details])
> Nice!

Thanks for the super fast review.  :)
Comment 12 WebKit Review Bot 2012-01-11 19:47:04 PST
Comment on attachment 122150 [details]
Add include for EFL, no review needed

Clearing flags on attachment: 122150

Committed r104782: <http://trac.webkit.org/changeset/104782>
Comment 13 WebKit Review Bot 2012-01-11 19:47:09 PST
All reviewed patches have been landed.  Closing bug.