Use dictionaries for context creation attributes. Eliminate custom bindings.

Use dictionaries for context creation attributes. Eliminate custom bindings.

Change Canvas2DContextAttributes and WebGLContextAttributes to
dictionaries, following their specification. Delete the handwritten
C++ code for these types. Add helper functions for working with them.
Fix a bug in the V8 bindings generator related to returning of
nullable dictionary types.

Define a CanvasContextCreationAttributes dictionary containing all of
the context creation attributes for all of the context types, which is
functionally equivalent to the specification's IDL. Delete the custom
binding for HTMLCanvasElement's getContext().

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

[Ken Russell (switch to Gerrit), 2014-12-11 05:44:26]

kbr@chromium.org changed reviewers:
+ bajones@chromium.org, haraken@chromium.org, junov@chromium.org,
yhirano@chromium.org, zmo@chromium.org

[Ken Russell (switch to Gerrit), 2014-12-11 05:44:26]

Please review. Thank you.

This CL could be split into two or three, but the later ones would depend on the
earlier, and I think that's undesirable in case it has to be reverted for some
reason.

junov, bajones, zmo: please review everything.

haraken, yhirano: please review the bindings/ changes, and please also review
the change to HTMLCanvasElement.idl, which doesn't follow the web spec strictly,
but is functionally equivalent and allows a nasty custom binding to be deleted.

[haraken, 2014-12-11 05:46:48]

haraken@chromium.org changed reviewers:
+ bashi@chromium.org

[haraken, 2014-12-11 05:46:49]

+bashi for the dictionary part

[Justin Novosad, 2014-12-11 14:35:23]

The "storageMode" stuff in the 2D context does nothing. It is an aborted
experiment (my bad for forgetting about it and leaving it in the code). Instead
of maintaining it, you could just rip it out.

[Zhenyao Mo, 2014-12-11 17:22:26]

LGTM

[bajones, 2014-12-11 17:38:10]

LGTM, and thanks for getting this done! Death to all custom bindings!

[bashi, 2014-12-12 00:35:24]

Thank you for removing custom bindings! LGTM.

https://codereview.chromium.org/795833004/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/795833004/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:719: # non-nullable dictionaries or unions.
Or we can make IDL dictionaries be 'implicit_nullable'.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

That said, using Nullable<D> looks better because nullable dictionaries are not
commonly used.

https://codereview.chromium.org/795833004/diff/1/Source/core/frame/ImageBitma...
File Source/core/frame/ImageBitmapTest.cpp (right):

https://codereview.chromium.org/795833004/diff/1/Source/core/frame/ImageBitma...
Source/core/frame/ImageBitmapTest.cpp:211: CanvasContextCreationAttributes
attribs;
attribs -> attributes?

https://codereview.chromium.org/795833004/diff/1/Source/core/html/HTMLCanvasE...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/795833004/diff/1/Source/core/html/HTMLCanvasE...
Source/core/html/HTMLCanvasElement.idl:39: RenderingContext
getContext([Default=Undefined] optional DOMString contextId, optional
CanvasContextCreationAttributes attributes);
The return value is not nullable. Is this intentional too? If this is due to our
code generator's restriction (or bug), I'd happy to take a look.

https://codereview.chromium.org/795833004/diff/1/Source/core/html/canvas/Canv...
File Source/core/html/canvas/Canvas2DContextAttributes.idl (right):

https://codereview.chromium.org/795833004/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/Canvas2DContextAttributes.idl:30: // extended attribute
for dictionary members.
Could you replace "Note" with "FIXME"? I filed a bug entry for it.
http://crbug.com/441569

[haraken, 2014-12-12 00:40:55]

rubberstamp LGTM based on bashi-san's LG.

[yhirano, 2014-12-12 02:17:20]

lgtm

[Ken Russell (switch to Gerrit), 2014-12-12 06:53:43]

On 2014/12/11 14:35:23, junov wrote:
> The "storageMode" stuff in the 2D context does nothing. It is an aborted
> experiment (my bad for forgetting about it and leaving it in the code).
Instead
> of maintaining it, you could just rip it out.

Thanks. I'm going to do that in the next patch set.

[Ken Russell (switch to Gerrit), 2014-12-12 06:59:31]

bashi, bajones, junov: please take another look.

Again, I'm sorry for how large this patch has become, but given its current
state, I would still like to land it in one piece instead of splitting it into
two.

Changes since the last patch:

 - Fixed bugs in the context-attributes-default-value tests. They were incorrect
when passing null for the dictionaries' attributes. I verified this change is
correct by reading the Web IDL and ECMAScript specs.

 - Fixed the inspector/ layout test failures. I accidentally deleted the
InspectorInstrumentation calls along with the custom binding for getContext().
Used [CallWith=ScriptState] to allow the custom wrapping code to be added. Added
WrapCanvasContext.{cpp,h} for this purpose, added ScriptValue::createNull(), and
used it in place of WebGLAny(ScriptState*).

 - Fixed the canvas/philip/tests/2d.getcontext.extraargs.html failure. Added the
PermissiveDictionaryConversion extended attribute for this and documented it at
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes . Added
bindings tests for the new extended attribute. Suggestions welcome on how to
simplify this. Ideally it would only be specified on the dictionary type, but I
couldn't figure out how to get the extended attributes for the dictionary in the
context for the method's argument.

 - Addressed review feedback. Deleted the storageMode attribute entirely.

https://codereview.chromium.org/795833004/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/795833004/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:719: # non-nullable dictionaries or unions.
On 2014/12/12 00:35:24, bashi1 wrote:
> Or we can make IDL dictionaries be 'implicit_nullable'.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> That said, using Nullable<D> looks better because nullable dictionaries are
not
> commonly used.

I'd rather not make IDL dictionaries implicit_nullable. As mentioned below,
passing a nullable dictionary as an argument needs a lot more work, and no place
on the web platform requires that pattern. However, returning a nullable
dictionary from getContextAttributes does need to be supported (and is tested by
the WebGL conformance tests).

https://codereview.chromium.org/795833004/diff/1/Source/core/frame/ImageBitma...
File Source/core/frame/ImageBitmapTest.cpp (right):

https://codereview.chromium.org/795833004/diff/1/Source/core/frame/ImageBitma...
Source/core/frame/ImageBitmapTest.cpp:211: CanvasContextCreationAttributes
attribs;
On 2014/12/12 00:35:24, bashi1 wrote:
> attribs -> attributes?

Sure. Done.

https://codereview.chromium.org/795833004/diff/1/Source/core/html/HTMLCanvasE...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/795833004/diff/1/Source/core/html/HTMLCanvasE...
Source/core/html/HTMLCanvasElement.idl:39: RenderingContext
getContext([Default=Undefined] optional DOMString contextId, optional
CanvasContextCreationAttributes attributes);
On 2014/12/12 00:35:24, bashi1 wrote:
> The return value is not nullable. Is this intentional too? If this is due to
our
> code generator's restriction (or bug), I'd happy to take a look.

No, that was an accident, and the code generator would have handled it correctly
-- but it turns out that in order to handle the InspectorInstrumentation's
wrapping of canvas contexts, I had to change the calling convention of this
method anyway, and add a little handwritten code in the epilogue. Please see the
next patch.

https://codereview.chromium.org/795833004/diff/1/Source/core/html/canvas/Canv...
File Source/core/html/canvas/Canvas2DContextAttributes.idl (right):

https://codereview.chromium.org/795833004/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/Canvas2DContextAttributes.idl:30: // extended attribute
for dictionary members.
On 2014/12/12 00:35:24, bashi1 wrote:
> Could you replace "Note" with "FIXME"? I filed a bug entry for it.
> http://crbug.com/441569

Actually, I've deleted this member based on junov's code review feedback.

[bashi, 2014-12-12 08:31:33]

https://codereview.chromium.org/795833004/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/795833004/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:719: # non-nullable dictionaries or unions.
On 2014/12/12 06:59:31, Ken Russell wrote:
> On 2014/12/12 00:35:24, bashi1 wrote:
> > Or we can make IDL dictionaries be 'implicit_nullable'.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > That said, using Nullable<D> looks better because nullable dictionaries are
> not
> > commonly used.
> 
> I'd rather not make IDL dictionaries implicit_nullable. As mentioned below,
> passing a nullable dictionary as an argument needs a lot more work, and no
place
> on the web platform requires that pattern. However, returning a nullable
> dictionary from getContextAttributes does need to be supported (and is tested
by
> the WebGL conformance tests).

Acknowledged.

https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.idl:49: [CallWith=ScriptState] any
getContext([Default=Undefined] optional DOMString contextId,
[PermissiveDictionaryConversion] optional CanvasContextCreationAttributes
attributes);
Let me ask a question: is it possible to use union types here instead of adding
PermissiveDictionaryConversion? (e.g. (CanvasContextCreationAttributes or any)).

I was wondering if we could use the standard features, but I prefer this
approach if using union types could make the CL more complicated.

[dshwang, 2014-12-12 14:46:37]

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

[dshwang, 2014-12-12 14:46:38]

nice clean up!

https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.idl:49: [CallWith=ScriptState] any
getContext([Default=Undefined] optional DOMString contextId,
[PermissiveDictionaryConversion] optional CanvasContextCreationAttributes
attributes);
RenderingContext? getContext(...) doesn't require custom binding. The counter
example is Document.idl
RenderingContext getCSSCanvasContext(DOMString contextId, DOMString name, long
width, long height);

If you define "RenderingContext? getContext(...)", the binding code get the
CanvasRenderingContext2DOrWebGLRenderingContext& argument for return value like
Document::getCSSCanvasContext()
It has two benefits;
1. HTMLCanvasElement::getContext can be unified.
2. WrapCanvasContext is not needed.

[Ken Russell (switch to Gerrit), 2014-12-12 19:09:39]

https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.idl:49: [CallWith=ScriptState] any
getContext([Default=Undefined] optional DOMString contextId,
[PermissiveDictionaryConversion] optional CanvasContextCreationAttributes
attributes);
On 2014/12/12 08:31:33, bashi1 wrote:
> Let me ask a question: is it possible to use union types here instead of
adding
> PermissiveDictionaryConversion? (e.g. (CanvasContextCreationAttributes or
any)).
> 
> I was wondering if we could use the standard features, but I prefer this
> approach if using union types could make the CL more complicated.

Sorry, I just tested this and it's not possible. The code generator doesn't
allow "any" in a union type (not unexpected).

https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.idl:49: [CallWith=ScriptState] any
getContext([Default=Undefined] optional DOMString contextId,
[PermissiveDictionaryConversion] optional CanvasContextCreationAttributes
attributes);
On 2014/12/12 14:46:37, dshwang wrote:
> RenderingContext? getContext(...) doesn't require custom binding. The counter
> example is Document.idl
> RenderingContext getCSSCanvasContext(DOMString contextId, DOMString name, long
> width, long height);
> 
> If you define "RenderingContext? getContext(...)", the binding code get the
> CanvasRenderingContext2DOrWebGLRenderingContext& argument for return value
like
> Document::getCSSCanvasContext()
> It has two benefits;
> 1. HTMLCanvasElement::getContext can be unified.
> 2. WrapCanvasContext is not needed.

The manually-written epilogue is required in order to call
InspectorInstrumentation's wrapCanvas2DRenderingContextForInstrumentation and
wrapWebGLRenderingContextForInstrumentation. Otherwise the inspector layout
tests break. It would be better if these hooks were implemented differently, but
this CL can't break that functionality.

[Justin Novosad, 2014-12-12 19:36:51]

lgtm

[dshwang, 2014-12-12 21:53:15]

https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.idl:49: [CallWith=ScriptState] any
getContext([Default=Undefined] optional DOMString contextId,
[PermissiveDictionaryConversion] optional CanvasContextCreationAttributes
attributes);
On 2014/12/12 19:09:38, Ken Russell wrote:
> On 2014/12/12 14:46:37, dshwang wrote:
> > RenderingContext? getContext(...) doesn't require custom binding. The
counter
> > example is Document.idl
> > RenderingContext getCSSCanvasContext(DOMString contextId, DOMString name,
long
> > width, long height);
> > 
> > If you define "RenderingContext? getContext(...)", the binding code get the
> > CanvasRenderingContext2DOrWebGLRenderingContext& argument for return value
> like
> > Document::getCSSCanvasContext()
> > It has two benefits;
> > 1. HTMLCanvasElement::getContext can be unified.
> > 2. WrapCanvasContext is not needed.
> 
> The manually-written epilogue is required in order to call
> InspectorInstrumentation's wrapCanvas2DRenderingContextForInstrumentation and
> wrapWebGLRenderingContextForInstrumentation. Otherwise the inspector layout
> tests break. It would be better if these hooks were implemented differently,
but
> this CL can't break that functionality.

Thank you for explaining. Can Document.idl be changed like this? If so, I guess
two HTMLCanvasElement::getContext can be unified.

[dshwang, 2014-12-12 21:53:16]

lgtm

https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.idl:49: [CallWith=ScriptState] any
getContext([Default=Undefined] optional DOMString contextId,
[PermissiveDictionaryConversion] optional CanvasContextCreationAttributes
attributes);
On 2014/12/12 19:09:38, Ken Russell wrote:
> On 2014/12/12 14:46:37, dshwang wrote:
> > RenderingContext? getContext(...) doesn't require custom binding. The
counter
> > example is Document.idl
> > RenderingContext getCSSCanvasContext(DOMString contextId, DOMString name,
long
> > width, long height);
> > 
> > If you define "RenderingContext? getContext(...)", the binding code get the
> > CanvasRenderingContext2DOrWebGLRenderingContext& argument for return value
> like
> > Document::getCSSCanvasContext()
> > It has two benefits;
> > 1. HTMLCanvasElement::getContext can be unified.
> > 2. WrapCanvasContext is not needed.
> 
> The manually-written epilogue is required in order to call
> InspectorInstrumentation's wrapCanvas2DRenderingContextForInstrumentation and
> wrapWebGLRenderingContextForInstrumentation. Otherwise the inspector layout
> tests break. It would be better if these hooks were implemented differently,
but
> this CL can't break that functionality.

Thank you for explaining. Can Document.idl be changed like this? If so, I guess
two HTMLCanvasElement::getContext can be unified.

[dshwang, 2014-12-12 21:53:17]

lgtm

[Ken Russell (switch to Gerrit), 2014-12-13 00:04:56]

On 2014/12/12 21:53:16, dshwang wrote:
> lgtm
> 
>
https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCan...
> File Source/core/html/HTMLCanvasElement.idl (right):
> 
>
https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCan...
> Source/core/html/HTMLCanvasElement.idl:49: [CallWith=ScriptState] any
> getContext([Default=Undefined] optional DOMString contextId,
> [PermissiveDictionaryConversion] optional CanvasContextCreationAttributes
> attributes);
> On 2014/12/12 19:09:38, Ken Russell wrote:
> > On 2014/12/12 14:46:37, dshwang wrote:
> > > RenderingContext? getContext(...) doesn't require custom binding. The
> counter
> > > example is Document.idl
> > > RenderingContext getCSSCanvasContext(DOMString contextId, DOMString name,
> long
> > > width, long height);
> > > 
> > > If you define "RenderingContext? getContext(...)", the binding code get
the
> > > CanvasRenderingContext2DOrWebGLRenderingContext& argument for return value
> > like
> > > Document::getCSSCanvasContext()
> > > It has two benefits;
> > > 1. HTMLCanvasElement::getContext can be unified.
> > > 2. WrapCanvasContext is not needed.
> > 
> > The manually-written epilogue is required in order to call
> > InspectorInstrumentation's wrapCanvas2DRenderingContextForInstrumentation
and
> > wrapWebGLRenderingContextForInstrumentation. Otherwise the inspector layout
> > tests break. It would be better if these hooks were implemented differently,
> but
> > this CL can't break that functionality.
> 
> Thank you for explaining. Can Document.idl be changed like this? If so, I
guess
> two HTMLCanvasElement::getContext can be unified.

It's technically possible to do so but I don't think it's worth it. The
InspectorInstrumentation seems to enable only layout tests and experimental
canvas profiling features in DevTools. I'm personally interested in
investigating whether the inspector's code can be restructured to avoid the
handwritten calls to InspectorInstrumentation and plan to file a follow-on bug
about that.

[Ken Russell (switch to Gerrit), 2014-12-13 00:06:34]

I've verified that the layout test failures from the try job of the previous
patch set were unrelated to this CL (they aren't reproducible locally), and
double-checked that my changes to the layout tests are correct. Since all
interested parties have given a thumbs-up and there were no objections to the
last patch set, CQ'ing this now.

[Ken Russell (switch to Gerrit), 2014-12-13 00:06:40]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2014-12-13 00:07:54]

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

[commit-bot: I haz the power, 2014-12-13 03:12:34]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187077

[opettay, 2016-01-31 03:39:17]

So either Chrome had the buggy behavior already before or this bug introduced
that
document.body.innerHTML = "<canvas>"; document.body.firstChild.getContext("2d",
{ get stencil() { throw "Foobar"; } }); now throws in Chrome.
Per specs it shouldn't.

"Define a CanvasContextCreationAttributes dictionary containing all of the
context creation attributes for all of the context types, which is functionally
equivalent to the specification's IDL."
Is very much not true. Per spec you need to access all the relevant getters, so
now Chrome accesses all sorts of getters which have nothing to do with with 2d
context.
Per HTML spec 2d case should try to convert the object only to
CanvasRenderingContext2DSettings (which has just one property, alpha)

[opettay, 2016-01-31 03:41:01]

(It was https://bugzilla.mozilla.org/show_bug.cgi?id=1244480 which caused me to
test what browser do here.)

[simonp, 2016-02-01 04:22:14]

Also see this spec bug and in particular this comment:

https://github.com/whatwg/html/issues/595#issuecomment-177409473