Issue 145583005: [SVG] Remove existing event listeners when eventBase conditions are cleared

Issue 145583005: [SVG] Remove existing event listeners when eventBase conditions are cleared (Closed)

Created:
6 years, 10 months ago by pavane

Modified:
6 years, 10 months ago

Reviewers:
pdr., krit, f(malita), Stephen Chennney, rwlbuis

CC:
blink-reviews, ed+blinkwatch_opera.com, shans, rjwright, alancutter (OOO until 2018), Mike Lawther (Google), rwlbuis, fs, dstockwell, Timothy Loh, krit, f(malita), gyuyoung.kim_webkit.org, darktears, pdr, Stephen Chennney, Steve Block, dino_apple.com, Eric Willigers, vivekg

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

Visibility:
Public.

More Reviews

Description

[SVG] Remove existing event listeners when eventBase conditions are cleared This patch fixes a regression from [1] where we are not removing event listeners when eventBase conditions are cleared. [1] http://src.chromium.org/viewvc/blink?view=rev&revision=164697 R=pdr@chromium.org, schenney@chromium.org, dschulze@chromium.org, fmalita@chromium.org, rob.buis@samsung.com BUG=338124 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165880

Patch Set 1 #

Total comments: 6

Patch Set 2 : rebaselined and worked on review comments #

Created: 6 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+33 lines, -8 lines)			Patch
A	LayoutTests/svg/animations/remove-begin-event-crash.html	View	1	1 chunk	+19 lines, -0 lines	0 comments	Download
A	LayoutTests/svg/animations/remove-begin-event-crash-expected.txt	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/svg/animation/SVGSMILElement.h	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/svg/animation/SVGSMILElement.cpp	View	1	5 chunks	+11 lines, -8 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

Stephen Chennney

I'm basically OK with this. But you should do the fix verification - all of ...

6 years, 10 months ago (2014-01-27 15:24:28 UTC) #2

f(malita)

https://codereview.chromium.org/145583005/diff/1/Source/core/svg/animation/SVGSMILElement.cpp File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/145583005/diff/1/Source/core/svg/animation/SVGSMILElement.cpp#newcode510 Source/core/svg/animation/SVGSMILElement.cpp:510: m_conditions.clear(); Looks like m_conditions.clear() is always accompanied by disconnectEventBaseConditions() ...

6 years, 10 months ago (2014-01-27 15:35:32 UTC) #3

pavane

Changes Done. Please review it once again https://codereview.chromium.org/145583005/diff/1/LayoutTests/svg/animations/remove-begin-event-crash.html File LayoutTests/svg/animations/remove-begin-event-crash.html (right): https://codereview.chromium.org/145583005/diff/1/LayoutTests/svg/animations/remove-begin-event-crash.html#newcode2 LayoutTests/svg/animations/remove-begin-event-crash.html:2: <html onload="tCFcrash();"> ...

6 years, 10 months ago (2014-01-27 17:01:18 UTC) #4

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pavan.e@samsung.com/145583005/80001

6 years, 10 months ago (2014-01-27 17:04:01 UTC) #6

Message was sent while issue was closed.

Change committed as 165880

Expand Messages | Collapse Messages