Implemented ExternalData interface on DownloadItem and used it for SafeBrowsing.

Implemented ExternalData interface on DownloadItem and used it for SafeBrowsing.



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

[Randy Smith (Not in Mondays), 2011-11-16 02:42:44]

Would you guys take a look when you have a chance?  Asanka for the basic
mechanism (in download_item.*), Noe' for the use of it on SafeBrowsing (in
chrome_download_manager_delegate.*).  No rush--I think you're both working on
higher priority stuff.

[noelutz, 2011-11-16 02:59:38]

lgtm

Feel free to submit as is.  I just have some minor comments.

Thanks,
noé.

http://codereview.chromium.org/8571023/diff/2001/chrome/browser/download/chro...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8571023/diff/2001/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:50: static char
safe_browsing_id[] = "Safe Browsing ID";
nit: make that const?

http://codereview.chromium.org/8571023/diff/2001/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:353: state->pending
= false;
nit: add an if here?  Otherwise, this will crash in the release build if state
is NULL.

http://codereview.chromium.org/8571023/diff/2001/content/browser/download/dow...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/8571023/diff/2001/content/browser/download/dow...
content/browser/download/download_item.cc:731: if (external_data_map_.count(key)
!= 0) {
How about this to avoid all these lookups?

std::map<void*, ExternalData*>::iterator it = ....find(key);
if (it == ...end()) {
  external_data_map_[key] = data;
} else if (it->second != data) {
  delete it->second;
  it->second = data;
}

[asanka, 2011-11-16 03:52:19]

LGTM with a few nits.

http://codereview.chromium.org/8571023/diff/2001/chrome/browser/download/down...
File chrome/browser/download/download_item_unittest.cc (right):

http://codereview.chromium.org/8571023/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_item_unittest.cc:289: EXPECT_EQ(test2,
item->GetExternalData(&external_data_test_string));
Nit: This is comparing a TestExternalData* and a ExternalData*.

http://codereview.chromium.org/8571023/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_item_unittest.cc:297: // Ovewrriting with NULL
should result in destruction.
Nit: Overwriting

http://codereview.chromium.org/8571023/diff/2001/content/browser/download/dow...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8571023/diff/2001/content/browser/download/dow...
content/browser/download/download_item.h:519: std::map<void*, ExternalData*>
external_data_map_;
Would a std::map<const void*, ExternalData*> be better?

[Randy Smith (Not in Mondays), 2011-11-18 02:02:57]

Incorporated comments; no need for re-review.  Thanks!

http://codereview.chromium.org/8571023/diff/2001/chrome/browser/download/chro...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8571023/diff/2001/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:50: static char
safe_browsing_id[] = "Safe Browsing ID";
On 2011/11/16 02:59:38, noelutz wrote:
> nit: make that const?

Done.

http://codereview.chromium.org/8571023/diff/2001/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:353: state->pending
= false;
On 2011/11/16 02:59:38, noelutz wrote:
> nit: add an if here?  Otherwise, this will crash in the release build if state
> is NULL.

I wince, just because when I plot that case forward, the download will hang
(keeps sending out safe browsing requests from ShouldCompleteDownload).  But
that's probably better than crashing.  But it probably can't happen.  Hmmm.  Ok,
if statement.

http://codereview.chromium.org/8571023/diff/2001/chrome/browser/download/down...
File chrome/browser/download/download_item_unittest.cc (right):

http://codereview.chromium.org/8571023/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_item_unittest.cc:289: EXPECT_EQ(test2,
item->GetExternalData(&external_data_test_string));
On 2011/11/16 03:52:19, asanka wrote:
> Nit: This is comparing a TestExternalData* and a ExternalData*.

Done.

http://codereview.chromium.org/8571023/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_item_unittest.cc:297: // Ovewrriting with NULL
should result in destruction.
On 2011/11/16 03:52:19, asanka wrote:
> Nit: Overwriting

Done.

http://codereview.chromium.org/8571023/diff/2001/content/browser/download/dow...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/8571023/diff/2001/content/browser/download/dow...
content/browser/download/download_item.cc:731: if (external_data_map_.count(key)
!= 0) {
On 2011/11/16 02:59:38, noelutz wrote:
> How about this to avoid all these lookups?
> 
> std::map<void*, ExternalData*>::iterator it = ....find(key);
> if (it == ...end()) {
>   external_data_map_[key] = data;
> } else if (it->second != data) {
>   delete it->second;
>   it->second = data;
> }

Yes, noticeably better.  Thanks for the nudge.  Done.

http://codereview.chromium.org/8571023/diff/2001/content/browser/download/dow...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8571023/diff/2001/content/browser/download/dow...
content/browser/download/download_item.h:519: std::map<void*, ExternalData*>
external_data_map_;
On 2011/11/16 03:52:19, asanka wrote:
> Would a std::map<const void*, ExternalData*> be better?

Done.

[Randy Smith (Not in Mondays), 2011-12-07 00:48:53]

Sorry, I let this rot for a bit and had to do a bit of merging to get it cleanly
near TOT.  Noe', Asanka: One more round?

[noelutz, 2011-12-07 01:30:37]

lgtm

[asanka, 2011-12-07 04:10:57]

LGTM, where G=Great!

Comments are nits. I would love to see this on M17 because it leads to a cleaner
fix for the "dangerous download"-"Save As" issue.

http://codereview.chromium.org/8571023/diff/15001/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8571023/diff/15001/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:52: // The state of
a safebrowsing check.  If there is no data associated with the
cliffhanger :)

http://codereview.chromium.org/8571023/diff/15001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8571023/diff/15001/content/browser/download/do...
content/browser/download/download_item.h:329: // the passed key, it will be
destroyed on overwrite.
Nit: Should this mention the behavior when the object already held is the same
as the new one being set?

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

http://codereview.chromium.org/8571023/diff/15001/content/browser/download/do...
content/browser/download/download_item_impl.cc:767: DownloadItem::ExternalData*
Style nit: The method implementation order isn't the same as the order in the
header.

http://codereview.chromium.org/8571023/diff/15001/content/browser/download/do...
content/browser/download/download_item_impl.cc:769: if
(external_data_map_.count(key) == 0)
Just curious; why not ContainsKey()?

[Randy Smith (Not in Mondays), 2011-12-07 18:36:12]

Going ahead with try bots and the whole commit grind; thanks!

In terms of getting this into MS-17: I'll take a swing at it, but I think it'll
be a bit of a tricky sell.  Asanka: Can you give me a sense as to how you expect
to use it?  That would make it easier (the basic argument has to be that this
change + your change is less complex than your change would be without this
one).

http://codereview.chromium.org/8571023/diff/15001/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8571023/diff/15001/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:52: // The state of
a safebrowsing check.  If there is no data associated with the
On 2011/12/07 04:10:57, asanka wrote:
> cliffhanger :)

Removed :-}.

http://codereview.chromium.org/8571023/diff/15001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8571023/diff/15001/content/browser/download/do...
content/browser/download/download_item.h:329: // the passed key, it will be
destroyed on overwrite.
On 2011/12/07 04:10:57, asanka wrote:
> Nit: Should this mention the behavior when the object already held is the same
> as the new one being set?

Done.

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

http://codereview.chromium.org/8571023/diff/15001/content/browser/download/do...
content/browser/download/download_item_impl.cc:767: DownloadItem::ExternalData*
On 2011/12/07 04:10:57, asanka wrote:
> Style nit: The method implementation order isn't the same as the order in the
> header.

Done.

http://codereview.chromium.org/8571023/diff/15001/content/browser/download/do...
content/browser/download/download_item_impl.cc:769: if
(external_data_map_.count(key) == 0)
On 2011/12/07 04:10:57, asanka wrote:
> Just curious; why not ContainsKey()?

Prejudice on my part, which it's probably time for me to get over in the name of
drinking the Chrome Koolaid (I haven't seen the point of using a separate
routine that pretty much does the same thing as an available one liner, though
to be fair, the routine is probably better performing).  Changed.

[asanka, 2011-12-07 19:16:45]

On 2011/12/07 18:36:12, rdsmith wrote:
> Going ahead with try bots and the whole commit grind; thanks!
> 
> In terms of getting this into MS-17: I'll take a swing at it, but I think
it'll
> be a bit of a tricky sell.  Asanka: Can you give me a sense as to how you
expect
> to use it?  That would make it easier (the basic argument has to be that this
> change + your change is less complex than your change would be without this
> one).

My use case is to extract the notion of intermediate filenames for a potentially
dangerous download into chrome/b/download in order to (partially) resolve the
target_name/suggested_path/full_path confusion in content/b/download.  For the
moment I'm re-purposing (and renaming) SafeBrowsingState to keep track of the
generated intermediate filename.

Aside from the fact that using ExternalData would yield a cleaner design, it
would also reduce merge pains when these two CLs collide. This is why I liked
the idea of this already being in M17 :).  However, I'm not quite sure that the
reduction in complexity justifies merging both to M17.

>
http://codereview.chromium.org/8571023/diff/15001/chrome/browser/download/chr...
> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> 
>
http://codereview.chromium.org/8571023/diff/15001/chrome/browser/download/chr...
> chrome/browser/download/chrome_download_manager_delegate.cc:52: // The state
of
> a safebrowsing check.  If there is no data associated with the
> On 2011/12/07 04:10:57, asanka wrote:
> > cliffhanger :)
> 
> Removed :-}.
> 
>
http://codereview.chromium.org/8571023/diff/15001/content/browser/download/do...
> File content/browser/download/download_item.h (right):
> 
>
http://codereview.chromium.org/8571023/diff/15001/content/browser/download/do...
> content/browser/download/download_item.h:329: // the passed key, it will be
> destroyed on overwrite.
> On 2011/12/07 04:10:57, asanka wrote:
> > Nit: Should this mention the behavior when the object already held is the
same
> > as the new one being set?
> 
> Done.
> 
>
http://codereview.chromium.org/8571023/diff/15001/content/browser/download/do...
> File content/browser/download/download_item_impl.cc (right):
> 
>
http://codereview.chromium.org/8571023/diff/15001/content/browser/download/do...
> content/browser/download/download_item_impl.cc:767:
DownloadItem::ExternalData*
> On 2011/12/07 04:10:57, asanka wrote:
> > Style nit: The method implementation order isn't the same as the order in
the
> > header.
> 
> Done.
> 
>
http://codereview.chromium.org/8571023/diff/15001/content/browser/download/do...
> content/browser/download/download_item_impl.cc:769: if
> (external_data_map_.count(key) == 0)
> On 2011/12/07 04:10:57, asanka wrote:
> > Just curious; why not ContainsKey()?
> 
> Prejudice on my part, which it's probably time for me to get over in the name
of
> drinking the Chrome Koolaid (I haven't seen the point of using a separate
> routine that pretty much does the same thing as an available one liner, though
> to be fair, the routine is probably better performing).  Changed.

[commit-bot: I haz the power, 2011-12-07 21:28:12]

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

[commit-bot: I haz the power, 2011-12-07 23:03:25]

Change committed as 113494