Make CanvasRenderingContext2D use SkCanvas directly

Make CanvasRenderingContext2D use SkCanvas directly

This is part 1 of making 2D canvas independent of blink::GraphicsContext
This change bypasses GraphicsContext for managing matrix and  clip state,
and for state save/restore operations.

BUG=453113
TEST=covered by existing canvas layout tests

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

[Justin Novosad, 2015-02-03 21:51:57]

junov@chromium.org changed reviewers:
+ schenney@google.com

[Justin Novosad, 2015-02-03 21:51:57]

PTAL

[Justin Novosad, 2015-02-04 05:59:07]

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

[chrishtr, 2015-02-04 07:17:49]

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

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:429: // Temporary code
while crbug.com/453113 is a WIP: GraphicsContext state stack
How do you plan to get rid of this code later?

[dshwang, 2015-02-04 13:50:02]

dongseong.hwang@intel.com changed reviewers:
+ dongseong.hwang@intel.com

[dshwang, 2015-02-04 13:50:03]

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

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:429: // Temporary code
while crbug.com/453113 is a WIP: GraphicsContext state stack
On 2015/02/04 07:17:49, chrishtr wrote:
> How do you plan to get rid of this code later?

I guess @junov will remove GraphicsContext in ImageBuffer at all?

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:853:
c->rotate(angleInRadians * (180.0f / 3.14159265f));
how about using piFloat in MathExtras.h

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1001: GraphicsContext* c =
drawingContext();
why this method use GraphicsContext?

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1164: return
path.contains(transformedPoint, (WindRule)parseWinding(windingRuleString));
could you make converting inline function for WindRule?

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2260:
drawingContext()->setImageInterpolationQuality(enabled ?
CanvasDefaultInterpolationQuality : InterpolationNone);
Is it safe to mix to use SkCanvas and GraphicsContext?

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

On 2015/02/04 07:17:49, chrishtr wrote:
>
https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:429: // Temporary code
> while crbug.com/453113 is a WIP: GraphicsContext state stack
> How do you plan to get rid of this code later?

The plan is to stop relying on GraphicsContext for constructing an SkPaint, and
do that in CRC2D.  I was thinking that while refactoring that code, I might as
well make CRC2D store its state directly in SkPaint objects (m_strokePaint +
m_fillPaint), which would save the overhead of generating SkPaints on the fly at
each draw call.

[chrishtr, 2015-02-04 18:23:48]

On 2015/02/04 at 18:10:06, junov wrote:
> On 2015/02/04 07:17:49, chrishtr wrote:
> >
https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
> > Source/core/html/canvas/CanvasRenderingContext2D.cpp:429: // Temporary code
> > while crbug.com/453113 is a WIP: GraphicsContext state stack
> > How do you plan to get rid of this code later?
> 
> The plan is to stop relying on GraphicsContext for constructing an SkPaint,
and do that in CRC2D.  I was thinking that while refactoring that code, I might
as well make CRC2D store its state directly in SkPaint objects (m_strokePaint +
m_fillPaint), which would save the overhead of generating SkPaints on the fly at
each draw call.

Can you point me to where the equivalent code used to be? Hard to verify that
block of code since it needs to be complete.

Also, would it be straightforward to do all the easy parts of this CL in one
patch and then the harder ones in another?

[Justin Novosad, 2015-02-04 18:37:47]

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

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:853:
c->rotate(angleInRadians * (180.0f / 3.14159265f));
On 2015/02/04 13:50:03, dshwang wrote:
> how about using piFloat in MathExtras.h

Acknowledged.

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1001: GraphicsContext* c =
drawingContext();
On 2015/02/04 13:50:03, dshwang wrote:
> why this method use GraphicsContext?

One step at a time. For now I am still relying on GraphicsContext to convert
state to SkPaint. This patch does not touch the drawing methods much, I plan to
address this in a upcoming patch. I intend to do this work incrementally. It
will probably take me 5 or 6 CLs to completely separate CRC2D and ImageBuffer
from GraphicsContext.

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1164: return
path.contains(transformedPoint, (WindRule)parseWinding(windingRuleString));
On 2015/02/04 13:50:03, dshwang wrote:
> could you make converting inline function for WindRule?

Acknowledged.

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2260:
drawingContext()->setImageInterpolationQuality(enabled ?
CanvasDefaultInterpolationQuality : InterpolationNone);
On 2015/02/04 13:50:03, dshwang wrote:
> Is it safe to mix to use SkCanvas and GraphicsContext?

This CL brings us into an intermediate state where matrix and clip are tracked
without going through GraphicsContext, but we still rely on GraphicsContext for
the application of the paint styles.  This is safe, but a bit fragile I admit.
It is just an intermediate step to removing GC completely from this code.

[Justin Novosad, 2015-02-04 18:44:02]

On 2015/02/04 18:23:48, chrishtr wrote:
> On 2015/02/04 at 18:10:06, junov wrote:
> > On 2015/02/04 07:17:49, chrishtr wrote:
> > >
>
https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
> > > Source/core/html/canvas/CanvasRenderingContext2D.cpp:429: // Temporary
code
> > > while crbug.com/453113 is a WIP: GraphicsContext state stack
> > > How do you plan to get rid of this code later?
> > 
> > The plan is to stop relying on GraphicsContext for constructing an SkPaint,
> and do that in CRC2D.  I was thinking that while refactoring that code, I
might
> as well make CRC2D store its state directly in SkPaint objects (m_strokePaint
+
> m_fillPaint), which would save the overhead of generating SkPaints on the fly
at
> each draw call.
> 
> Can you point me to where the equivalent code used to be? Hard to verify that
> block of code since it needs to be complete.

Basically, this code has to cover all the cases in CRC2D where setter functions
are setting state on drawingContext.
Because we are no longer exercising the GC state stack. The restore from the
CRC2D state stack has to be propagated to GC. The CRC2D state stack was already
redundantly tracking all the GC state that we care about.

> 
> Also, would it be straightforward to do all the easy parts of this CL in one
> patch and then the harder ones in another?

[Justin Novosad, 2015-02-04 18:45:32]

On 2015/02/04 18:23:48, chrishtr wrote:
> Also, would it be straightforward to do all the easy parts of this CL in one
> patch and then the harder ones in another?

That is kinda what I am going for. I left all the hard parts out and will be
landing them incrementally

[chrishtr, 2015-02-04 18:56:16]

lgtm

[Justin Novosad, 2015-02-04 18:59:08]

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

[dshwang, 2015-02-04 19:01:38]

lgtm

[Stephen Chennney, 2015-02-04 19:18:59]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2015-02-04 19:19:00]

Some typos, I think. Also lgtm.

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

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:129:
ASSERT(static_cast<size_t>(skCanvas->getSaveCount() - 1) ==
m_stateStack.size());
Do we have tests covering this assert? I believe it's right, but it seems easy
to get wrong.

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:254: c->save();
Yippee. Getting rid of a call to GraphicsContext::canvas().

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:264: // GraphicsContext in
canvasElementElement::createImageBuffer()!
Typo?

https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1765: ImageBuffer* buffer
=canvas()->buffer();
typo?

[Justin Novosad, 2015-02-04 19:25:52]

On 2015/02/04 19:19:00, Stephen Chenney wrote:
> Some typos, I think. Also lgtm.
> 
>
https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/895153002/diff/60001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:129:
> ASSERT(static_cast<size_t>(skCanvas->getSaveCount() - 1) ==
> m_stateStack.size());
> Do we have tests covering this assert? I believe it's right, but it seems easy
> to get wrong.

I actually made several mistakes while working on this CL, and this assert
pinpointed my errors. So I can confirm that the assert is correct, gets plenty
of coverage, and most importantly that it is super useful.

[Justin Novosad, 2015-02-04 22:09:55]

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

[Justin Novosad, 2015-02-04 22:10:10]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2015-02-04 22:10:51]

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

[commit-bot: I haz the power, 2015-02-05 00:17:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-05 00:17:08]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...)

[Stephen Chennney, 2015-02-05 17:54:48]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2015-02-05 17:55:17]

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

[Stephen Chennney, 2015-02-05 18:05:39]

The CQ bit was unchecked by schenney@chromium.org

[Stephen Chennney, 2015-02-05 18:05:53]

Actually, the failures seem real and consistent.

[Justin Novosad, 2015-02-05 18:14:38]

On 2015/02/05 18:05:53, Stephen Chenney wrote:
> Actually, the failures seem real and consistent.

Yeah, but they don't repro locally, which is really weird.

[Stephen Chennney, 2015-02-05 18:18:56]

I'll see if I can repro.

[Justin Novosad, 2015-02-05 18:26:45]

On 2015/02/05 18:18:56, Stephen Chenney wrote:
> I'll see if I can repro.

Took a closer look at the failure reports. They are failing in
validateStateStack.  That method is supposed to be a no-op in release builds. I
guess the release bots are making special release builds that have assertions
enabled?  That would explain why I was not reproing.

[chrishtr, 2015-02-05 18:29:02]

On 2015/02/05 at 18:26:45, junov wrote:
> On 2015/02/05 18:18:56, Stephen Chenney wrote:
> > I'll see if I can repro.
> 
> Took a closer look at the failure reports. They are failing in
validateStateStack.  That method is supposed to be a no-op in release builds. I
guess the release bots are making special release builds that have assertions
enabled?  That would explain why I was not reproing.

Yes, the release bots have assert on them for testing. There is a way to make it
compile that way, but I never found it. I just test in debug to repro this kind
of thing.

[Justin Novosad, 2015-02-05 18:29:14]

On 2015/02/05 18:26:45, junov wrote:
> On 2015/02/05 18:18:56, Stephen Chenney wrote:
> > I'll see if I can repro.
> 
> Took a closer look at the failure reports. They are failing in
> validateStateStack.  That method is supposed to be a no-op in release builds.
I
> guess the release bots are making special release builds that have assertions
> enabled?  That would explain why I was not reproing.

I figured it out. clearRect is still saving and restoring using the GC

[Justin Novosad, 2015-02-05 19:38:49]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2015-02-05 19:39:39]

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

[commit-bot: I haz the power, 2015-02-05 21:38:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-05 21:38:50]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...)

[Justin Novosad, 2015-02-05 22:08:10]

On 2015/02/05 21:38:50, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...)

D'oh! I fixed all the display list canvas issues, but created a bunch of
failures in the non-DL code paths. It's Whack-a-mole time!
chrishtr: You see, this is what I meant when I said this area of the code is
sensitive. Hopefully a lot less when I am done whith this refactor.

[Justin Novosad, 2015-02-05 22:30:45]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2015-02-05 22:32:13]

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

[commit-bot: I haz the power, 2015-02-06 01:17:54]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189599