Issue 2032283002: Remove refcounting from SavePackage.

Keyboard Shortcuts

	File
u :	up to issue
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line

	Issue
u :	up to list of issues
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue

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.

More Reviews

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

Created: 4 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	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)

Expand Messages | Collapse Messages

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.

Expand Messages | Collapse Messages