Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
663023007:
Include high-fidelity metadata about a download in incident reports. (Closed)
|
Created:
6 years, 2 months ago by grt (UTC plus 2) Modified:
6 years, 1 month ago CC:
chromium-reviews, benjhayden+dwatch_chromium.org, asanka, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git/+/master Project:
chromium Visibility:
Public. |
DescriptionInclude high-fidelity metadata about a download in incident reports.
This is accomplished by the introduction of the DownloadMetadataManager,
which maintains state about the most recent binary download.
BUG=389123, 386915, 389643
Committed: https://crrev.com/1f31ae2baee245dd2cdb456b685ce94a94bdc10e
Cr-Commit-Position: refs/heads/master@{#302624}
Patch Set 1 #Patch Set 2 : sync to r300417 #
Total comments: 45
Patch Set 3 : robert comments (and sync to r300494, oops) #
Total comments: 14
Patch Set 4 : skip on shutdown #
Total comments: 6
Patch Set 5 : i can spell #Patch Set 6 : sync to r301101 #
Total comments: 12
Patch Set 7 : asvitkine and asanka comments #Patch Set 8 : sync to commit position 302085 #Patch Set 9 : mattm comments #Patch Set 10 : check for presence of safe_browsing_service before using #
Total comments: 1
Patch Set 11 : sync to commit position 302604 #Patch Set 12 : added DCHECK #Messages
Total messages: 33 (10 generated)
grt@chromium.org changed reviewers: + mattm@chromium.org, robertshield@chromium.org
robertshield: please review everything mattm: please review at least the safe_browsing bits outside of incident_reporting. i'd appreciate your comments for the portions within incident_reporting if you have the time. thanks.
Preliminary comments, only part way through. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.cc:789: // This is a supported download for which a ClientDownloadRequest is not What does a supported download mean in this context? Also, please clarify that a ClientDownloadRequest is a safe browsing ping. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service.h (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.h:63: ClientDownloadRequestCallback; nit: line-break here to make it clearer the comment doesn't apply to the next two typedefs. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.h:66: ClientDownloadRequestCallbackList; Does the list typedef need to be public? https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service_unittest.cc:591: // TODO(grt): Make the DPS complete producing the request even when the URL is s/DPS/DownloadProtectionService/ https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:56: // Returns true if |metadata| appears to be a valid truncated sentence? https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:278: // Do not block shutdown on reads since nothing will come of it. double space https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:281: // Block shutdown on writes in the hopes of persisting important data. Which data being persisted is important enough to block shutdown? What's the effect of not persisting some of this metadata?
Almost done a first pass, some more comments. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:68: using base::File; DCHECK(metadata) ? https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:200: // null if the download has been destroyed. If non-null, the item is being item(NULL), removed(false)? (I find that clearer) https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:318: const GetDownloadDetailsCallback& callback) { This is part of the public interface, so should you DCHECK() that browser_context is non-null? Unsure whether that might hit a false positive in the context scan on 324. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.h (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.h:37: // details for the given BrowserContext. s/given BrowserContext/|browser_context| passed to GetDownloadDetails/ https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.h:56: // |request| is null, metadata for the download's BrowserContext are cleared. isn't it metatdata for the download's ManagerContext that is removed? https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.h:68: virtual content::DownloadManager* GetDownloadManagerForContext( please rename to GetDownloadManagerForBrowserContext. There are two types of Context in the .cc file, ManagerContext and BrowserContext and I found reading calls to this confusing. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:46: // Specializations for a history::DownloadRow. Why are template specializations better than overloads here? https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:321: // been deleted. which instance?
Thanks for the comments. Please take another look. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.cc:789: // This is a supported download for which a ClientDownloadRequest is not On 2014/10/21 13:15:38, robertshield wrote: > What does a supported download mean in this context? One for which CheckClientDownload was called. > Also, please clarify that a ClientDownloadRequest is a safe browsing ping. Although we throw around the word "ping" in conversation, that isn't what this thing is generally called in the code. A "safe browsing ping" (see ping_manager.h) is something different. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service.h (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.h:63: ClientDownloadRequestCallback; On 2014/10/21 13:15:38, robertshield wrote: > nit: line-break here to make it clearer the comment doesn't apply to the next > two typedefs. Done. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.h:66: ClientDownloadRequestCallbackList; On 2014/10/21 13:15:38, robertshield wrote: > Does the list typedef need to be public? Yes, since the contained Subscription type is part of the "register a callback" interface; see below. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service_unittest.cc:591: // TODO(grt): Make the DPS complete producing the request even when the URL is On 2014/10/21 13:15:38, robertshield wrote: > s/DPS/DownloadProtectionService/ Done. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:56: // Returns true if |metadata| appears to be a valid On 2014/10/21 13:15:38, robertshield wrote: > truncated sentence? I'm not sure what your point https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:68: using base::File; On 2014/10/22 02:28:56, robertshield wrote: > DCHECK(metadata) ? Done. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:200: // null if the download has been destroyed. If non-null, the item is being On 2014/10/22 02:28:56, robertshield wrote: > item(NULL), removed(false)? (I find that clearer) get off my lawn. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:278: // Do not block shutdown on reads since nothing will come of it. On 2014/10/21 13:15:38, robertshield wrote: > double space Done. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:281: // Block shutdown on writes in the hopes of persisting important data. On 2014/10/21 13:15:38, robertshield wrote: > Which data being persisted is important enough to block shutdown? What's the > effect of not persisting some of this metadata? The effect is just that we don't have the data. Are you concerned about blocking shutdown? https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:318: const GetDownloadDetailsCallback& callback) { On 2014/10/22 02:28:56, robertshield wrote: > This is part of the public interface, so should you DCHECK() that > browser_context is non-null? Unsure whether that might hit a false positive in > the context scan on 324. Done. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.h (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.h:37: // details for the given BrowserContext. On 2014/10/22 02:28:56, robertshield wrote: > s/given BrowserContext/|browser_context| passed to GetDownloadDetails/ Done. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.h:56: // |request| is null, metadata for the download's BrowserContext are cleared. On 2014/10/22 02:28:56, robertshield wrote: > isn't it metatdata for the download's ManagerContext that is removed? ManagerContext is an implementation detail. The important result of passing in null is that subsequent calls to GetDownloadDetails for |download|'s BrowserContext (content::DownloadItem::GetBrowserContext()) will invoke the callback with null. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.h:68: virtual content::DownloadManager* GetDownloadManagerForContext( On 2014/10/22 02:28:56, robertshield wrote: > please rename to GetDownloadManagerForBrowserContext. There are two types of > Context in the .cc file, ManagerContext and BrowserContext and I found reading > calls to this confusing. Done. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:46: // Specializations for a history::DownloadRow. On 2014/10/22 02:28:56, robertshield wrote: > Why are template specializations better than overloads here? um, because they're harder to read and write? https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:321: // been deleted. On 2014/10/22 02:28:56, robertshield wrote: > which instance? |this|. Comment tweaked.
Ok, cool beans. Comments on comments here, will do another full pass tonight. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service.h (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.h:66: ClientDownloadRequestCallbackList; On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > On 2014/10/21 13:15:38, robertshield wrote: > > Does the list typedef need to be public? > > Yes, since the contained Subscription type is part of the "register a callback" > interface; see below. The Subscription is but not the SubscriptionList. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:200: // null if the download has been destroyed. If non-null, the item is being On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > On 2014/10/22 02:28:56, robertshield wrote: > > item(NULL), removed(false)? (I find that clearer) > > get off my lawn. If I do will you make your code more readable? https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:281: // Block shutdown on writes in the hopes of persisting important data. On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > On 2014/10/21 13:15:38, robertshield wrote: > > Which data being persisted is important enough to block shutdown? What's the > > effect of not persisting some of this metadata? > > The effect is just that we don't have the data. Are you concerned about blocking > shutdown? A little. I am basing this off the comment here: https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq... Which says that we should generally only do this for user data. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.h (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.h:56: // |request| is null, metadata for the download's BrowserContext are cleared. On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > On 2014/10/22 02:28:56, robertshield wrote: > > isn't it metatdata for the download's ManagerContext that is removed? > > ManagerContext is an implementation detail. The important result of passing in > null is that subsequent calls to GetDownloadDetails for |download|'s > BrowserContext (content::DownloadItem::GetBrowserContext()) will invoke the > callback with null. Acknowledged.
Thanks. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service.h (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.h:66: ClientDownloadRequestCallbackList; On 2014/10/23 20:30:27, robertshield wrote: > On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > > On 2014/10/21 13:15:38, robertshield wrote: > > > Does the list typedef need to be public? > > > > Yes, since the contained Subscription type is part of the "register a > callback" > > interface; see below. > > The Subscription is but not the SubscriptionList. The two ways I've found of not making the CallbackList public are to either remove the ClientDownloadRequestCallbackList typedef altogether or to mix a private: section in the public: section like this: // A type of callback run on the main thread when a ClientDownloadRequest has // been formed for a download, or when one has not been formed for a supported // download. typedef base::Callback<void(content::DownloadItem*, const ClientDownloadRequest*)> ClientDownloadRequestCallback; private: // A list of ClientDownloadRequest callbacks. typedef base::CallbackList<void(content::DownloadItem*, const ClientDownloadRequest*)> ClientDownloadRequestCallbackList; public: // A subscription to a registered ClientDownloadRequest callback. typedef scoped_ptr<ClientDownloadRequestCallbackList::Subscription> ClientDownloadRequestSubscription; Although ClientDownloadRequestCallbackList doesn't need to be public, I think it's the more readable of the options that I can think of. Please give a concrete suggestion if you know of a better way. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:200: // null if the download has been destroyed. If non-null, the item is being On 2014/10/23 20:30:27, robertshield wrote: > On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > > On 2014/10/22 02:28:56, robertshield wrote: > > > item(NULL), removed(false)? (I find that clearer) > > > > get off my lawn. > > If I do will you make your code more readable? Value-initialization is a fundamental part of the language. It is a common way to initialize nearly anything (instance of a class, array, or primitive type). If you can show me that your aversion to it is more than just personal preference, I'm happy to change it. Think of it this way: if there was a foozit_ member of some class type that had a default constructor and a constructor that took a parameter, would you ask me to call the ctor that took a parameter and pass in the value that is used by the default ctor (e.g., foozit_(0) rather than foozit_()) when the behavior of the default ctor is really what is desired? https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:281: // Block shutdown on writes in the hopes of persisting important data. On 2014/10/23 20:30:27, robertshield wrote: > On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > > On 2014/10/21 13:15:38, robertshield wrote: > > > Which data being persisted is important enough to block shutdown? What's the > > > effect of not persisting some of this metadata? > > > > The effect is just that we don't have the data. Are you concerned about > blocking > > shutdown? > > A little. I am basing this off the comment here: > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq... > > Which says that we should generally only do this for user data. Yeah, I saw that comment. I'd been thinking of it as user data, but I suppose it's not in the same ballpark as preferences or history. Do you think SKIP_ON_SHUTDOWN is better for this?
Heya, comments on comments on comments :-) Apologies if I sounded grumpy! Moar general reading coming soon. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service.h (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.h:66: ClientDownloadRequestCallbackList; On 2014/10/23 21:02:18, grt (expect delays oct 27-29) wrote: > On 2014/10/23 20:30:27, robertshield wrote: > > On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > > > On 2014/10/21 13:15:38, robertshield wrote: > > > > Does the list typedef need to be public? > > > > > > Yes, since the contained Subscription type is part of the "register a > > callback" > > > interface; see below. > > > > The Subscription is but not the SubscriptionList. > > The two ways I've found of not making the CallbackList public are to either > remove the ClientDownloadRequestCallbackList typedef altogether or to mix a > private: section in the public: section like this: > > // A type of callback run on the main thread when a ClientDownloadRequest has > // been formed for a download, or when one has not been formed for a supported > // download. > typedef base::Callback<void(content::DownloadItem*, > const ClientDownloadRequest*)> > ClientDownloadRequestCallback; > > private: > // A list of ClientDownloadRequest callbacks. > typedef base::CallbackList<void(content::DownloadItem*, > const ClientDownloadRequest*)> > ClientDownloadRequestCallbackList; > > public: > // A subscription to a registered ClientDownloadRequest callback. > typedef scoped_ptr<ClientDownloadRequestCallbackList::Subscription> > ClientDownloadRequestSubscription; > > Although ClientDownloadRequestCallbackList doesn't need to be public, I think > it's the more readable of the options that I can think of. Please give a > concrete suggestion if you know of a better way. Can't you move the list typedef down into the private section? The style guide states that the typedefs-first ordering rule is per-visibility section: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:200: // null if the download has been destroyed. If non-null, the item is being On 2014/10/23 21:02:18, grt (expect delays oct 27-29) wrote: > On 2014/10/23 20:30:27, robertshield wrote: > > On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > > > On 2014/10/22 02:28:56, robertshield wrote: > > > > item(NULL), removed(false)? (I find that clearer) > > > > > > get off my lawn. > > > > If I do will you make your code more readable? > > Value-initialization is a fundamental part of the language. It is a common way > to initialize nearly anything (instance of a class, array, or primitive type). > If you can show me that your aversion to it is more than just personal > preference, I'm happy to change it. My readability comment was really based on the notion that if I see removed(false) I am pretty sure the member is a bool without looking down, similarly if I see item(NULL), it's probably a pointer. One could argue that neither of those are necessarily true and it may be dangerous to assume such things but they are true often enough in Chrome that in the general case it IMO enhances readability. There's also an element of convention of explicit initialization in initializer lists: https://code.google.com/p/chromium/codesearch#search/&q=%5C:.*%5C(false%5C)&s... > Think of it this way: if there was a foozit_ > member of some class type that had a default constructor and a constructor that > took a parameter, would you ask me to call the ctor that took a parameter and > pass in the value that is used by the default ctor (e.g., foozit_(0) rather than > foozit_()) when the behavior of the default ctor is really what is desired? In that case calling the default constructor would be the most readable thing which would generally in Chrome means leaving the member out of the initializer list altogether. Again, this is just convention, but it's a convention I have grown used to and that I feel speeds up reading. In any case, apologies for the somewhat punchy reply. I believe that the reasons I list above (code base convention and one less visual jump required when reading) are sufficient to motivate the change, but if you feel strongly you can leave this as is. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:281: // Block shutdown on writes in the hopes of persisting important data. On 2014/10/23 21:02:18, grt (expect delays oct 27-29) wrote: > On 2014/10/23 20:30:27, robertshield wrote: > > On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > > > On 2014/10/21 13:15:38, robertshield wrote: > > > > Which data being persisted is important enough to block shutdown? What's > the > > > > effect of not persisting some of this metadata? > > > > > > The effect is just that we don't have the data. Are you concerned about > > blocking > > > shutdown? > > > > A little. I am basing this off the comment here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq... > > > > Which says that we should generally only do this for user data. > > Yeah, I saw that comment. I'd been thinking of it as user data, but I suppose > it's not in the same ballpark as preferences or history. Do you think > SKIP_ON_SHUTDOWN is better for this? I think so, yes. My thinking is that this data won't be used by all users but stalling shutdown on it may affect all users. I'm trying to think of a good counter-argument that would motivate blocking shutdown, but haven't convinced myself there is one yet.
https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service.h (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.h:66: ClientDownloadRequestCallbackList; On 2014/10/23 21:43:03, robertshield wrote: > On 2014/10/23 21:02:18, grt (expect delays oct 27-29) wrote: > > On 2014/10/23 20:30:27, robertshield wrote: > > > On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > > > > On 2014/10/21 13:15:38, robertshield wrote: > > > > > Does the list typedef need to be public? > > > > > > > > Yes, since the contained Subscription type is part of the "register a > > > callback" > > > > interface; see below. > > > > > > The Subscription is but not the SubscriptionList. > > > > The two ways I've found of not making the CallbackList public are to either > > remove the ClientDownloadRequestCallbackList typedef altogether or to mix a > > private: section in the public: section like this: > > > > // A type of callback run on the main thread when a ClientDownloadRequest > has > > // been formed for a download, or when one has not been formed for a > supported > > // download. > > typedef base::Callback<void(content::DownloadItem*, > > const ClientDownloadRequest*)> > > ClientDownloadRequestCallback; > > > > private: > > // A list of ClientDownloadRequest callbacks. > > typedef base::CallbackList<void(content::DownloadItem*, > > const ClientDownloadRequest*)> > > ClientDownloadRequestCallbackList; > > > > public: > > // A subscription to a registered ClientDownloadRequest callback. > > typedef scoped_ptr<ClientDownloadRequestCallbackList::Subscription> > > ClientDownloadRequestSubscription; > > > > Although ClientDownloadRequestCallbackList doesn't need to be public, I think > > it's the more readable of the options that I can think of. Please give a > > concrete suggestion if you know of a better way. > > Can't you move the list typedef down into the private section? Didn't work when I tried it: ...\download_protection_service.h(66) : error C2653: 'ClientDownloadRequestCallbackList' : is not a class or namespace name It did work when I defined it in a private: section that preceded its use for the ClientDownloadRequestSubscription typedef (as above). So whaddya think: leave it as I had it with it being overly public, introduce an ugly private section up here for it, or the third thing that I haven't thought of yet (have you thought of it?)? https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:200: // null if the download has been destroyed. If non-null, the item is being On 2014/10/23 21:43:03, robertshield wrote: > On 2014/10/23 21:02:18, grt (expect delays oct 27-29) wrote: > > On 2014/10/23 20:30:27, robertshield wrote: > > > On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > > > > On 2014/10/22 02:28:56, robertshield wrote: > > > > > item(NULL), removed(false)? (I find that clearer) > > > > > > > > get off my lawn. > > > > > > If I do will you make your code more readable? > > > > Value-initialization is a fundamental part of the language. It is a common way > > to initialize nearly anything (instance of a class, array, or primitive type). > > If you can show me that your aversion to it is more than just personal > > preference, I'm happy to change it. > > My readability comment was really based on the notion that if I see > removed(false) I am pretty sure the member is a bool without looking down, > similarly if I see item(NULL), it's probably a pointer. What does that buy you? As you point out, members generally aren't present in the initializer list at all if they don't need to be, so what makes bool and * types (but not scoped_ptr or scoped_refptr, etc) so special that seeing (false) or (NULL^H^H^H^Hnullptr) gives you the warm fuzzies? :-) > One could argue that > neither of those are necessarily true and it may be dangerous to assume such > things but they are true often enough in Chrome that in the general case it IMO > enhances readability. > > There's also an element of convention of explicit initialization in initializer > lists: > https://code.google.com/p/chromium/codesearch#search/&q=%5C:.*%5C(false%5C)&s... > > > Think of it this way: if there was a foozit_ > > member of some class type that had a default constructor and a constructor > that > > took a parameter, would you ask me to call the ctor that took a parameter and > > pass in the value that is used by the default ctor (e.g., foozit_(0) rather > than > > foozit_()) when the behavior of the default ctor is really what is desired? > > In that case calling the default constructor would be the most readable thing > which would generally in Chrome means leaving the member out of the initializer > list altogether. This is true. It occurred to me after running out the door that this type of value initialization also applies for POD structs, where there's an even stronger argument for it (otherwise, you're left to individually zeroing out elements in your ctor, which is ugly). > Again, this is just convention, but it's a convention I have > grown used to and that I feel speeds up reading. > > In any case, apologies for the somewhat punchy reply. I believe that the reasons > I list above (code base convention and one less visual jump required when > reading) are sufficient to motivate the change, but if you feel strongly you can > leave this as is. Harrumph. I think it's an elegant part of the language. It involves less typing and less reading. Your consistency argument is the only one that can possibly sway me. I'll think about it. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:281: // Block shutdown on writes in the hopes of persisting important data. On 2014/10/23 21:43:03, robertshield wrote: > On 2014/10/23 21:02:18, grt (expect delays oct 27-29) wrote: > > On 2014/10/23 20:30:27, robertshield wrote: > > > On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > > > > On 2014/10/21 13:15:38, robertshield wrote: > > > > > Which data being persisted is important enough to block shutdown? What's > > the > > > > > effect of not persisting some of this metadata? > > > > > > > > The effect is just that we don't have the data. Are you concerned about > > > blocking > > > > shutdown? > > > > > > A little. I am basing this off the comment here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq... > > > > > > Which says that we should generally only do this for user data. > > > > Yeah, I saw that comment. I'd been thinking of it as user data, but I suppose > > it's not in the same ballpark as preferences or history. Do you think > > SKIP_ON_SHUTDOWN is better for this? > > I think so, yes. My thinking is that this data won't be used by all users but > stalling shutdown on it may affect all users. > I'm trying to think of a good counter-argument that would motivate blocking > shutdown, but haven't convinced myself there is one yet. SGTM, done.
https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.cc:788: reason == REASON_TRUSTED_EXECUTABLE)) { This seems error prone, I'm not confident this condition is correct, though I'm not entirely sure what it's trying to do. https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.cc:790: // callbakcs were not run with one. Run them with null so they see that a callbacks https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.cc:791: // previously-seen ClientDownloadRequest is now stale. This comment doesn't make sense to me. How would anything have seen a ClientDownloadRequest if none was ever generated? https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service_unittest.cc:520: #if defined(OS_MACOSX) These nested/repetitious ifdefs are a little ick. Maybe a separate condition like if WIN || MAC EXPECT_TRUE(Hasblah()) clearblah() else EXPECT_FALSE(hasblah()) endif https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:32: // ClientIncidentReport_DownloadDetails instance. what function? https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:34: // history::DownloadRow. what? https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:54: // ClientIncidentReport_DownloadDetails. what?.. oh. These comments aren't really clear.. https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:207: profiles_[profile] = WAITING_FOR_METADATA; this is confusing now.. maybe the variable should be named profile_states_ ? https://codereview.chromium.org/663023007/diff/60001/chrome/browser/download/... File chrome/browser/download/download_service.cc (right): https://codereview.chromium.org/663023007/diff/60001/chrome/browser/download/... chrome/browser/download/download_service.cc:70: g_browser_process->safe_browsing_service()->AddDownloadManager(manager); does this need an ifdef? And could this go in ChromeDownloadManagerDelegate::SetDownloadManager instead?
LGTM, I defer the remaining safebrowsing review bits to Matt https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service.h (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.h:66: ClientDownloadRequestCallbackList; On 2014/10/24 01:51:30, grt (expect delays oct 27-29) wrote: > On 2014/10/23 21:43:03, robertshield wrote: > > On 2014/10/23 21:02:18, grt (expect delays oct 27-29) wrote: > > > On 2014/10/23 20:30:27, robertshield wrote: > > > > On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > > > > > On 2014/10/21 13:15:38, robertshield wrote: > > > > > > Does the list typedef need to be public? > > > > > > > > > > Yes, since the contained Subscription type is part of the "register a > > > > callback" > > > > > interface; see below. > > > > > > > > The Subscription is but not the SubscriptionList. > > > > > > The two ways I've found of not making the CallbackList public are to either > > > remove the ClientDownloadRequestCallbackList typedef altogether or to mix a > > > private: section in the public: section like this: > > > > > > // A type of callback run on the main thread when a ClientDownloadRequest > > has > > > // been formed for a download, or when one has not been formed for a > > supported > > > // download. > > > typedef base::Callback<void(content::DownloadItem*, > > > const ClientDownloadRequest*)> > > > ClientDownloadRequestCallback; > > > > > > private: > > > // A list of ClientDownloadRequest callbacks. > > > typedef base::CallbackList<void(content::DownloadItem*, > > > const ClientDownloadRequest*)> > > > ClientDownloadRequestCallbackList; > > > > > > public: > > > // A subscription to a registered ClientDownloadRequest callback. > > > typedef scoped_ptr<ClientDownloadRequestCallbackList::Subscription> > > > ClientDownloadRequestSubscription; > > > > > > Although ClientDownloadRequestCallbackList doesn't need to be public, I > think > > > it's the more readable of the options that I can think of. Please give a > > > concrete suggestion if you know of a better way. > > > > Can't you move the list typedef down into the private section? > > Didn't work when I tried it: > > ...\download_protection_service.h(66) : error C2653: > 'ClientDownloadRequestCallbackList' : is not a class or namespace name > > It did work when I defined it in a private: section that preceded its use for > the ClientDownloadRequestSubscription typedef (as above). > > So whaddya think: leave it as I had it with it being overly public, introduce an > ugly private section up here for it, or the third thing that I haven't thought > of yet (have you thought of it?)? Huh, nm then, as is works for me. https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc (right): https://codereview.chromium.org/663023007/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:200: // null if the download has been destroyed. If non-null, the item is being On 2014/10/24 01:51:30, grt (expect delays oct 27-29) wrote: > On 2014/10/23 21:43:03, robertshield wrote: > > On 2014/10/23 21:02:18, grt (expect delays oct 27-29) wrote: > > > On 2014/10/23 20:30:27, robertshield wrote: > > > > On 2014/10/23 19:53:13, grt (expect delays oct 27-29) wrote: > > > > > On 2014/10/22 02:28:56, robertshield wrote: > > > > > > item(NULL), removed(false)? (I find that clearer) > > > > > > > > > > get off my lawn. > > > > > > > > If I do will you make your code more readable? > > > > > > Value-initialization is a fundamental part of the language. It is a common > way > > > to initialize nearly anything (instance of a class, array, or primitive > type). > > > If you can show me that your aversion to it is more than just personal > > > preference, I'm happy to change it. > > > > My readability comment was really based on the notion that if I see > > removed(false) I am pretty sure the member is a bool without looking down, > > similarly if I see item(NULL), it's probably a pointer. > > What does that buy you? As you point out, members generally aren't present in > the initializer list at all if they don't need to be, so what makes bool and * > types (but not scoped_ptr or scoped_refptr, etc) so special that seeing (false) > or (NULL^H^H^H^Hnullptr) gives you the warm fuzzies? :-) > > > One could argue that > > neither of those are necessarily true and it may be dangerous to assume such > > things but they are true often enough in Chrome that in the general case it > IMO > > enhances readability. > > > > There's also an element of convention of explicit initialization in > initializer > > lists: > > > https://code.google.com/p/chromium/codesearch#search/&q=%5C:.*%5C(false%5C)&s... > > > > > Think of it this way: if there was a foozit_ > > > member of some class type that had a default constructor and a constructor > > that > > > took a parameter, would you ask me to call the ctor that took a parameter > and > > > pass in the value that is used by the default ctor (e.g., foozit_(0) rather > > than > > > foozit_()) when the behavior of the default ctor is really what is desired? > > > > In that case calling the default constructor would be the most readable thing > > which would generally in Chrome means leaving the member out of the > initializer > > list altogether. > > This is true. It occurred to me after running out the door that this type of > value initialization also applies for POD structs, where there's an even > stronger argument for it (otherwise, you're left to individually zeroing out > elements in your ctor, which is ugly). > > > Again, this is just convention, but it's a convention I have > > grown used to and that I feel speeds up reading. > > > > In any case, apologies for the somewhat punchy reply. I believe that the > reasons > > I list above (code base convention and one less visual jump required when > > reading) are sufficient to motivate the change, but if you feel strongly you > can > > leave this as is. > > Harrumph. I think it's an elegant part of the language. It involves less typing > and less reading. Your consistency argument is the only one that can possibly > sway me. I'll think about it. Thank you for considering it. https://codereview.chromium.org/663023007/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/663023007/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.cc:790: // callbakcs were not run with one. Run them with null so they see that a callbacks https://codereview.chromium.org/663023007/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.h (right): https://codereview.chromium.org/663023007/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.h:79: // search for metadata is initated immediately. initiated
grt@chromium.org changed reviewers: + asvitkine@chromium.org
grt@chromium.org changed reviewers: + rdsmith@chromium.org
Thanks. ping mattm for safe_browsing OWNERS review +asvitkine for histograms.xml OWNERS review +rdsmith for chrome/browser/download OWNERS review https://codereview.chromium.org/663023007/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/663023007/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.cc:790: // callbakcs were not run with one. Run them with null so they see that a On 2014/10/24 03:05:46, robertshield wrote: > callbacks Done. https://codereview.chromium.org/663023007/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.h (right): https://codereview.chromium.org/663023007/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.h:79: // search for metadata is initated immediately. On 2014/10/24 03:05:46, robertshield wrote: > initiated Done.
Patchset #6 (id:100001) has been deleted
LGTM % a couple of comments https://codereview.chromium.org/663023007/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc (right): https://codereview.chromium.org/663023007/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc:156: safe_browsing::LastDownloadFinder::DownloadDetailsGetter Make the code in this file be in safe_browsing namespace, that way you won't need to prefix references to safe_browsing:: code and possibly make wrapping better throughout the file. https://codereview.chromium.org/663023007/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/663023007/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29412: +<histogram name="SBIRS.DownloadMetadataDeleteSuccess" enum="BooleanSuccess"> Nit: I suggest naming these histograms with an intermediate "." in the name, so there's better grouping on the dashboard. My suggestion for the names: SBIRS.DownloadMetadata.DeleteSuccess SBIRS.DownloadMetadata.ReadResult SBIRS.DownloadMetadata.WriteResult Change here and in the code.
asanka@chromium.org changed reviewers: + asanka@chromium.org
/download/ lgtm https://codereview.chromium.org/663023007/diff/120001/chrome/browser/download... File chrome/browser/download/download_service.cc (right): https://codereview.chromium.org/663023007/diff/120001/chrome/browser/download... chrome/browser/download/download_service.cc:70: g_browser_process->safe_browsing_service()->AddDownloadManager(manager); #if defined(FULL_SAFE_BROWSING) || defined(MOBILE_SAFE_BROWSING) https://codereview.chromium.org/663023007/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc (right): https://codereview.chromium.org/663023007/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:150: // provide cosnumers with persisted details of a download. nit: consumers https://codereview.chromium.org/663023007/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:245: // When true, the context is waiting for a pending read operation to complete. Nit: this appears to document a boolean. https://codereview.chromium.org/663023007/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:383: (get_details_callbacks_.empty() && pending_items_.empty())) { nit: state == LOAD_COMPLETE => (no callbacks && no pending items) The right hand check should be sufficient.
Thanks for the suggestions. All are done. mattm: ping https://codereview.chromium.org/663023007/diff/120001/chrome/browser/download... File chrome/browser/download/download_service.cc (right): https://codereview.chromium.org/663023007/diff/120001/chrome/browser/download... chrome/browser/download/download_service.cc:70: g_browser_process->safe_browsing_service()->AddDownloadManager(manager); On 2014/10/28 20:55:20, asanka wrote: > #if defined(FULL_SAFE_BROWSING) || defined(MOBILE_SAFE_BROWSING) Done. https://codereview.chromium.org/663023007/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc (right): https://codereview.chromium.org/663023007/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:150: // provide cosnumers with persisted details of a download. On 2014/10/28 20:55:20, asanka wrote: > nit: consumers Done. https://codereview.chromium.org/663023007/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:245: // When true, the context is waiting for a pending read operation to complete. On 2014/10/28 20:55:20, asanka wrote: > Nit: this appears to document a boolean. Done. https://codereview.chromium.org/663023007/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc:383: (get_details_callbacks_.empty() && pending_items_.empty())) { On 2014/10/28 20:55:20, asanka wrote: > nit: state == LOAD_COMPLETE => (no callbacks && no pending items) > > The right hand check should be sufficient. Done. https://codereview.chromium.org/663023007/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc (right): https://codereview.chromium.org/663023007/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc:156: safe_browsing::LastDownloadFinder::DownloadDetailsGetter On 2014/10/27 17:09:07, Alexei Svitkine wrote: > Make the code in this file be in safe_browsing namespace, that way you won't > need to prefix references to safe_browsing:: code and possibly make wrapping > better throughout the file. Done. https://codereview.chromium.org/663023007/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/663023007/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29412: +<histogram name="SBIRS.DownloadMetadataDeleteSuccess" enum="BooleanSuccess"> On 2014/10/27 17:09:07, Alexei Svitkine wrote: > Nit: I suggest naming these histograms with an intermediate "." in the name, so > there's better grouping on the dashboard. > > My suggestion for the names: > > SBIRS.DownloadMetadata.DeleteSuccess > SBIRS.DownloadMetadata.ReadResult > SBIRS.DownloadMetadata.WriteResult > > Change here and in the code. Done.
mattm comments addressed; PTAL. https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.cc:788: reason == REASON_TRUSTED_EXECUTABLE)) { On 2014/10/24 02:11:10, mattm wrote: > This seems error prone, I'm not confident this condition is correct, though I'm > not entirely sure what it's trying to do. I've clarified the comment and added TODOs to the related code in CheckWhitelists. I hope to address these TODOs in a separate change since they'll involve changing the CheckClientDownloadRequest state machine. https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.cc:790: // callbakcs were not run with one. Run them with null so they see that a On 2014/10/24 02:11:09, mattm wrote: > callbacks Done. https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service.cc:791: // previously-seen ClientDownloadRequest is now stale. On 2014/10/24 02:11:10, mattm wrote: > This comment doesn't make sense to me. How would anything have seen a > ClientDownloadRequest if none was ever generated? From an unrelated previous download. I'll clarify in the comment. https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/download_protection_service_unittest.cc:520: #if defined(OS_MACOSX) On 2014/10/24 02:11:10, mattm wrote: > These nested/repetitious ifdefs are a little ick. Maybe a separate condition > like > if WIN || MAC > EXPECT_TRUE(Hasblah()) > clearblah() > else > EXPECT_FALSE(hasblah()) > endif Looks better. Done. https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:54: // ClientIncidentReport_DownloadDetails. On 2014/10/24 02:11:10, mattm wrote: > what?.. oh. These comments aren't really clear.. Modified comments and re-ordered the functions for clarity. Let me know what you think. https://codereview.chromium.org/663023007/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:207: profiles_[profile] = WAITING_FOR_METADATA; On 2014/10/24 02:11:10, mattm wrote: > this is confusing now.. maybe the variable should be named profile_states_ ? Done. https://codereview.chromium.org/663023007/diff/60001/chrome/browser/download/... File chrome/browser/download/download_service.cc (right): https://codereview.chromium.org/663023007/diff/60001/chrome/browser/download/... chrome/browser/download/download_service.cc:70: g_browser_process->safe_browsing_service()->AddDownloadManager(manager); On 2014/10/24 02:11:10, mattm wrote: > does this need an ifdef? ifdef added. > And could this go in ChromeDownloadManagerDelegate::SetDownloadManager instead? Done. Is there an advantage other than keeping all safe_browsing integration points in one file?
So I was going through this CL again and feeling sad about how complicated it all is. I was thinking of some alternate approaches: 1) Update the history::DownloadRow to store the file hash from the DownloadItem, send that along with the incident report, and then match it up server side with what safebrowsing knows about that file to get the certs and any other information you need. This would be a pretty small change to chrome. 2) Serialize the whole safebrowsing metadata in the download history row. Would be equivalent to the current impl, though it would use more on disk space since you'd be keeping all the extra stuff around for the whole download history, but the code would still be a lot simpler than the CL now (not as small as #1 though). I really like the sound of #1, wdyt?
On 2014/10/31 23:26:43, mattm wrote: > So I was going through this CL again and feeling sad about how complicated it > all is. > I was thinking of some alternate approaches: > > 1) Update the history::DownloadRow to store the file hash from the DownloadItem, > send that along with the incident report, and then match it up server side with > what safebrowsing knows about that file to get the certs and any other > information you need. This would be a pretty small change to chrome. > > 2) Serialize the whole safebrowsing metadata in the download history row. Would > be equivalent to the current impl, though it would use more on disk space since > you'd be keeping all the extra stuff around for the whole download history, but > the code would still be a lot simpler than the CL now (not as small as #1 > though). > > I really like the sound of #1, wdyt? I had proposed #2 to download history owners, but they were not so excited about it. I'll forward the thread to you so you can see the discussion. I had also looked into a variation on #1 (having the csd server unconditionally return a token in all ClientDownloadResponse messages and storing that in history). This would also work for many cases, but it misses downloads for which pings aren't sent due to whitelisting. The approach I've taken here does have more complexity due to the async load of the metadata (which I agree isn't great in and of itself), but it doesn't depend on back-end behavior, which was also a concern of the safe browsing back-end team.
ok, SBS and DPS lgtm (with nit) https://codereview.chromium.org/663023007/diff/200001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/663023007/diff/200001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:983: const ClientDownloadRequestCallback& callback) { Add DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
The CQ bit was checked by grt@chromium.org
The CQ bit was unchecked by grt@chromium.org
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/663023007/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/663023007/240001
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/1f31ae2baee245dd2cdb456b685ce94a94bdc10e Cr-Commit-Position: refs/heads/master@{#302624} |
