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

Issue 37263002: Unify the GlobalEventHandlers list on the C++ side (Closed)

Created:
5 years, 3 months ago by philipj_slow
Modified:
5 years, 3 months ago
CC:
blink-reviews, kojih, jsbell+bindings_chromium.org, abarth-chromium, marja+watch_chromium.org, adamk+blink_chromium.org, Nate Chapin, Inactive
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Unify the GlobalEventHandlers list on the C++ side Several changes need to be made in sync to achieve this. Teach the IDL compiler to generate static calls for EventTarget, allowing GlobalEventHandlers.h to be the single list of the event attributes on the C++ side. The handling of the 5 special event handler attributes on body and frameset was very strange, with both virtual getters/setters in C++ and overrides at the IDL level. Remove the former mechanism, and fix up the later to fill the job it was clearly added to do. Also fix some style issues in EventTarget.h to please presubmit.tributes on the C++ side. BUG=305112 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160676

Patch Set 1 #

Total comments: 2

Patch Set 2 : testing and cleanup #

Patch Set 3 : one list to rule the C++ side #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+304 lines, -363 lines) Patch
M Source/bindings/scripts/code_generator_v8.pm View 1 2 2 chunks +36 lines, -6 lines 0 comments Download
M Source/bindings/tests/idls/TestImplements.idl View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/bindings/tests/results/V8TestInterface.cpp View 1 2 3 chunks +31 lines, -0 lines 0 comments Download
M Source/core/dom/Document.h View 1 2 1 chunk +0 lines, -57 lines 0 comments Download
M Source/core/dom/Element.h View 1 2 1 chunk +0 lines, -60 lines 0 comments Download
M Source/core/dom/Element.cpp View 1 2 1 chunk +0 lines, -6 lines 0 comments Download
M Source/core/dom/GlobalEventHandlers.h View 1 2 1 chunk +59 lines, -0 lines 0 comments Download
M Source/core/events/EventTarget.h View 1 2 1 chunk +173 lines, -173 lines 3 comments Download
M Source/core/frame/DOMWindow.h View 1 2 1 chunk +0 lines, -57 lines 0 comments Download
M Source/core/html/HTMLBodyElement.h View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/html/HTMLBodyElement.idl View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/html/HTMLFrameSetElement.h View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/html/HTMLFrameSetElement.idl View 1 2 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 17 (0 generated)
philipj_slow
I'm not exactly a Perl pro, so before I spend any time making this pretty ...
5 years, 3 months ago (2013-10-23 13:41:48 UTC) #1
Nils Barth (inactive)
On 2013/10/23 13:41:48, philipj wrote: > I'm not exactly a Perl pro, so before I ...
5 years, 3 months ago (2013-10-24 02:41:13 UTC) #2
Nils Barth (inactive)
Some comments on simplifying logic; understand it's not polished yet, just wanted to highlight. https://codereview.chromium.org/37263002/diff/1/Source/bindings/scripts/code_generator_v8.pm ...
5 years, 3 months ago (2013-10-24 02:41:38 UTC) #3
philipj_slow
Thanks Nils, I've cleaned it up a bit, added tests and verified that all of ...
5 years, 3 months ago (2013-10-24 20:28:51 UTC) #4
philipj_slow
I consider this done now, PTAL.
5 years, 3 months ago (2013-10-26 05:02:04 UTC) #5
abarth-chromium
The C++ parts look good. We should have Nils or Haraken review the changes to ...
5 years, 3 months ago (2013-10-26 15:49:48 UTC) #6
haraken
LGTM. Great change, thanks! https://codereview.chromium.org/37263002/diff/110001/Source/core/events/EventTarget.h File Source/core/events/EventTarget.h (right): https://codereview.chromium.org/37263002/diff/110001/Source/core/events/EventTarget.h#newcode163 Source/core/events/EventTarget.h:163: void setOn##attribute(PassRefPtr<EventListener> listener, DOMWrapperWorld* isolatedWorld ...
5 years, 3 months ago (2013-10-26 17:03:14 UTC) #7
haraken
On 2013/10/26 17:03:14, haraken wrote: > LGTM. Great change, thanks! > > https://codereview.chromium.org/37263002/diff/110001/Source/core/events/EventTarget.h > File ...
5 years, 3 months ago (2013-10-26 17:04:14 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/philipj@opera.com/37263002/110001
5 years, 3 months ago (2013-10-26 18:07:55 UTC) #9
commit-bot: I haz the power
Retried try job too often on win_blink_rel for step(s) webkit_unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_rel&number=14117
5 years, 3 months ago (2013-10-26 19:08:06 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/philipj@opera.com/37263002/110001
5 years, 3 months ago (2013-10-26 19:38:46 UTC) #11
philipj_slow
https://codereview.chromium.org/37263002/diff/110001/Source/core/events/EventTarget.h File Source/core/events/EventTarget.h (right): https://codereview.chromium.org/37263002/diff/110001/Source/core/events/EventTarget.h#newcode163 Source/core/events/EventTarget.h:163: void setOn##attribute(PassRefPtr<EventListener> listener, DOMWrapperWorld* isolatedWorld = 0) { setAttributeEventListener(EventTypeNames::attribute, ...
5 years, 3 months ago (2013-10-26 20:27:49 UTC) #12
commit-bot: I haz the power
Retried try job too often on win_blink_rel for step(s) webkit_unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_rel&number=14130
5 years, 3 months ago (2013-10-26 20:27:58 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/philipj@opera.com/37263002/110001
5 years, 3 months ago (2013-10-26 20:32:54 UTC) #14
commit-bot: I haz the power
Retried try job too often on win_blink_rel for step(s) webkit_unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_rel&number=14149
5 years, 3 months ago (2013-10-26 21:31:17 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/philipj@opera.com/37263002/110001
5 years, 3 months ago (2013-10-27 03:55:50 UTC) #16
commit-bot: I haz the power
5 years, 3 months ago (2013-10-27 14:54:32 UTC) #17
Message was sent while issue was closed.
Change committed as 160676

Powered by Google App Engine
This is Rietveld 408576698