Add a const overload to DownloadItem::GetExternalData()

Add a const overload to DownloadItem::GetExternalData().

BUG=78085
TEST=unit_tests


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

[Randy Smith (Not in Mondays), 2012-02-29 01:06:38]

What's the motivation for this?  The code looks good for the intended goal, and
I don't have major objections to the intended goal, but it makes me a little
uncomfortable since we're not returning a const pointer and in some sense the
external data is part of the download item.  So on a conceptual level we're
claiming that the routine doesn't modify the contents of the DownloadItem, but
it does return a pointer which allows modification of data that is sort of part
of the DownloadItem.  It's not a strong objection, but I would like to
understand the need before this goes in.

[asanka, 2012-02-29 20:26:28]

On 2012/02/29 01:06:38, rdsmith wrote:
> What's the motivation for this?  The code looks good for the intended goal,
and
> I don't have major objections to the intended goal, but it makes me a little
> uncomfortable since we're not returning a const pointer and in some sense the
> external data is part of the download item.  So on a conceptual level we're
> claiming that the routine doesn't modify the contents of the DownloadItem, but
> it does return a pointer which allows modification of data that is sort of
part
> of the DownloadItem.  It's not a strong objection, but I would like to
> understand the need before this goes in.

This was to allow a consumer of a 'const DownloadItem*' to examine ExternalData.
I keep running into the issue of needing a non-const DownloadItem* everywhere
during some of the recent refactors.

I had considered ExternalData to be owned by DownloadItem, but not necessarily a
part of it I.e. DownloadItem doesn't consider its state to have changed if the
contents of an ExternalData object changes. If that assumption is incorrect,
then so would this change.

Perhaps a better option would be to have a const overload for GetExternalData().

[Randy Smith (Not in Mondays), 2012-02-29 20:37:43]

On 2012/02/29 20:26:28, asanka wrote:
> On 2012/02/29 01:06:38, rdsmith wrote:
> > What's the motivation for this?  The code looks good for the intended goal,
> and
> > I don't have major objections to the intended goal, but it makes me a little
> > uncomfortable since we're not returning a const pointer and in some sense
the
> > external data is part of the download item.  So on a conceptual level we're
> > claiming that the routine doesn't modify the contents of the DownloadItem,
but
> > it does return a pointer which allows modification of data that is sort of
> part
> > of the DownloadItem.  It's not a strong objection, but I would like to
> > understand the need before this goes in.
> 
> This was to allow a consumer of a 'const DownloadItem*' to examine
ExternalData.
> I keep running into the issue of needing a non-const DownloadItem* everywhere
> during some of the recent refactors.
> 
> I had considered ExternalData to be owned by DownloadItem, but not necessarily
a
> part of it I.e. DownloadItem doesn't consider its state to have changed if the
> contents of an ExternalData object changes. If that assumption is incorrect,
> then so would this change.

It's a fuzzy issue--I don't think it has an obvious right answer.  It's just
enough to make me uncomfortable. 

> Perhaps a better option would be to have a const overload for
GetExternalData().

Yeah, I think I'd prefer that, and have it return a const void *.  That'll have
to be casted, so it'll be easy to cast away the const, but we'll at least be
raising the flag.

[asanka, 2012-03-01 17:40:49]

PTAL?

[Randy Smith (Not in Mondays), 2012-03-01 21:49:05]

LGTM.

http://codereview.chromium.org/9518008/diff/8001/content/browser/download/dow...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/9518008/diff/8001/content/browser/download/dow...
content/browser/download/download_item_impl.cc:1133:
DownloadItemImpl::GetExternalData(const void* key) {
I'd be inclined to implement this with a call to the other one and a comment
about why it's ok to cast away const, just to avoid the code duplication, but
it's up to you.

[asanka, 2012-03-02 16:09:25]

http://codereview.chromium.org/9518008/diff/8001/content/browser/download/dow...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/9518008/diff/8001/content/browser/download/dow...
content/browser/download/download_item_impl.cc:1133:
DownloadItemImpl::GetExternalData(const void* key) {
On 2012/03/01 21:49:05, rdsmith wrote:
> I'd be inclined to implement this with a call to the other one and a comment
> about why it's ok to cast away const, just to avoid the code duplication, but
> it's up to you.

Done.

[commit-bot: I haz the power, 2012-03-02 16:10:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/9518008/25001

[commit-bot: I haz the power, 2012-03-02 17:44:58]

Change committed as 124678