Kill DownloadManagerImpl::save_page_downloads_

Kill DownloadManagerImpl::save_page_downloads_
Use an ExternalData for the cases where we still need to distinguish save page downloads. Eventually, this should not be necessary.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146135

[benjhayden, 2012-07-10 17:39:15]

PTAL

[asanka, 2012-07-10 18:43:31]

http://codereview.chromium.org/10695087/diff/4001/content/browser/download/do...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10695087/diff/4001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:69: return
reinterpret_cast<const void*>(&SavePageExternalData::Get);
Multiple functions with identical code can be aliased in optimized builds. So if
there was, say, a FooExternalData class with a similar Get() method, then we
might end up with FooExternalData::Get() and SavePageExternalData::Get() being
at the same address.

[Randy Smith (Not in Mondays), 2012-07-10 19:06:05]

So it will probably not surprise you that I consider this a hack.  OTOH, I feel
the same way about the save_page_downloads_ list (not to mention the save page
system as a whole) and this is a somewhat simpler hack for the downloads code
than those other two areas.  So I'm ok with it, with a bit of wincing :-J.

http://codereview.chromium.org/10695087/diff/4001/content/browser/download/do...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10695087/diff/4001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:57: static
SavePageExternalData* Get(DownloadItem* item) {
Why not make this return bool?  That's what you're actually curious about, and
you don't ever do anything with this once you've created it.

http://codereview.chromium.org/10695087/diff/4001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:69: return
reinterpret_cast<const void*>(&SavePageExternalData::Get);
On 2012/07/10 18:43:31, asanka wrote:
> Multiple functions with identical code can be aliased in optimized builds. So
if
> there was, say, a FooExternalData class with a similar Get() method, then we
> might end up with FooExternalData::Get() and SavePageExternalData::Get() being
> at the same address.

Agreed.  I also think it's useful for debugging (in the abstract--I've never
actually used it) if we maintain the pattern that the key is a pointer to a
const static char * which describes the subsystem using the key.

http://codereview.chromium.org/10695087/diff/4001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:1149: DCHECK(download);  //
TODO(benjhayden) Pass download in.
I don't understand the TODO?

[benjhayden, 2012-07-10 20:00:08]

PTAL

http://codereview.chromium.org/10695087/diff/4001/content/browser/download/do...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10695087/diff/4001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:57: static
SavePageExternalData* Get(DownloadItem* item) {
On 2012/07/10 19:06:05, rdsmith wrote:
> Why not make this return bool?  That's what you're actually curious about, and
> you don't ever do anything with this once you've created it.

Done.

http://codereview.chromium.org/10695087/diff/4001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:69: return
reinterpret_cast<const void*>(&SavePageExternalData::Get);
On 2012/07/10 18:43:31, asanka wrote:
> Multiple functions with identical code can be aliased in optimized builds. So
if
> there was, say, a FooExternalData class with a similar Get() method, then we
> might end up with FooExternalData::Get() and SavePageExternalData::Get() being
> at the same address.

Done.

http://codereview.chromium.org/10695087/diff/4001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:69: return
reinterpret_cast<const void*>(&SavePageExternalData::Get);
On 2012/07/10 19:06:05, rdsmith wrote:
> On 2012/07/10 18:43:31, asanka wrote:
> > Multiple functions with identical code can be aliased in optimized builds.
So
> if
> > there was, say, a FooExternalData class with a similar Get() method, then we
> > might end up with FooExternalData::Get() and SavePageExternalData::Get()
being
> > at the same address.
> 
> Agreed.  I also think it's useful for debugging (in the abstract--I've never
> actually used it) if we maintain the pattern that the key is a pointer to a
> const static char * which describes the subsystem using the key.

Done.

http://codereview.chromium.org/10695087/diff/4001/content/browser/download/do...
content/browser/download/download_manager_impl.cc:1149: DCHECK(download);  //
TODO(benjhayden) Pass download in.
On 2012/07/10 19:06:05, rdsmith wrote:
> I don't understand the TODO?

I realized that it would be easier to do the todo than to explain the todo, so I
did it.

[Randy Smith (Not in Mondays), 2012-07-10 20:48:01]

LGTM.

[commit-bot: I haz the power, 2012-07-11 15:55:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10695087/2004

[commit-bot: I haz the power, 2012-07-11 17:38:59]

Change committed as 146135