Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2035753004:
Add implementation of PermissionReporter (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@refactor-certificate-report-sender Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd implementation of PermissionReporter
BUG=613883
Committed: https://crrev.com/7fea7386282ce4457ede733f5baae3a66b3aec2a
Cr-Commit-Position: refs/heads/master@{#400917}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : Rebased #Patch Set 5 : Fix small mistakes #
Total comments: 20
Patch Set 6 : #
Total comments: 8
Patch Set 7 : #
Total comments: 11
Patch Set 8 : Change functions signature #
Total comments: 9
Patch Set 9 : Resolve nits #Patch Set 10 #Patch Set 11 #Patch Set 12 : Remove static marker in anon namespace #
Dependent Patchsets: Messages
Total messages: 28 (7 generated)
stefanocs@google.com changed reviewers: + kcarattini@chromium.org
raymes@chromium.org changed reviewers: + raymes@chromium.org
raymes@chromium.org changed reviewers: + raymes@chromium.org
Hey Stefano - let me know when you've rebased this and I'll take a closer look. Thanks!
Hey Stefano - let me know when you've rebased this and I'll take a closer look. Thanks!
On 2016/06/08 03:01:24, raymes wrote: > Hey Stefano - let me know when you've rebased this and I'll take a closer look. > Thanks! Hey Raymes, I have rebased this CL. Please take a look! Thanks.
This looks good :) https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:81: DCHECK(permission_report_sender_); nit: this isn't needed (because it's clear from code immediately above) https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:87: DCHECK(permission_report_sender_); nit: I'd also say this isn't really needed. It's only used for tests and the code will crash clearly if it's not specified. https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:110: // TODO(stefanocs): Collect other data for the report from global variables nit: "." at end https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:31: explicit PermissionReporter(std::unique_ptr<net::ReportSender> report_sender); We might as well make this private and make the test a friend. https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:50: static bool BuildReport(const GURL& origin, Can this be private? https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:31: // A mock ReportSender that keeps track of the last report nit : fill 80 chars https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:58: ~PermissionReporterTest() override {} nit: You shouldn't need to define the constructor/destructor https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:62: // report string nit: "." at end of line https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:73: } I actually don't think we need to test BuildReport (at least for now). I think the SendReport test covers it. https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:78: MockReportSender* mock_report_sender = new MockReportSender(); nit: inline this to avoid dealing with a raw pointer
https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:81: DCHECK(permission_report_sender_); On 2016/06/09 01:30:40, raymes wrote: > nit: this isn't needed (because it's clear from code immediately above) Done. https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:87: DCHECK(permission_report_sender_); On 2016/06/09 01:30:40, raymes wrote: > nit: I'd also say this isn't really needed. It's only used for tests and the > code will crash clearly if it's not specified. Done. https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:110: // TODO(stefanocs): Collect other data for the report from global variables On 2016/06/09 01:30:40, raymes wrote: > nit: "." at end Done. https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:31: explicit PermissionReporter(std::unique_ptr<net::ReportSender> report_sender); On 2016/06/09 01:30:40, raymes wrote: > We might as well make this private and make the test a friend. Done. https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.h:50: static bool BuildReport(const GURL& origin, On 2016/06/09 01:30:41, raymes wrote: > Can this be private? Done. https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:31: // A mock ReportSender that keeps track of the last report On 2016/06/09 01:30:41, raymes wrote: > nit : fill 80 chars Done. https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:58: ~PermissionReporterTest() override {} On 2016/06/09 01:30:41, raymes wrote: > nit: You shouldn't need to define the constructor/destructor Done. https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:62: // report string On 2016/06/09 01:30:41, raymes wrote: > nit: "." at end of line Done. https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:73: } On 2016/06/09 01:30:41, raymes wrote: > I actually don't think we need to test BuildReport (at least for now). I think > the SendReport test covers it. Done. https://codereview.chromium.org/2035753004/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:78: MockReportSender* mock_report_sender = new MockReportSender(); On 2016/06/09 01:30:41, raymes wrote: > nit: inline this to avoid dealing with a raw pointer Do you mean it should be wrapped with unique_ptr?
lgtm https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:42: friend class PermissionReporterTest; Since the only thing used in the test is the additional constructor now, I'm ok with leaving that as public and not having a friend class (to keep the test simple). But either way is ok. https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:60: new PermissionReporter(base::WrapUnique(mock_report_sender_))) {} What I mean is, can we replace mock_report_sender_ with new MockReportSender() ?
https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.h:42: friend class PermissionReporterTest; On 2016/06/09 06:03:47, raymes wrote: > Since the only thing used in the test is the additional constructor now, I'm ok > with leaving that as public and not having a friend class (to keep the test > simple). But either way is ok. Acknowledged. https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:60: new PermissionReporter(base::WrapUnique(mock_report_sender_))) {} On 2016/06/09 06:03:47, raymes wrote: > What I mean is, can we replace mock_report_sender_ with new MockReportSender() ? I was trying to do this earlier but I figured the report sender still need to be accessed by the tests. If we remove the mock report sender as the member of this class, we can still access the report sender from permission_reporter_->permission_report_sender_ but I'm not sure how to cast that into MockReportSender without moving the ownership from PermissionReporter class.
https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:60: new PermissionReporter(base::WrapUnique(mock_report_sender_))) {} Oh sorry, I understand now. This seems ok to me. See comment below. https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:61: Add a comment about the ownership of this: // Owned by |permission_reporter_|.
Thanks Raymes! https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:60: new PermissionReporter(base::WrapUnique(mock_report_sender_))) {} On 2016/06/09 06:43:41, raymes wrote: > Oh sorry, I understand now. This seems ok to me. See comment below. Acknowledged. https://codereview.chromium.org/2035753004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:61: On 2016/06/09 06:43:41, raymes wrote: > Add a comment about the ownership of this: > // Owned by |permission_reporter_|. Done.
https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:20: PermissionReport::PermissionType PermissionTypeToPermissionReportEnum( same as below. https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:51: PermissionReport::Action PermissionActionToPermissionReportEnum( I think "PermissionActionToPermissionReportAction" is more clear. https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:26: static const PermissionReport::PermissionType kDummyProtobufPermission = kDummyPermissionReportPermission (or similar if you can think of something better ;) ). https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:28: static const PermissionReport::Action kDummyProtobufAction = kDummyPermissionReportAction https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:63: MockReportSender* mock_report_sender_; Why does this and permissions_reporter_ need to be owned by the test class instead of being instantiated by each test case individually?
https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:20: PermissionReport::PermissionType PermissionTypeToPermissionReportEnum( On 2016/06/13 20:58:46, kcarattini wrote: > same as below. Done. https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:51: PermissionReport::Action PermissionActionToPermissionReportEnum( On 2016/06/13 20:58:46, kcarattini wrote: > I think "PermissionActionToPermissionReportAction" is more clear. Done. https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:26: static const PermissionReport::PermissionType kDummyProtobufPermission = On 2016/06/13 20:58:46, kcarattini wrote: > kDummyPermissionReportPermission (or similar if you can think of something > better ;) ). Done. https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:28: static const PermissionReport::Action kDummyProtobufAction = On 2016/06/13 20:58:46, kcarattini wrote: > kDummyPermissionReportAction Done. https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:63: MockReportSender* mock_report_sender_; On 2016/06/13 20:58:46, kcarattini wrote: > Why does this and permissions_reporter_ need to be owned by the test class > instead of being instantiated by each test case individually? The called constructor of PermissionReporter is private and can only be called inside of the test class since it is a friend class.
https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2035753004/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:63: MockReportSender* mock_report_sender_; On 2016/06/13 23:57:11, stefanocs wrote: > On 2016/06/13 20:58:46, kcarattini wrote: > > Why does this and permissions_reporter_ need to be owned by the test class > > instead of being instantiated by each test case individually? > > The called constructor of PermissionReporter is private and can only be called > inside of the test class since it is a friend class. Acknowledged. https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:77: EXPECT_EQ(kDummyPermissionReportPermission, permission_report.permission()); I think you should test the BuildReport functionality in a separate test. You can make that new test a friend test so it can call a private method.
https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:77: EXPECT_EQ(kDummyPermissionReportPermission, permission_report.permission()); On 2016/06/14 21:17:27, kcarattini wrote: > I think you should test the BuildReport functionality in a separate test. You > can make that new test a friend test so it can call a private method. I said the opposite and asked Stefano to remove that test :P I disagree because BuildReport can be fully tested/exercised through SendReport, which is the public interface anyway. As far as I could tell, the test that was there didn't test anything additional to this one. If you feel strongly then we can add it back (sorry Stefano!) but my thought is to leave it as-is :)
https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:77: EXPECT_EQ(kDummyPermissionReportPermission, permission_report.permission()); On 2016/06/14 23:07:28, raymes wrote: > On 2016/06/14 21:17:27, kcarattini wrote: > > I think you should test the BuildReport functionality in a separate test. You > > can make that new test a friend test so it can call a private method. > > I said the opposite and asked Stefano to remove that test :P I disagree because > BuildReport can be fully tested/exercised through SendReport, which is the > public interface anyway. As far as I could tell, the test that was there didn't > test anything additional to this one. > > If you feel strongly then we can add it back (sorry Stefano!) but my thought is > to leave it as-is :) I do not feel strongly :). Comments withdrawn.
kcarattini@chromium.org changed reviewers: + nparker@chromium.org
lgtm
lgtm https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:20: PermissionReport::PermissionType PermissionTypeToPermissionReportPermission( wholey smokes that's a lot of Permissions. Could this be shorter? Maybe PermissionTypeForProto, or PermissonTypeForReport? (same below) That makes it clear that PermissionReportPermission is a proto enum, which explains a bit why you're converting it. https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:78: : permission_report_sender_(new net::ReportSender( nit: This could call the constructor below. That way if you add members later, they get initialized just in one function rather than two. https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:106: // TODO(stefanocs): Collect other data for the report from global variables. What other data do you need? You could list them here. Relevant experiment states...
https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:20: PermissionReport::PermissionType PermissionTypeToPermissionReportPermission( On 2016/06/20 16:10:09, Nathan Parker wrote: > wholey smokes that's a lot of Permissions. Could this be shorter? Maybe > PermissionTypeForProto, or PermissonTypeForReport? (same below) That makes it > clear that PermissionReportPermission is a proto enum, which explains a bit why > you're converting it. Done. https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:78: : permission_report_sender_(new net::ReportSender( On 2016/06/20 16:10:09, Nathan Parker wrote: > nit: This could call the constructor below. That way if you add members later, > they get initialized just in one function rather than two. Done. https://codereview.chromium.org/2035753004/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:106: // TODO(stefanocs): Collect other data for the report from global variables. On 2016/06/20 16:10:09, Nathan Parker wrote: > What other data do you need? You could list them here. Relevant experiment > states... Done.
The CQ bit was checked by stefanocs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, kcarattini@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2035753004/#ps220001 (title: "Remove static marker in anon namespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2035753004/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add implementation of PermissionReporter BUG=613883 ========== to ========== Add implementation of PermissionReporter BUG=613883 Committed: https://crrev.com/7fea7386282ce4457ede733f5baae3a66b3aec2a Cr-Commit-Position: refs/heads/master@{#400917} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/7fea7386282ce4457ede733f5baae3a66b3aec2a Cr-Commit-Position: refs/heads/master@{#400917} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
