Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2024753002:
Add permission reporter skeleton (Closed)
|
Created:
4 years, 6 months ago by stefanocs Modified:
4 years, 6 months ago CC:
chromium-reviews, grt+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@protocol-buffer-for-permission-reports Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd permission reporter skeleton for permission action reporting to SafeBrowsing servers.
BUG=613883
Committed: https://crrev.com/4169c6ebee28555870861cbc0dc2625c38f7f06f
Cr-Commit-Position: refs/heads/master@{#398461}
Patch Set 1 #
Total comments: 26
Patch Set 2 : #Patch Set 3 : #
Total comments: 3
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : #
Total comments: 4
Patch Set 7 : #
Total comments: 12
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : Change SendReport function signature #
Total comments: 6
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #
Total comments: 2
Patch Set 15 : #
Messages
Total messages: 52 (17 generated)
Description was changed from ========== Add permission reporter skeleton Add permission reporter skeleton for permission action reporting to SafeBrowsing servers. BUG=613883 ========== to ========== Add permission reporter skeleton for permission action reporting to SafeBrowsing servers. BUG=613883 ==========
stefanocs@google.com changed reviewers: + kcarattini@chromium.org
https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.cc:26: // TODO(stefanocs): Implement SendReport function Add a full stop. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.cc:33: // TODO(stefanocs): Implement BuildReport function Add a full stop. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:21: // Provides functionality for building and serializing reports about permission s/permission/permissions/ https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:28: net::URLRequestContext* request_context, You can move this up to the previous line. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:34: // |serialized_report| is expected to be a serialized protobuf implemented in "A serialized protobuf of type PermissionReport defined in ..." https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:41: static PermissionReport BuildReport( This is fine for now, though if the list of arguments gets much longer we should create a struct to hold all of the information in one place. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:42: const GURL& origin, Can move this up a line too. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:48: // report sender class when it has been implemented. Isn't it CertificateReportSender that you want to refactor?
https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.cc:12: net::URLRequestContext* request_context, Should I move this up to the previous line too? https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.cc:26: // TODO(stefanocs): Implement SendReport function On 2016/05/31 03:13:37, kcarattini wrote: > Add a full stop. Done. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.cc:30: const GURL& origin, And move this up to the previous line? https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.cc:33: // TODO(stefanocs): Implement BuildReport function On 2016/05/31 03:13:37, kcarattini wrote: > Add a full stop. Done. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:21: // Provides functionality for building and serializing reports about permission On 2016/05/31 03:13:38, kcarattini wrote: > s/permission/permissions/ Done. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:28: net::URLRequestContext* request_context, On 2016/05/31 03:13:38, kcarattini wrote: > You can move this up to the previous line. Done. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:34: // |serialized_report| is expected to be a serialized protobuf implemented in On 2016/05/31 03:13:38, kcarattini wrote: > "A serialized protobuf of type PermissionReport defined in ..." Done. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:41: static PermissionReport BuildReport( On 2016/05/31 03:13:38, kcarattini wrote: > This is fine for now, though if the list of arguments gets much longer we should > create a struct to hold all of the information in one place. Can the other required information obtained within this function scope? https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:42: const GURL& origin, On 2016/05/31 03:13:38, kcarattini wrote: > Can move this up a line too. Done. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:48: // report sender class when it has been implemented. On 2016/05/31 03:13:38, kcarattini wrote: > Isn't it CertificateReportSender that you want to refactor? What I meant is that since we are probably going to rename the CertificateReportSender class, we will need to change the class name used here too. Maybe we need to change the TODO comment here.
https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.cc:12: net::URLRequestContext* request_context, On 2016/05/31 03:34:22, stefanocs wrote: > Should I move this up to the previous line too? Sounds good but I think you need to fix the indentation. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.cc:30: const GURL& origin, On 2016/05/31 03:34:22, stefanocs wrote: > And move this up to the previous line? sounds good. Thanks for catching this. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:41: static PermissionReport BuildReport( On 2016/05/31 03:34:22, stefanocs wrote: > On 2016/05/31 03:13:38, kcarattini wrote: > > This is fine for now, though if the list of arguments gets much longer we > should > > create a struct to hold all of the information in one place. > > Can the other required information obtained within this function scope? I'm not certain yet but some of it definitely will be. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:48: // report sender class when it has been implemented. On 2016/05/31 03:34:23, stefanocs wrote: > On 2016/05/31 03:13:38, kcarattini wrote: > > Isn't it CertificateReportSender that you want to refactor? > > What I meant is that since we are probably going to rename the > CertificateReportSender class, we will need to change the class name used here > too. Maybe we need to change the TODO comment here. Ah, I see. i don't think you need a todo for that because you'll need to update the class name everywhere it's used or chrome won't compile. How about we have this be a TODO for the actual refactoring of the CertificateReportSender class?
https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.cc:12: net::URLRequestContext* request_context, On 2016/05/31 03:53:36, kcarattini wrote: > On 2016/05/31 03:34:22, stefanocs wrote: > > Should I move this up to the previous line too? > > Sounds good but I think you need to fix the indentation. Done. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.cc:30: const GURL& origin, On 2016/05/31 03:53:36, kcarattini wrote: > On 2016/05/31 03:34:22, stefanocs wrote: > > And move this up to the previous line? > > sounds good. Thanks for catching this. Done. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:41: static PermissionReport BuildReport( On 2016/05/31 03:53:36, kcarattini wrote: > On 2016/05/31 03:34:22, stefanocs wrote: > > On 2016/05/31 03:13:38, kcarattini wrote: > > > This is fine for now, though if the list of arguments gets much longer we > > should > > > create a struct to hold all of the information in one place. > > > > Can the other required information obtained within this function scope? > > I'm not certain yet but some of it definitely will be. Acknowledged. https://codereview.chromium.org/2024753002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/permission_reporter.h:48: // report sender class when it has been implemented. On 2016/05/31 03:53:36, kcarattini wrote: > On 2016/05/31 03:34:23, stefanocs wrote: > > On 2016/05/31 03:13:38, kcarattini wrote: > > > Isn't it CertificateReportSender that you want to refactor? > > > > What I meant is that since we are probably going to rename the > > CertificateReportSender class, we will need to change the class name used here > > too. Maybe we need to change the TODO comment here. > > Ah, I see. i don't think you need a todo for that because you'll need to update > the class name everywhere it's used or chrome won't compile. How about we have > this be a TODO for the actual refactoring of the CertificateReportSender class? Done.
https://codereview.chromium.org/2024753002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:41: static PermissionReport BuildReport(const GURL& origin, Would it be better for this function to return a serialized report string instead?
https://codereview.chromium.org/2024753002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:41: static PermissionReport BuildReport(const GURL& origin, On 2016/05/31 04:17:56, stefanocs wrote: > Would it be better for this function to return a serialized report string > instead? Yes, good call.
https://codereview.chromium.org/2024753002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:41: static PermissionReport BuildReport(const GURL& origin, On 2016/05/31 04:27:34, kcarattini wrote: > On 2016/05/31 04:17:56, stefanocs wrote: > > Would it be better for this function to return a serialized report string > > instead? > > Yes, good call. Done.
https://codereview.chromium.org/2024753002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:42: PermissionAction action); Actually, you can add a final argument which is a pointer to a string so that you can fill in the serialized report there. I think this is what the protocol buffer library function does so this will save copying the string around.
https://codereview.chromium.org/2024753002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:42: PermissionAction action); On 2016/05/31 04:56:22, kcarattini wrote: > Actually, you can add a final argument which is a pointer to a string so that > you can fill in the serialized report there. I think this is what the protocol > buffer library function does so this will save copying the string around. Yes, I can do that. Do you think it should returns a boolean instead (whether the serialization is successful or not)? It seems the serialization won't fail though since it doesn't have required fields.
https://codereview.chromium.org/2024753002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:42: PermissionAction action); On 2016/05/31 05:05:26, stefanocs wrote: > On 2016/05/31 04:56:22, kcarattini wrote: > > Actually, you can add a final argument which is a pointer to a string so that > > you can fill in the serialized report there. I think this is what the protocol > > buffer library function does so this will save copying the string around. > > Yes, I can do that. Do you think it should returns a boolean instead (whether > the serialization is successful or not)? It seems the serialization won't fail > though since it doesn't have required fields. Sure you can return the return value of SerializeToString directly.
https://codereview.chromium.org/2024753002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:42: PermissionAction action); On 2016/05/31 07:02:28, kcarattini wrote: > On 2016/05/31 05:05:26, stefanocs wrote: > > On 2016/05/31 04:56:22, kcarattini wrote: > > > Actually, you can add a final argument which is a pointer to a string so > that > > > you can fill in the serialized report there. I think this is what the > protocol > > > buffer library function does so this will save copying the string around. > > > > Yes, I can do that. Do you think it should returns a boolean instead (whether > > the serialization is successful or not)? It seems the serialization won't fail > > though since it doesn't have required fields. > > Sure you can return the return value of SerializeToString directly. Done.
kcarattini@chromium.org changed reviewers: + nparker@chromium.org
LGTM. Nathan, please take a look.
https://codereview.chromium.org/2024753002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:12: PermissionReporter::PermissionReporter(net::URLRequestContext* request_context, How about having the URL hardcoded in this file (as a static string)? That way the caller doesn't need to know the URL or how the upload works. https://codereview.chromium.org/2024753002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:48: std::unique_ptr<net::CertificateReportSender> permission_report_sender_; I think we should do some refactoring before than making this dependency. It could be as simple as renaming CertificateReportSender to something more generic.
https://codereview.chromium.org/2024753002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:12: PermissionReporter::PermissionReporter(net::URLRequestContext* request_context, On 2016/05/31 22:24:57, Nathan Parker wrote: > How about having the URL hardcoded in this file (as a static string)? That way > the caller doesn't need to know the URL or how the upload works. Done. https://codereview.chromium.org/2024753002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:48: std::unique_ptr<net::CertificateReportSender> permission_report_sender_; On 2016/05/31 22:24:57, Nathan Parker wrote: > I think we should do some refactoring before than making this dependency. It > could be as simple as renaming CertificateReportSender to something more > generic. Done.
raymes@chromium.org changed reviewers: + raymes@chromium.org
https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:20: : upload_url_(GURL(kPermissionActionReportingUploadUrl)) {} nit: I think you should be able to omit the "GURL(" here, the constructor should be called implicitly. https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:27: nit: // static https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:34: void SendReport(const std::string& serialized_report); nit: I wonder if SendReport should just call BuildReport internally, rather than having 2 methods? kcarattini: is there a benefit to keeping these separate? https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:42: content::PermissionType permission, nit: #include permission_type.h
https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:20: : upload_url_(GURL(kPermissionActionReportingUploadUrl)) {} On 2016/06/01 06:45:44, raymes wrote: > nit: I think you should be able to omit the "GURL(" here, the constructor should > be called implicitly. Done. https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:27: On 2016/06/01 06:45:44, raymes wrote: > nit: // static I think we can't mark this as static here. https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:42: content::PermissionType permission, On 2016/06/01 06:45:44, raymes wrote: > nit: #include permission_type.h Done.
https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:27: On 2016/06/01 07:25:02, stefanocs wrote: > On 2016/06/01 06:45:44, raymes wrote: > > nit: // static > > I think we can't mark this as static here. What he means is to put a comment above the function. That's how we mark static functions in cc files. https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:34: void SendReport(const std::string& serialized_report); On 2016/06/01 06:45:45, raymes wrote: > nit: I wonder if SendReport should just call BuildReport internally, rather than > having 2 methods? kcarattini: is there a benefit to keeping these separate? Yup, that could work. BuildReport should remain a separate method so that it can be tested separately, but I'm find with SendReport calling it directly.
https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:27: On 2016/06/01 12:25:49, kcarattini wrote: > On 2016/06/01 07:25:02, stefanocs wrote: > > On 2016/06/01 06:45:44, raymes wrote: > > > nit: // static > > > > I think we can't mark this as static here. > > What he means is to put a comment above the function. That's how we mark static > functions in cc files. Done. https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:34: void SendReport(const std::string& serialized_report); On 2016/06/01 12:25:49, kcarattini wrote: > On 2016/06/01 06:45:45, raymes wrote: > > nit: I wonder if SendReport should just call BuildReport internally, rather > than > > having 2 methods? kcarattini: is there a benefit to keeping these separate? > > Yup, that could work. BuildReport should remain a separate method so that it can > be tested separately, but I'm find with SendReport calling it directly. Then SendReport signature should be modified to have the same arguments as BuildReport except the string output?
On 2016/06/02 00:24:33, stefanocs wrote: > https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/permission_reporter.cc (right): > > https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... > chrome/browser/safe_browsing/permission_reporter.cc:27: > On 2016/06/01 12:25:49, kcarattini wrote: > > On 2016/06/01 07:25:02, stefanocs wrote: > > > On 2016/06/01 06:45:44, raymes wrote: > > > > nit: // static > > > > > > I think we can't mark this as static here. > > > > What he means is to put a comment above the function. That's how we mark > static > > functions in cc files. > > Done. > > https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/permission_reporter.h (right): > > https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... > chrome/browser/safe_browsing/permission_reporter.h:34: void SendReport(const > std::string& serialized_report); > On 2016/06/01 12:25:49, kcarattini wrote: > > On 2016/06/01 06:45:45, raymes wrote: > > > nit: I wonder if SendReport should just call BuildReport internally, rather > > than > > > having 2 methods? kcarattini: is there a benefit to keeping these separate? > > > > Yup, that could work. BuildReport should remain a separate method so that it > can > > be tested separately, but I'm find with SendReport calling it directly. > > Then SendReport signature should be modified to have the same arguments as > BuildReport except the string output? That would make sense, yes.
https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2024753002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:34: void SendReport(const std::string& serialized_report); On 2016/06/01 06:45:45, raymes wrote: > nit: I wonder if SendReport should just call BuildReport internally, rather than > having 2 methods? kcarattini: is there a benefit to keeping these separate? Done.
lgtm!
lgtm https://codereview.chromium.org/2024753002/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:15: static const char kPermissionActionReportingUploadUrl[] = No need to say "static" if it's in an anonymous namespace. That has basically the same effect. https://codereview.chromium.org/2024753002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:21: : upload_url_(kPermissionActionReportingUploadUrl) {} You probably don't need this as a member of the class, unless you need to override it for tests. https://codereview.chromium.org/2024753002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:28: // TODO(stefanocs): Implement SendReport function. I'm assuming this will use a ReportSender, when the other refactoring CL lands, ya?
https://codereview.chromium.org/2024753002/diff/180001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:15: static const char kPermissionActionReportingUploadUrl[] = On 2016/06/06 22:55:49, Nathan Parker wrote: > No need to say "static" if it's in an anonymous namespace. That has basically > the same effect. Done. https://codereview.chromium.org/2024753002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:21: : upload_url_(kPermissionActionReportingUploadUrl) {} On 2016/06/06 22:55:49, Nathan Parker wrote: > You probably don't need this as a member of the class, unless you need to > override it for tests. Done. https://codereview.chromium.org/2024753002/diff/180001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:28: // TODO(stefanocs): Implement SendReport function. On 2016/06/06 22:55:49, Nathan Parker wrote: > I'm assuming this will use a ReportSender, when the other refactoring CL lands, > ya? Yes, that's right.
The CQ bit was checked by stefanocs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kcarattini@chromium.org, raymes@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2024753002/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2024753002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by stefanocs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kcarattini@chromium.org, raymes@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2024753002/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2024753002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2024753002/diff/260001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:29: ALLOW_UNUSED_LOCAL(kPermissionActionReportingUploadUrl); Since the constant is now unused in this skeleton, is it ok to use this to silence the compiler warning?
lgtm https://codereview.chromium.org/2024753002/diff/260001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2024753002/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:29: ALLOW_UNUSED_LOCAL(kPermissionActionReportingUploadUrl); On 2016/06/07 06:02:09, stefanocs wrote: > Since the constant is now unused in this skeleton, is it ok to use this to > silence the compiler warning? Sure, just be sure to remove it later.
The CQ bit was checked by stefanocs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kcarattini@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2024753002/#ps260001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2024753002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by stefanocs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kcarattini@chromium.org, raymes@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2024753002/#ps280001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2024753002/280001
Message was sent while issue was closed.
Description was changed from ========== Add permission reporter skeleton for permission action reporting to SafeBrowsing servers. BUG=613883 ========== to ========== Add permission reporter skeleton for permission action reporting to SafeBrowsing servers. BUG=613883 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add permission reporter skeleton for permission action reporting to SafeBrowsing servers. BUG=613883 ========== to ========== Add permission reporter skeleton for permission action reporting to SafeBrowsing servers. BUG=613883 Committed: https://crrev.com/4169c6ebee28555870861cbc0dc2625c38f7f06f Cr-Commit-Position: refs/heads/master@{#398461} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/4169c6ebee28555870861cbc0dc2625c38f7f06f Cr-Commit-Position: refs/heads/master@{#398461} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
