Bug 58907 – [chromium] Don't unnecessarily resize skia/cg canvases when painting in compositor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adrienne Walker]

[chromium] Don't unnecessarily resize skia/cg canvases when painting in compositor

[Adrienne Walker]

Created attachment 90220 [details]
Patch

[Adrienne Walker]

James brought up this issue during a code inspection of how LayerTilerChromium worked, but when looking into fixing it, it looked like the save and restore context logic wasn't correct either.

[Adrienne Walker]

Comment on attachment 90220 [details]
Patch

Actually, I'd like to write a layout test for this.  I only caught the context save/restore logic snafu because I manually tested this two line patch.

So, I'm taking off request for review until I do that.

[Adrienne Walker]

Created attachment 91343 [details]
Patch

[James Robinson]

I'm not sure I understand the save/restore context stuff here - could you explain?  What was wrong with the previous code?

[Adrienne Walker]

The previous code (and the reason for including this test case) put the save and restore in the wrong place.  In LayerTilerChromium, there's a translation of the canvas as well which wasn't getting popped.  So, multiple redraws of the same canvas would accumulate an incorrect transform.  This was masked because we were always recreating the canvas.  Oops.

The save/restore was moved to be in the Painter functor itself, where it can wrap all state changing operations.

[James Robinson]

Comment on attachment 91343 [details]
Patch

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

R=me, few things to address before landing.

> LayoutTests/compositing/repaint/same-size-invalidation.html:22
> +        setTimeout("changeBackground1();", 0);

nit: setTimeout(changeBackground1, 0) is a bit better. move doTest below the declarations of the functions.

> LayoutTests/platform/chromium/test_expectations.txt:3973
> +BUGENNE : compositing/repaint/same-size-invalidation.html = FAIL

= FAIL isn't sufficient - these will fail with MISSING on windows and mac since there is no baseline for those platforms.

This test should render the same on all platforms so I'd just check same-size-invalidation-expected.png/checksum into platform/chromium-gpu/ and not modify test_expectations at all.

> Source/WebCore/platform/graphics/chromium/PlatformCanvas.h:84
> +        // Destructor restores canvas to pre-construction state.

does the Painter manipulate any state on the PlatformCanvas?  Painter::Painter() and Painter::~Painter() only seem to mutate their m_context, which doesn't do anything to the PlatformCanvas other than render into it.

[Adrienne Walker]

(In reply to comment #7)
> (From update of attachment 91343 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91343&action=review
> 
> R=me, few things to address before landing.
> 
> > LayoutTests/compositing/repaint/same-size-invalidation.html:22
> > +        setTimeout("changeBackground1();", 0);
> 
> nit: setTimeout(changeBackground1, 0) is a bit better. move doTest below the declarations of the functions.

Will do.  Using a string is just sloppy.  Also, I don't think I need to reorder anything because Javascript hoisting will take care of it.

> > LayoutTests/platform/chromium/test_expectations.txt:3973
> > +BUGENNE : compositing/repaint/same-size-invalidation.html = FAIL
> 
> = FAIL isn't sufficient - these will fail with MISSING on windows and mac since there is no baseline for those platforms.

Yeah, I've changed this locally already.  I learned that the hard way just yesterday.

> This test should render the same on all platforms so I'd just check same-size-invalidation-expected.png/checksum into platform/chromium-gpu/ and not modify test_expectations at all.

Too much gardening experience lately has made me cautious.  I was going to let the rebaseline tool tell me that.

> > Source/WebCore/platform/graphics/chromium/PlatformCanvas.h:84
> > +        // Destructor restores canvas to pre-construction state.
> 
> does the Painter manipulate any state on the PlatformCanvas?  Painter::Painter() and Painter::~Painter() only seem to mutate their m_context, which doesn't do anything to the PlatformCanvas other than render into it.

Perhaps the comment should be clarified to be "restores canvas context" instead.

[Adrienne Walker]

Committed r85527: <http://trac.webkit.org/changeset/85527>

[Adrienne Walker]

Committed r85539: <http://trac.webkit.org/changeset/85539>