Move overdraw tracking code from GraphicsContext to CanvasRenderingContext2D

Move overdraw tracking code from GraphicsContext to CanvasRenderingContext2D

This change is another step towards making CanvasRenderingContext2D
independent of GraphicsContext.

BUG=453113
TEST=fast/canvas/* and canvas/philip/*

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190059

[Justin Novosad, 2015-02-06 18:27:54]

junov@chromium.org changed reviewers:
+ chrishtr@chromium.org, dongseong.hwang@intel.com, schenney@chromium.org

[Justin Novosad, 2015-02-06 18:29:10]

On 2015/02/06 18:27:54, junov wrote:

Not final: still working on moving the unit tests that are commented out in
patch 1.

[dshwang, 2015-02-06 19:10:37]

I have some questions.

https://codereview.chromium.org/907453003/diff/1/Source/core/html/canvas/Canv...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/907453003/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1525:
checkOverdraw(dstRect, &c->fillPaint(), imageSource->isOpaque() ? OpaqueImage :
NonOpaqueImage, NormalFill);
Why drawImage uses NormalFill while fillRect use ClipFill? what's different?
IMO this code is also already known to cover the current clip.

https://codereview.chromium.org/907453003/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2412: // an enclsed rect,
but that would probably be overkill.
s/enclsed/enclosed/

https://codereview.chromium.org/907453003/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2447: // unknown xfermode
ASSERT_NOT_REACHED?

https://codereview.chromium.org/907453003/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2461: if
(shader->isOpaque())
don't we need to consider alpha even if shader is opaque?

https://codereview.chromium.org/907453003/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2475: return;
Can we overdraw when imageType == OpaqueImage && alpha < 0xFF ?

https://codereview.chromium.org/907453003/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContextTest.cpp (right):

https://codereview.chromium.org/907453003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContextTest.cpp:396: */
why don't you delete code?

https://codereview.chromium.org/907453003/diff/1/Source/platform/graphics/Rec...
File Source/platform/graphics/RecordingImageBufferSurface.h (right):

https://codereview.chromium.org/907453003/diff/1/Source/platform/graphics/Rec...
Source/platform/graphics/RecordingImageBufferSurface.h:44: virtual void
willOverwriteCanvas() override;
after c++11, code style is changed to using only 'override' for subclass; void
willOverwriteCanvas() override;
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance

[Justin Novosad, 2015-02-06 19:37:31]

https://codereview.chromium.org/907453003/diff/1/Source/core/html/canvas/Canv...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/907453003/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1525:
checkOverdraw(dstRect, &c->fillPaint(), imageSource->isOpaque() ? OpaqueImage :
NonOpaqueImage, NormalFill);
On 2015/02/06 19:10:37, dshwang wrote:
> Why drawImage uses NormalFill while fillRect use ClipFill? what's different?
> IMO this code is also already known to cover the current clip.

You are right. And this was the only place NormalFill was used so we can get rid
of it, which will simplify checkOverdraw.

https://codereview.chromium.org/907453003/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2412: // an enclsed rect,
but that would probably be overkill.
On 2015/02/06 19:10:37, dshwang wrote:
> s/enclsed/enclosed/

Acknowledged.

https://codereview.chromium.org/907453003/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2447: // unknown xfermode
On 2015/02/06 19:10:37, dshwang wrote:
> ASSERT_NOT_REACHED?

Acknowledged.

https://codereview.chromium.org/907453003/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2461: if
(shader->isOpaque())
On 2015/02/06 19:10:37, dshwang wrote:
> don't we need to consider alpha even if shader is opaque?
Did some digging in the code. In skia-land, the shader overrides the SkPaint's
color, except for the alpha, which modulates the shader's output.  This means
there was a bug in the old code too, that there is an error in the old unit
tests, and that we have no layout test coverage that exercises that specific
case.  Thanks for spotting this. I will take care of it and I will add adequate
testing.

https://codereview.chromium.org/907453003/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2475: return;
On 2015/02/06 19:10:37, dshwang wrote:
> Can we overdraw when imageType == OpaqueImage && alpha < 0xFF ?

Yep, Made the same mistake here as with shaders above.

https://codereview.chromium.org/907453003/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContextTest.cpp (right):

https://codereview.chromium.org/907453003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContextTest.cpp:396: */
On 2015/02/06 19:10:37, dshwang wrote:
> why don't you delete code?

I will. I am currently working on moving these test to make them CRC2D unit
tests.  That will be taken care of in my next upload.

https://codereview.chromium.org/907453003/diff/1/Source/platform/graphics/Rec...
File Source/platform/graphics/RecordingImageBufferSurface.h (right):

https://codereview.chromium.org/907453003/diff/1/Source/platform/graphics/Rec...
Source/platform/graphics/RecordingImageBufferSurface.h:44: virtual void
willOverwriteCanvas() override;
On 2015/02/06 19:10:37, dshwang wrote:
> after c++11, code style is changed to using only 'override' for subclass; void
> willOverwriteCanvas() override;
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance

Acknowledged.

[Justin Novosad, 2015-02-10 23:07:46]

New Patch.

- Addressed review comments.
- Replaced old GC tests with CRC2D tests
- Fixed some bugs that were revealed by the unit tests

[Justin Novosad, 2015-02-10 23:13:01]

New patch.

Added missing test file

[Justin Novosad, 2015-02-10 23:20:34]

junov@chromium.org changed reviewers:
+ kbr@chromium.org

[Justin Novosad, 2015-02-10 23:20:35]

+kbr

[dshwang, 2015-02-11 08:37:52]

lgtm with nits

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:618: int msaaSampleCount;
need to initialize. it causes build bot failure.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1260: }
why didDraw is removed?

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.h:166: void
drawImage(CanvasImageSource*, float sx, float sy, float sw, float sh, float dx,
float dy, float dw, float dh, ExceptionState&);
why does it become public? I guess it's for unittests, but existing public
drawImage methods might have enough coverage.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2DTest.cpp (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:157:
TEST_F(CanvasRenderingContext2DTest, detectOverwrite)
nice test!

[Ken Russell (switch to Gerrit), 2015-02-11 10:11:44]

LGTM with the comments below and Dongseong's addressed. Also a couple of
questions.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1259:
m_hitRegionManager->removeHitRegionsInRect(rect, state().m_transform);
Why was the check removed above of whether the state's cumulative transform
matrix is invertible? I'm assuming that if it's a singular matrix this will
cause problems for removeHitRegionsInRect. Or maybe this branch should be
short-circuited if !state().m_invertibleCTM?

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2DTest.cpp (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:78: #define
TEST_OVERWRITE_SETUP(EXPECTED_OVERWRITES) \
Throughout this test file, would it make sense to replace "overwrite" with
"overdraw"? If so please do -- the use of two different terms in the source code
and the test is confusing.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:157:
TEST_F(CanvasRenderingContext2DTest, detectOverwrite)
Consider splitting up this test into a few so it's less monolithic, like the
GraphicsContextTest. Perhaps detectOverwriteWithFillRect,
detectOverwriteWithClearRect, etc. This would mean the local variables would
have to be members initialized in SetUp.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:189:
TEST_OVERWRITE_2(0, translate(1, 1), fillRect(0, 0, 10, 10));
Also test transformed rects that would still overdraw: e.g. translate(1, 1),
fillRect(-1, -1, 11, 11).

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:194:
TEST_OVERWRITE_2(1, setGlobalCompositeOperation(String("copy")), fillRect(0, 0,
9, 9));
I don't understand why setting the globalCompositeOperation to copy causes this
fillRect call to fully overdraw the canvas. Is it because the entire scratch
bitmap is "copied" over the canvas's displayed bitmap, and the scratch bitmap
doesn't have any alpha?

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:203:
TEST_OVERWRITE_3(0, rect(0, 0, 5, 5), clip(), clearRect(0, 0, 10, 10));
What about transforms in conjunction with clearRect? Would this exercise a
different code path than fillRect?

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:222:
TEST_OVERWRITE_2(1, translate(-1, 0), drawImage(&opaqueBitmap, 0, 0, 10, 10, 1,
0, 10, 10, exceptionState));
Should also test the case where a transform makes it so that a drawn image
doesn't fully overdraw.

[Stephen Chennney, 2015-02-11 14:33:20]

My 2c worth.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:621: surface = externalSurface;
Should the msaaSampleCount be set somehow from the external surface? I know it's
test only, but one might still want to test the subsequent code that controls
antialiasing.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.h:129: void
createImageBufferUsingSurface(PassOwnPtr<ImageBufferSurface>); // For unit
testing
Nit: Could you put all test methods at the bottom of the public section.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2390: if (!c ||
!canvas()->buffer()->isRecording())
There's a lot of implicit non-null here. As written the code requires all of:
drawingCanvas(), canvas(), canvas()->buffer() to be non-null. Is that intended?

[Justin Novosad, 2015-02-11 18:58:32]

New patchsets have been uploaded after l-g-t-m from
dongseong.hwang@intel.com,kbr@chromium.org

[Justin Novosad, 2015-02-11 18:59:55]

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:618: int msaaSampleCount;
On 2015/02/11 08:37:52, dshwang wrote:
> need to initialize. it causes build bot failure.

Done.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:621: surface = externalSurface;
On 2015/02/11 14:33:20, Stephen Chenney wrote:
> Should the msaaSampleCount be set somehow from the external surface? I know
it's
> test only, but one might still want to test the subsequent code that controls
> antialiasing.

Can we cross that bridge when we get there? If I handle that in this CL it will
be dead code because the current tests only use unaccelerated surfaces.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.h:129: void
createImageBufferUsingSurface(PassOwnPtr<ImageBufferSurface>); // For unit
testing
On 2015/02/11 14:33:20, Stephen Chenney wrote:
> Nit: Could you put all test methods at the bottom of the public section.

Done.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1259:
m_hitRegionManager->removeHitRegionsInRect(rect, state().m_transform);
On 2015/02/11 10:11:44, Ken Russell wrote:
> Why was the check removed above of whether the state's cumulative transform
> matrix is invertible? I'm assuming that if it's a singular matrix this will
> cause problems for removeHitRegionsInRect. Or maybe this branch should be
> short-circuited if !state().m_invertibleCTM?

Good catch, my thinking was that we already check for invertibility in fillRect,
but I did not think about how that might affect removeHitRegionsInRect.

Done.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1260: }
On 2015/02/11 08:37:52, dshwang wrote:
> why didDraw is removed?

didDraw is taken care of by fillRect now. I'll add a comment to that effect for
clarity.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2390: if (!c ||
!canvas()->buffer()->isRecording())
On 2015/02/11 14:33:20, Stephen Chenney wrote:
> There's a lot of implicit non-null here. As written the code requires all of:
> drawingCanvas(), canvas(), canvas()->buffer() to be non-null. Is that
intended?

Those checks are taken care of in drawingCanvas().  So the short-circuiting of
the || operator based on !c is sufficient.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.h:166: void
drawImage(CanvasImageSource*, float sx, float sy, float sw, float sh, float dx,
float dy, float dw, float dh, ExceptionState&);
On 2015/02/11 08:37:52, dshwang wrote:
> why does it become public? I guess it's for unittests, but existing public
> drawImage methods might have enough coverage.

Yes it is for testing purposes. The problem is that I can't wrap a
FakeCanvasImageSource into a CanvasImageSourceUnion without exposing
FakeCanvasImageSource in places where it should not be. So I needed an entry
point that takes a CanvasImageSoure*.  It's either this or I make the test class
a friend of CRC2D. I am actually not sure which is preferable.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2DTest.cpp (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:78: #define
TEST_OVERWRITE_SETUP(EXPECTED_OVERWRITES) \
On 2015/02/11 10:11:44, Ken Russell wrote:
> Throughout this test file, would it make sense to replace "overwrite" with
> "overdraw"? If so please do -- the use of two different terms in the source
code
> and the test is confusing.

Done.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:157:
TEST_F(CanvasRenderingContext2DTest, detectOverwrite)
On 2015/02/11 10:11:44, Ken Russell wrote:
> Consider splitting up this test into a few so it's less monolithic, like the
> GraphicsContextTest. Perhaps detectOverwriteWithFillRect,
> detectOverwriteWithClearRect, etc. This would mean the local variables would
> have to be members initialized in SetUp.

Done.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:189:
TEST_OVERWRITE_2(0, translate(1, 1), fillRect(0, 0, 10, 10));
On 2015/02/11 10:11:44, Ken Russell wrote:
> Also test transformed rects that would still overdraw: e.g. translate(1, 1),
> fillRect(-1, -1, 11, 11).

Done.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:194:
TEST_OVERWRITE_2(1, setGlobalCompositeOperation(String("copy")), fillRect(0, 0,
9, 9));
On 2015/02/11 10:11:44, Ken Russell wrote:
> I don't understand why setting the globalCompositeOperation to copy causes
this
> fillRect call to fully overdraw the canvas. Is it because the entire scratch
> bitmap is "copied" over the canvas's displayed bitmap, and the scratch bitmap
> doesn't have any alpha?

Close. The way the spec defines the behavior: primitives are drawn in to a
temporary buffer the same size as the canvas that is initially cleared, then
that buffer is composited onto the canvas using the global composite op. So
drawing anything, even an out of bounds primitive with the coposite op set to
'copy' will overwrite the current clip.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:203:
TEST_OVERWRITE_3(0, rect(0, 0, 5, 5), clip(), clearRect(0, 0, 10, 10));
On 2015/02/11 10:11:44, Ken Russell wrote:
> What about transforms in conjunction with clearRect? Would this exercise a
> different code path than fillRect?

Done.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:222:
TEST_OVERWRITE_2(1, translate(-1, 0), drawImage(&opaqueBitmap, 0, 0, 10, 10, 1,
0, 10, 10, exceptionState));
On 2015/02/11 10:11:44, Ken Russell wrote:
> Should also test the case where a transform makes it so that a drawn image
> doesn't fully overdraw.

Done.

[Justin Novosad, 2015-02-11 19:01:24]

Significant changes to CanvasRenderingContext2DTest.cpp since previous l-g-t-m

[Stephen Chennney, 2015-02-11 19:29:48]

LGTM, but maybe let others weigh in too.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:621: surface = externalSurface;
On 2015/02/11 18:59:54, junov wrote:
> On 2015/02/11 14:33:20, Stephen Chenney wrote:
> > Should the msaaSampleCount be set somehow from the external surface? I know
> it's
> > test only, but one might still want to test the subsequent code that
controls
> > antialiasing.
> 
> Can we cross that bridge when we get there? If I handle that in this CL it
will
> be dead code because the current tests only use unaccelerated surfaces.

Sure.

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/907453003/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2390: if (!c ||
!canvas()->buffer()->isRecording())
On 2015/02/11 18:59:54, junov wrote:
> On 2015/02/11 14:33:20, Stephen Chenney wrote:
> > There's a lot of implicit non-null here. As written the code requires all
of:
> > drawingCanvas(), canvas(), canvas()->buffer() to be non-null. Is that
> intended?
> 
> Those checks are taken care of in drawingCanvas().  So the short-circuiting of
> the || operator based on !c is sufficient.

OK.

[Justin Novosad, 2015-02-11 20:19:32]

Looks like Mocks need to be exported for the component build. How weird is that?

[Justin Novosad, 2015-02-11 20:22:01]

On 2015/02/11 20:19:32, junov wrote:
> Looks like Mocks need to be exported for the component build. How weird is
that?

Never mind: there was an irrelevant "PLATFORM_EXPORT" in front of it (copy
pasta)

[Justin Novosad, 2015-02-11 21:14:19]

New patchsets have been uploaded after l-g-t-m from schenney@chromium.org

[Justin Novosad, 2015-02-11 21:16:13]

Patch set six has significant changes to CanvasRenderingContext2D.cpp

[Stephen Chennney, 2015-02-11 21:24:51]

Nasty to have a canvas change underneath you. LGTM. It's ugly but I presume it
can be cleaned up somewhat when GraphicsContext is entirely removed.

https://codereview.chromium.org/907453003/diff/100001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/907453003/diff/100001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:576: return;
Nit: Can we move this to the start of the method? I'm guessing it's rare, so I'm
not blocking commit on it.

[Justin Novosad, 2015-02-11 21:49:47]

https://codereview.chromium.org/907453003/diff/100001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/907453003/diff/100001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:576: return;
On 2015/02/11 21:24:51, Stephen Chenney wrote:
> Nit: Can we move this to the start of the method? I'm guessing it's rare, so
I'm
> not blocking commit on it.

What you suggest would have web-facing side-effects. Right now, if you have a
broken canvas (backing store allocation failed), context attributes can still be
written to and read from without any data loss.

[Stephen Chennney, 2015-02-11 22:07:15]

On 2015/02/11 21:49:47, junov wrote:
>
https://codereview.chromium.org/907453003/diff/100001/Source/core/html/canvas...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/907453003/diff/100001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:576: return;
> On 2015/02/11 21:24:51, Stephen Chenney wrote:
> > Nit: Can we move this to the start of the method? I'm guessing it's rare, so
> I'm
> > not blocking commit on it.
> 
> What you suggest would have web-facing side-effects. Right now, if you have a
> broken canvas (backing store allocation failed), context attributes can still
be
> written to and read from without any data loss.

Of course. I retract my comment. I shouldn't review at the end of the day.

[Ken Russell (switch to Gerrit), 2015-02-12 11:48:06]

Still LGTM

https://codereview.chromium.org/907453003/diff/100001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2DTest.cpp (right):

https://codereview.chromium.org/907453003/diff/100001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2DTest.cpp:230: TEST_OVERDRAW_2(1,
translate(1, 1), fillRect(-1, -1, 10, 10));
These two fillRects should be clearRect.

[Justin Novosad, 2015-02-12 15:46:50]

New patchsets have been uploaded after l-g-t-m from
schenney@chromium.org,kbr@chromium.org

[Justin Novosad, 2015-02-12 15:47:14]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2015-02-12 15:49:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/907453003/120001

[commit-bot: I haz the power, 2015-02-12 17:22:51]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190059