BrowserContext should simply own DownloadManager

BrowserContext should simply own DownloadManager

This patch changes BrowserContext so it store a DownloadManager
directly (as UserData), instead of storing a refptr to it.

Client code (mostly tests) that used refptrs were changed to use
either raw pointers (when they are only referencing) or to use
scoped_ptr/ScopedVector in case of DownloadManagers created by them.

In cases where DownloadManager was used with Bind() to create a
callback, care was taken to make sure that we didn't get invalid
callbacks -- we were protected from this before because
DownloadManager was refcounted.

The Bind() in DownloadManagerImpl::CheckForFileRemoval() is using
a weak pointer to itself. The Bind() calls in
DownloadTestObserver were changed to go first thru the object
itself and then check whether the DownloadManager was still
valid.

Removed DownloadManager parameter from
PluginInstaller::DownloadStarted() since it was not used
anymore.

BUG=237871
TEST=content_unittests and browser_tests filtered with Download\*

[Jói, 2013-05-17 03:05:36]

I was about to ask you for background on this change but I see it's in
the bug it refers to.

With so many reviewers, it is good to specify one as the main reviewer
(I assume benjhayden@?) and specify which areas you want the other
reviewers to look at.

Cheers,
Jói


On Thu, May 16, 2013 at 5:18 PM,  <caio.de.oliveira.filho@intel.com> wrote:
> Reviewers: benjhayden_chromium, sky, Bernhard Bauer, Jói, rdsmith,
>
> Description:
> BrowserContext should simply own DownloadManager
>
> This patch changes BrowserContext so it store a DownloadManager
> directly (as UserData), instead of storing a refptr to it.
>
> Client code (mostly tests) that used refptrs were changed to use
> either raw pointers (when they are only referencing) or to use
> scoped_ptr/ScopedVector in case of DownloadManagers created by them.
>
> In cases where DownloadManager was used with Bind() to create a
> callback, care was taken to make sure that we didn't get invalid
> callbacks -- we were protected from this before because
> DownloadManager was refcounted.
>
> The Bind() in DownloadManagerImpl::CheckForFileRemoval() is using
> a weak pointer to itself. The Bind() calls in
> DownloadTestObserver were changed to go first thru the object
> itself and then check whether the DownloadManager was still
> valid.
>
> Removed DownloadManager parameter from
> PluginInstaller::DownloadStarted() since it was not used
> anymore.
>
> BUG=237871
> TEST=content_unittests and browser_tests filtered with Download\*
>
> Please review this at https://codereview.chromium.org/14593012/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files:
>   M chrome/browser/download/all_download_item_notifier_unittest.cc
>   M chrome/browser/download/chrome_download_manager_delegate.h
>   M chrome/browser/download/chrome_download_manager_delegate_unittest.cc
>   M chrome/browser/download/download_browsertest.cc
>   M chrome/browser/download/download_history_unittest.cc
>   M chrome/browser/download/download_shelf_unittest.cc
>   M chrome/browser/download/download_status_updater_unittest.cc
>   M chrome/browser/download/download_ui_controller_unittest.cc
>   M chrome/browser/download/test_download_shelf.h
>   M chrome/browser/download/test_download_shelf.cc
>   M chrome/browser/plugins/plugin_installer.h
>   M chrome/browser/plugins/plugin_installer.cc
>   M content/browser/browser_context.cc
>   M content/browser/download/download_id_unittest.cc
>   M content/browser/download/download_manager_impl.h
>   M content/browser/download/download_manager_impl.cc
>   content/browser/download/download_manager_impl_unittest.cc
>   M content/public/browser/download_manager.h
>   M content/public/test/download_test_observer.h
>   M content/public/test/download_test_observer.cc
>   M content/public/test/mock_download_manager.h
>   M content/public/test/test_file_error_injector.h
>   M content/public/test/test_file_error_injector.cc
>
>

[Bernhard Bauer, 2013-05-17 07:12:03]

chrome/browser/plugins LGTM (I assume that's what you want me to look at?)

[benjhayden, 2013-05-17 14:50:02]

https://codereview.chromium.org/14593012/diff/1/chrome/browser/download/downl...
File chrome/browser/download/download_shelf_unittest.cc (right):

https://codereview.chromium.org/14593012/diff/1/chrome/browser/download/downl...
chrome/browser/download/download_shelf_unittest.cc:46:
content::MockDownloadManager* download_manager_;
Should this be a scoped_ptr?

https://codereview.chromium.org/14593012/diff/1/chrome/browser/download/test_...
File chrome/browser/download/test_download_shelf.cc (right):

https://codereview.chromium.org/14593012/diff/1/chrome/browser/download/test_...
chrome/browser/download/test_download_shelf.cc:31: download_manager_ =
download_manager;
Should TestDownloadShelf observe download_manager_ and NULLify it when
ManagerGoingDown()?

https://codereview.chromium.org/14593012/diff/1/content/public/test/download_...
File content/public/test/download_test_observer.cc (right):

https://codereview.chromium.org/14593012/diff/1/content/public/test/download_...
content/public/test/download_test_observer.cc:80:
download_manager_->RemoveObserver(this);
if (download_manager_)

https://codereview.chromium.org/14593012/diff/1/content/public/test/download_...
File content/public/test/download_test_observer.h (right):

https://codereview.chromium.org/14593012/diff/1/content/public/test/download_...
content/public/test/download_test_observer.h:58: // TODO: Transition to finish
when manager goes down.
Does this require more than `if (waiting_)
base::MessageLoopForUI::current()->Quit();`?

[cmarcelo, 2013-05-20 15:22:06]

Thanks for the review. I'll break this in a couple of smaller pieces that will
hopefully avoid the big overlap in reviewers and make things simpler to review.

https://codereview.chromium.org/14593012/diff/1/chrome/browser/download/downl...
File chrome/browser/download/download_shelf_unittest.cc (right):

https://codereview.chromium.org/14593012/diff/1/chrome/browser/download/downl...
chrome/browser/download/download_shelf_unittest.cc:46:
content::MockDownloadManager* download_manager_;
On 2013/05/17 14:50:03, benjhayden_chromium wrote:
> Should this be a scoped_ptr?

Indeed, thanks.

https://codereview.chromium.org/14593012/diff/1/chrome/browser/download/test_...
File chrome/browser/download/test_download_shelf.cc (right):

https://codereview.chromium.org/14593012/diff/1/chrome/browser/download/test_...
chrome/browser/download/test_download_shelf.cc:31: download_manager_ =
download_manager;
On 2013/05/17 14:50:03, benjhayden_chromium wrote:
> Should TestDownloadShelf observe download_manager_ and NULLify it when
> ManagerGoingDown()?

It's not strictly necessary for the existing code using it because in the only
test where the download manager is set the lifetime of downloadmanager is larger
than the shelf.

However I think is good that it does observe. I will update this.

https://codereview.chromium.org/14593012/diff/1/content/public/test/download_...
File content/public/test/download_test_observer.cc (right):

https://codereview.chromium.org/14593012/diff/1/content/public/test/download_...
content/public/test/download_test_observer.cc:80:
download_manager_->RemoveObserver(this);
On 2013/05/17 14:50:03, benjhayden_chromium wrote:
> if (download_manager_)

You're right, thanks.

https://codereview.chromium.org/14593012/diff/1/content/public/test/download_...
File content/public/test/download_test_observer.h (right):

https://codereview.chromium.org/14593012/diff/1/content/public/test/download_...
content/public/test/download_test_observer.h:58: // TODO: Transition to finish
when manager goes down.
On 2013/05/17 14:50:03, benjhayden_chromium wrote:
> Does this require more than `if (waiting_)
> base::MessageLoopForUI::current()->Quit();`?

It would work in current code base, but would break the concept of "Finished"
for this object. Its documentation states that we can wait or probe ourselves
(via IsFinished()), quitting the message loop works for the former but not the
latter.

I'll try to fix this, though.

[cmarcelo, 2013-05-23 19:58:06]

I split this patch in smaller patches. The last piece, which does the ownership
change, is now in https://codereview.chromium.org/15701007