Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
845663004:
Initial script request detector. (Closed)
|
Created:
5 years, 11 months ago by robertshield Modified:
5 years, 10 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial script request detector.
BUG=
TEST=
Committed: https://crrev.com/3426390224d3b6ba9c39dc827cf71a1e7c656108
Cr-Commit-Position: refs/heads/master@{#315970}
Patch Set 1 #Patch Set 2 : Wire up reporting service. #Patch Set 3 : Merge with latest IRS changes. #Patch Set 4 : pre-review cleanup #
Total comments: 46
Patch Set 5 : grt feedback #
Total comments: 16
Patch Set 6 : grt feedback, add host domain reporting #Patch Set 7 : Fix initializer order. #
Total comments: 4
Patch Set 8 : Rebase on top of latest incident reporting hotness. #
Total comments: 28
Patch Set 9 : grt feedback, switch to gmock #
Total comments: 10
Patch Set 10 : #
Total comments: 12
Patch Set 11 : grt feedback #Patch Set 12 : rename fields #
Total comments: 2
Patch Set 13 : #
Total comments: 2
Patch Set 14 : Add comment to ComputeDigest. #
Total comments: 2
Patch Set 15 : Use GetLastCommittedURL instead of going through the SiteInstance on the RFH #Messages
Total messages: 33 (7 generated)
robertshield@chromium.org changed reviewers: + grt@chromium.org
PTAL (I know there's a linux compile failure, but have to go to bed, will fix tomorrow). Thanks!
Looks good, although I'm concerned about the way the incident is added to the service. Initial comments below. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. here, too https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:19: const char* kBase64Hashes[] = { hex-encode these (and make it "const char* const kHexHashes[]") so that you don't need a wasteful base64 step. the loop in InitializeScriptSet would become: std::string raw_hash; for (const char* hash_literal : kHexHashes) { raw_hash.assign(hash_literal, crypto::kSHA256Length); script_set_.insert(raw_hash); } https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:101: const content::ResourceType resource_type = request_info->GetResourceType(); only used once, remove https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:103: if (resource_type == content::RESOURCE_TYPE_SCRIPT) { to reduce indentation below, how about: if (request_info->GetResourceType() != content::RESOURCE_TYPE_SCRIPT) return; https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:104: VLOG(1) << "Script request: " << request->url().spec(); DVLOG https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:107: std::string raw_hash; std::string raw_hash(crypto::kSHA256Length, '\0'); and remove the .resize https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:111: if (script_set_.find(raw_hash) != script_set_.end()) { if (script_set_.count(raw_hash)) { https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:112: VLOG(1) << "Script detector match found."; DVLOG https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:155: std::string raw_hash; move this out of the loop (as above) https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. So this is the new year? I don't feel any different. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:8: #include "base/callback.h" unused? https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:12: #include "chrome/common/safe_browsing/csd.pb.h" replace this with a forward decl of ClientIncidentReport_IncidentData_ScriptRequestIncident https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:13: #include "content/public/common/resource_type.h" unused? https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:26: IncidentReportingService* incident_reporting_service); Passing in a ptr to the service defeats the whole purpose of the AddIncidentCallback. I'll have to think about this a bit. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:34: void AddScriptHashForTesting(const std::string& raw_hash); unused https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:37: bool ContainsScriptHashForTesting(const std::string& raw_hash); unused https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:41: virtual bool AllowNullProfileForTesting(); interesting. gab@ chose to do GetProfileForRenderProcessId via virtual dispatch so that tests could make it return the proper testing profile. i wonder if one approach is better/worse than the other. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc (right): https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc:29: // Returns the sanitized path of the module. stale comment
Thanks, PTAL! https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/28 14:49:05, grt wrote: > here, too It's 2015 you say, that's just like your opinion man. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:19: const char* kBase64Hashes[] = { On 2015/01/28 14:49:04, grt wrote: > hex-encode these (and make it "const char* const kHexHashes[]") so that you > don't need a wasteful base64 step. the loop in InitializeScriptSet would become: > std::string raw_hash; > for (const char* hash_literal : kHexHashes) { > raw_hash.assign(hash_literal, crypto::kSHA256Length); > script_set_.insert(raw_hash); > } Done. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:101: const content::ResourceType resource_type = request_info->GetResourceType(); On 2015/01/28 14:49:05, grt wrote: > only used once, remove Done. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:103: if (resource_type == content::RESOURCE_TYPE_SCRIPT) { On 2015/01/28 14:49:05, grt wrote: > to reduce indentation below, how about: > if (request_info->GetResourceType() != content::RESOURCE_TYPE_SCRIPT) > return; Done. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:104: VLOG(1) << "Script request: " << request->url().spec(); On 2015/01/28 14:49:05, grt wrote: > DVLOG Done. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:107: std::string raw_hash; On 2015/01/28 14:49:05, grt wrote: > std::string raw_hash(crypto::kSHA256Length, '\0'); > and remove the .resize Done. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:111: if (script_set_.find(raw_hash) != script_set_.end()) { On 2015/01/28 14:49:05, grt wrote: > if (script_set_.count(raw_hash)) { Done. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:112: VLOG(1) << "Script detector match found."; On 2015/01/28 14:49:05, grt wrote: > DVLOG Done. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:155: std::string raw_hash; On 2015/01/28 14:49:05, grt wrote: > move this out of the loop (as above) Done. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/28 14:49:05, grt wrote: > So this is the new year? I don't feel any different. I find your assumption that I too am bound to the Gregorian calendar inconsiderate. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:8: #include "base/callback.h" On 2015/01/28 14:49:05, grt wrote: > unused? Done. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:12: #include "chrome/common/safe_browsing/csd.pb.h" On 2015/01/28 14:49:05, grt wrote: > replace this with a forward decl of > ClientIncidentReport_IncidentData_ScriptRequestIncident Done. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:13: #include "content/public/common/resource_type.h" On 2015/01/28 14:49:05, grt wrote: > unused? Done. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:26: IncidentReportingService* incident_reporting_service); On 2015/01/28 14:49:05, grt wrote: > Passing in a ptr to the service defeats the whole purpose of the > AddIncidentCallback. I'll have to think about this a bit. Agreed, at the same time with the current API this is the only way that comes to mind to do it. For musing: AddIncidentCallbacks currently require a Profile to conjure up and I don't have one until the request comes in. Perhaps a callback that also took a profile parameter could be handed out? https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:34: void AddScriptHashForTesting(const std::string& raw_hash); On 2015/01/28 14:49:05, grt wrote: > unused Done. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:37: bool ContainsScriptHashForTesting(const std::string& raw_hash); On 2015/01/28 14:49:05, grt wrote: > unused Done. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:41: virtual bool AllowNullProfileForTesting(); On 2015/01/28 14:49:05, grt wrote: > interesting. gab@ chose to do GetProfileForRenderProcessId via virtual dispatch > so that tests could make it return the proper testing profile. i wonder if one > approach is better/worse than the other. Interesting, I don't believe one is better than the other. I did the other thing first, but then didn't like the amount of code that was overridden for testing. This is an explicit testing hook, which I felt was cleaner. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc (right): https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc:29: // Returns the sanitized path of the module. On 2015/01/28 14:49:05, grt wrote: > stale comment Done.
https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:26: IncidentReportingService* incident_reporting_service); On 2015/01/28 22:20:33, robertshield wrote: > On 2015/01/28 14:49:05, grt wrote: > > Passing in a ptr to the service defeats the whole purpose of the > > AddIncidentCallback. I'll have to think about this a bit. > > Agreed, at the same time with the current API this is the only way that comes to > mind to do it. > > For musing: > AddIncidentCallbacks currently require a Profile to conjure up and I don't have > one until the request comes in. Perhaps a callback that also took a profile > parameter could be handed out? The more I think about it, the less comfortable I am with this. I'll get started right away on allowing the caller to pass the Profile in at the time of running the callback. It looks like it won't be at all intrusive. Please address other comments, and maybe I'll finish before you. I'll reconsider my position if it looks hairy. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:34: void AddScriptHashForTesting(const std::string& raw_hash); On 2015/01/28 22:20:32, robertshield wrote: > On 2015/01/28 14:49:05, grt wrote: > > unused > > Done. Hard to kill, it seems. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:37: bool ContainsScriptHashForTesting(const std::string& raw_hash); On 2015/01/28 22:20:33, robertshield wrote: > On 2015/01/28 14:49:05, grt wrote: > > unused > > Done. I don't think this word means what you think it means. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:41: virtual bool AllowNullProfileForTesting(); On 2015/01/28 22:20:32, robertshield wrote: > On 2015/01/28 14:49:05, grt wrote: > > interesting. gab@ chose to do GetProfileForRenderProcessId via virtual > dispatch > > so that tests could make it return the proper testing profile. i wonder if one > > approach is better/worse than the other. > > Interesting, I don't believe one is better than the other. I did the other thing > first, but then didn't like the amount of code that was overridden for testing. > This is an explicit testing hook, which I felt was cleaner. How do you feel about getting rid of virtual dispatch and just having a bool member that is modified by a protected setter (which can be made public in a subclass-for-test)? https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:19: const char* const kHashes[] = { blank line before this https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:111: if (browser_context) { nit: no braces https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:157: // TODO(robertshield): Set the domain of the parent frame: when will this TODO be addressed? https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:175: void ScriptRequestDetector::AddScriptHashForTesting( remove https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:180: bool ScriptRequestDetector::ContainsScriptHashForTesting( remove https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:192: for (const char* encoded_hash : kHashes) { nit: remove braces https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:10: #include "chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h" can you forward decl IncidentReportingService instead of this? https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:53: base::WeakPtrFactory<ScriptRequestDetector> weak_ptr_factory_; #include "base/memory/weak_ptr.h"
PTAL https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:26: IncidentReportingService* incident_reporting_service); On 2015/01/30 16:15:23, grt wrote: > On 2015/01/28 22:20:33, robertshield wrote: > > On 2015/01/28 14:49:05, grt wrote: > > > Passing in a ptr to the service defeats the whole purpose of the > > > AddIncidentCallback. I'll have to think about this a bit. > > > > Agreed, at the same time with the current API this is the only way that comes > to > > mind to do it. > > > > For musing: > > AddIncidentCallbacks currently require a Profile to conjure up and I don't > have > > one until the request comes in. Perhaps a callback that also took a profile > > parameter could be handed out? > > The more I think about it, the less comfortable I am with this. I'll get started > right away on allowing the caller to pass the Profile in at the time of running > the callback. It looks like it won't be at all intrusive. Please address other > comments, and maybe I'll finish before you. I'll reconsider my position if it > looks hairy. Sure, happy to merge either way, let me know. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:34: void AddScriptHashForTesting(const std::string& raw_hash); On 2015/01/30 16:15:23, grt wrote: > On 2015/01/28 22:20:32, robertshield wrote: > > On 2015/01/28 14:49:05, grt wrote: > > > unused > > > > Done. > > Hard to kill, it seems. Huh, I stabbed it repeatedly this time. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:37: bool ContainsScriptHashForTesting(const std::string& raw_hash); On 2015/01/30 16:15:23, grt wrote: > On 2015/01/28 22:20:33, robertshield wrote: > > On 2015/01/28 14:49:05, grt wrote: > > > unused > > > > Done. > > I don't think this word means what you think it means. It would seem I fell victim to one of the classic blunders. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:41: virtual bool AllowNullProfileForTesting(); On 2015/01/30 16:15:23, grt wrote: > On 2015/01/28 22:20:32, robertshield wrote: > > On 2015/01/28 14:49:05, grt wrote: > > > interesting. gab@ chose to do GetProfileForRenderProcessId via virtual > > dispatch > > > so that tests could make it return the proper testing profile. i wonder if > one > > > approach is better/worse than the other. > > > > Interesting, I don't believe one is better than the other. I did the other > thing > > first, but then didn't like the amount of code that was overridden for > testing. > > This is an explicit testing hook, which I felt was cleaner. > > How do you feel about getting rid of virtual dispatch and just having a bool > member that is modified by a protected setter (which can be made public in a > subclass-for-test)? Equally ok. I am chronically unopinionated about such things. Doing as you suggest. https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:19: const char* const kHashes[] = { On 2015/01/30 16:15:23, grt wrote: > blank line before this Done. https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:111: if (browser_context) { On 2015/01/30 16:15:24, grt wrote: > nit: no braces Done. https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:157: // TODO(robertshield): Set the domain of the parent frame: On 2015/01/30 16:15:24, grt wrote: > when will this TODO be addressed? hrm, so I had deferred this while I figured out the Ward wiring up. Since you're looking into a better wiring method, I'll see about adding this to the current CL. EDIT: Now implemented, please take a look. https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:175: void ScriptRequestDetector::AddScriptHashForTesting( On 2015/01/30 16:15:23, grt wrote: > remove Done. https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:180: bool ScriptRequestDetector::ContainsScriptHashForTesting( On 2015/01/30 16:15:24, grt wrote: > remove Done. https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:192: for (const char* encoded_hash : kHashes) { On 2015/01/30 16:15:23, grt wrote: > nit: remove braces Done. https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:10: #include "chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h" On 2015/01/30 16:15:24, grt wrote: > can you forward decl IncidentReportingService instead of this? Done. https://codereview.chromium.org/845663004/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:10: #include "chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h" On 2015/01/30 16:15:24, grt wrote: > can you forward decl IncidentReportingService instead of this? Done.
Patchset #6 (id:100001) has been deleted
looks good. https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:26: IncidentReportingService* incident_reporting_service); On 2015/02/01 04:25:57, robertshield wrote: > On 2015/01/30 16:15:23, grt wrote: > > On 2015/01/28 22:20:33, robertshield wrote: > > > On 2015/01/28 14:49:05, grt wrote: > > > > Passing in a ptr to the service defeats the whole purpose of the > > > > AddIncidentCallback. I'll have to think about this a bit. > > > > > > Agreed, at the same time with the current API this is the only way that > comes > > to > > > mind to do it. > > > > > > For musing: > > > AddIncidentCallbacks currently require a Profile to conjure up and I don't > > have > > > one until the request comes in. Perhaps a callback that also took a profile > > > parameter could be handed out? > > > > The more I think about it, the less comfortable I am with this. I'll get > started > > right away on allowing the caller to pass the Profile in at the time of > running > > the callback. It looks like it won't be at all intrusive. Please address other > > comments, and maybe I'll finish before you. I'll reconsider my position if it > > looks hairy. > > Sure, happy to merge either way, let me know. The latest work-in-progress is out for review here: https://codereview.chromium.org/845663004/. https://codereview.chromium.org/845663004/diff/140001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:208: // See if we can add the URL obtained from the RenderFrameHost. nit: no "we". consider: // Add the URL obtained from the RenderFrameHost if available. https://codereview.chromium.org/845663004/diff/140001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:27: virtual ~ScriptRequestDetector(); can this be made non-virtual now?
Thanks, PTAL https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:26: IncidentReportingService* incident_reporting_service); On 2015/02/02 18:45:22, grt wrote: > On 2015/02/01 04:25:57, robertshield wrote: > > On 2015/01/30 16:15:23, grt wrote: > > > On 2015/01/28 22:20:33, robertshield wrote: > > > > On 2015/01/28 14:49:05, grt wrote: > > > > > Passing in a ptr to the service defeats the whole purpose of the > > > > > AddIncidentCallback. I'll have to think about this a bit. > > > > > > > > Agreed, at the same time with the current API this is the only way that > > comes > > > to > > > > mind to do it. > > > > > > > > For musing: > > > > AddIncidentCallbacks currently require a Profile to conjure up and I don't > > > have > > > > one until the request comes in. Perhaps a callback that also took a > profile > > > > parameter could be handed out? > > > > > > The more I think about it, the less comfortable I am with this. I'll get > > started > > > right away on allowing the caller to pass the Profile in at the time of > > running > > > the callback. It looks like it won't be at all intrusive. Please address > other > > > comments, and maybe I'll finish before you. I'll reconsider my position if > it > > > looks hairy. > > > > Sure, happy to merge either way, let me know. > > The latest work-in-progress is out for review here: > https://codereview.chromium.org/845663004/. Rebased on top of the latest hotness. https://codereview.chromium.org/845663004/diff/140001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:208: // See if we can add the URL obtained from the RenderFrameHost. On 2015/02/02 18:45:22, grt wrote: > nit: no "we". consider: > // Add the URL obtained from the RenderFrameHost if available. They got to you too, didn't they? :-) So changed. https://codereview.chromium.org/845663004/diff/140001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:27: virtual ~ScriptRequestDetector(); On 2015/02/02 18:45:22, grt wrote: > can this be made non-virtual now? Done.
https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:7: #include "base/base64.h" unused https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:9: #include "chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h" change this to incident_receiver.h https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:180: render_frame_id, base::Passed(incident_data.Pass()))); base::Passed(&incident_data) https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:205: nit: remove blank line https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:12: #include "chrome/browser/safe_browsing/incident_reporting/incident_receiver.h" can you forward decl IncidentReceiver instead of including here? https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015, please. :-) https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:24: const char* kMatchingTestUrl = "http://example.com/foo/bar/baz.js"; const char kFoo[] = "..." for these three https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:32: class FakeIncidentReceiver : public IncidentReceiver { why not use MockIncidentReceiver (in mock_incident_receiver.h)? you could remove this class and set up the fake detector with a StrictMock<MockIncidentReceiver> and remove the count member. EventForBaseMatchingScript and EventForMatchingScriptWithParams would each have a single EXPECT_CALL, whereas the others would need nothing at all (the StrictMock will fail the tests if any method is called). you can use the TakeIncident action to get and then inspect the incident: scoped_ptr<Incident> incident; EXPECT_CALL(mock_receiver, DoAddIncidentForProfile(IsNull(), _)) .WillOnce(WithArg<1>(TakeIncident(&incident))); // EXPECT/ASSERT various things on |incident|... see the other unittests in this directory for examples. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc:34: return HashMessage(payload()->script_request()); return a constant here, no? https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_incident.h (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_incident.h:15: // An incident representing a suspicious script detection incident. nit: reword comment to avoid using "incident" twice (possibly just drop the second one). https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.cc:237: incident_service_->GetIncidentReceiver().Pass())); do you need the .Pass() here? does it compile if you remove it? https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.cc:286: // Note that it is important that incident_service_ be destroyed AFTER the why do you say that? i don't think it's true.
PTAL https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:7: #include "base/base64.h" On 2015/02/06 15:24:54, grt wrote: > unused Done. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:9: #include "chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h" On 2015/02/06 15:24:54, grt wrote: > change this to incident_receiver.h Done. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:180: render_frame_id, base::Passed(incident_data.Pass()))); On 2015/02/06 15:24:54, grt wrote: > base::Passed(&incident_data) Done. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:205: On 2015/02/06 15:24:54, grt wrote: > nit: remove blank line Done. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:12: #include "chrome/browser/safe_browsing/incident_reporting/incident_receiver.h" On 2015/02/06 15:24:54, grt wrote: > can you forward decl IncidentReceiver instead of including here? Sadly, no. I tried this yesterday and got an error about the destructor of an undefined type being called. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/02/06 15:24:54, grt wrote: > 2015, please. :-) Done. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:24: const char* kMatchingTestUrl = "http://example.com/foo/bar/baz.js"; On 2015/02/06 15:24:55, grt wrote: > const char kFoo[] = "..." for these three Done. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:32: class FakeIncidentReceiver : public IncidentReceiver { On 2015/02/06 15:24:54, grt wrote: > why not use MockIncidentReceiver (in mock_incident_receiver.h)? you could remove > this class and set up the fake detector with a StrictMock<MockIncidentReceiver> > and remove the count member. EventForBaseMatchingScript and > EventForMatchingScriptWithParams would each have a single EXPECT_CALL, whereas > the others would need nothing at all (the StrictMock will fail the tests if any > method is called). you can use the TakeIncident action to get and then inspect > the incident: > > scoped_ptr<Incident> incident; > EXPECT_CALL(mock_receiver, DoAddIncidentForProfile(IsNull(), _)) > .WillOnce(WithArg<1>(TakeIncident(&incident))); > // EXPECT/ASSERT various things on |incident|... > > see the other unittests in this directory for examples. Done. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc:34: return HashMessage(payload()->script_request()); On 2015/02/06 15:24:55, grt wrote: > return a constant here, no? ugh, didn't save before committing. a week spent using real developer tools in a modern language ruins ya I tell ya. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_incident.h (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_incident.h:15: // An incident representing a suspicious script detection incident. On 2015/02/06 15:24:55, grt wrote: > nit: reword comment to avoid using "incident" twice (possibly just drop the > second one). Done. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.cc:237: incident_service_->GetIncidentReceiver().Pass())); On 2015/02/06 15:24:55, grt wrote: > do you need the .Pass() here? does it compile if you remove it? Done. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.cc:286: // Note that it is important that incident_service_ be destroyed AFTER the On 2015/02/06 15:24:55, grt wrote: > why do you say that? i don't think it's true. If you recall the earlier revision of the CL, it used to be true. It no longer is, so comment removed.
https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:12: #include "chrome/browser/safe_browsing/incident_reporting/incident_receiver.h" On 2015/02/06 22:36:48, robertshield wrote: > On 2015/02/06 15:24:54, grt wrote: > > can you forward decl IncidentReceiver instead of including here? > > Sadly, no. I tried this yesterday and got an error about the destructor of an > undefined type being called. Inicdentally, I looked at this again and I'm still not sure why this header gets a compilation error at line 47 when I forward declare IncidentReceiver. Maybe we can take a look at this together on Monday.
Looking good. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:12: #include "chrome/browser/safe_browsing/incident_reporting/incident_receiver.h" On 2015/02/06 22:41:18, robertshield wrote: > On 2015/02/06 22:36:48, robertshield wrote: > > On 2015/02/06 15:24:54, grt wrote: > > > can you forward decl IncidentReceiver instead of including here? > > > > Sadly, no. I tried this yesterday and got an error about the destructor of an > > undefined type being called. > > Inicdentally, I looked at this again and I'm still not sure why this header gets > a compilation error at line 47 when I forward declare IncidentReceiver. Maybe we > can take a look at this together on Monday. Heh, you said 47. https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.cc:286: // Note that it is important that incident_service_ be destroyed AFTER the On 2015/02/06 22:36:48, robertshield wrote: > On 2015/02/06 15:24:55, grt wrote: > > why do you say that? i don't think it's true. > > If you recall the earlier revision of the CL, it used to be true. > > It no longer is, so comment removed. Oh, right. Apologies if that comment sounded snotty or something. https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:25: // |incident_reporting_service| MUST outlive this class. Oops, stale comment. Sorry I missed this before. https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc (right): https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:49: return static_cast<MockIncidentReceiver*>(incident_receiver_.get()); You could instead keep a naked pointer to the mock in the test fixture with a comment explaining that it is only valid for the lifetime of the detector. I think that's a nicer way since it minimizes the test-specific noise in the impl. https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:89: ++script_detection_event_count_; Remove this data member and method. https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:123: EXPECT_CALL( I would move this above the call to OnResourceRequest so that the test doesn't depend on the incident being added by a posted task. Unless you think that's an important thing to test. https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:131: const ClientIncidentReport_IncidentData_ScriptRequestIncident& Assert true has_script_request() before this.
Thanks! Another round! https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:12: #include "chrome/browser/safe_browsing/incident_reporting/incident_receiver.h" On 2015/02/06 23:37:46, grt wrote: > On 2015/02/06 22:41:18, robertshield wrote: > > On 2015/02/06 22:36:48, robertshield wrote: > > > On 2015/02/06 15:24:54, grt wrote: > > > > can you forward decl IncidentReceiver instead of including here? > > > > > > Sadly, no. I tried this yesterday and got an error about the destructor of > an > > > undefined type being called. > > > > Inicdentally, I looked at this again and I'm still not sure why this header > gets > > a compilation error at line 47 when I forward declare IncidentReceiver. Maybe > we > > can take a look at this together on Monday. > > Heh, you said 47. Acknowledged. https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.h (right): https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.h:25: // |incident_reporting_service| MUST outlive this class. On 2015/02/06 23:37:47, grt wrote: > Oops, stale comment. Sorry I missed this before. Done. https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc (right): https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:49: return static_cast<MockIncidentReceiver*>(incident_receiver_.get()); On 2015/02/06 23:37:47, grt wrote: > You could instead keep a naked pointer to the mock in the test fixture with a > comment explaining that it is only valid for the lifetime of the detector. I > think that's a nicer way since it minimizes the test-specific noise in the impl. Done. https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:89: ++script_detection_event_count_; On 2015/02/06 23:37:47, grt wrote: > Remove this data member and method. Done. https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:123: EXPECT_CALL( On 2015/02/06 23:37:47, grt wrote: > I would move this above the call to OnResourceRequest so that the test doesn't > depend on the incident being added by a posted task. Unless you think that's an > important thing to test. Done. https://codereview.chromium.org/845663004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:131: const ClientIncidentReport_IncidentData_ScriptRequestIncident& On 2015/02/06 23:37:47, grt wrote: > Assert true has_script_request() before this. Done.
looks really good. a few remaining nits, and one simplification suggestion. https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:178: weak_ptr_factory_.GetWeakPtr(), render_process_id, i'm wondering about this. you could easily get rid of the weak pointer factory by making ReportIncidentOnUIThread a private func in the unnamed namespace and passing allow_null_profile_for_testing_ and incident_receiver_ as arguments. can you think of any reason why this would be a bad thing? https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc (right): https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:49: public: protected https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:57: protected: remove https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:78: protected: remove https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:109: EXPECT_CALL( formatting nit: EXPECT_CALL(*mock_incident_receiver_, DoAddIncidentForProfile(IsNull(), _)) .WillOnce(WithArg<1>(TakeIncident(&incident))); and below https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc (right): https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc:18: // TODO(robertshield): Add inclusion of host domain. this is done in ReportIncidentOnUIThread, no?
robertshield@chromium.org changed reviewers: + mattm@chromium.org
Thanks Greg, sending to Matt for OWNERS for the safebrowsing bits. Matt, please could you take a look? https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:178: weak_ptr_factory_.GetWeakPtr(), render_process_id, On 2015/02/09 16:02:25, grt wrote: > i'm wondering about this. you could easily get rid of the weak pointer factory > by making ReportIncidentOnUIThread a private func in the unnamed namespace and > passing allow_null_profile_for_testing_ and incident_receiver_ as arguments. can > you think of any reason why this would be a bad thing? As discussed, this would be hard since incident_reciever_ is owned by the ScriptRequestDetector instance which would imply dodgy use of a non-owned, non-refcounted object. Will leave as is. https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc (right): https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:49: public: On 2015/02/09 16:02:25, grt wrote: > protected Done. https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:57: protected: On 2015/02/09 16:02:25, grt wrote: > remove Done. https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:78: protected: On 2015/02/09 16:02:25, grt wrote: > remove Done. https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector_unittest.cc:109: EXPECT_CALL( On 2015/02/09 16:02:25, grt wrote: > formatting nit: > EXPECT_CALL(*mock_incident_receiver_, DoAddIncidentForProfile(IsNull(), _)) > .WillOnce(WithArg<1>(TakeIncident(&incident))); > and below git cl format has done its thing. https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc (right): https://codereview.chromium.org/845663004/diff/200001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc:18: // TODO(robertshield): Add inclusion of host domain. On 2015/02/09 16:02:25, grt wrote: > this is done in ReportIncidentOnUIThread, no? Done.
lgtm
New patchsets have been uploaded after l-g-t-m from grt@chromium.org
lgtm again with one little suggestion. https://codereview.chromium.org/845663004/diff/240001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:157: std::string raw_hash(crypto::kSHA256Length, '\0'); raw_hash -> script_digest?
New patchsets have been uploaded after l-g-t-m from grt@chromium.org
lgtm^2
https://codereview.chromium.org/845663004/diff/240001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:157: std::string raw_hash(crypto::kSHA256Length, '\0'); On 2015/02/11 16:54:15, grt wrote: > raw_hash -> script_digest? Done.
https://codereview.chromium.org/845663004/diff/260001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc (right): https://codereview.chromium.org/845663004/diff/260001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc:33: return 42; hmm
New patchsets have been uploaded after l-g-t-m from grt@chromium.org
Thanks Matt, PTAL. https://codereview.chromium.org/845663004/diff/260001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc (right): https://codereview.chromium.org/845663004/diff/260001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_incident.cc:33: return 42; On 2015/02/11 22:03:55, mattm wrote: > hmm Indeed, this is worthy of a comment. As per the Incident API, to report an incident once for each type of event (meaning in this case each distinct script hash), ComputeDigest must return a constant value. Comment added.
https://codereview.chromium.org/845663004/diff/280001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:124: return render_frame_host->GetSiteInstance()->GetSiteURL(); Oh, I have been advised not to use SiteInstance for this sort of thing. (Not guaranteed to be accurate.) I think you should use render_frame_host->GetLastCommittedURL()
https://codereview.chromium.org/845663004/diff/280001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc (right): https://codereview.chromium.org/845663004/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:124: return render_frame_host->GetSiteInstance()->GetSiteURL(); On 2015/02/11 23:00:55, mattm wrote: > Oh, I have been advised not to use SiteInstance for this sort of thing. (Not > guaranteed to be accurate.) > > I think you should use render_frame_host->GetLastCommittedURL() Interesting, thanks. Switched to use last committed url instead.
lgtm
The CQ bit was checked by robertshield@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/845663004/300001
Message was sent while issue was closed.
Committed patchset #15 (id:300001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/3426390224d3b6ba9c39dc827cf71a1e7c656108 Cr-Commit-Position: refs/heads/master@{#315970} |
