Issue 10689093: Move Rename functionality from DownloadFileManager to DownloadFileImple.

Issue 10689093: Move Rename functionality from DownloadFileManager to DownloadFileImple. (Closed)

Created:
8 years, 5 months ago by Randy Smith (Not in Mondays)

Modified:
8 years, 5 months ago

Reviewers:
asanka, Jay Civelli

CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, rginda+watch_chromium.org, jam, rdsmith+dwatch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Move Rename functionality from DownloadFileManager to DownloadFileImple. BUG=123998 R=asanka@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146162

Patch Set 2 : Sync'd to TOT to avoid showing already committed changes in Rietveld. #

Total comments: 22

Patch Set 3 : Incorporated comments. #

Total comments: 2

Patch Set 4 : Merged to LKGR. #

Total comments: 1

Patch Set 5 : Switching to making rename file by making the directory unwritable. #

Patch Set 6 : Fixed some try job problems. #

Patch Set 7 : Upload fater merging past revert to figure out if I still have a patch. #

Created: 8 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+332 lines, -190 lines)			Patch
M	content/browser/download/download_file.h	View	1 2 3 4 5 6	3 chunks	+16 lines, -3 lines	0 comments	Download
M	content/browser/download/download_file_impl.h	View	1 2 3 4 5 6	2 chunks	+6 lines, -5 lines	0 comments	Download
M	content/browser/download/download_file_impl.cc	View	1	3 chunks	+39 lines, -7 lines	0 comments	Download
M	content/browser/download/download_file_manager.h	View	1 2 3 4 5 6	3 chunks	+3 lines, -2 lines	0 comments	Download
M	content/browser/download/download_file_manager.cc	View	1	1 chunk	+5 lines, -33 lines	0 comments	Download
M	content/browser/download/download_file_manager_unittest.cc	View	1 2	8 chunks	+24 lines, -96 lines	0 comments	Download
M	content/browser/download/download_file_unittest.cc	View	1 2 3 4 5	17 chunks	+173 lines, -11 lines	0 comments	Download
M	content/browser/download/download_item_impl.h	View	1 2 3 4 5 6	1 chunk	+3 lines, -1 line	0 comments	Download
M	content/browser/download/download_item_impl.cc	View	1 2	4 chunks	+23 lines, -6 lines	0 comments	Download
M	content/browser/download/download_item_impl_unittest.cc	View		1 chunk	+3 lines, -2 lines	0 comments	Download
M	content/browser/download/download_manager_impl.h	View	1 2 3 4 5 6	1 chunk	+0 lines, -2 lines	0 comments	Download
M	content/browser/download/download_manager_impl.cc	View	1 2 3 4 5 6	2 chunks	+1 line, -4 lines	0 comments	Download
M	content/browser/download/download_manager_impl_unittest.cc	View	1 2 3 4 5 6	1 chunk	+1 line, -5 lines	0 comments	Download
M	content/browser/download/mock_download_file.h	View	1 2 3 4 5 6	2 chunks	+4 lines, -0 lines	0 comments	Download
M	content/public/browser/download_manager.h	View	1 2 3 4 5 6	1 chunk	+0 lines, -2 lines	0 comments	Download
M	content/public/test/mock_download_manager.h	View	1 2 3 4 5 6	1 chunk	+1 line, -3 lines	0 comments	Download
M	content/test/test_file_error_injector.cc	View	1	5 chunks	+30 lines, -8 lines	0 comments	Download

Messages

Total messages: 8 (0 generated)

Expand Messages | Collapse Messages

Randy Smith (Not in Mondays)

Asanka, PTAL when you have a chance. Thanks!

8 years, 5 months ago (2012-07-03 22:12:44 UTC) #1

asanka

Nice cleanup! http://codereview.chromium.org/10689093/diff/2001/content/browser/download/download_file.h File content/browser/download/download_file.h (right): http://codereview.chromium.org/10689093/diff/2001/content/browser/download/download_file.h#newcode41 content/browser/download/download_file.h:41: // Returns net::OK on success, or a ...

8 years, 5 months ago (2012-07-05 18:47:21 UTC) #2

Randy Smith (Not in Mondays)

Comments incorporated; PTAL. http://codereview.chromium.org/10689093/diff/2001/content/browser/download/download_file.h File content/browser/download/download_file.h (right): http://codereview.chromium.org/10689093/diff/2001/content/browser/download/download_file.h#newcode41 content/browser/download/download_file.h:41: // Returns net::OK on success, or ...

8 years, 5 months ago (2012-07-09 20:35:50 UTC) #3

Comments incorporated; PTAL.

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
content/browser/download/download_file.h:41: // Returns net::OK on success, or a
network error code on failure.
On 2012/07/05 18:47:21, asanka wrote:
> Mention new arguments. In particular also mention that |callback| will be
> invoked on the UI thread.

Good point--thanks for catching this.  Done.

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
File content/browser/download/download_file_manager_unittest.cc (right):

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
content/browser/download/download_file_manager_unittest.cc:218: const FilePath&
unique_path,
On 2012/07/05 18:47:21, asanka wrote:
> |unique_path| should be unused.

Yeah--I got lazy here since this file is about to go away.  But temporary
solutions aren't; I've cleaned it up.

Done.

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
content/browser/download/download_file_manager_unittest.cc:219:
content::DownloadInterruptReason rename_error,
On 2012/07/05 18:47:21, asanka wrote:
> |rename_error| is unused.

Done.

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
content/browser/download/download_file_manager_unittest.cc:220: RenameFileState
state,
On 2012/07/05 18:47:21, asanka wrote:
> |state| is unused now.

Done.

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
content/browser/download/download_file_manager_unittest.cc:226:
EXPECT_CALL(*file, Rename(unique_path, (should_overwrite == OVERWRITE), _))
On 2012/07/05 18:47:21, asanka wrote:
> Should be expecting Rename(new_path,...) since unique_path hasn't be
determined
> at the time download_file->Rename() is called.

Done.

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
content/browser/download/download_file_manager_unittest.cc:328:
TEST_F(DownloadFileManagerTest, RenameInProgress) {
On 2012/07/05 18:47:21, asanka wrote:
> The state of the download (whether it's IN_PROGRESS or COMPLETE) is now
> irrelevant to DFM during a rename.

Done.

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
content/browser/download/download_file_manager_unittest.cc:344:
TEST_F(DownloadFileManagerTest, RenameInProgressWithError) {
On 2012/07/05 18:47:21, asanka wrote:
> This can't be tested here anymore. See suggestion for adding test in
> download_file_unittest.cc.

Done.

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
File content/browser/download/download_file_unittest.cc (right):

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
content/browser/download/download_file_unittest.cc:261:
CHECK(callback_was_called);
On 2012/07/05 18:47:21, asanka wrote:
> The last time came up, I ended up creating a local
WeakPtrFactory<TestClassName>
> which vended a weak reference to |this|. The weak reference was used to bind
to
> TestClassName::HandleCallback() that handled the callback. This way when the
> WeakPtrFactory<> went out of scope, it would invalidate the callback and
prevent
> it from stomping memory.

Nice.  Done.

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
content/browser/download/download_file_unittest.cc:402: 
On 2012/07/05 18:47:21, asanka wrote:
> Also throw in a RenameError test now that error reporting on renames can't be
> tested in DownloadFileManagerTest?
> 
> A simple one where should_overwrite = true and a conflicting file exists and
is
> unwriteable would be sufficient. base/test/test_file_util.h has
> MakeFileUnwriteable() and PermissionRestorer which will help create a
temporary
> unwriteable file.

Good thought!  I didn't know about MakeFileUnwritable (or had forgotten).  Done.

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
content/browser/download/download_file_unittest.cc:465:
EXPECT_CALL(*(download_manager_.get()),
On 2012/07/05 18:47:21, asanka wrote:
> Expect a sequence restricted UpdateDownload() call before the
> OnDownloadInterrupted() call. I think we are now sensitive to getting
> UpdateDownload() before OnDownloadInterrupted().
> 
> Also applies to other tests below. 

So the tricky thing here is that we're going to have an arbitrary number of
UpdateDownload() calls coming in generated by the timer.  So I think the
invariant that we want is that the last UpdateDownload call to come in before
the OnDownloadInterrupted had the information we wanted.  This isn't idea, as if
the UpdateDownload call wasn't sparked by the interrupt, the test may still
sometimes pass.  But I don't think that's likely, and I'll put a comment next to
the flaky failure location indicating what it means.  Let me know what you
think.

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10689093/diff/2001/content/browser/download/do...
content/browser/download/download_item_impl.cc:768: // full_path is now the
current and target file path.
On 2012/07/05 18:47:21, asanka wrote:
> Nit: DCHECK(!full_path.empty())

Done.

asanka

I'd say LGTM with a clean try run, but MakeFileUnwriteable() still leaves the file overwriteable ...

8 years, 5 months ago (2012-07-10 18:11:45 UTC) #4

Randy Smith (Not in Mondays)

Jay, could I get a review/rubberstamp from you for changes to content/test and content/public/test?

8 years, 5 months ago (2012-07-11 01:08:50 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10689093/5003

8 years, 5 months ago (2012-07-11 17:13:28 UTC) #7

Change committed as 146162

Expand Messages | Collapse Messages