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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 23995018: Fix lifetime handling of SVGPropertyTearOffs. (Closed)

Created:
7 years, 3 months ago by ojan
Modified:
7 years, 3 months ago
CC:
blink-reviews, Nils Barth (inactive), kojih, jsbell+bindings_chromium.org, pdr, abarth-chromium, marja+watch_chromium.org, f(malita), adamk+blink_chromium.org, haraken, Stephen Chennney, Nate Chapin, do-not-use, kouhei (in TOK)
Visibility:
Public.

Description

Fix lifetime handling of SVGPropertyTearOffs. -Replace SVGStaticPropertyWithParentTearOff with SVGMatrixTearOff since it's only used in that one place. This means we can get rid of the templatizing and the method pointer. -Change SVGPropertyTearOff to keep track of it's child tearoffs and call detachWrapper on its child tearoffs when it's destroyed or when it's wrapper is detached. -Have SVGPropertyTearOff hold WeakPtrs to the child tearoffs to avoid having a cycle. BUG=288771 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157801

Patch Set 1 #

Total comments: 4

Patch Set 2 : address review comments #

Patch Set 3 : fix ownership #

Unified diffs Side-by-side diffs Delta from patch set Stats (+60 lines, -100 lines) Patch
A LayoutTests/svg/transforms/svg-matrix-tearoff-crash.html View 1 1 chunk +16 lines, -0 lines 0 comments Download
A LayoutTests/svg/transforms/svg-matrix-tearoff-crash-expected.txt View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/bindings/scripts/deprecated_code_generator_v8.pm View 1 1 chunk +3 lines, -4 lines 0 comments Download
A + Source/core/svg/properties/SVGMatrixTearOff.h View 1 2 1 chunk +15 lines, -24 lines 0 comments Download
M Source/core/svg/properties/SVGPropertyTearOff.h View 1 2 3 chunks +25 lines, -2 lines 0 comments Download
D Source/core/svg/properties/SVGStaticPropertyWithParentTearOff.h View 1 chunk +0 lines, -70 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
ojan
7 years, 3 months ago (2013-09-13 21:52:56 UTC) #1
pdr.
LGTM, thank you and Adam for driving this home https://codereview.chromium.org/23995018/diff/1/LayoutTests/svg/transforms/svg-matrix-tearoff-crash.html File LayoutTests/svg/transforms/svg-matrix-tearoff-crash.html (right): https://codereview.chromium.org/23995018/diff/1/LayoutTests/svg/transforms/svg-matrix-tearoff-crash.html#newcode1 LayoutTests/svg/transforms/svg-matrix-tearoff-crash.html:1: ...
7 years, 3 months ago (2013-09-13 22:01:15 UTC) #2
ojan
https://codereview.chromium.org/23995018/diff/1/Source/core/svg/properties/SVGPropertyTearOff.h File Source/core/svg/properties/SVGPropertyTearOff.h (right): https://codereview.chromium.org/23995018/diff/1/Source/core/svg/properties/SVGPropertyTearOff.h#newcode151 Source/core/svg/properties/SVGPropertyTearOff.h:151: for (Vector<RefPtr<SVGPropertyTearOffBase> >::iterator iter = m_childTearOffs.begin(); iter != m_childTearOffs.end(); ...
7 years, 3 months ago (2013-09-13 22:12:04 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ojan@chromium.org/23995018/7001
7 years, 3 months ago (2013-09-13 22:12:21 UTC) #4
commit-bot: I haz the power
Retried try job too often on blink_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_presubmit&number=6989
7 years, 3 months ago (2013-09-13 22:22:28 UTC) #5
ojan
Fixed lifetime issue, abarth, needs your LGTM for the bindings directory.
7 years, 3 months ago (2013-09-13 22:52:25 UTC) #6
abarth-chromium
lgtm
7 years, 3 months ago (2013-09-13 22:56:30 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ojan@chromium.org/23995018/14001
7 years, 3 months ago (2013-09-13 22:56:39 UTC) #8
commit-bot: I haz the power
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_rel&number=5666
7 years, 3 months ago (2013-09-14 01:54:58 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ojan@chromium.org/23995018/14001
7 years, 3 months ago (2013-09-14 02:35:16 UTC) #10
commit-bot: I haz the power
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_rel&number=5695
7 years, 3 months ago (2013-09-14 03:58:28 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ojan@chromium.org/23995018/14001
7 years, 3 months ago (2013-09-14 04:59:42 UTC) #12
commit-bot: I haz the power
Change committed as 157801
7 years, 3 months ago (2013-09-14 05:56:38 UTC) #13
haraken
7 years, 3 months ago (2013-09-14 09:28:33 UTC) #14
Message was sent while issue was closed. 
+kouhei

Powered by Google App Engine
This is Rietveld 408576698