[webkit-reviews] review granted: [Bug 58907] [chromium] Don't unnecessarily resize skia/cg canvases when painting in compositor : [Attachment 91343] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Adrienne Walker
<enne at google.com>'s request for review:
Bug 58907: [chromium] Don't unnecessarily resize skia/cg canvases when painting
in compositor
https://bugs.webkit.org/show_bug.cgi?id=58907

Attachment 91343: Patch
https://bugs.webkit.org/attachment.cgi?id=91343&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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.