Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(368)

Issue 340443004: IDL: reuse more code between CG for methods and constructors (Closed)

Created:
6 years, 6 months ago by Jens Widell
Modified:
6 years, 6 months ago
CC:
blink-reviews, blink-reviews-html_chromium.org, arv+blink, philipj_slow, eric.carlson_apple.com, abarth-chromium, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews-bindings_chromium.org, Inactive, watchdog-blink-watchlist_google.com
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

IDL: reuse more code between CG for methods and constructors The CG for constructors and named constructors had its own simplified mechanism for generating the actual call to the C++ constructor, rather than using v8_methods.cpp_value(). Except for constructors with optional arguments without default values, which used cpp_value() via v8_methods.generate_argument() to generate the "short-cut" call used when the function is called without the optional arguments. With this patch, v8_methods.cpp_value() is used for all calls. This will make it easier to adjust the handling of arguments later on in the more complex cases. Some IDL files that declare named constructors are modified by adding ConstructorCallWith=Document, which was previously implied for named constructors. BUG=258153 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176390

Patch Set 1 #

Total comments: 13

Patch Set 2 : rename scriptContext -> executionContext #

Unified diffs Side-by-side diffs Delta from patch set Stats (+90 lines, -85 lines) Patch
M Source/bindings/scripts/idl_definitions.py View 2 chunks +2 lines, -0 lines 0 comments Download
M Source/bindings/scripts/v8_attributes.py View 2 chunks +5 lines, -2 lines 0 comments Download
M Source/bindings/scripts/v8_interface.py View 3 chunks +4 lines, -21 lines 0 comments Download
M Source/bindings/scripts/v8_methods.py View 2 chunks +9 lines, -2 lines 0 comments Download
M Source/bindings/scripts/v8_utilities.py View 1 2 chunks +4 lines, -4 lines 0 comments Download
M Source/bindings/templates/attributes.cpp View 1 2 chunks +2 lines, -2 lines 0 comments Download
M Source/bindings/templates/methods.cpp View 1 4 chunks +8 lines, -7 lines 0 comments Download
M Source/bindings/tests/idls/TestInterfaceEventTarget.idl View 1 chunk +1 line, -0 lines 0 comments Download
M Source/bindings/tests/idls/TestInterfaceNamedConstructor.idl View 1 chunk +1 line, -0 lines 0 comments Download
M Source/bindings/tests/results/V8TestInterface.cpp View 1 4 chunks +8 lines, -8 lines 0 comments Download
M Source/bindings/tests/results/V8TestInterfaceConstructor.cpp View 1 2 chunks +4 lines, -4 lines 0 comments Download
M Source/bindings/tests/results/V8TestInterfaceEventTarget.cpp View 1 chunk +5 lines, -4 lines 0 comments Download
M Source/bindings/tests/results/V8TestInterfaceNamedConstructor.cpp View 3 chunks +6 lines, -5 lines 0 comments Download
M Source/bindings/tests/results/V8TestInterfaceNamedConstructor2.cpp View 2 chunks +5 lines, -4 lines 0 comments Download
M Source/bindings/tests/results/V8TestInterfaceWillBeGarbageCollected.cpp View 2 chunks +5 lines, -4 lines 0 comments Download
M Source/bindings/tests/results/V8TestObject.cpp View 1 8 chunks +16 lines, -16 lines 0 comments Download
M Source/core/html/HTMLAudioElement.idl View 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/html/HTMLImageElement.idl View 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/html/HTMLOptionElement.idl View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
Jens Widell
Please review. Cleanup to make other argument handling changes more straight-forward, by not having (as ...
6 years, 6 months ago (2014-06-17 10:40:21 UTC) #1
haraken
Hmm, I'm neutral about this change. constructor and method are different things. As commented below, ...
6 years, 6 months ago (2014-06-17 11:28:05 UTC) #2
Jens Widell
On 2014/06/17 11:28:05, haraken wrote: > Hmm, I'm neutral about this change. I shall try ...
6 years, 6 months ago (2014-06-17 12:29:41 UTC) #3
haraken
Thanks for the details; I'm convinced :) LGTM. https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/methods.cpp File Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/methods.cpp#newcode492 Source/bindings/templates/methods.cpp:492: ExecutionContext* ...
6 years, 6 months ago (2014-06-17 12:38:54 UTC) #4
Jens Widell
https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/methods.cpp File Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/methods.cpp#newcode492 Source/bindings/templates/methods.cpp:492: ExecutionContext* scriptContext = currentExecutionContext(isolate); On 2014/06/17 12:38:53, haraken wrote: ...
6 years, 6 months ago (2014-06-17 12:53:16 UTC) #5
haraken
https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/methods.cpp File Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/methods.cpp#newcode538 Source/bindings/templates/methods.cpp:538: Document* documentPtr = currentDOMWindow(isolate)->document(); On 2014/06/17 12:53:16, Jens Lindström ...
6 years, 6 months ago (2014-06-17 12:57:40 UTC) #6
Jens Widell
On 2014/06/17 12:57:40, haraken wrote: > https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/methods.cpp > File Source/bindings/templates/methods.cpp (right): > > https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/methods.cpp#newcode538 > ...
6 years, 6 months ago (2014-06-17 13:04:31 UTC) #7
haraken
On 2014/06/17 13:04:31, Jens Lindström wrote: > On 2014/06/17 12:57:40, haraken wrote: > > > ...
6 years, 6 months ago (2014-06-17 13:08:13 UTC) #8
Jens Widell
https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/methods.cpp File Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/methods.cpp#newcode540 Source/bindings/templates/methods.cpp:540: Document& document = *documentPtr; On 2014/06/17 12:38:53, haraken wrote: ...
6 years, 6 months ago (2014-06-17 13:31:02 UTC) #9
Jens Widell
The CQ bit was checked by jl@opera.com
6 years, 6 months ago (2014-06-17 13:31:36 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jl@opera.com/340443004/20001
6 years, 6 months ago (2014-06-17 13:32:42 UTC) #11
haraken
On 2014/06/17 13:31:02, Jens Lindström wrote: > https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/methods.cpp > File Source/bindings/templates/methods.cpp (right): > > https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/methods.cpp#newcode540 ...
6 years, 6 months ago (2014-06-17 14:15:04 UTC) #12
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 6 months ago (2014-06-17 15:46:24 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered_tests/builds/18098)
6 years, 6 months ago (2014-06-17 15:46:25 UTC) #14
Nils Barth (inactive)
To briefly concur with Jens: as much as possible, we'd like to share a single ...
6 years, 6 months ago (2014-06-18 02:44:53 UTC) #15
Jens Widell
The CQ bit was checked by jl@opera.com
6 years, 6 months ago (2014-06-18 05:47:27 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jl@opera.com/340443004/20001
6 years, 6 months ago (2014-06-18 05:47:45 UTC) #17
commit-bot: I haz the power
6 years, 6 months ago (2014-06-18 05:48:31 UTC) #18
Message was sent while issue was closed.
Change committed as 176390

Powered by Google App Engine
This is Rietveld 408576698