Bug 75730 – Repaint all graphics layers when their renderer offset changes

• Component: New Bugs
• Status: RESOLVED FIXED

[Adrienne Walker]

Repaint all graphics layers when their renderer offset changes

[Adrienne Walker]

Created attachment 121478 [details]
Patch

[Adrienne Walker]

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.

[James Robinson]

Comment on attachment 121478 [details]
Patch

Looks reasonable, but I think we'll need some tests

[Simon Fraser (smfr)]

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?)

[Adrienne Walker]

(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?

[Simon Fraser (smfr)]

(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.

[Adrienne Walker]

Created attachment 122143 [details]
Test case, move offset handling

[Simon Fraser (smfr)]

Comment on attachment 122143 [details]
Test case, move offset handling

Nice!

[Gyuyoung Kim]

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

[Adrienne Walker]

Created attachment 122150 [details]
Add include for EFL, no review needed

[Adrienne Walker]

(In reply to comment #8)
> (From update of attachment 122143 [details])
> Nice!

Thanks for the super fast review.  :)

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.