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

Issue 1019453002: IDL: Add [Custom=CallEpilogue] to eliminate CG special cases (Closed)

Created:
5 years, 9 months ago by Jens Widell
Modified:
5 years, 9 months ago
Reviewers:
haraken, vivekg, bashi, Yuki
CC:
blink-reviews, kenneth.christiansen, Yoav Weiss, arv+blink, vivekg_samsung, blink-reviews-css, ed+blinkwatch_opera.com, dglazkov+blink, blink-reviews-bindings_chromium.org, Inactive, darktears, apavlov+blink_chromium.org, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

IDL: Add [Custom=CallEpilogue] to eliminate CG special cases The effect is similar to having [Custom] on a method, but instead of completely replacing the generated method implementation, the custom callback is just called after the regular method implementation call. Use this to reimplement EventTarget.{add,remove}EventListener and MediaQueryList.{add,remove}Listener's code generation special cases. Also use this to reduce the custom code for History.{push,replace}State to such a call epilogue. While doing so, update History.idl so that the now generated code is behaves the same as the old custom code. BUG=345519 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192146

Patch Set 1 #

Patch Set 2 : add 'impl' argument; add test #

Patch Set 3 : rebased #

Patch Set 4 : fix after rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -61 lines) Patch
M Source/bindings/IDLExtendedAttributes.txt View 1 chunk +1 line, -1 line 0 comments Download
M Source/bindings/core/v8/custom/V8EventTargetCustom.cpp View 1 1 chunk +12 lines, -0 lines 0 comments Download
M Source/bindings/core/v8/custom/V8HistoryCustom.cpp View 1 2 1 chunk +2 lines, -38 lines 0 comments Download
A + Source/bindings/core/v8/custom/V8MediaQueryListCustom.cpp View 1 1 chunk +10 lines, -5 lines 0 comments Download
M Source/bindings/core/v8/custom/custom.gypi View 1 chunk +1 line, -0 lines 0 comments Download
M Source/bindings/scripts/v8_methods.py View 2 chunks +3 lines, -1 line 0 comments Download
M Source/bindings/templates/interface.h View 1 1 chunk +5 lines, -0 lines 0 comments Download
M Source/bindings/templates/methods.cpp View 1 1 chunk +2 lines, -10 lines 0 comments Download
M Source/bindings/tests/idls/core/TestObject.idl View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/bindings/tests/results/core/V8TestObject.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/bindings/tests/results/core/V8TestObject.cpp View 1 2 chunks +15 lines, -0 lines 0 comments Download
M Source/core/css/MediaQueryList.idl View 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/events/EventTarget.idl View 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/frame/History.h View 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/frame/History.idl View 1 2 3 1 chunk +3 lines, -2 lines 0 comments Download

Messages

Total messages: 24 (3 generated)
Jens Widell
PTAL Note that I'm not completely convinced myself this is something we should do (like ...
5 years, 9 months ago (2015-03-18 11:41:19 UTC) #2
Jens Widell
On 2015/03/18 11:41:19, Jens Widell wrote: > - All of these epilogues could simply be ...
5 years, 9 months ago (2015-03-18 11:50:36 UTC) #3
haraken
I'd +1 to the idea if there are more use cases :)
5 years, 9 months ago (2015-03-18 13:28:46 UTC) #4
vivekg
On 2015/03/18 at 13:28:46, haraken wrote: > I'd +1 to the idea if there are ...
5 years, 9 months ago (2015-03-18 13:44:37 UTC) #5
vivekg
On 2015/03/18 at 13:44:37, vivekg_ wrote: > On 2015/03/18 at 13:28:46, haraken wrote: > > ...
5 years, 9 months ago (2015-03-18 13:48:14 UTC) #6
Jens Widell
On 2015/03/18 13:48:14, vivekg_ wrote: > On 2015/03/18 at 13:44:37, vivekg_ wrote: > > On ...
5 years, 9 months ago (2015-03-18 13:51:20 UTC) #7
vivekg
On 2015/03/18 at 13:51:20, jl wrote: > On 2015/03/18 13:48:14, vivekg_ wrote: > > On ...
5 years, 9 months ago (2015-03-18 13:54:29 UTC) #8
Jens Widell
On 2015/03/18 13:28:46, haraken wrote: > I'd +1 to the idea if there are more ...
5 years, 9 months ago (2015-03-18 13:58:11 UTC) #9
haraken
bashi-san, shiino-san: What do you think?
5 years, 9 months ago (2015-03-18 14:02:55 UTC) #11
Yuki
On 2015/03/18 14:02:55, haraken wrote: > bashi-san, shiino-san: What do you think? Interesting idea. Looks ...
5 years, 9 months ago (2015-03-18 14:29:23 UTC) #12
Jens Widell
On 2015/03/18 14:29:23, Yuki wrote: > On 2015/03/18 14:02:55, haraken wrote: > > bashi-san, shiino-san: ...
5 years, 9 months ago (2015-03-18 14:38:17 UTC) #13
bashi
I like this idea :)
5 years, 9 months ago (2015-03-19 01:34:39 UTC) #14
haraken
On 2015/03/19 01:34:39, bashi1 wrote: > I like this idea :) +1.
5 years, 9 months ago (2015-03-19 02:01:26 UTC) #15
Jens Widell
PS#4 adjusts History.idl to match the changes made in r192095 (second arguments to pushState/replaceState no ...
5 years, 9 months ago (2015-03-19 07:45:55 UTC) #16
Yuki
lgtm
5 years, 9 months ago (2015-03-19 08:05:16 UTC) #17
vivekg
On 2015/03/19 at 07:45:55, jl wrote: > PS#4 adjusts History.idl to match the changes made ...
5 years, 9 months ago (2015-03-19 08:07:36 UTC) #18
haraken
LGTM. I believe this will lead to less bugs :)
5 years, 9 months ago (2015-03-19 08:58:32 UTC) #19
Jens Widell
Thank you all for reviewing!
5 years, 9 months ago (2015-03-19 09:00:04 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1019453002/60001
5 years, 9 months ago (2015-03-19 09:00:21 UTC) #22
vivekg
Can you or someone with access to www.chromium.org/blink/webidl/blink-idl-extended-attributes update the information about this new addition?
5 years, 9 months ago (2015-03-19 09:05:59 UTC) #23
commit-bot: I haz the power
5 years, 9 months ago (2015-03-19 09:56:55 UTC) #24
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192146

Powered by Google App Engine
This is Rietveld 408576698