Chromium Code Reviews
Help | Chromium Project | Sign in
(76)

Issue 2708503002: Reporting: Implement cache. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by Julia Tuttle
Modified:
1 month ago
CC:
chromium-reviews, Randy Smith (Not in Mondays)
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Reporting: Implement cache. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/ for the spec, or https://goo.gl/pygX5I for details of the planned implementation in Chromium. This implements the cache, which holds report delivery configuration and queued reports. BUG=704259 Review-Url: https://codereview.chromium.org/2708503002 Cr-Commit-Position: refs/heads/master@{#459797} Committed: https://chromium.googlesource.com/chromium/src/+/5868433300e998d6aee22dd603d95c5e42830776

Patch Set 1 #

Total comments: 10

Patch Set 2 : splat #

Patch Set 3 : Fix new style error, other stuff. #

Patch Set 4 : Tidy things up a bit. #

Patch Set 5 : Add unit tests, etc. #

Patch Set 6 : Add test_util, switch back to vector #

Patch Set 7 : Don't expose doomed reports in GetReports. #

Patch Set 8 : Fix compile error. #

Patch Set 9 : rebase #

Patch Set 10 : rebase #

Total comments: 59

Patch Set 11 : Make requested changes. #

Total comments: 34

Patch Set 12 : Make requested changes. #

Total comments: 12

Patch Set 13 : Make requested changes. #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+822 lines, -0 lines) Patch
M net/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +9 lines, -0 lines 0 comments Download
A net/reporting/reporting_cache.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +166 lines, -0 lines 9 comments Download
A net/reporting/reporting_cache.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +170 lines, -0 lines 0 comments Download
A net/reporting/reporting_cache_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +253 lines, -0 lines 0 comments Download
A net/reporting/reporting_client.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +53 lines, -0 lines 0 comments Download
A net/reporting/reporting_client.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +28 lines, -0 lines 0 comments Download
A net/reporting/reporting_report.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +57 lines, -0 lines 0 comments Download
A net/reporting/reporting_report.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +31 lines, -0 lines 0 comments Download
A net/reporting/reporting_test_util.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +27 lines, -0 lines 0 comments Download
A net/reporting/reporting_test_util.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +28 lines, -0 lines 0 comments Download
Trybot results:  ios-simulator-xcode-clang   ios-simulator   linux_android_rel_ng   chromeos_amd64-generic_chromium_compile_only_ng   android_clang_dbg_recipe   win_chromium_compile_dbg_ng   linux_chromium_rel_ng   linux_chromium_asan_rel_ng   android_n5x_swarming_rel   mac_chromium_compile_dbg_ng   cast_shell_linux   chromium_presubmit   win_chromium_x64_rel_ng   ios-device-xcode-clang   mac_chromium_rel_ng   android_cronet   linux_chromium_compile_dbg_ng   linux_chromium_chromeos_rel_ng   chromeos_daisy_chromium_compile_only_ng   linux_chromium_chromeos_ozone_rel_ng   cast_shell_android   ios-simulator-xcode-clang   win_chromium_rel_ng   win_clang   linux_chromium_tsan_rel_ng   ios-device   android_compile_dbg   android_arm64_dbg_recipe   win_chromium_x64_rel_ng   win_clang   win_chromium_compile_dbg_ng   ios-simulator-xcode-clang   android_cronet   mac_chromium_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_chromeos_ozone_rel_ng   chromeos_amd64-generic_chromium_compile_only_ng   ios-device-xcode-clang   win_chromium_x64_rel_ng   linux_chromium_rel_ng   win_chromium_rel_ng   chromeos_daisy_chromium_compile_only_ng   android_n5x_swarming_rel   ios-device   ios-simulator   linux_chromium_compile_dbg_ng   android_compile_dbg   mac_chromium_compile_dbg_ng   cast_shell_android   android_arm64_dbg_recipe   linux_chromium_tsan_rel_ng   android_clang_dbg_recipe   chromium_presubmit   cast_shell_linux   win_clang   linux_chromium_asan_rel_ng   linux_android_rel_ng 
Commit queue not available (can’t edit this change).

Messages

Total messages: 77 (57 generated)
Randy Smith (Not in Mondays)
Initial design thoughts. https://codereview.chromium.org/2708503002/diff/1/components/reporting/reporting_cache.h File components/reporting/reporting_cache.h (right): https://codereview.chromium.org/2708503002/diff/1/components/reporting/reporting_cache.h#newcode47 components/reporting/reporting_cache.h:47: const Report* get() { return weak_pointer_.get(); ...
2 months, 1 week ago (2017-02-21 16:33:02 UTC) #2
Julia Tuttle
PTAL, rdsmith. https://codereview.chromium.org/2708503002/diff/1/components/reporting/reporting_cache.h File components/reporting/reporting_cache.h (right): https://codereview.chromium.org/2708503002/diff/1/components/reporting/reporting_cache.h#newcode47 components/reporting/reporting_cache.h:47: const Report* get() { return weak_pointer_.get(); } ...
2 months ago (2017-02-23 20:01:38 UTC) #5
Randy Smith (Not in Mondays)
https://codereview.chromium.org/2708503002/diff/1/components/reporting/reporting_cache.h File components/reporting/reporting_cache.h (right): https://codereview.chromium.org/2708503002/diff/1/components/reporting/reporting_cache.h#newcode47 components/reporting/reporting_cache.h:47: const Report* get() { return weak_pointer_.get(); } On 2017/02/23 ...
2 months ago (2017-02-24 21:26:59 UTC) #8
Julia Tuttle
PTAL, shivanisha.
2 months ago (2017-02-27 16:22:11 UTC) #11
jkarlin
Looking good. First pass comments. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reporting_cache.cc File net/reporting/reporting_cache.cc (right): https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reporting_cache.cc#newcode11 net/reporting/reporting_cache.cc:11: ReportingCache::Report::Report() : attempts(0), pending(false), ...
1 month, 2 weeks ago (2017-03-15 18:54:33 UTC) #43
Julia Tuttle
PTAL, whoever's around. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reporting_cache.cc File net/reporting/reporting_cache.cc (right): https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reporting_cache.cc#newcode11 net/reporting/reporting_cache.cc:11: ReportingCache::Report::Report() : attempts(0), pending(false), doomed(false) {} ...
1 month, 2 weeks ago (2017-03-16 14:50:39 UTC) #44
jkarlin
https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reporting_cache_unittest.cc File net/reporting/reporting_cache_unittest.cc (right): https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reporting_cache_unittest.cc#newcode23 net/reporting/reporting_cache_unittest.cc:23: const GURL kUrl1("https://origin1/path"); On 2017/03/16 14:50:38, Julia Tuttle wrote: ...
1 month, 1 week ago (2017-03-17 15:07:03 UTC) #49
Julia Tuttle
PTAL, shivanisha. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reporting_cache.cc File net/reporting/reporting_cache.cc (right): https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reporting_cache.cc#newcode51 net/reporting/reporting_cache.cc:51: DCHECK(!base::ContainsKey(pending_reports_, report)); On 2017/03/17 15:07:01, jkarlin wrote: ...
1 month, 1 week ago (2017-03-21 18:41:03 UTC) #51
jkarlin
lgtm with nit https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reporting_cache_unittest.cc File net/reporting/reporting_cache_unittest.cc (right): https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reporting_cache_unittest.cc#newcode34 net/reporting/reporting_cache_unittest.cc:34: const base::TimeTicks kExpires2 = kExpires1 + ...
1 month, 1 week ago (2017-03-23 15:05:37 UTC) #57
shivanisha
Some comments on reviewing reporting_cache* https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reporting_cache.cc File net/reporting/reporting_cache.cc (right): https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reporting_cache.cc#newcode36 net/reporting/reporting_cache.cc:36: reports_[report.get()] = std::move(report); The ...
1 month, 1 week ago (2017-03-23 19:11:59 UTC) #58
Julia Tuttle
PTAL, shivanisha. https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reporting_cache.cc File net/reporting/reporting_cache.cc (right): https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reporting_cache.cc#newcode36 net/reporting/reporting_cache.cc:36: reports_[report.get()] = std::move(report); On 2017/03/23 19:11:59, shivanisha ...
1 month, 1 week ago (2017-03-23 19:24:04 UTC) #61
shivanisha
On 2017/03/23 at 19:24:04, juliatuttle wrote: > PTAL, shivanisha. > > https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reporting_cache.cc > File net/reporting/reporting_cache.cc ...
1 month ago (2017-03-27 14:27:40 UTC) #64
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/2708503002/240001
1 month ago (2017-03-27 15:08:15 UTC) #67
Julia Tuttle
PTAL, rdsmith.
1 month ago (2017-03-27 15:13:33 UTC) #69
Randy Smith (Not in Mondays)
Initial comments. I'm going to focus my comments on the interface, and rely on Shivani's ...
1 month ago (2017-03-27 16:13:25 UTC) #70
commit-bot: I haz the power
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/5868433300e998d6aee22dd603d95c5e42830776
1 month ago (2017-03-27 16:23:38 UTC) #73
Julia Tuttle
On 2017/03/27 16:23:38, commit-bot: I haz the power wrote: > Committed patchset #13 (id:240001) as ...
1 month ago (2017-03-27 16:39:03 UTC) #74
Julia Tuttle
These are apparently going in a followup CL! https://codereview.chromium.org/2708503002/diff/240001/net/reporting/reporting_cache.h File net/reporting/reporting_cache.h (right): https://codereview.chromium.org/2708503002/diff/240001/net/reporting/reporting_cache.h#newcode33 net/reporting/reporting_cache.h:33: // ...
1 month ago (2017-03-27 16:39:56 UTC) #75
jkarlin
On 2017/03/27 16:39:03, Julia Tuttle wrote: > On 2017/03/27 16:23:38, commit-bot: I haz the power ...
1 month ago (2017-03-27 16:40:02 UTC) #76
Randy Smith (Not in Mondays)
1 month ago (2017-03-28 15:30:09 UTC) #77
Message was sent while issue was closed.
https://codereview.chromium.org/2708503002/diff/240001/net/reporting/reportin...
File net/reporting/reporting_cache.h (right):

https://codereview.chromium.org/2708503002/diff/240001/net/reporting/reportin...
net/reporting/reporting_cache.h:53: // flag has been set to true using
|SetReportsPending|. Does not return
On 2017/03/27 16:39:56, Julia Tuttle wrote:
> On 2017/03/27 16:13:25, Randy Smith (Not in Mondays) wrote:
> > This continues to bother me; handing out raw pointers with lifetime
guarantees
> > is dangerous, and more dangerous if those lifetime guarantees are
complicated.
> 
> > If you handed out weak pointers with the same guarantee and DCHECK'd them
> > everywhere they were supposed to be valid I'd be happier.  Would that change
> be
> > a large one?
> 
> It wouldn't work great for upcoming CLs -- I need the reports to be around to
> log metrics, so I can't actually remove them and let the weak pointer go
> dangling.

I wasn't suggesting changing the lifetime behavior, just not vending raw
pointers.

> Do you want the weak pointer just as a precaution against crashes, or as the
> actual lifetime management?

Precaution against use-after-frees; it would turn a use-after-free into a crash.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46