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

Issue 2032283002: Remove refcounting from SavePackage.

Created:
4 years, 6 months ago by Mario Pistrich
Modified:
4 years, 6 months ago
Reviewers:
asanka
CC:
asanka, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove refcounting from SavePackage. WebContentsImpl is the only owner of SavePackage in production code. Test code does not depend on SavePackage being refcounted, so it is safe to remove. BUG=596953 R=asanka@chromium.org

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+44 lines, -42 lines) Patch
M content/browser/download/save_package.h View 8 chunks +13 lines, -19 lines 1 comment Download
M content/browser/download/save_package.cc View 4 chunks +8 lines, -6 lines 0 comments Download
M content/browser/download/save_package_browsertest.cc View 3 chunks +8 lines, -6 lines 0 comments Download
M content/browser/download/save_package_unittest.cc View 4 chunks +10 lines, -8 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.h View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 3 chunks +4 lines, -2 lines 1 comment Download

Messages

Total messages: 2 (0 generated)
Mario Pistrich
4 years, 6 months ago (2016-06-03 01:43:23 UTC) #1
asanka
4 years, 6 months ago (2016-06-06 18:55:09 UTC) #2
https://codereview.chromium.org/2032283002/diff/1/content/browser/download/sa...
File content/browser/download/save_package.h (right):

https://codereview.chromium.org/2032283002/diff/1/content/browser/download/sa...
content/browser/download/save_package.h:276: SavePageType type,
There's a bunch of unrelated formatting changes in this CL. Could you remove
those? You can use 'git cl format' to format just the blocks that you are
touching.

https://codereview.chromium.org/2032283002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2032283002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2758: save_package_ =
base::WrapUnique(new SavePackage(this));
Here and elsewhere:

Assigning a unique_ptr and ref_ptr confers entirely different lifetime
semantics. Previously, the old SavePackage will live long enough for the child
operations to complete due to the references held on it by the posted tasks etc.

Now, the old SavePackage is immediately destroyed when the new SavePackage is
assigned here. This makes all the Unretained() references in this CL to become
unsafe. The SP can go away pretty much at any point. You'll need to account for
those, perhaps by figuring out what the proper semantics should be for an old SP
when a new one is initiated.

Powered by Google App Engine
This is Rietveld 408576698