Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(33)

Issue 795833004: Use dictionaries for context creation attributes. Eliminate custom bindings. (Closed)

Created:
5 years, 9 months ago by Ken Russell (switch to Gerrit)
Modified:
4 years, 7 months ago
CC:
aandrey+blink_chromium.org, arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, Rik, Inactive, dglazkov+blink, dshwang, eae+blinkwatch, rwlbuis, sof
Target Ref:
refs/remotes/origin/master
Project:
blink
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 9

Patch Set 2 : Fixed layout test failures. Addressed review feedback. #

Total comments: 5

Patch Set 3 : Rebased. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+587 lines, -764 lines) Patch
M LayoutTests/fast/canvas/canvas-context-attributes-default-value.html View 1 2 chunks +8 lines, -4 lines 0 comments Download
M LayoutTests/fast/canvas/canvas-context-attributes-default-value-expected.txt View 1 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/canvas/webgl/webgl-context-attributes-default-value.html View 1 3 chunks +16 lines, -4 lines 0 comments Download
M LayoutTests/fast/canvas/webgl/webgl-context-attributes-default-value-expected.txt View 1 1 chunk +3 lines, -3 lines 0 comments Download
M Source/bindings/IDLExtendedAttributes.txt View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/bindings/core/v8/ScriptValue.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/bindings/core/v8/ScriptValue.cpp View 1 1 chunk +5 lines, -0 lines 0 comments Download
M Source/bindings/core/v8/WebGLAny.h View 1 1 chunk +0 lines, -1 line 0 comments Download
M Source/bindings/core/v8/WebGLAny.cpp View 1 1 chunk +0 lines, -5 lines 0 comments Download
A Source/bindings/core/v8/WrapCanvasContext.h View 1 1 chunk +22 lines, -0 lines 0 comments Download
A Source/bindings/core/v8/WrapCanvasContext.cpp View 1 1 chunk +41 lines, -0 lines 0 comments Download
M Source/bindings/core/v8/custom/V8HTMLCanvasElementCustom.cpp View 1 chunk +0 lines, -82 lines 0 comments Download
M Source/bindings/core/v8/v8.gypi View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/bindings/scripts/v8_dictionary.py View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/bindings/scripts/v8_methods.py View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/bindings/scripts/v8_types.py View 3 chunks +16 lines, -0 lines 0 comments Download
M Source/bindings/templates/dictionary_v8.cpp View 1 1 chunk +4 lines, -0 lines 0 comments Download
M Source/bindings/templates/methods.cpp View 1 2 chunks +2 lines, -0 lines 0 comments Download
M Source/bindings/tests/idls/core/TestObject.idl View 1 1 chunk +2 lines, -0 lines 0 comments Download
A Source/bindings/tests/idls/core/TestPermissiveDictionary.idl View 1 1 chunk +14 lines, -0 lines 0 comments Download
A Source/bindings/tests/results/core/TestPermissiveDictionary.h View 1 1 chunk +34 lines, -0 lines 0 comments Download
A + Source/bindings/tests/results/core/TestPermissiveDictionary.cpp View 1 1 chunk +3 lines, -4 lines 0 comments Download
M Source/bindings/tests/results/core/V8TestObject.cpp View 1 2 chunks +38 lines, -0 lines 0 comments Download
A + Source/bindings/tests/results/core/V8TestPermissiveDictionary.h View 1 1 chunk +11 lines, -11 lines 0 comments Download
A Source/bindings/tests/results/core/V8TestPermissiveDictionary.cpp View 1 1 chunk +61 lines, -0 lines 0 comments Download
M Source/core/core.gypi View 1 2 7 chunks +11 lines, -8 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 2 chunks +2 lines, -9 lines 0 comments Download
M Source/core/frame/ImageBitmapTest.cpp View 1 2 chunks +5 lines, -2 lines 0 comments Download
M Source/core/html/HTMLCanvasElement.h View 1 3 chunks +7 lines, -2 lines 0 comments Download
M Source/core/html/HTMLCanvasElement.cpp View 1 2 4 chunks +26 lines, -8 lines 0 comments Download
M Source/core/html/HTMLCanvasElement.idl View 1 2 chunks +16 lines, -2 lines 0 comments Download
D Source/core/html/canvas/Canvas2DContextAttributes.h View 1 chunk +0 lines, -65 lines 0 comments Download
D Source/core/html/canvas/Canvas2DContextAttributes.cpp View 1 chunk +0 lines, -76 lines 0 comments Download
M Source/core/html/canvas/Canvas2DContextAttributes.idl View 1 chunk +5 lines, -6 lines 0 comments Download
D Source/core/html/canvas/CanvasContextAttributes.h View 1 chunk +0 lines, -49 lines 0 comments Download
D Source/core/html/canvas/CanvasContextAttributes.cpp View 1 chunk +0 lines, -39 lines 0 comments Download
A Source/core/html/canvas/CanvasContextCreationAttributes.idl View 1 1 chunk +36 lines, -0 lines 0 comments Download
M Source/core/html/canvas/CanvasRenderingContext2D.h View 1 5 chunks +4 lines, -4 lines 0 comments Download
M Source/core/html/canvas/CanvasRenderingContext2D.cpp View 1 2 chunks +4 lines, -7 lines 0 comments Download
A Source/core/html/canvas/ContextAttributeHelpers.h View 1 chunk +27 lines, -0 lines 0 comments Download
A Source/core/html/canvas/ContextAttributeHelpers.cpp View 1 1 chunk +57 lines, -0 lines 0 comments Download
D Source/core/html/canvas/WebGLContextAttributes.h View 1 chunk +0 lines, -102 lines 0 comments Download
D Source/core/html/canvas/WebGLContextAttributes.cpp View 1 chunk +0 lines, -169 lines 0 comments Download
M Source/core/html/canvas/WebGLContextAttributes.idl View 1 chunk +10 lines, -11 lines 0 comments Download
M Source/core/html/canvas/WebGLRenderingContext.h View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/html/canvas/WebGLRenderingContext.cpp View 6 chunks +7 lines, -12 lines 0 comments Download
M Source/core/html/canvas/WebGLRenderingContextBase.h View 5 chunks +4 lines, -4 lines 0 comments Download
M Source/core/html/canvas/WebGLRenderingContextBase.cpp View 1 33 chunks +75 lines, -71 lines 0 comments Download
M Source/core/html/canvas/WebGLRenderingContextBase.idl View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 28 (4 generated)
Ken Russell (switch to Gerrit)
Please review. Thank you. This CL could be split into two or three, but the ...
5 years, 9 months ago (2014-12-11 05:44:26 UTC) #2
haraken
+bashi for the dictionary part
5 years, 9 months ago (2014-12-11 05:46:49 UTC) #4
Justin Novosad
The "storageMode" stuff in the 2D context does nothing. It is an aborted experiment (my ...
5 years, 9 months ago (2014-12-11 14:35:23 UTC) #5
Zhenyao Mo
LGTM
5 years, 9 months ago (2014-12-11 17:22:26 UTC) #6
bajones
LGTM, and thanks for getting this done! Death to all custom bindings!
5 years, 9 months ago (2014-12-11 17:38:10 UTC) #7
bashi
Thank you for removing custom bindings! LGTM. https://codereview.chromium.org/795833004/diff/1/Source/bindings/scripts/v8_types.py File Source/bindings/scripts/v8_types.py (right): https://codereview.chromium.org/795833004/diff/1/Source/bindings/scripts/v8_types.py#newcode719 Source/bindings/scripts/v8_types.py:719: # non-nullable ...
5 years, 9 months ago (2014-12-12 00:35:24 UTC) #8
haraken
rubberstamp LGTM based on bashi-san's LG.
5 years, 9 months ago (2014-12-12 00:40:55 UTC) #9
yhirano
lgtm
5 years, 9 months ago (2014-12-12 02:17:20 UTC) #10
Ken Russell (switch to Gerrit)
On 2014/12/11 14:35:23, junov wrote: > The "storageMode" stuff in the 2D context does nothing. ...
5 years, 9 months ago (2014-12-12 06:53:43 UTC) #11
Ken Russell (switch to Gerrit)
bashi, bajones, junov: please take another look. Again, I'm sorry for how large this patch ...
5 years, 9 months ago (2014-12-12 06:59:31 UTC) #12
bashi
https://codereview.chromium.org/795833004/diff/1/Source/bindings/scripts/v8_types.py File Source/bindings/scripts/v8_types.py (right): https://codereview.chromium.org/795833004/diff/1/Source/bindings/scripts/v8_types.py#newcode719 Source/bindings/scripts/v8_types.py:719: # non-nullable dictionaries or unions. On 2014/12/12 06:59:31, Ken ...
5 years, 9 months ago (2014-12-12 08:31:33 UTC) #13
dshwang
nice clean up! https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCanvasElement.idl File Source/core/html/HTMLCanvasElement.idl (right): https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCanvasElement.idl#newcode49 Source/core/html/HTMLCanvasElement.idl:49: [CallWith=ScriptState] any getContext([Default=Undefined] optional DOMString contextId, ...
5 years, 9 months ago (2014-12-12 14:46:38 UTC) #15
Ken Russell (switch to Gerrit)
https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCanvasElement.idl File Source/core/html/HTMLCanvasElement.idl (right): https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCanvasElement.idl#newcode49 Source/core/html/HTMLCanvasElement.idl:49: [CallWith=ScriptState] any getContext([Default=Undefined] optional DOMString contextId, [PermissiveDictionaryConversion] optional CanvasContextCreationAttributes ...
5 years, 9 months ago (2014-12-12 19:09:39 UTC) #16
Justin Novosad
lgtm
5 years, 9 months ago (2014-12-12 19:36:51 UTC) #17
dshwang
https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCanvasElement.idl File Source/core/html/HTMLCanvasElement.idl (right): https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCanvasElement.idl#newcode49 Source/core/html/HTMLCanvasElement.idl:49: [CallWith=ScriptState] any getContext([Default=Undefined] optional DOMString contextId, [PermissiveDictionaryConversion] optional CanvasContextCreationAttributes ...
5 years, 9 months ago (2014-12-12 21:53:15 UTC) #18
dshwang
lgtm https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCanvasElement.idl File Source/core/html/HTMLCanvasElement.idl (right): https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCanvasElement.idl#newcode49 Source/core/html/HTMLCanvasElement.idl:49: [CallWith=ScriptState] any getContext([Default=Undefined] optional DOMString contextId, [PermissiveDictionaryConversion] optional ...
5 years, 9 months ago (2014-12-12 21:53:16 UTC) #19
dshwang
lgtm
5 years, 9 months ago (2014-12-12 21:53:17 UTC) #20
Ken Russell (switch to Gerrit)
On 2014/12/12 21:53:16, dshwang wrote: > lgtm > > https://codereview.chromium.org/795833004/diff/20001/Source/core/html/HTMLCanvasElement.idl > File Source/core/html/HTMLCanvasElement.idl (right): > ...
5 years, 9 months ago (2014-12-13 00:04:56 UTC) #21
Ken Russell (switch to Gerrit)
I've verified that the layout test failures from the try job of the previous patch ...
5 years, 9 months ago (2014-12-13 00:06:34 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/795833004/40001
5 years, 9 months ago (2014-12-13 00:07:54 UTC) #24
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187077
5 years, 9 months ago (2014-12-13 03:12:34 UTC) #25
opettay
So either Chrome had the buggy behavior already before or this bug introduced that document.body.innerHTML ...
4 years, 7 months ago (2016-01-31 03:39:17 UTC) #26
opettay
(It was https://bugzilla.mozilla.org/show_bug.cgi?id=1244480 which caused me to test what browser do here.)
4 years, 7 months ago (2016-01-31 03:41:01 UTC) #27
simonp
4 years, 7 months ago (2016-02-01 04:22:14 UTC) #28
Message was sent while issue was closed.
Also see this spec bug and in particular this comment:

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

Powered by Google App Engine
This is Rietveld 408576698