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

Issue 328663003: IDL: restructure logic handling registration of methods (Closed)

Created:
6 years, 6 months ago by Jens Widell
Modified:
6 years, 6 months ago
CC:
blink-reviews, blink-reviews-bindings_chromium.org, arv+blink, abarth-chromium
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

IDL: restructure logic handling registration of methods Methods are registered in one of three different ways on an interface's prototype object; 1) via an entry in a static array, 2) via custom code in the interface's configure{{class}}Template(), or 3) via custom code in its installPerContextEnabledMethods(); depending on whether the method is static and its extended attributes. E.g., [RuntimeEnabled] leads to 2 and [PerContextEnabled] leads to 3. The old method of setting flags on each method signalling how it ought to be registered, and filtering the single list of methods in the templates based on these flags, was becoming seriously unwieldy for complicated cases such as overloaded methods. To address this, change the logic to instead filter methods into three lists in the scripts, one for each type of registration. The templates simply use a different list of methods when generating the different code blocks. For overloaded methods, take the attributes of all overloads into account, instead of looking at only one of them (the first in IDL file order, as it were.) Also introduce a compile-time check that a set of overloaded methods doesn't contain methods with conflicting extended attributes that affect how the method ought to be registered. BUG=339000 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176017

Patch Set 1 #

Total comments: 8

Patch Set 2 : #

Total comments: 3

Patch Set 3 : s/needs_/has_/ + check for conflicting ext attrs #

Total comments: 5

Patch Set 4 : address nits #

Patch Set 5 : improve custom registration ext attrs check #

Total comments: 3

Patch Set 6 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+95 lines, -29 lines) Patch
M Source/bindings/scripts/v8_interface.py View 1 2 3 4 6 chunks +70 lines, -13 lines 0 comments Download
M Source/bindings/scripts/v8_methods.py View 1 2 3 4 5 3 chunks +18 lines, -5 lines 0 comments Download
M Source/bindings/templates/interface.h View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/bindings/templates/interface.cpp View 1 2 3 4 5 5 chunks +6 lines, -9 lines 0 comments Download
M Source/bindings/tests/results/V8TestInterface.cpp View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 24 (0 generated)
Jens Widell
PTAL These changes are extracted from the per-overload-RuntimeEnabled patch (and slightly modified from what I ...
6 years, 6 months ago (2014-06-10 09:42:47 UTC) #1
Nils Barth (inactive)
One question for haraken, otherwise fine. This is a technical CL that regularizes behavior, and ...
6 years, 6 months ago (2014-06-11 06:28:28 UTC) #2
Nils Barth (inactive)
BTW, thanks for digging into this and for splitting this off; these are exactly the ...
6 years, 6 months ago (2014-06-11 06:30:00 UTC) #3
haraken
https://codereview.chromium.org/328663003/diff/1/Source/bindings/scripts/v8_interface.py File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/328663003/diff/1/Source/bindings/scripts/v8_interface.py#newcode257 Source/bindings/scripts/v8_interface.py:257: overloads['do_not_check_signature_any'] and On 2014/06/11 06:28:28, Nils Barth wrote: > ...
6 years, 6 months ago (2014-06-11 06:38:08 UTC) #4
Nils Barth (inactive)
On 2014/06/11 06:38:08, haraken wrote: > I think it doesn't make sense at all to ...
6 years, 6 months ago (2014-06-11 06:47:08 UTC) #5
Jens Widell
On 2014/06/11 06:47:08, Nils Barth wrote: > * In v8_methods, could you make a tuple ...
6 years, 6 months ago (2014-06-11 07:16:09 UTC) #6
Nils Barth (inactive)
On 2014/06/11 07:16:09, Jens Lindström wrote: > On 2014/06/11 06:47:08, Nils Barth wrote: > > ...
6 years, 6 months ago (2014-06-11 08:49:23 UTC) #7
Jens Widell
On 2014/06/11 08:49:23, Nils Barth wrote: > On 2014/06/11 07:16:09, Jens Lindström wrote: > > ...
6 years, 6 months ago (2014-06-11 09:38:14 UTC) #8
Nils Barth (inactive)
https://codereview.chromium.org/328663003/diff/20001/Source/bindings/tests/results/V8TestInterface.cpp File Source/bindings/tests/results/V8TestInterface.cpp (right): https://codereview.chromium.org/328663003/diff/20001/Source/bindings/tests/results/V8TestInterface.cpp#newcode1499 Source/bindings/tests/results/V8TestInterface.cpp:1499: prototypeTemplate->Set(v8AtomicString(isolate, "partial2StaticVoidMethod"), v8::FunctionTemplate::New(isolate, TestInterfaceImplementationV8Internal::partial2StaticVoidMethodMethodCallback, v8Undefined(), defaultSignature, 0)->GetFunction()); On 2014/06/11 ...
6 years, 6 months ago (2014-06-11 09:44:47 UTC) #9
Nils Barth (inactive)
On 2014/06/11 09:38:14, Jens Lindström wrote: > I ended up calling the global CUSTOM_REGISTRATION_EXTENDED_ATTRIBUTES, and ...
6 years, 6 months ago (2014-06-11 09:48:43 UTC) #10
Nils Barth (inactive)
One meaningful code change, otherwise looks great (and rather simpler and more extensible). https://codereview.chromium.org/328663003/diff/20001/Source/bindings/scripts/v8_methods.py File ...
6 years, 6 months ago (2014-06-11 10:10:28 UTC) #11
Nils Barth (inactive)
BTW, could you update title and description, since CL has changed some? Thanks!
6 years, 6 months ago (2014-06-11 10:11:25 UTC) #12
Jens Widell
https://codereview.chromium.org/328663003/diff/40001/Source/bindings/scripts/v8_interface.py File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/328663003/diff/40001/Source/bindings/scripts/v8_interface.py#newcode414 Source/bindings/scripts/v8_interface.py:414: if (common_value(overloads, 'is_do_not_check_security') is None or On 2014/06/11 10:10:28, ...
6 years, 6 months ago (2014-06-11 10:20:57 UTC) #13
haraken
On 2014/06/11 09:48:43, Nils Barth wrote: > On 2014/06/11 09:38:14, Jens Lindström wrote: > > ...
6 years, 6 months ago (2014-06-11 10:30:44 UTC) #14
Nils Barth (inactive)
On 2014/06/11 10:20:57, Jens Lindström wrote: > Sounds good, except at this point I only ...
6 years, 6 months ago (2014-06-12 03:08:14 UTC) #15
Jens Widell
On 2014/06/12 03:08:14, Nils Barth wrote: > How about instead including just these attributes, via: ...
6 years, 6 months ago (2014-06-12 06:32:32 UTC) #16
Jens Widell
On 2014/06/11 10:11:25, Nils Barth wrote: > BTW, could you update title and description, since ...
6 years, 6 months ago (2014-06-12 06:44:51 UTC) #17
Nils Barth (inactive)
LGTM, thanks for revisions! https://codereview.chromium.org/328663003/diff/80001/Source/bindings/scripts/v8_interface.py File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/328663003/diff/80001/Source/bindings/scripts/v8_interface.py#newcode794 Source/bindings/scripts/v8_interface.py:794: def common(dicts, f): Slick ;) ...
6 years, 6 months ago (2014-06-12 09:35:14 UTC) #18
Jens Widell
On 2014/06/12 09:35:14, Nils Barth wrote: > LGTM, thanks for revisions! Thanks! Will rebase and ...
6 years, 6 months ago (2014-06-12 09:42:30 UTC) #19
Jens Widell
rebased
6 years, 6 months ago (2014-06-12 09:54:25 UTC) #20
Jens Widell
The CQ bit was checked by jl@opera.com
6 years, 6 months ago (2014-06-12 09:55:01 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jl@opera.com/328663003/100001
6 years, 6 months ago (2014-06-12 09:55:20 UTC) #22
haraken
LGTM
6 years, 6 months ago (2014-06-12 10:53:51 UTC) #23
commit-bot: I haz the power
6 years, 6 months ago (2014-06-12 11:03:59 UTC) #24
Message was sent while issue was closed.
Change committed as 176017

Powered by Google App Engine
This is Rietveld 408576698