Issue 1922093003: Add AddEventListenerOptions addEventListenerOptions interface.

Issue 1922093003: Add AddEventListenerOptions addEventListenerOptions interface. (Closed)

Created:
4 years, 8 months ago by dtapuska

Modified:
4 years, 7 months ago

Reviewers:
haraken, bokan

CC:
chromium-reviews, shans, tzik, eae+blinkwatch, fs, scheib+watch_chromium.org, ortuno+watch_chromium.org, kouhei+svg_chromium.org, rwlbuis, jsbell+serviceworker_chromium.org, krit, toyoshim+midi_chromium.org, kinuko+serviceworker, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews-bindings_chromium.org, jkarlin+watch_chromium.org, blink-reviews, gyuyoung2, pdr+svgwatchlist_chromium.org, Eric Willigers, rjwright, sof, nhiroki, darktears, haraken, michaeln, blink-reviews-animation_chromium.org, serviceworker-reviews, falken, f(malita), horo+watch_chromium.org, Stephen Chennney

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Add AddEventListenerOptions addEventListenerOptions interface. Change the 'passive' field from EventListenerOptions to AddEventListenerOptions dictionary. Change argument on addEventListener to take a AddEventListenerOptions dictionary. Intent to Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/k6v-puVgMLs/sftw2oJ7AAAJ BUG=602735 Committed: https://crrev.com/b4cc79f5db2ea11ff1c865ce43bca1b035c6ef97 Cr-Commit-Position: refs/heads/master@{#392075}

Patch Set 1 #

Total comments: 6

Patch Set 2 : Rebase against cleanup change #

Patch Set 3 : Rebase #

Created: 4 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+109 lines, -42 lines)			Patch
M	third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_dispatch.html	View		1 chunk	+2 lines, -2 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_inequality.html	View		1 chunk	+43 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_query.html	View		1 chunk	+0 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/bindings/core/v8/custom/V8EventTargetCustom.cpp	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/core/core.gypi	View	1 2	2 chunks	+3 lines, -0 lines	0 comments	Download
A	third_party/WebKit/Source/core/events/AddEventListenerOptions.idl	View		1 chunk	+9 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/events/EventListenerMap.h	View	1 2	2 chunks	+2 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/events/EventListenerMap.cpp	View	1 2	3 chunks	+3 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/core/events/EventListenerOptions.idl	View		2 chunks	+1 line, -2 lines	0 comments	Download
M	third_party/WebKit/Source/core/events/EventTarget.h	View	1 2	2 chunks	+3 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/core/events/EventTarget.cpp	View	1 2	3 chunks	+18 lines, -8 lines	0 comments	Download
M	third_party/WebKit/Source/core/events/EventTarget.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/events/RegisteredEventListener.h	View	1 2	4 chunks	+10 lines, -5 lines	0 comments	Download
M	third_party/WebKit/Source/core/frame/EventHandlerRegistry.h	View	1	4 chunks	+5 lines, -5 lines	0 comments	Download
M	third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp	View		3 chunks	+4 lines, -4 lines	0 comments	Download
M	third_party/WebKit/Source/core/svg/SVGElement.cpp	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download

Depends on Patchset:

Issue 1942723004 Patch 140001

Dependent Patchsets:

Issue 1956733002 Patch 1

Issue 1956733002 Patch 20001

Messages

Total messages: 14 (7 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

dtapuska

https://codereview.chromium.org/1922093003/diff/1/third_party/WebKit/Source/core/events/EventListenerMap.cpp File third_party/WebKit/Source/core/events/EventListenerMap.cpp (right): https://codereview.chromium.org/1922093003/diff/1/third_party/WebKit/Source/core/events/EventListenerMap.cpp#newcode139 third_party/WebKit/Source/core/events/EventListenerMap.cpp:139: // Aassign the |registeredListener| the value that was in ...

4 years, 8 months ago (2016-04-26 14:29:39 UTC) #4

bokan

WebKit core lgtm % minor nits. There's a suggested simplification I'd like to see but ...

4 years, 8 months ago (2016-04-26 19:54:59 UTC) #5

WebKit core lgtm % minor nits.

There's a suggested simplification I'd like to see but not necessary to land
this patch.

https://codereview.chromium.org/1922093003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_inequality.html
(right):

https://codereview.chromium.org/1922093003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_inequality.html:2:
<body/>
Self closing tags aren't a thing in HTML are they? In any case, layout test
style guidelines say omit the <body>, <head> and <html> tags.

https://codereview.chromium.org/1922093003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/custom/V8EventTargetCustom.cpp
(right):

https://codereview.chromium.org/1922093003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/custom/V8EventTargetCustom.cpp:98:
AddEventListenerOptionsOrBoolean options;
This can be moved below the `if` block, right?

https://codereview.chromium.org/1922093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/1922093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Node.cpp:1797: void
Node::removedEventListener(const AtomicString& eventType, const
RegisteredEventListener& registeredListener)
Could you do this for all the other removeEventListenerInternal and
addEventListenerInternal overrides? I can see NetworkInformation, SVGElement,
and LocalDOMWindow for remove and more for add. All these overrides basically
just want to tack on some extra actions after the add/removal so there's no
sense in allowing them to actually change how listeners are added/removed. In a
few cases some of the addListener overrides do something before adding but after
a quick look I think all of these could/should be done after (and only if the
listener actually got added).

Once that's done you could actually make the remove/addInternal methods private,
or just fold them into the public remove/add methods.

This should probably happen in a separate CL, either before or after this one.

https://codereview.chromium.org/1922093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/EventListenerMap.cpp (right):

https://codereview.chromium.org/1922093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/EventListenerMap.cpp:139: // Aassign the
|registeredListener| the value that was in the vector
On 2016/04/26 14:29:39, dtapuska wrote:
> I recognize the typo on this line |Aassign|. I have it fixed locally. But will
> wait to post a subsequent patch when there are more comments to address.

Acknowledged.

haraken

LGTM https://codereview.chromium.org/1922093003/diff/1/third_party/WebKit/Source/bindings/core/v8/custom/V8EventTargetCustom.cpp File third_party/WebKit/Source/bindings/core/v8/custom/V8EventTargetCustom.cpp (right): https://codereview.chromium.org/1922093003/diff/1/third_party/WebKit/Source/bindings/core/v8/custom/V8EventTargetCustom.cpp#newcode98 third_party/WebKit/Source/bindings/core/v8/custom/V8EventTargetCustom.cpp:98: AddEventListenerOptionsOrBoolean options; On 2016/04/26 19:54:59, bokan wrote: > ...

4 years, 8 months ago (2016-04-27 06:36:15 UTC) #7

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1922093003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1922093003/40001

4 years, 7 months ago (2016-05-06 14:52:11 UTC) #10

commit-bot: I haz the power

4 years, 7 months ago (2016-05-06 16:45:13 UTC) #14

Message was sent while issue was closed.

Patchset 3 (id:??) landed as
https://crrev.com/b4cc79f5db2ea11ff1c865ce43bca1b035c6ef97
Cr-Commit-Position: refs/heads/master@{#392075}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages