GTTF: Clean up DownloadFileManager

GTTF: Clean up DownloadFileManager 

This removes a lot of duplication, locking, and thread jumping. 

Most of the operations run on the FILE thread, and we do not duplicate 
so much information. Each DownloadFile keeps track of its DownloadManager 
(each Profile has its own DownloadManager). This allows us to remove 
many maps from DownloadFileManager that were duplicating that information. 

There is still SaveFileManager, but hopefully I will be able 
to merge those two in small steps. 

Hopefully, this is http://codereview.chromium.org/3245005 done right.

TEST=unit_tests, browser_tests, ui_tests 
BUG=48913
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=59139

[Paweł Hajdan Jr., 2010-09-08 18:32:55]

This is a relanding of my earlier patch with the fix for the race condition, and
the new crash detected by pyauto tests, and stack trace extracted by Nirnimesh:

03a5fb50 635abe81 chrome_63360000!DownloadHistory::UpdateEntry+0x23
[d:\b\slave\chrome-official\build\src\chrome\browser\download\download_history.cc
@ 76]
03a5fbb0 635abcbc chrome_63360000!DownloadManager::Shutdown+0x11f
[d:\b\slave\chrome-official\build\src\chrome\browser\download\download_manager.cc
@ 99]
03a5fbd4 635abc3c chrome_63360000!DownloadManager::~DownloadManager+0x6c
[d:\b\slave\chrome-official\build\src\chrome\browser\download\download_manager.cc
@ 71]
03a5fbe0 63d27574 chrome_63360000!DownloadManager::`scalar deleting
destructor'+0x9
03a5fbe8 63613ff0
chrome_63360000!base::RefCountedThreadSafe<DevToolsRemoteListener,base::DefaultRefCountedThreadSafeTraits<DevToolsRemoteListener>
>::Release+0x20 [d:\b\slave\chrome-official\build\src\base\ref_counted.h @ 144]
03a5fbf4 63613fc0 chrome_63360000!DownloadFile::~DownloadFile+0x1c
[d:\b\slave\chrome-official\build\src\chrome\browser\download\download_file.cc @
30]
03a5fbfc 6351e273 chrome_63360000!DownloadFile::`scalar deleting destructor'+0x8
03a5fc2c 6351d4d8
chrome_63360000!STLDeleteValues<stdext::hash_map<int,DownloadFile
*,stdext::hash_compare<int,std::less<int> >,std::allocator<std::pair<int const
,DownloadFile *> > > >+0x57
[d:\b\slave\chrome-official\build\src\base\stl_util-inl.h @ 243]
03a5fc38 634221a1 chrome_63360000!DownloadFileManager::OnShutdown+0x14
[d:\b\slave\chrome-official\build\src\chrome\browser\download\download_file_manager.cc
@ 74]
03a5fc68 6342222d chrome_63360000!MessageLoop::RunTask+0x97
[d:\b\slave\chrome-official\build\src\base\message_loop.cc @ 409]

The changes for last fix are just in profile_impl.cc and download_manager.

[darin (slow to review), 2010-09-09 16:04:35]

OK, LGTM... w/ one possible issue:

http://codereview.chromium.org/3347018/diff/1/7
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/3347018/diff/1/7#newcode128
chrome/browser/download/download_file_manager.cc:128:
update_timer_.Start(base::TimeDelta::FromMilliseconds(kUpdatePeriodMs),
one slight concern with this is that the FILE thread can be janky, but this
timer drives UI updates.  perhaps that is not so ideal?

[Paweł Hajdan Jr., 2010-09-09 17:19:41]

http://codereview.chromium.org/3347018/diff/1/7
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/3347018/diff/1/7#newcode128
chrome/browser/download/download_file_manager.cc:128:
update_timer_.Start(base::TimeDelta::FromMilliseconds(kUpdatePeriodMs),
On 2010/09/09 16:04:35, darin wrote:
> one slight concern with this is that the FILE thread can be janky, but this
> timer drives UI updates.  perhaps that is not so ideal?

Right, I didn't think about that. However, the old code was using locks for
ui_progress_ map. It's quite possible that if the FILE thread was janky, then
the timer could keep running on the UI thread, but it would still get the same
old data.

We can restore the ui_progress_ map in case of problems, or try moving the timer
back to UI thread, and using PostTask from FILE thread to UI thread instead of
the locks.

[Nirnimesh, 2010-09-09 17:38:58]

I'm not too familiar with the internals of this code, so please don't wait for
review from me.
I'll be on the lookout to make sure that pyauto tests for downloads pass after
you commit.
My only concern is that since most of the download uitests are disabled at the
moment, this is risky.

[darin (slow to review), 2010-09-09 18:30:08]

On Thu, Sep 9, 2010 at 10:19 AM, <phajdan.jr@chromium.org> wrote:

>
> http://codereview.chromium.org/3347018/diff/1/7
> File chrome/browser/download/download_file_manager.cc (right):
>
> http://codereview.chromium.org/3347018/diff/1/7#newcode128
> chrome/browser/download/download_file_manager.cc:128:
> update_timer_.Start(base::TimeDelta::FromMilliseconds(kUpdatePeriodMs),
> On 2010/09/09 16:04:35, darin wrote:
>
>> one slight concern with this is that the FILE thread can be janky, but
>>
> this
>
>> timer drives UI updates.  perhaps that is not so ideal?
>>
>
> Right, I didn't think about that. However, the old code was using locks
> for ui_progress_ map. It's quite possible that if the FILE thread was
> janky, then the timer could keep running on the UI thread, but it would
> still get the same old data.
>
> We can restore the ui_progress_ map in case of problems, or try moving
> the timer back to UI thread, and using PostTask from FILE thread to UI
> thread instead of the locks.


The old way is actually the common way that we do progress notifications.
 Instead of blasting notifications to the UI thread each time a progress
counter increments, it is better to have a timer running on the UI thread
that runs at a good interval for UI updates (not necessarily as fast as the
progress counter is incrementing).

-Darin


>
>
> http://codereview.chromium.org/3347018/show
>

[darin (slow to review), 2010-09-09 18:37:47]

LGTM

[Paweł Hajdan Jr., 2010-09-09 20:36:04]

I have sent a new Patch Set to fix trybot problems and it seems to be working.
Links to changes:

http://codereview.chromium.org/3347018/diff2/8001:11001/12010
http://codereview.chromium.org/3347018/diff/11001/12012

[darin (slow to review), 2010-09-10 18:27:04]

LGTM