Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1436273002:
Send safe browsing ThreatDetails to track download CTR when user tries to recover blocked downloads (Closed)
|
Created:
5 years, 1 month ago by Jialiu Lin Modified:
5 years ago CC:
asanka, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, grt+watch_chromium.org, tfarina, zysxqn Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSend safe browsing ThreatDetails to track download CTR when user tries to recover blocked downloads
BUG=515313
Committed: https://crrev.com/7f11b742c1a7634dbd1467fa6cd8ca85bb349f57
Cr-Commit-Position: refs/heads/master@{#361816}
Patch Set 1 #Patch Set 2 : test browser test #Patch Set 3 : fix mac_chromium_rel_gn test #Patch Set 4 : nit #Patch Set 5 : nit #
Total comments: 17
Patch Set 6 : address mws's comments #Patch Set 7 : nit #
Total comments: 6
Patch Set 8 : fix comment #Patch Set 9 : remove extra files #Patch Set 10 : remove extra file #
Total comments: 8
Patch Set 11 : address msw's comments #Patch Set 12 : tweak on comment #
Total comments: 2
Patch Set 13 : nit #
Total comments: 14
Patch Set 14 : address asanka's comments #
Total comments: 2
Patch Set 15 : scoped_refptr #
Total comments: 24
Patch Set 16 : addressing asanka and Lei's comments #
Total comments: 4
Patch Set 17 : nit #
Messages
Total messages: 42 (14 generated)
Description was changed from ========== Send safe browsing ThreatDetails to track download CTR when user tries to recover blocked downloads BUG=515313 ========== to ========== Send safe browsing ThreatDetails to track download CTR when user tries to recover blocked downloads BUG=515313 ==========
jialiul@chromium.org changed reviewers: + nparker@chromium.org
jialiul@chromium.org changed reviewers: + asanka@chromium.org, msw@chromium.org, thestig@chromium.org
+ nparker@chromium.org for bug review and owner review under safe_browsing/* + msw@chromium.org: for OWNER review on chrome/browser/ui/views/download/download_danger_prompt_views.cc + asanka@chromium.org: for OWNER review on chrome/browser/download/download_danger_prompt* +thestig@chromium.org: for OWNER review on chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc
Some things strike me as odd here, from a quick look. https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/download... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/download... chrome/browser/download/download_danger_prompt.h:63: void SendSerializedReport(const std::string& report); Why require impls to call CreateSafeBrowsingDownloadRecoveryReport and then pass the result to this function, which has a stern warning over the contents of the string? Instead, just pass |did_proceed| (maybe named |did_accept|?) and |url| to this function, which can create and send the report in one shot. https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right): https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_danger_prompt_views.cc:6: Remove this blank line or just put download_danger_prompt.h up top. https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_danger_prompt_views.cc:46: const GURL& testing_url) override; nit: use |url| here and below to match the base class' identifier. https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_danger_prompt_views.cc:137: std::string DownloadDangerPromptViews::InvokeActionForTesting( This looks like a near-copy of DownloadDangerPromptImpl::InvokeActionForTesting, with different handling of dismiss... I think this code needs a cleanup pass. https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_danger_prompt_views.cc:337: void DownloadDangerPromptViews::RunDone(Action action) { Why does this have the same exact impl as DownloadDangerPromptImpl::RunDone? That seems very strange to me... https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_danger_prompt_views.cc:343: if (download_->GetURL() != GURL::EmptyGURL()) { nit: use GURL::is_empty()
Patchset #6 (id:90001) has been deleted
Thanks for your review, msw. Please see my reply inline. https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/download... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/download... chrome/browser/download/download_danger_prompt.h:63: void SendSerializedReport(const std::string& report); On 2015/11/13 19:01:28, msw wrote: > Why require impls to call CreateSafeBrowsingDownloadRecoveryReport and then pass > the result to this function, which has a stern warning over the contents of the > string? Instead, just pass |did_proceed| (maybe named |did_accept|?) and |url| > to this function, which can create and send the report in one shot. It is for testing purpose. Such that CreateSafeBrowsingDownloadRecoveryReport can be separately called in InvokeActionForTesting(action) without actually sending report to server. https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right): https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_danger_prompt_views.cc:6: On 2015/11/13 19:01:28, msw wrote: > Remove this blank line or just put download_danger_prompt.h up top. oops, sorry https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_danger_prompt_views.cc:46: const GURL& testing_url) override; On 2015/11/13 19:01:28, msw wrote: > nit: use |url| here and below to match the base class' identifier. Done. https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_danger_prompt_views.cc:137: std::string DownloadDangerPromptViews::InvokeActionForTesting( On 2015/11/13 19:01:28, msw wrote: > This looks like a near-copy of DownloadDangerPromptImpl::InvokeActionForTesting, > with different handling of dismiss... I think this code needs a cleanup pass. They looks like the same, however, the Accept() in DownloadDangerPromptViews is actually calling views::DialogDelegate::Accept() And the Accept() in DownloadDangerPromptImpt is calling TabModalConfirmDialogDelegate::Accept() Similarly, OnDownloadUpdated() though look exactly the same, they are actually calling different things. https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_danger_prompt_views.cc:337: void DownloadDangerPromptViews::RunDone(Action action) { On 2015/11/13 19:01:28, msw wrote: > Why does this have the same exact impl as DownloadDangerPromptImpl::RunDone? > That seems very strange to me... Very hard to move this function to parent class, unfortunately. Similar to the above reason, the Callback typed defined as OnDone in parent class, but are actually different things in these two subclasses. Also parent class does not implement content::DownloadItem::Observer, so cannot call download_->RemoveObserver(this). https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_danger_prompt_views.cc:343: if (download_->GetURL() != GURL::EmptyGURL()) { On 2015/11/13 19:01:28, msw wrote: > nit: use GURL::is_empty() Done.
https://codereview.chromium.org/1436273002/diff/130001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/1436273002/diff/130001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:60: // Since this reporting is not gated by extended_reporting, it only records I'd swap the two phrases: Since it only records url and action, it isn't gated by e_r. https://codereview.chromium.org/1436273002/diff/130001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/130001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:81: if (!download().IsDangerous()) { If it changes to safe, doesn't that mean did_proceed = true? It'll only change if the user proceeds. https://codereview.chromium.org/1436273002/diff/130001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/1436273002/diff/130001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:649: void SafeBrowsingService::SendDownloadRecoveryReport( How about moving the thread-hopping to the caller? That would mean one less function, and simpler function naming.
Patchset #9 (id:170001) has been deleted
Patchset #8 (id:150001) has been deleted
Thanks, nparker. Please see my reply inline. And somehow after I did a rebase, there are 2 more files have delta in them: tools/grit/LICENSE tools/grit/codereview.settings Reviewers can ignore them. https://codereview.chromium.org/1436273002/diff/130001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/1436273002/diff/130001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:60: // Since this reporting is not gated by extended_reporting, it only records On 2015/11/14 01:04:27, nparker wrote: > I'd swap the two phrases: Since it only records url and action, it isn't gated > by e_r. Done. https://codereview.chromium.org/1436273002/diff/130001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/130001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:81: if (!download().IsDangerous()) { On 2015/11/14 01:04:27, nparker wrote: > If it changes to safe, doesn't that mean did_proceed = true? It'll only change > if the user proceeds. Resolved in person. This is a corner case, where a previous blocked dangerous download is no longer dangerous when user tries to recover it. A DISMISS action will be triggered instead of an ACCEPT. https://codereview.chromium.org/1436273002/diff/130001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/1436273002/diff/130001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_service.cc:649: void SafeBrowsingService::SendDownloadRecoveryReport( On 2015/11/14 01:04:27, nparker wrote: > How about moving the thread-hopping to the caller? That would mean one less > function, and simpler function naming. Actually, it is more convenient to do thread hopping on SafeBrowsingService, otherwise, DownloadDangerousPromp might need to implement base::RefCountedThreadSafe<> class to be thread-safe.
https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/download... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/download... chrome/browser/download/download_danger_prompt.h:63: void SendSerializedReport(const std::string& report); On 2015/11/13 23:47:00, JialiuLin wrote: > On 2015/11/13 19:01:28, msw wrote: > > Why require impls to call CreateSafeBrowsingDownloadRecoveryReport and then > pass > > the result to this function, which has a stern warning over the contents of > the > > string? Instead, just pass |did_proceed| (maybe named |did_accept|?) and |url| > > to this function, which can create and send the report in one shot. > > It is for testing purpose. Such that CreateSafeBrowsingDownloadRecoveryReport > can be separately called in InvokeActionForTesting(action) without actually > sending report to server. Change SendSerializedReport to call CreateSafeBrowsingDownloadRecoveryReport internally, and you'll probably want to rename the former to "SendSafeBrowsingDownloadRecoveryReport" or similar. https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right): https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_danger_prompt_views.cc:137: std::string DownloadDangerPromptViews::InvokeActionForTesting( On 2015/11/13 23:47:00, JialiuLin wrote: > On 2015/11/13 19:01:28, msw wrote: > > This looks like a near-copy of > DownloadDangerPromptImpl::InvokeActionForTesting, > > with different handling of dismiss... I think this code needs a cleanup pass. > > They looks like the same, however, the Accept() in DownloadDangerPromptViews is > actually calling views::DialogDelegate::Accept() > And the Accept() in DownloadDangerPromptImpt is calling > TabModalConfirmDialogDelegate::Accept() > > Similarly, OnDownloadUpdated() though look exactly the same, they are actually > calling different things. This is troubling... someone more familiar here should review this CL and audit this area; there ought to be a better pattern that doesn't 'duplicate' so much code. That said, this CL doesn't cause that mess; deferring cleanup may be okay. https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.cc (right): https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:292: return ""; nit: return std::string() https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:302: if (sb_service) { nit: curlies not needed https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/extensi... File chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/extensi... chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:4043: GURL unused_url = GURL::EmptyGURL(); nit: can you just inline the default ctor below? ie. GURL() https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right): https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_danger_prompt_views.cc:4: #include "chrome/browser/download/download_danger_prompt.h" nit: add a blank line above this.
Patchset #11 (id:250001) has been deleted
Thanks,msw! https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/download... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/download... chrome/browser/download/download_danger_prompt.h:63: void SendSerializedReport(const std::string& report); On 2015/11/17 01:17:37, msw wrote: > On 2015/11/13 23:47:00, JialiuLin wrote: > > On 2015/11/13 19:01:28, msw wrote: > > > Why require impls to call CreateSafeBrowsingDownloadRecoveryReport and then > > pass > > > the result to this function, which has a stern warning over the contents of > > the > > > string? Instead, just pass |did_proceed| (maybe named |did_accept|?) and > |url| > > > to this function, which can create and send the report in one shot. > > > > It is for testing purpose. Such that CreateSafeBrowsingDownloadRecoveryReport > > can be separately called in InvokeActionForTesting(action) without actually > > sending report to server. > > Change SendSerializedReport to call CreateSafeBrowsingDownloadRecoveryReport > internally, and you'll probably want to rename the former to > "SendSafeBrowsingDownloadRecoveryReport" or similar. Done. https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.cc (right): https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:292: return ""; On 2015/11/17 01:17:37, msw wrote: > nit: return std::string() Done. https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:302: if (sb_service) { On 2015/11/17 01:17:37, msw wrote: > nit: curlies not needed Done. https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/extensi... File chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/extensi... chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:4043: GURL unused_url = GURL::EmptyGURL(); On 2015/11/17 01:17:38, msw wrote: > nit: can you just inline the default ctor below? ie. GURL() Done. https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right): https://codereview.chromium.org/1436273002/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_danger_prompt_views.cc:4: #include "chrome/browser/download/download_danger_prompt.h" On 2015/11/17 01:17:38, msw wrote: > nit: add a blank line above this. Done.
Just a nit, but I'd really like someone more familiar here to review this CL. https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right): https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_danger_prompt_views.cc:137: std::string DownloadDangerPromptViews::InvokeActionForTesting( On 2015/11/17 01:17:37, msw wrote: > On 2015/11/13 23:47:00, JialiuLin wrote: > > On 2015/11/13 19:01:28, msw wrote: > > > This looks like a near-copy of > > DownloadDangerPromptImpl::InvokeActionForTesting, > > > with different handling of dismiss... I think this code needs a cleanup > pass. > > > > They looks like the same, however, the Accept() in DownloadDangerPromptViews > is > > actually calling views::DialogDelegate::Accept() > > And the Accept() in DownloadDangerPromptImpt is calling > > TabModalConfirmDialogDelegate::Accept() > > > > Similarly, OnDownloadUpdated() though look exactly the same, they are actually > > calling different things. > > This is troubling... someone more familiar here should review this CL and audit > this area; there ought to be a better pattern that doesn't 'duplicate' so much > code. That said, this CL doesn't cause that mess; deferring cleanup may be okay. My approval is gated on someone familiar with this area reviewing this CL. https://codereview.chromium.org/1436273002/diff/290001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.cc (right): https://codereview.chromium.org/1436273002/diff/290001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:301: if (sb_service) nit: now that the body is two lines, this does need curlies, sorry.
Thanks, msw! asanka@, could you take a look at this CL, since you're more familiar with this part of code? Thank you! https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right): https://codereview.chromium.org/1436273002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_danger_prompt_views.cc:137: std::string DownloadDangerPromptViews::InvokeActionForTesting( On 2015/11/17 18:54:40, msw wrote: > On 2015/11/17 01:17:37, msw wrote: > > On 2015/11/13 23:47:00, JialiuLin wrote: > > > On 2015/11/13 19:01:28, msw wrote: > > > > This looks like a near-copy of > > > DownloadDangerPromptImpl::InvokeActionForTesting, > > > > with different handling of dismiss... I think this code needs a cleanup > > pass. > > > > > > They looks like the same, however, the Accept() in DownloadDangerPromptViews > > is > > > actually calling views::DialogDelegate::Accept() > > > And the Accept() in DownloadDangerPromptImpt is calling > > > TabModalConfirmDialogDelegate::Accept() > > > > > > Similarly, OnDownloadUpdated() though look exactly the same, they are > actually > > > calling different things. > > > > This is troubling... someone more familiar here should review this CL and > audit > > this area; there ought to be a better pattern that doesn't 'duplicate' so much > > code. That said, this CL doesn't cause that mess; deferring cleanup may be > okay. > > My approval is gated on someone familiar with this area reviewing this CL. asanka (already in reviewer list) would be the expert who is very familiar with this part of code. https://codereview.chromium.org/1436273002/diff/290001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.cc (right): https://codereview.chromium.org/1436273002/diff/290001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:301: if (sb_service) On 2015/11/17 18:54:40, msw wrote: > nit: now that the body is two lines, this does need curlies, sorry. curlies added back. (I should read the style guide more carefully. my bad.) Thanks for pointing this out.
lgtm for /safe_browsing/* code
On 2015/11/13 01:49:52, JialiuLin wrote: > + mailto:asanka@chromium.org: for OWNER review on > chrome/browser/download/download_danger_prompt* > > mailto:+thestig@chromium.org: for OWNER review on > chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc Please just ask the download code reviewer to review. Happy to stamp as needed.
Hi asanka, could you take a look at this CL? Thanks!
There doesn't seem to be any tests that verify that the two DownloadDangerPrompt implementations invoke SafeBrowsing to report the dangerous download recovery. It's a bit tricky to test it, but one option is to use a test SafeBrowsingService that'll verify that the SendDownloadRecoveryReport method is called with the expected parameter. https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.cc (right): https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:14: #include "chrome/browser/safe_browsing/ping_manager.h" Does this need to be conditional on FULL_SAFE_BROWSING? https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:21: #include "content/public/browser/browser_thread.h" Why? https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:251: if (!download_->GetURL().is_empty()) { Is this check for preventing SendSafeBrowsingDownloadRecoveryReport() from being invoked during tests? If so, doesn't the attempt fail silently during tests? https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:52: const GURL& url) = 0; Let's separate out the ClientSafeBrowsingReportRequest tests from InvokeActionForTesting(). The purpose of InvokeActionForTesting() is to be an interface into the respective implementation of DownloadDangerPrompt. https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:56: std::string CreateSafeBrowsingDownloadRecoveryReport(bool did_proceed, I was going to object to putting this here, but then realized to my dismay that DownloadDangerPromptImpl and DownloadDangerPromptViews are independent implementations of DownloadDanterPrompt and don't share code :-(. Anyhoo, since these two methods can't depend on any DownloadDangerPrompt fields and can't be overridden, let's make these static. Also test them independently of the individual DownloadDangerPrompt implementations since these methods are independent. https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:62: // information in this report. Can you add a note about the parameters? There are multiple URLs associated with a download. https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:81: if (!download().IsDangerous()) { Nit: no braces
Can you also make sure we don't send these reports for incognito downloads?
Sure, I will make sure of it. Thank you, Asanka! On Wed, Nov 18, 2015 at 1:25 PM, <asanka@chromium.org> wrote: > Can you also make sure we don't send these reports for incognito downloads? > > https://codereview.chromium.org/1436273002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Addressed asanka's comments in the following way: 1. now the download recovery report will not send in incognito mode 2. Remove Create ClientSafeBrowsingReportRequest from InvokeActionForTesting() function. Instead, use a fake SafeBrowsingService to test the report content in download_danger_prompt_browsertest.cc. Note, this browser test class triggers different implementations of DownloadDangerPrompt (i.e. DownloadDangerPromptView, and DownloadDangerPromptImpl) for different platforms respectively. 3. did some code cleaning, and addressed readability nits. https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.cc (right): https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:14: #include "chrome/browser/safe_browsing/ping_manager.h" On 2015/11/18 21:23:23, asanka wrote: > Does this need to be conditional on FULL_SAFE_BROWSING? Oops, my bad. this input should not be here. removed. https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:21: #include "content/public/browser/browser_thread.h" On 2015/11/18 21:23:23, asanka wrote: > Why? Sorry, removed. https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:251: if (!download_->GetURL().is_empty()) { On 2015/11/18 21:23:23, asanka wrote: > Is this check for preventing SendSafeBrowsingDownloadRecoveryReport() from being > invoked during tests? If so, doesn't the attempt fail silently during tests? After refactoring, this check makes sure we don't send empty report to server. Also added the condition to gate incognito mode. https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:52: const GURL& url) = 0; On 2015/11/18 21:23:23, asanka wrote: > Let's separate out the ClientSafeBrowsingReportRequest tests from > InvokeActionForTesting(). > > The purpose of InvokeActionForTesting() is to be an interface into the > respective implementation of DownloadDangerPrompt. Done. Reverted changes to this function. https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:56: std::string CreateSafeBrowsingDownloadRecoveryReport(bool did_proceed, On 2015/11/18 21:23:23, asanka wrote: > I was going to object to putting this here, but then realized to my dismay that > DownloadDangerPromptImpl and DownloadDangerPromptViews are independent > implementations of DownloadDanterPrompt and don't share code :-(. > > Anyhoo, since these two methods can't depend on any DownloadDangerPrompt fields > and can't be overridden, let's make these static. Also test them independently > of the individual DownloadDangerPrompt implementations since these methods are > independent. Merged CreateSafeBrowsingDownloadRecoveryReport logic into SendSafeBrowsingDownloadRecoveryReport function, since we don't call it separately in InvokeActionForTesting anymore. https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:62: // information in this report. On 2015/11/18 21:23:23, asanka wrote: > Can you add a note about the parameters? There are multiple URLs associated with > a download. Comments added and mark this function as a static one. https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/310001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:81: if (!download().IsDangerous()) { On 2015/11/18 21:23:23, asanka wrote: > Nit: no braces Done.
https://codereview.chromium.org/1436273002/diff/330001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/330001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:68: FakeSafeBrowsingService* fake_safe_browsing_service_; scoped_ptr<> ?
Thanks, Nathan. comment addressed. https://codereview.chromium.org/1436273002/diff/330001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/330001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:68: FakeSafeBrowsingService* fake_safe_browsing_service_; On 2015/11/23 21:08:57, nparker wrote: > scoped_ptr<> ? Good point. Changed to scoped_refptr<> instead since SafeBrowsingService implements base::RefCountedThreadSafe.
LGTM w/ comments addressed. https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:32: static const char* kTestDownloadUrl = "http://evildownload.com"; Nit: You want 'static const char* const kTestDownloadUrl'. The extra const means that you can't change what kTestDownloadUrl points to. E.g.: static const char* const kConstConst = "foo"; static const char* kConst = "bar"; kConst = "baz"; // valid. kConst is not const although *kConst is. kConstConst = "baz"; // fails to compile. Both kConstConst and *kConstConst are const. https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:55: TestSafeBrowsingServiceFactory() : fake_safe_browsing_service_(NULL) {} NULL -> nullptr for new code. https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:59: fake_safe_browsing_service_ = new FakeSafeBrowsingService(); Check that fake_safe_browsing_service_ isn't set already. Also you'll want to call Initialize() since it's a real SBService. https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:79: safe_browsing::SafeBrowsingService::RegisterFactory( Do this in SetUp().
https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.cc (right): https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:282: if (sb_service) { This should be always true, right? https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:9: #include "url/gurl.h" forward decl GURL instead? https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:53: // action (click through or not), it isn't gated by extended_reporting. We What does "extended_reporting" refer to here? https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:7: #include "base/memory/weak_ptr.h" Not used? https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:32: static const char* kTestDownloadUrl = "http://evildownload.com"; Or just const char kFoo[] https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:42: std::string GetDownloadRecoveryReport() { return report_; } const method https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:74: : prompt_(NULL), Also nullptr while you are here?
Patchset #16 (id:370001) has been deleted
Thanks for all your comments. They are all addressed. Happy Thanksgiving! https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.cc (right): https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.cc:282: if (sb_service) { On 2015/11/24 18:55:27, Lei Zhang wrote: > This should be always true, right? Yes, you're right. This check is not necessary. Thanks for catching this. https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:9: #include "url/gurl.h" On 2015/11/24 18:55:27, Lei Zhang wrote: > forward decl GURL instead? Done. https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:53: // action (click through or not), it isn't gated by extended_reporting. We On 2015/11/24 18:55:27, Lei Zhang wrote: > What does "extended_reporting" refer to here? This refers to prefs::kSafeBrowsingExtendedReportingEnabled. Updated this comment to make it clearer. https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:7: #include "base/memory/weak_ptr.h" On 2015/11/24 18:55:27, Lei Zhang wrote: > Not used? Thanks for catching this. Done. https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:32: static const char* kTestDownloadUrl = "http://evildownload.com"; On 2015/11/24 18:48:14, asanka wrote: > Nit: You want 'static const char* const kTestDownloadUrl'. The extra const means > that you can't change what kTestDownloadUrl points to. E.g.: > > static const char* const kConstConst = "foo"; > static const char* kConst = "bar"; > > kConst = "baz"; // valid. kConst is not const although *kConst is. > kConstConst = "baz"; // fails to compile. Both kConstConst and *kConstConst > are const. Thanks, good to know. https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:32: static const char* kTestDownloadUrl = "http://evildownload.com"; On 2015/11/24 18:55:27, Lei Zhang wrote: > Or just const char kFoo[] Done. https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:42: std::string GetDownloadRecoveryReport() { return report_; } On 2015/11/24 18:55:27, Lei Zhang wrote: > const method Done. https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:55: TestSafeBrowsingServiceFactory() : fake_safe_browsing_service_(NULL) {} On 2015/11/24 18:48:14, asanka wrote: > NULL -> nullptr for new code. Done. https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:59: fake_safe_browsing_service_ = new FakeSafeBrowsingService(); On 2015/11/24 18:48:14, asanka wrote: > Check that fake_safe_browsing_service_ isn't set already. > > Also you'll want to call Initialize() since it's a real SBService. Added check to see if fake_safe_browsing_service_ is set already. Actually Initialize() function will be triggered inside InProcessBrowserTest::SetUp(), so no need to specifically call it here.(double verified by adding debug printout in SafeBrowsingService::Initialize() function) https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:74: : prompt_(NULL), On 2015/11/24 18:55:27, Lei Zhang wrote: > Also nullptr while you are here? Done. https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:79: safe_browsing::SafeBrowsingService::RegisterFactory( On 2015/11/24 18:48:14, asanka wrote: > Do this in SetUp(). Moved to SetUp()
lgtm https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:32: static const char* kTestDownloadUrl = "http://evildownload.com"; On 2015/11/25 23:43:21, JialiuLin wrote: > On 2015/11/24 18:48:14, asanka wrote: > > Nit: You want 'static const char* const kTestDownloadUrl'. The extra const > means > > that you can't change what kTestDownloadUrl points to. E.g.: > > > > static const char* const kConstConst = "foo"; > > static const char* kConst = "bar"; > > > > kConst = "baz"; // valid. kConst is not const although *kConst is. > > kConstConst = "baz"; // fails to compile. Both kConstConst and *kConstConst > > are const. > Thanks, good to know. In general, prefer const char kFoo[] unless you really need a pointer. https://codereview.chromium.org/1436273002/diff/390001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/390001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:59: if (fake_safe_browsing_service_ == nullptr) { Just: if (!fake_safe_browsing_service_) ? https://codereview.chromium.org/1436273002/diff/390001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:90: SafeBrowsingService::RegisterFactory(nullptr); Also call InProcessBrowserTest::TearDown() ?
Thank you so much for your thorough review, Lei. All the nits resolved. Cheers, https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/350001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:32: static const char* kTestDownloadUrl = "http://evildownload.com"; On 2015/11/25 23:48:15, Lei Zhang wrote: > On 2015/11/25 23:43:21, JialiuLin wrote: > > On 2015/11/24 18:48:14, asanka wrote: > > > Nit: You want 'static const char* const kTestDownloadUrl'. The extra const > > means > > > that you can't change what kTestDownloadUrl points to. E.g.: > > > > > > static const char* const kConstConst = "foo"; > > > static const char* kConst = "bar"; > > > > > > kConst = "baz"; // valid. kConst is not const although *kConst is. > > > kConstConst = "baz"; // fails to compile. Both kConstConst and > *kConstConst > > > are const. > > Thanks, good to know. > > In general, prefer const char kFoo[] unless you really need a pointer. Done. https://codereview.chromium.org/1436273002/diff/390001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/1436273002/diff/390001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:59: if (fake_safe_browsing_service_ == nullptr) { On 2015/11/25 23:48:15, Lei Zhang wrote: > Just: if (!fake_safe_browsing_service_) ? Done. https://codereview.chromium.org/1436273002/diff/390001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:90: SafeBrowsingService::RegisterFactory(nullptr); On 2015/11/25 23:48:15, Lei Zhang wrote: > Also call InProcessBrowserTest::TearDown() ? Done.
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, asanka@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1436273002/#ps410001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436273002/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436273002/410001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436273002/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436273002/410001
Message was sent while issue was closed.
Description was changed from ========== Send safe browsing ThreatDetails to track download CTR when user tries to recover blocked downloads BUG=515313 ========== to ========== Send safe browsing ThreatDetails to track download CTR when user tries to recover blocked downloads BUG=515313 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:410001)
Message was sent while issue was closed.
Description was changed from ========== Send safe browsing ThreatDetails to track download CTR when user tries to recover blocked downloads BUG=515313 ========== to ========== Send safe browsing ThreatDetails to track download CTR when user tries to recover blocked downloads BUG=515313 Committed: https://crrev.com/7f11b742c1a7634dbd1467fa6cd8ca85bb349f57 Cr-Commit-Position: refs/heads/master@{#361816} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/7f11b742c1a7634dbd1467fa6cd8ca85bb349f57 Cr-Commit-Position: refs/heads/master@{#361816} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
