Plumb in flag to enable antialiased clipping in HTML 5 Canvas

Plumb in flag to enable antialiased clipping in HTML 5 Canvas

Here is the OWP tracking bug: https://code.google.com/p/chromium/issues/detail?id=422984
Here is the link to the entry on the feature dashboard: https://www.chromestatus.com/features/4871530282483712

Currently canvas clips are not antialiased which differs what what other
browsers are doing and what the users expect. Skia supports antialiased clipping
but Skia's gpu backend (which is used for canvas) can be significantly slower
for concave clips. 

This CL starts surfacing the desired antialiased clipping behavior behind a flag
so developers can start assessing the capability. There is a lot more work that
needs to occur in Skia before we have a consistently good story for gpu-backed
antialiased clipping so we will not be ready to switch behaviors for a while.

BUG=crbug.com/7508
BUG=crbug.com/422984

junov: core & platform
dglazkov: Source/web & public/web

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

[robertphillips, 2014-10-13 14:11:29]

robertphillips@google.com changed reviewers:
+ bsalomon@chromium.org, junov@chromium.org

[robertphillips, 2014-10-13 14:52:41]

robertphillips@google.com changed reviewers:
+ adamk@chromium.org

[robertphillips, 2014-10-13 14:55:19]

robertphillips@google.com changed reviewers:
+ dglazkov@chromium.org
- adamk@chromium.org

[robertphillips, 2014-10-13 14:55:46]

robertphillips@google.com changed reviewers:
- bsalomon@chromium.org

[dglazkov, 2014-10-13 15:46:27]

On 2014/10/13 at 14:57:41, robertphillips wrote:
> 

Give me a little bit more context... Why do we need this flag? Is this a
user/developer-visible change? Is there a corresponding
intent-to-implement/ship? Might be good to put these into a bug description.

[robertphillips, 2014-10-13 16:01:17]

On 2014/10/13 15:46:27, dglazkov wrote:
> On 2014/10/13 at 14:57:41, robertphillips wrote:
> > 
> 
> Give me a little bit more context... Why do we need this flag? Is this a
> user/developer-visible change? Is there a corresponding
> intent-to-implement/ship? Might be good to put these into a bug description.

Currently canvas clips are not antialiased which differs what what other
browsers are doing and what the users expect. Skia supports antialiased clipping
but Skia's gpu backend (which is used for canvas) can be significantly slower
for concave clips. 

This CL starts surfacing the desired antialiased clipping behavior behind a flag
so developers can start assessing the capability. There is a lot more work that
needs to occur in Skia before we have a consistently good story for gpu-backed
antialiased clipping so we will not be ready to switch behaviors for a while.

[dglazkov, 2014-10-13 16:13:38]

On 2014/10/13 at 15:46:27, dglazkov wrote:
> Might be good to put these into a bug description.

Errr. I meant CL description.

[dglazkov, 2014-10-13 16:14:17]

On 2014/10/13 at 16:01:17, robertphillips wrote:
> On 2014/10/13 15:46:27, dglazkov wrote:
> > On 2014/10/13 at 14:57:41, robertphillips wrote:
> > > 
> > 
> > Give me a little bit more context... Why do we need this flag? Is this a
> > user/developer-visible change? Is there a corresponding
> > intent-to-implement/ship? Might be good to put these into a bug description.
> 
> Currently canvas clips are not antialiased which differs what what other
browsers are doing and what the users expect. Skia supports antialiased clipping
but Skia's gpu backend (which is used for canvas) can be significantly slower
for concave clips. 
> 
> This CL starts surfacing the desired antialiased clipping behavior behind a
flag so developers can start assessing the capability. There is a lot more work
that needs to occur in Skia before we have a consistently good story for
gpu-backed antialiased clipping so we will not be ready to switch behaviors for
a while.

Soo.. sounds like intent-to-implement might be a good idea here?

[robertphillips, 2014-10-13 17:35:35]

On 2014/10/13 16:14:17, dglazkov wrote:
> On 2014/10/13 at 16:01:17, robertphillips wrote:
> > On 2014/10/13 15:46:27, dglazkov wrote:
> > > On 2014/10/13 at 14:57:41, robertphillips wrote:
> > > > 
> > > 
> > > Give me a little bit more context... Why do we need this flag? Is this a
> > > user/developer-visible change? Is there a corresponding
> > > intent-to-implement/ship? Might be good to put these into a bug
description.
> > 
> > Currently canvas clips are not antialiased which differs what what other
> browsers are doing and what the users expect. Skia supports antialiased
clipping
> but Skia's gpu backend (which is used for canvas) can be significantly slower
> for concave clips. 
> > 
> > This CL starts surfacing the desired antialiased clipping behavior behind a
> flag so developers can start assessing the capability. There is a lot more
work
> that needs to occur in Skia before we have a consistently good story for
> gpu-backed antialiased clipping so we will not be ready to switch behaviors
for
> a while.
> 
> Soo.. sounds like intent-to-implement might be a good idea here?

I believe I have correctly initiated the intent-to-implement process and have
updated the CL description.

[dglazkov, 2014-10-13 18:05:53]

On 2014/10/13 at 17:35:35, robertphillips wrote:
> I believe I have correctly initiated the intent-to-implement process and have
updated the CL description.

\o/ Source/web & public/web LGTM.

[Justin Novosad, 2014-10-14 14:46:43]

https://codereview.chromium.org/652483003/diff/160001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/652483003/diff/160001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:176: m_context =
CanvasRenderingContext2D::create(this,
static_cast<Canvas2DContextAttributes*>(attrs), document().inQuirksMode(),
antialiasClips);
Me wonders if we should just pass 'this' or 'document()' to
CanvasRenderingContext2D::create so that it can extract whatever settings it
needs (quirksMode & antialias clips).

https://codereview.chromium.org/652483003/diff/160001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/652483003/diff/160001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:1575: clipPath(path, antialias ?
AntiAliased : NotAntiAliased);
Since we already have this enumr this, we should use it rather than bool for the
antialias argument to this function. Same for
GraphicsRenderingContext2D::m_antialiasClips, etc. 
The rule in blink is that we use enum rather than bool whenever the argument
would be specified as a literal, which is not the case in any of the new code,
but since we have to convert here, we may as well convert to enum earlier.

[robertphillips, 2014-10-16 13:24:18]

https://codereview.chromium.org/652483003/diff/160001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/652483003/diff/160001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:176: m_context =
CanvasRenderingContext2D::create(this,
static_cast<Canvas2DContextAttributes*>(attrs), document().inQuirksMode(),
antialiasClips);
On 2014/10/14 14:46:43, junov wrote:
> Me wonders if we should just pass 'this' or 'document()' to
> CanvasRenderingContext2D::create so that it can extract whatever settings it
> needs (quirksMode & antialias clips).

Done - passed in document.

https://codereview.chromium.org/652483003/diff/160001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/652483003/diff/160001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:1575: clipPath(path, antialias ?
AntiAliased : NotAntiAliased);
On 2014/10/14 14:46:43, junov wrote:
> Since we already have this enumr this, we should use it rather than bool for
the
> antialias argument to this function. Same for
> GraphicsRenderingContext2D::m_antialiasClips, etc. 
> The rule in blink is that we use enum rather than bool whenever the argument
> would be specified as a literal, which is not the case in any of the new code,
> but since we have to convert here, we may as well convert to enum earlier.

Done.

[Justin Novosad, 2014-10-16 15:34:55]

lgtm

[robertphillips, 2014-10-16 16:23:44]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-10-16 16:24:04]

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

[commit-bot: I haz the power, 2014-10-16 17:31:20]

Committed patchset #4 (id:280001) as 183830