Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1020)

Issue 2089383005: Add throttling to permission reporter (Closed)

Created:
4 years, 6 months ago by stefanocs
Modified:
4 years, 5 months ago
CC:
chromium-reviews, grt+watch_chromium.org, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@add-hooks-to-permission-layer
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add throttling to permission reporter Report sending is restricted to a maximum of 5 reports per minute per origin per permission. BUG=613883 Committed: https://crrev.com/3c0cfd954534699d67193f0d96281c3fb046c8f9 Cr-Commit-Position: refs/heads/master@{#406236}

Patch Set 1 #

Patch Set 2 : Use queue instead of set to store sent time #

Patch Set 3 : Use existing hash combine function #

Patch Set 4 : Rename constant #

Total comments: 6

Patch Set 5 : Resolve nits #

Patch Set 6 : Add throttling test #

Patch Set 7 : Remove a constructor #

Patch Set 8 : Add comment #

Total comments: 12

Patch Set 9 : Resolve comments and rebase #

Total comments: 6

Patch Set 10 : Resolve comments #

Total comments: 6

Patch Set 11 : Resolve comments #

Total comments: 10

Patch Set 12 : Rename member variable sent_histories #

Patch Set 13 : todo #

Patch Set 14 : todo #

Patch Set 15 : typo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+166 lines, -24 lines) Patch
M chrome/browser/safe_browsing/permission_reporter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +37 lines, -2 lines 0 comments Download
M chrome/browser/safe_browsing/permission_reporter.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +50 lines, -6 lines 0 comments Download
M chrome/browser/safe_browsing/permission_reporter_unittest.cc View 1 2 3 4 5 6 7 8 9 10 6 chunks +79 lines, -16 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 42 (16 generated)
stefanocs
4 years, 5 months ago (2016-06-27 01:06:44 UTC) #2
raymes
We should also add a test for this. What you can do is pass in ...
4 years, 5 months ago (2016-06-27 06:26:57 UTC) #3
stefanocs
https://codereview.chromium.org/2089383005/diff/60001/chrome/browser/safe_browsing/permission_reporter.cc File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/60001/chrome/browser/safe_browsing/permission_reporter.cc#newcode25 chrome/browser/safe_browsing/permission_reporter.cc:25: const int kMaximumReportsPerOriginPerPermission = 5; On 2016/06/27 06:26:56, raymes ...
4 years, 5 months ago (2016-06-27 07:34:58 UTC) #4
stefanocs
Test added.
4 years, 5 months ago (2016-06-28 04:47:22 UTC) #5
raymes
https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_browsing/permission_reporter.cc File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_browsing/permission_reporter.cc#newcode143 chrome/browser/safe_browsing/permission_reporter.cc:143: current_time - history.front() > base::TimeDelta::FromMinutes(1)) nit: add {} https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_browsing/permission_reporter.h ...
4 years, 5 months ago (2016-06-29 07:09:17 UTC) #6
stefanocs
https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_browsing/permission_reporter.cc File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/140001/chrome/browser/safe_browsing/permission_reporter.cc#newcode143 chrome/browser/safe_browsing/permission_reporter.cc:143: current_time - history.front() > base::TimeDelta::FromMinutes(1)) On 2016/06/29 07:09:17, raymes ...
4 years, 5 months ago (2016-06-30 00:24:51 UTC) #7
raymes
lgtm with a couple of comments We should still see if we want to simplify ...
4 years, 5 months ago (2016-06-30 01:30:20 UTC) #8
stefanocs
https://codereview.chromium.org/2089383005/diff/160001/chrome/browser/safe_browsing/permission_reporter_unittest.cc File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2089383005/diff/160001/chrome/browser/safe_browsing/permission_reporter_unittest.cc#newcode71 chrome/browser/safe_browsing/permission_reporter_unittest.cc:71: int NumberOfNewReports() { On 2016/06/30 01:30:19, raymes wrote: > ...
4 years, 5 months ago (2016-06-30 04:14:28 UTC) #9
kcarattini
https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_browsing/permission_reporter.h File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_browsing/permission_reporter.h#newcode65 chrome/browser/safe_browsing/permission_reporter.h:65: explicit PermissionReporter(std::unique_ptr<net::ReportSender> report_sender, This no longer needs to be ...
4 years, 5 months ago (2016-07-06 06:28:20 UTC) #10
stefanocs
https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_browsing/permission_reporter.h File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/180001/chrome/browser/safe_browsing/permission_reporter.h#newcode65 chrome/browser/safe_browsing/permission_reporter.h:65: explicit PermissionReporter(std::unique_ptr<net::ReportSender> report_sender, On 2016/07/06 06:28:20, kcarattini wrote: > ...
4 years, 5 months ago (2016-07-06 07:17:39 UTC) #11
kcarattini
lgtm
4 years, 5 months ago (2016-07-07 01:18:04 UTC) #12
stefanocs
nparker@chromium.org: Please review changes in chrome/browser/safe_browsing/ Thanks
4 years, 5 months ago (2016-07-07 01:24:45 UTC) #15
stefanocs
nparker@chromium.org: Please review changes in chrome/browser/safe_browsing/ Thanks
4 years, 5 months ago (2016-07-07 01:24:45 UTC) #16
stefanocs
ping :)
4 years, 5 months ago (2016-07-13 06:29:33 UTC) #17
stefanocs
ping Nathan :)
4 years, 5 months ago (2016-07-13 07:12:38 UTC) #18
Nathan Parker
lgtm Sorry for the delay! CLs were getting slurped up by Inbox. https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_browsing/permission_reporter.cc File chrome/browser/safe_browsing/permission_reporter.cc ...
4 years, 5 months ago (2016-07-14 22:00:24 UTC) #19
stefanocs
https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_browsing/permission_reporter.cc File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_browsing/permission_reporter.cc#newcode158 chrome/browser/safe_browsing/permission_reporter.cc:158: std::queue<base::Time>& history = sent_histories[{permission, origin}]; On 2016/07/14 22:00:24, Nathan ...
4 years, 5 months ago (2016-07-15 02:04:29 UTC) #20
Nathan Parker
https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_browsing/permission_reporter.h File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_browsing/permission_reporter.h#newcode36 chrome/browser/safe_browsing/permission_reporter.h:36: const PermissionAndOrigin& permission_and_origin) const; On 2016/07/15 02:04:29, stefanocs wrote: ...
4 years, 5 months ago (2016-07-15 23:39:40 UTC) #21
raymes
https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_browsing/permission_reporter.cc File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_browsing/permission_reporter.cc#newcode158 chrome/browser/safe_browsing/permission_reporter.cc:158: std::queue<base::Time>& history = sent_histories[{permission, origin}]; We also discussed just ...
4 years, 5 months ago (2016-07-18 01:16:52 UTC) #22
kcarattini
https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_browsing/permission_reporter.cc File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_browsing/permission_reporter.cc#newcode158 chrome/browser/safe_browsing/permission_reporter.cc:158: std::queue<base::Time>& history = sent_histories[{permission, origin}]; On 2016/07/18 01:16:51, raymes ...
4 years, 5 months ago (2016-07-18 01:56:36 UTC) #23
stefanocs
https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_browsing/permission_reporter.h File chrome/browser/safe_browsing/permission_reporter.h (right): https://codereview.chromium.org/2089383005/diff/200001/chrome/browser/safe_browsing/permission_reporter.h#newcode36 chrome/browser/safe_browsing/permission_reporter.h:36: const PermissionAndOrigin& permission_and_origin) const; On 2016/07/15 23:39:40, Nathan Parker ...
4 years, 5 months ago (2016-07-18 06:06:18 UTC) #24
Nathan Parker
lgtm Yea I may be smoking something on the hash<>() thing. Do what works.
4 years, 5 months ago (2016-07-18 16:11:19 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2089383005/270001
4 years, 5 months ago (2016-07-19 09:20:53 UTC) #38
commit-bot: I haz the power
Committed patchset #15 (id:270001)
4 years, 5 months ago (2016-07-19 09:57:57 UTC) #39
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-07-19 09:58:06 UTC) #40
commit-bot: I haz the power
4 years, 5 months ago (2016-07-19 10:00:00 UTC) #42
Message was sent while issue was closed.
Patchset 15 (id:??) landed as
https://crrev.com/3c0cfd954534699d67193f0d96281c3fb046c8f9
Cr-Commit-Position: refs/heads/master@{#406236}

Powered by Google App Engine
This is Rietveld 408576698