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

Issue 965343004: Oilpan: Fix untraced weak members. (Closed)

Created:
5 years, 9 months ago by Yuta Kitamura
Modified:
5 years, 9 months ago
CC:
blink-reviews, krit, ed+blinkwatch_opera.com, eric.carlson_apple.com, feature-media-reviews_chromium.org, f(malita), fs, gyuyoung.kim_webkit.org, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org, philipj_slow, rwlbuis, Stephen Chennney, tommyw+watchlist_chromium.org, oilpan-reviews
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Oilpan: Fix untraced weak members. With updated BlinkGCPlugin (not deployed yet), I have discovered a few untraced members, which seems like a real bug. This patch fixes them. For SVGElementRareData, I sorted trace() calls in the declaration order of the member variables. For RTCDataChannel, m_scheduledEvents can be wrapped in ENABLE(OILPAN) because it is a WillBeHeapVector<RefPtrWillBeMember<Event>>. R=kouhei@chromium.org, philipj@opera.com, tommi@chromium.org CC=oilpan-reviews@chromium.org BUG=455524 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191176

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -2 lines) Patch
M Source/core/svg/SVGElementRareData.cpp View 1 chunk +4 lines, -2 lines 0 comments Download
M Source/modules/mediasource/MediaSource.cpp View 1 chunk +1 line, -0 lines 1 comment Download
M Source/modules/mediastream/RTCDataChannel.cpp View 1 chunk +3 lines, -0 lines 1 comment Download

Messages

Total messages: 17 (2 generated)
Yuta Kitamura
kouhei: Please review. philipj, tommi: Can you give me an OWNERS approval? Thanks!
5 years, 9 months ago (2015-03-03 03:02:15 UTC) #1
kouhei (in TOK)
lgtm
5 years, 9 months ago (2015-03-03 03:13:04 UTC) #2
philipj_slow
lgtm
5 years, 9 months ago (2015-03-03 03:58:32 UTC) #3
tommi (sloooow) - chröme
lgtm
5 years, 9 months ago (2015-03-03 06:10:44 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/965343004/1
5 years, 9 months ago (2015-03-03 06:52:38 UTC) #6
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=191176
5 years, 9 months ago (2015-03-03 08:09:27 UTC) #7
Yuta Kitamura
(I forgot to actually cc oilpan-reviews...)
5 years, 9 months ago (2015-03-03 08:43:47 UTC) #8
sof
not lgtm https://codereview.chromium.org/965343004/diff/1/Source/modules/mediasource/MediaSource.cpp File Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/965343004/diff/1/Source/modules/mediasource/MediaSource.cpp#newcode283 Source/modules/mediasource/MediaSource.cpp:283: visitor->trace(m_attachedElement); the weak callback handles this weak ...
5 years, 9 months ago (2015-03-03 08:50:37 UTC) #10
haraken
On 2015/03/03 08:50:37, sof wrote: > not lgtm > > https://codereview.chromium.org/965343004/diff/1/Source/modules/mediasource/MediaSource.cpp > File Source/modules/mediasource/MediaSource.cpp (right): ...
5 years, 9 months ago (2015-03-03 10:41:21 UTC) #11
sof
On 2015/03/03 10:41:21, haraken wrote: > On 2015/03/03 08:50:37, sof wrote: > > not lgtm ...
5 years, 9 months ago (2015-03-03 18:44:15 UTC) #12
haraken
> As the callbacks added by these trace()s end up on the stack below the ...
5 years, 9 months ago (2015-03-04 01:24:32 UTC) #13
haraken
On 2015/03/04 01:24:32, haraken wrote: > > As the callbacks added by these trace()s end ...
5 years, 9 months ago (2015-03-04 01:46:02 UTC) #14
sof
On 2015/03/04 01:24:32, haraken wrote: > > As the callbacks added by these trace()s end ...
5 years, 9 months ago (2015-03-04 07:07:10 UTC) #15
haraken
On 2015/03/04 07:07:10, sof wrote: > On 2015/03/04 01:24:32, haraken wrote: > > > As ...
5 years, 9 months ago (2015-03-04 07:10:32 UTC) #16
Yuta Kitamura
5 years, 9 months ago (2015-03-04 08:22:56 UTC) #17
Message was sent while issue was closed.
(I can hardly follow the discussion, though) thank you for
pointing out the defect of my patch and reverting.

There was an issue in BlinkGCPlugin regarding
registerWeakMembers detection, and I have landed a change
to fix it so that the plugin does not make a false alarm
in this case.

Powered by Google App Engine
This is Rietveld 408576698