This CL implements the first draft of Canvas 2D Context Attributes, aka getContext('2d', { alpha: f…

This CL implements the first draft of Canvas 2D Context Attributes, aka getContext('2d', { alpha: false }).

See also WhatWG proposal http://wiki.whatwg.org/wiki/CanvasOpaque and OWP launch bug http://crbug.com/234297.

Canvas2DLayerBridge and ImageBuffer now have an opacityMode in their constructor, which indicates NonOpaque (the usual, RGBA buffer) or Opaque.
ImageBuffer will be cleared to opaque black instead of transparent black.  Canvas2DLayerBridge will also mark its compositing layer as opaque if set. CanvasRenderingContext2D creates its buffers as Opaque when the getContext() call includes { alpha: false }.

To avoid flag proliferation, this feature is protected by the new "experimental canvas features" flag, under which it and other not-ready-to-ship-by-default canvas features will be developed.

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

[Stephen White, 2013-04-25 16:56:44]

James, could you take a look?  Thanks!

[jamesr, 2013-04-26 00:41:31]

https://codereview.chromium.org/14298018/diff/9027/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp (right):

https://codereview.chromium.org/14298018/diff/9027/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:81:
Canvas2DContextAttributes* canvas2DAttrs =
static_cast<Canvas2DContextAttributes*>(attrs.get());
do you have to downcast? can you just keep a more specific pointer and then
assign it to the base attrs after the fact?

https://codereview.chromium.org/14298018/diff/9027/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:86:
canvas2DAttrs->setAlpha(jsAttrs->Get(alpha)->BooleanValue());
this seems to imply that the default value for 'alpha' is false, but don't 2d
canvases get an alpha channel by default?

https://codereview.chromium.org/14298018/diff/9027/Source/core/html/canvas/Ca...
File Source/core/html/canvas/Canvas2DContextAttributes.h (right):

https://codereview.chromium.org/14298018/diff/9027/Source/core/html/canvas/Ca...
Source/core/html/canvas/Canvas2DContextAttributes.h:35: class
Canvas2DContextAttributes : public CanvasContextAttributes {
WebGLContextAttributes has an alpha bit. why can't you just promote that up to
the base class?

[Stephen White, 2013-04-26 15:51:03]

Thanks for your review!

https://codereview.chromium.org/14298018/diff/9027/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp (right):

https://codereview.chromium.org/14298018/diff/9027/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:81:
Canvas2DContextAttributes* canvas2DAttrs =
static_cast<Canvas2DContextAttributes*>(attrs.get());
On 2013/04/26 00:41:31, jamesr wrote:
> do you have to downcast? can you just keep a more specific pointer and then
> assign it to the base attrs after the fact?

Good point. I was blindly following the pattern of the WebGL case above. I've
changed them both.

https://codereview.chromium.org/14298018/diff/9027/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:86:
canvas2DAttrs->setAlpha(jsAttrs->Get(alpha)->BooleanValue());
On 2013/04/26 00:41:31, jamesr wrote:
> this seems to imply that the default value for 'alpha' is false, but don't 2d
> canvases get an alpha channel by default?

I think this should work: Canvas2DContextAttributes::m_alpha is set to true in
the constructor, so if the object isn't specified, or if it doesn't contain an
"alpha" property, it should default to true.  I've added a test case for the
latter (the former is test case 1).

https://codereview.chromium.org/14298018/diff/9027/Source/core/html/canvas/Ca...
File Source/core/html/canvas/Canvas2DContextAttributes.h (right):

https://codereview.chromium.org/14298018/diff/9027/Source/core/html/canvas/Ca...
Source/core/html/canvas/Canvas2DContextAttributes.h:35: class
Canvas2DContextAttributes : public CanvasContextAttributes {
On 2013/04/26 00:41:31, jamesr wrote:
> WebGLContextAttributes has an alpha bit. why can't you just promote that up to
> the base class?

On the C++ side, WebGLContextAttributes stores all its data in a
GraphicsContext3D::Attributes internally, so I'd have to pull it out of there
and change the callers to reference the new flag as well as the ::Attributes. I
don't see a big benefit to that.

On the IDL side, there is no base class between the two currently, so I think
that would have to be changed for both WebGL and Canvas at the spec level
(they're just two dictionaries that quack alike).

[Stephen White, 2013-04-26 18:18:57]

New version up w/changes for review comments.  PTAL

[jamesr, 2013-04-26 18:30:09]

https://codereview.chromium.org/14298018/diff/22001/Source/core/html/HTMLCanv...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/14298018/diff/22001/Source/core/html/HTMLCanv...
Source/core/html/HTMLCanvasElement.cpp:164: Settings* settings =
document()->settings();
not sure why this moved, it doesn't appear to be used

https://codereview.chromium.org/14298018/diff/22001/Source/core/html/HTMLCanv...
Source/core/html/HTMLCanvasElement.cpp:337:
context->drawImage(m_presentedImage.get(), ColorSpaceDeviceRGB,
pixelSnappedIntRect(r), !m_context || m_context->hasAlpha() ?
CompositeSourceOver : CompositeCopy, DoNotRespectImageOrientation,
useLowQualityScale);
i'd suggest moving the ternary bit out to a local since it's duplicated in both
the if and else branches

https://codereview.chromium.org/14298018/diff/22001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/14298018/diff/22001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2258: // prevent the user
from mutating any cached version.
i don't follow - what is the 'user' here? javascript?

https://codereview.chromium.org/14298018/diff/22001/Source/core/platform/grap...
File Source/core/platform/graphics/skia/ImageBufferSkia.cpp (right):

https://codereview.chromium.org/14298018/diff/22001/Source/core/platform/grap...
Source/core/platform/graphics/skia/ImageBufferSkia.cpp:172:
m_context->setShouldSmoothFonts(opacityMode == Opaque);
is this a sufficient check? what about transforms, etc etc?

[Stephen White, 2013-04-26 18:53:18]

New version up.

https://codereview.chromium.org/14298018/diff/22001/Source/core/html/HTMLCanv...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/14298018/diff/22001/Source/core/html/HTMLCanv...
Source/core/html/HTMLCanvasElement.cpp:164: Settings* settings =
document()->settings();
On 2013/04/26 18:30:09, jamesr wrote:
> not sure why this moved, it doesn't appear to be used

Leftover from an earlier change.  Reverted.

https://codereview.chromium.org/14298018/diff/22001/Source/core/html/HTMLCanv...
Source/core/html/HTMLCanvasElement.cpp:337:
context->drawImage(m_presentedImage.get(), ColorSpaceDeviceRGB,
pixelSnappedIntRect(r), !m_context || m_context->hasAlpha() ?
CompositeSourceOver : CompositeCopy, DoNotRespectImageOrientation,
useLowQualityScale);
On 2013/04/26 18:30:09, jamesr wrote:
> i'd suggest moving the ternary bit out to a local since it's duplicated in
both
> the if and else branches

Done.

https://codereview.chromium.org/14298018/diff/22001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/14298018/diff/22001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2258: // prevent the user
from mutating any cached version.
On 2013/04/26 18:30:09, jamesr wrote:
> i don't follow - what is the 'user' here? javascript?

This comment was lifted from WebGLRenderingContext::getContextAttributes().

s/user/caller/

[jamesr, 2013-04-26 20:03:07]

On 2013/04/26 18:53:18, Stephen White wrote:
>
https://codereview.chromium.org/14298018/diff/22001/Source/core/html/canvas/C...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/14298018/diff/22001/Source/core/html/canvas/C...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:2258: // prevent the user
> from mutating any cached version.
> On 2013/04/26 18:30:09, jamesr wrote:
> > i don't follow - what is the 'user' here? javascript?
> 
> This comment was lifted from WebGLRenderingContext::getContextAttributes().
> 
> s/user/caller/

I don't understand what it's doing in there either.  What does this code do?  If
you just return a ref to the existing attributes, what breaks?

[Stephen White, 2013-04-27 10:44:12]

On 2013/04/26 20:03:07, jamesr wrote:
> On 2013/04/26 18:53:18, Stephen White wrote:
> >
>
https://codereview.chromium.org/14298018/diff/22001/Source/core/html/canvas/C...
> > File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> > 
> >
>
https://codereview.chromium.org/14298018/diff/22001/Source/core/html/canvas/C...
> > Source/core/html/canvas/CanvasRenderingContext2D.cpp:2258: // prevent the
user
> > from mutating any cached version.
> > On 2013/04/26 18:30:09, jamesr wrote:
> > > i don't follow - what is the 'user' here? javascript?
> > 
> > This comment was lifted from WebGLRenderingContext::getContextAttributes().
> > 
> > s/user/caller/
> 
> I don't understand what it's doing in there either.  What does this code do? 
If
> you just return a ref to the existing attributes, what breaks?

I think the comment is trying to say that attempts to mutate the existing
attributes not only shouldn't affect the canvas's opacity, but that they
shouldn't affect the values returned by subsequent calls to
getContextAttributes().  e.g.,

var ctx = canvas.getContext('2d', { alpha: false });
var attrs = canvas.getContextAttributes();
attrs.alpha = true;
var attrs2 = canvas.getContextAttributes();
// attrs2.alpha should still be false here

I'll add this as a test.

On other hand, the comment is kind of misleading, since there are no "existing
attributes" to return -- they're utilized at initialization on getContext(), but
not stored anywhere, so there's no real danger of mutating them in the current
implementation, AFAICT.

[jamesr, 2013-04-28 18:50:39]

I'm pretty sure that copy does nothing for 2d or webgl.  Write some tests so
we're sure, though.

lgtm with that

[Stephen White, 2013-04-29 03:01:08]

Dimitri:  could you take a look for V8HTMLCanvasElementCustom.cpp?  Thanks!

[dglazkov, 2013-04-29 23:12:40]

lgtm.

[commit-bot: I haz the power, 2013-04-29 23:16:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/senorblanco@chromium.org/14298018/27005

[commit-bot: I haz the power, 2013-04-29 23:30:42]

Change committed as 149384