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

Issue 11416166: Clean up ScopedVectorTest's LifeCycleObject. (Closed)

Created:
8 years ago by gavinp
Modified:
8 years ago
CC:
chromium-reviews, erikwright+watch_chromium.org, gavinp+memory_chromium.org
Visibility:
Public.

Description

Clean up ScopedVectorTest's LifeCycleObject. The scoped_ptr<LifeCycleObject> was unncessary and confusing. The LifeCycleWatcher safely owns its LifeCycleObject and deletes it. The scoped_ptr made things a bit confusing even, since the only reason we weren't getting a double delete was because the scoped_ptr had an (ever so) slightly narrower scope than watcher. Making the LifeCycleObject mostly private should encourage future callers not to add scary aliases. R=willchan@chromium.org BUG=None Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171058

Patch Set 1 #

Patch Set 2 : ... switch off static_cast while I'm here. #

Patch Set 3 : ... lose the casts, make LifeCycleObject more private. #

Total comments: 1

Patch Set 4 : ... ok, private and friendship is confusing with respect to base classes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -9 lines) Patch
M base/memory/scoped_vector_unittest.cc View 1 2 3 2 chunks +10 lines, -9 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
gavinp
Will, I cam across this while I was looking at ScopedVector. It's a minor change, ...
8 years ago (2012-11-24 13:44:59 UTC) #1
gavinp
I made one last upload that makes the LifeCycleObject much more private, guarding against this ...
8 years ago (2012-11-24 14:27:18 UTC) #2
willchan no longer on Chromium
lgtm
8 years ago (2012-12-04 01:42:50 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/11416166/13003
8 years ago (2012-12-04 01:44:39 UTC) #4
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) browser_tests
8 years ago (2012-12-04 04:49:25 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/11416166/13003
8 years ago (2012-12-04 19:10:06 UTC) #6
commit-bot: I haz the power
8 years ago (2012-12-04 21:51:45 UTC) #7
Message was sent while issue was closed.
Change committed as 171058

Powered by Google App Engine
This is Rietveld 408576698