Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2708503002:
Reporting: Implement cache. (Closed)
|
Created:
3 years, 10 months ago by Julia Tuttle Modified:
3 years, 8 months ago CC:
chromium-reviews, Randy Smith (Not in Mondays) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReporting: 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
Messages
Total messages: 77 (57 generated)
rdsmith@chromium.org changed reviewers: + rdsmith@chromium.org
Initial design thoughts. https://codereview.chromium.org/2708503002/diff/1/components/reporting/report... File components/reporting/reporting_cache.h (right): https://codereview.chromium.org/2708503002/diff/1/components/reporting/report... components/reporting/reporting_cache.h:47: const Report* get() { return weak_pointer_.get(); } This has basically the identical interface to a weak pointer, so I don't think we gain anything by creating it (all the reasonable arguments against weak pointer apply to it, in addition to the one that it's different, so will pose a roadblock for people to reason about). I think it's better to use WeakPtr directly. I had imagined when we were talking that this class would be provided to the ReportingCache to lookup the report, and the ReportingCache would return a weak pointer or nullptr. But thinking about it now I'm not sure why that's better; the calling code still has to handle the race.6 It occurs to me that your idea of having a ReportingHandle reference a group of reports might be a cleaner interface, in that when that ReportingHandle is handed back to the cache, the result will *always* be a list of reports that need to be iterated through. The size of that list might decrease over time, but there's no conditional in the handling code. Having said all that, I'm ok with just using a WeakPtr<> interface. We've done our due diligence looking for alternatives :-} :-|. https://codereview.chromium.org/2708503002/diff/1/components/reporting/report... components/reporting/reporting_cache.h:55: }; nit: Worth a comment that it's intentionally copyable. https://codereview.chromium.org/2708503002/diff/1/components/reporting/report... components/reporting/reporting_cache.h:86: void RemoveReports(const std::set<ReportHandle>& reports); Given that this interface is always to std::set<ReportHandle>, it might make sense to go with your original idea and make a ReportHandle reference a set of reports; it'd make the API simpler. https://codereview.chromium.org/2708503002/diff/1/components/reporting/report... components/reporting/reporting_cache.h:89: std::set<const Client*> GetClients(); Hmmm. Consider |void GetClients(std::set<const Client*>*)|? I think this is going to result in some extra copying and deletion up and down the stack.
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, rdsmith. https://codereview.chromium.org/2708503002/diff/1/components/reporting/report... File components/reporting/reporting_cache.h (right): https://codereview.chromium.org/2708503002/diff/1/components/reporting/report... components/reporting/reporting_cache.h:47: const Report* get() { return weak_pointer_.get(); } On 2017/02/21 16:33:02, Randy Smith (Not in Mondays) wrote: > This has basically the identical interface to a weak pointer, so I don't think > we gain anything by creating it (all the reasonable arguments against weak > pointer apply to it, in addition to the one that it's different, so will pose a > roadblock for people to reason about). I think it's better to use WeakPtr > directly. > > I had imagined when we were talking that this class would be provided to the > ReportingCache to lookup the report, and the ReportingCache would return a weak > pointer or nullptr. But thinking about it now I'm not sure why that's better; > the calling code still has to handle the race.6 > > It occurs to me that your idea of having a ReportingHandle reference a group of > reports might be a cleaner interface, in that when that ReportingHandle is > handed back to the cache, the result will *always* be a list of reports that > need to be iterated through. The size of that list might decrease over time, > but there's no conditional in the handling code. > > Having said all that, I'm ok with just using a WeakPtr<> interface. We've done > our due diligence looking for alternatives :-} :-|. The one thing this has over a weak pointer is that the caller can get a const pointer but the ReportingCache can get a non-const pointer. Maybe that's not worth it? An issue with the group one is that GetReports may return reports destined for two different endpoints, which will have to be passed to MarkReportsAttempted or RemoveReports separately, so there would need to be a way of saying "wrap this set of reports in a plural-ReportHandle". Given that, I'd rather stick with the singular-ReportHandle interface. https://codereview.chromium.org/2708503002/diff/1/components/reporting/report... components/reporting/reporting_cache.h:55: }; On 2017/02/21 16:33:02, Randy Smith (Not in Mondays) wrote: > nit: Worth a comment that it's intentionally copyable. Oops! Forgot to add this to the latest patchset but it's in my git locally and will go out with the next one. https://codereview.chromium.org/2708503002/diff/1/components/reporting/report... components/reporting/reporting_cache.h:86: void RemoveReports(const std::set<ReportHandle>& reports); On 2017/02/21 16:33:02, Randy Smith (Not in Mondays) wrote: > Given that this interface is always to std::set<ReportHandle>, it might make > sense to go with your original idea and make a ReportHandle reference a set of > reports; it'd make the API simpler. I've actually switched to std::vector<ReportHandle>. Honestly, I could just "using ReportHandleVector = std::vector<ReportHandle>" and be done with it; it would deal with the concern I mentioned above while making the actual prototypes prettier. https://codereview.chromium.org/2708503002/diff/1/components/reporting/report... components/reporting/reporting_cache.h:89: std::set<const Client*> GetClients(); On 2017/02/21 16:33:02, Randy Smith (Not in Mondays) wrote: > Hmmm. Consider |void GetClients(std::set<const Client*>*)|? I think this is > going to result in some extra copying and deletion up and down the stack. Done, here and elsewhere.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2708503002/diff/1/components/reporting/report... File components/reporting/reporting_cache.h (right): https://codereview.chromium.org/2708503002/diff/1/components/reporting/report... components/reporting/reporting_cache.h:47: const Report* get() { return weak_pointer_.get(); } On 2017/02/23 20:01:38, Julia Tuttle wrote: > On 2017/02/21 16:33:02, Randy Smith (Not in Mondays) wrote: > > This has basically the identical interface to a weak pointer, so I don't think > > we gain anything by creating it (all the reasonable arguments against weak > > pointer apply to it, in addition to the one that it's different, so will pose > a > > roadblock for people to reason about). I think it's better to use WeakPtr > > directly. > > > > I had imagined when we were talking that this class would be provided to the > > ReportingCache to lookup the report, and the ReportingCache would return a > weak > > pointer or nullptr. But thinking about it now I'm not sure why that's better; > > the calling code still has to handle the race.6 > > > > It occurs to me that your idea of having a ReportingHandle reference a group > of > > reports might be a cleaner interface, in that when that ReportingHandle is > > handed back to the cache, the result will *always* be a list of reports that > > need to be iterated through. The size of that list might decrease over time, > > but there's no conditional in the handling code. > > > > Having said all that, I'm ok with just using a WeakPtr<> interface. We've > done > > our due diligence looking for alternatives :-} :-|. > > The one thing this has over a weak pointer is that the caller can get a const > pointer but the ReportingCache can get a non-const pointer. Maybe that's not > worth it? Hmmm. Good point. Sure, it's worth it, just document that's what you're doing. > An issue with the group one is that GetReports may return reports destined for > two different endpoints, which will have to be passed to MarkReportsAttempted or > RemoveReports separately, so there would need to be a way of saying "wrap this > set of reports in a plural-ReportHandle". Given that, I'd rather stick with the > singular-ReportHandle interface. Also a good point. Ok. https://codereview.chromium.org/2708503002/diff/1/components/reporting/report... components/reporting/reporting_cache.h:86: void RemoveReports(const std::set<ReportHandle>& reports); On 2017/02/23 20:01:38, Julia Tuttle wrote: > On 2017/02/21 16:33:02, Randy Smith (Not in Mondays) wrote: > > Given that this interface is always to std::set<ReportHandle>, it might make > > sense to go with your original idea and make a ReportHandle reference a set of > > reports; it'd make the API simpler. > > I've actually switched to std::vector<ReportHandle>. > > Honestly, I could just "using ReportHandleVector = std::vector<ReportHandle>" > and be done with it; it would deal with the concern I mentioned above while > making the actual prototypes prettier. +1, FWIW. (I don't care a lot.)
Description was changed from ========== Reporting: Create //components/reporting and ReportingCache. BUG= ========== to ========== Reporting: Create Reporting cache. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. This implements the Reporting cache, which holds report delivery configuration and queued reports. ==========
juliatuttle@chromium.org changed reviewers: + shivanisha@chromium.org - rdsmith@chromium.org
PTAL, shivanisha.
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reporting: Create Reporting cache. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. This implements the Reporting cache, which holds report delivery configuration and queued reports. ========== to ========== Reporting: Create Cache. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. This implements the Reporting cache, which holds report delivery configuration and queued reports. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reporting: Create Cache. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. This implements the Reporting cache, which holds report delivery configuration and queued reports. ========== to ========== Reporting: Create cache. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. This implements the Reporting cache, which holds report delivery configuration and queued reports. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jkarlin@chromium.org changed reviewers: + jkarlin@chromium.org
Looking good. First pass comments. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... File net/reporting/reporting_cache.cc (right): https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:11: ReportingCache::Report::Report() : attempts(0), pending(false), doomed(false) {} nit: I prefer to put default member values in the declaration. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:47: for (auto& it : reports_) { const auto& https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:78: DCHECK_EQ(1u, reports_.count(report)); DCHECK(base::ContainsKey(reports, report)) https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:85: DCHECK_EQ(1u, reports_.count(report)); DCHECK(base::ContainsKey(reports, report)) https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:95: for (auto& it : clients_) const auto& https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:96: for (auto& jt : it.second) const auto& https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:110: for (auto& jt : it->second) { const auto& https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:122: NOTREACHED(); Best not to handle an exceptional case with a DCHECK/NOTREACHED. Either handle it, or don't and use a DCHECK. See the DCHECKS section in https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:137: DCHECK_EQ(1u, clients_[client->origin].count(client->endpoint)); DCHECK(base::ContainsKey(clients_[client->origin], client->endpoint)) https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:144: const GURL& endpoint) { DCHECK that the origin exists in clients? https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... File net/reporting/reporting_cache.h (right): https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:21: class NET_EXPORT ReportingCache { Class needs a comment. E.g., call out what in the spec this refers to (The Storage Concept) and what its purpose is (e.g., memory-only storage of reports and clients). https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:24: class Report { Seems like this should be a struct, given that all the members are public and don't have _ suffixes https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:26: Report(); How about a constructor that takes all of the params? https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:42: base::TimeTicks queued; queued_time? https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:46: int attempts; failed_attempts? https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:53: // active delivery attempt. (Should never be set unless |pending| also is.) Perhaps replace the parens with, "If true, |pending| must also be true." https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:55: Don't forget to add a CL to keep track of the memory requirements of net/reporting/. Talk to Helen about how to do that. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:85: base::TimeTicks expires; Perhaps rename to ttl to match the spec? https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:97: void AddReport(const GURL& url, Why not AddReport(std::unique_ptr<Report> report)? Otherwise this is more of a CreateAndAddReport. It looks like the implementation just makes a new Report on the heap anyway so a unique_ptr should work fine. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:108: void GetReports(std::vector<const Report*>* reports_out) const; Specify that reports_out gets cleared. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:131: // trigger a NOTREACHED() and return without setting the client. I'd leave it at |endpoint| must use a cryptographic scheme. Don't reveal the implementation details of what will happen if it's not. Those may change and the caller shouldn't need to know or care. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:139: void GetClients(std::vector<const Client*>* clients_out) const; Specify that clients_out gets cleared. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:175: // Map from raw pointer to unique_ptr. whitespace between comment and previous line https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... File net/reporting/reporting_cache_unittest.cc (right): https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:23: const GURL kUrl1("https://origin1/path"); Globals need to be PODs or arrays/structs of PODs, here and below. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:105: cache_.SetClient(kOrigin1, kEndpoint1, false, kGroup1, kExpires1); , false /* sub-domains */, kGroup1, ... https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:115: cache_.SetClient(kOrigin1, kEndpoint1, true, kGroup2, kExpires2); ditto with the bool coomment https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:115: cache_.SetClient(kOrigin1, kEndpoint1, true, kGroup2, kExpires2); Add a comment that this replaces the origin client with different domains, group, and expiration time https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:125: std::vector<const Client*> clients; std::vector<const Client*> clients {client}; https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:134: cache_.SetClient(kOrigin1, kEndpoint1, false, kGroup1, kExpires1); In fact, you use this enough, why not make that bool into an enum?
PTAL, whoever's around. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... File net/reporting/reporting_cache.cc (right): https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:11: ReportingCache::Report::Report() : attempts(0), pending(false), doomed(false) {} On 2017/03/15 18:54:31, jkarlin wrote: > nit: I prefer to put default member values in the declaration. Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:47: for (auto& it : reports_) { On 2017/03/15 18:54:31, jkarlin wrote: > const auto& Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:78: DCHECK_EQ(1u, reports_.count(report)); On 2017/03/15 18:54:31, jkarlin wrote: > DCHECK(base::ContainsKey(reports, report)) Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:85: DCHECK_EQ(1u, reports_.count(report)); On 2017/03/15 18:54:31, jkarlin wrote: > DCHECK(base::ContainsKey(reports, report)) Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:95: for (auto& it : clients_) On 2017/03/15 18:54:31, jkarlin wrote: > const auto& Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:96: for (auto& jt : it.second) On 2017/03/15 18:54:31, jkarlin wrote: > const auto& Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:110: for (auto& jt : it->second) { On 2017/03/15 18:54:32, jkarlin wrote: > const auto& Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:122: NOTREACHED(); On 2017/03/15 18:54:32, jkarlin wrote: > Best not to handle an exceptional case with a DCHECK/NOTREACHED. Either handle > it, or don't and use a DCHECK. > > See the DCHECKS section in > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Hrm. My feeling here is that: 1. This shouldn't happen. 2. If it does, we still shouldn't store it in the cache. I'll go for #2, then. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:137: DCHECK_EQ(1u, clients_[client->origin].count(client->endpoint)); On 2017/03/15 18:54:31, jkarlin wrote: > DCHECK(base::ContainsKey(clients_[client->origin], client->endpoint)) Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.cc:144: const GURL& endpoint) { On 2017/03/15 18:54:31, jkarlin wrote: > DCHECK that the origin exists in clients? Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... File net/reporting/reporting_cache.h (right): https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:21: class NET_EXPORT ReportingCache { On 2017/03/15 18:54:32, jkarlin wrote: > Class needs a comment. E.g., call out what in the spec this refers to (The > Storage Concept) and what its purpose is (e.g., memory-only storage of reports > and clients). Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:24: class Report { On 2017/03/15 18:54:32, jkarlin wrote: > Seems like this should be a struct, given that all the members are public and > don't have _ suffixes Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:26: Report(); On 2017/03/15 18:54:32, jkarlin wrote: > How about a constructor that takes all of the params? Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:42: base::TimeTicks queued; On 2017/03/15 18:54:32, jkarlin wrote: > queued_time? Already says "time" in the type name :) https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:46: int attempts; On 2017/03/15 18:54:32, jkarlin wrote: > failed_attempts? The *last* one might not be failed. Also, the spec calls it "attempts". https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:53: // active delivery attempt. (Should never be set unless |pending| also is.) On 2017/03/15 18:54:32, jkarlin wrote: > Perhaps replace the parens with, "If true, |pending| must also be true." Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:55: On 2017/03/15 18:54:32, jkarlin wrote: > Don't forget to add a CL to keep track of the memory requirements of > net/reporting/. Talk to Helen about how to do that. Wilco. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:85: base::TimeTicks expires; On 2017/03/15 18:54:32, jkarlin wrote: > Perhaps rename to ttl to match the spec? The spec stores "creation" and "ttl", but never uses them individually, just "is creation + ttl < now" to check if it's expired. It would be a waste of a field to store both. Calling the expiration time itself "ttl" isn't quite right; it's a fixed point in time, not the total or remaining time before expiration. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:97: void AddReport(const GURL& url, On 2017/03/15 18:54:32, jkarlin wrote: > Why not AddReport(std::unique_ptr<Report> report)? Otherwise this is more of a > CreateAndAddReport. It looks like the implementation just makes a new Report on > the heap anyway so a unique_ptr should work fine. I'd rather save callers the hassle of assembling it, and keep the constructor to Report private, to clarify that the cache owns them all. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:108: void GetReports(std::vector<const Report*>* reports_out) const; On 2017/03/15 18:54:32, jkarlin wrote: > Specify that reports_out gets cleared. Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:131: // trigger a NOTREACHED() and return without setting the client. On 2017/03/15 18:54:32, jkarlin wrote: > I'd leave it at |endpoint| must use a cryptographic scheme. Don't reveal the > implementation details of what will happen if it's not. Those may change and the > caller shouldn't need to know or care. Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:139: void GetClients(std::vector<const Client*>* clients_out) const; On 2017/03/15 18:54:32, jkarlin wrote: > Specify that clients_out gets cleared. Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache.h:175: // Map from raw pointer to unique_ptr. On 2017/03/15 18:54:32, jkarlin wrote: > whitespace between comment and previous line Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... File net/reporting/reporting_cache_unittest.cc (right): https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:23: const GURL kUrl1("https://origin1/path"); On 2017/03/15 18:54:32, jkarlin wrote: > Globals need to be PODs or arrays/structs of PODs, here and below. > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables But it's a unittest x.x I suppose static members are just as bad as globals? I guess I could make them tiny getter functions? https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:105: cache_.SetClient(kOrigin1, kEndpoint1, false, kGroup1, kExpires1); On 2017/03/15 18:54:32, jkarlin wrote: > , false /* sub-domains */, kGroup1, ... Obsolete. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:115: cache_.SetClient(kOrigin1, kEndpoint1, true, kGroup2, kExpires2); On 2017/03/15 18:54:32, jkarlin wrote: > Add a comment that this replaces the origin client with different domains, > group, and expiration time Done. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:115: cache_.SetClient(kOrigin1, kEndpoint1, true, kGroup2, kExpires2); On 2017/03/15 18:54:33, jkarlin wrote: > ditto with the bool coomment Obsolete. https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:125: std::vector<const Client*> clients; On 2017/03/15 18:54:33, jkarlin wrote: > std::vector<const Client*> clients {client}; Whoa. Better yet, all three lines as: cache_.RemoveClients(std::vector<const ReportingClient*> { client }); https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:134: cache_.SetClient(kOrigin1, kEndpoint1, false, kGroup1, kExpires1); On 2017/03/15 18:54:32, jkarlin wrote: > In fact, you use this enough, why not make that bool into an enum? You are irritatingly correct. Done.
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... File net/reporting/reporting_cache_unittest.cc (right): https://codereview.chromium.org/2708503002/diff/180001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:23: const GURL kUrl1("https://origin1/path"); On 2017/03/16 14:50:38, Julia Tuttle wrote: > On 2017/03/15 18:54:32, jkarlin wrote: > > Globals need to be PODs or arrays/structs of PODs, here and below. > > > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > But it's a unittest x.x > > I suppose static members are just as bad as globals? I guess I could make them > tiny getter functions? Yes even tests :p It's important that test code is up to the same standard as production code. People look to test code for examples. Make the strings into const char []'s and then make the GURLs const members of your test class and access them from your test class. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... File net/reporting/reporting_cache.cc (right): https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.cc:51: DCHECK(!base::ContainsKey(pending_reports_, report)); As it's unclear that this would be bad to the caller, you should specify in the declaration that you shouldn't set a report as pending that's already pending. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.cc:85: reports_.erase(report); Also remove from doomed_reports_? Seems like we should have a test for this. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.cc:107: for (const auto& jt : it->second) { prefer s/jt/endpoint_and_client/ then you can say endpoint_and_client.second and it's clear what it's referring to. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.cc:118: if (!endpoint.SchemeIsCryptographic()) You could LOG() this, or return an error that's required to be checked by the caller. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... File net/reporting/reporting_cache.h (right): https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.h:133: // Map from origin to map from endpoint to unique_ptr-to-Client. This comment is just spelling out the type and seems unnecessary. Instead, describe its purpose. E.g., Stores all of the clients, arranged by origin and endpoint. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.h:134: std::map<url::Origin, std::map<GURL, std::unique_ptr<ReportingClient>>> Prefer unordered_map, unless you need the ordering? https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.h:138: std::map<const ReportingReport*, std::unique_ptr<ReportingReport>> reports_; Ditto. Perhaps: owns all active reports, indexed by raw pointer. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.h:138: std::map<const ReportingReport*, std::unique_ptr<ReportingReport>> reports_; prefer unordered_map, unless you need the ordering? https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.h:142: std::set<const ReportingReport*> pending_reports_; prefer unordered_set https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.h:146: std::set<const ReportingReport*> doomed_reports_; prefer unordered_set https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... File net/reporting/reporting_cache_unittest.cc (right): https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:32: const base::TimeTicks kExpires2(kExpires1 + base::TimeDelta::FromDays(7)); Note that the above still need to be POD https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... File net/reporting/reporting_client.h (right): https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_client.h:20: enum Subdomains { EXCLUDE_SUBDOMAINS = 0, INCLUDE_SUBDOMAINS = 1 }; Prefer 'enum class'. Then you can rename the values to EXCLUDE and INCLUDE since they'll always be prefixed by Subdomains:: https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_client.h:22: ReportingClient(); Any reason to allow the empty constructor? https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_client.h:39: Subdomains subdomains; Needs a default value. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_client.h:47: // Copy and assign allowed. Probably best to declare the copy and assignment operations and = default them. Though I can't find anywhere in the style guide that strictly requires that. So I guess this is optional. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... File net/reporting/reporting_report.h (right): https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_report.h:24: ReportingReport(); Any reason to allow this constructor? https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... File net/reporting/reporting_test_util.cc (right): https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_test_util.cc:20: cache->GetClients(&clients); Is a GetClientsForOriginAndEndpoint in order? Or is that only useful for testing? How about GetClientsForOrigin?
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
PTAL, shivanisha. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... File net/reporting/reporting_cache.cc (right): https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.cc:51: DCHECK(!base::ContainsKey(pending_reports_, report)); On 2017/03/17 15:07:01, jkarlin wrote: > As it's unclear that this would be bad to the caller, you should specify in the > declaration that you shouldn't set a report as pending that's already pending. Done. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.cc:85: reports_.erase(report); On 2017/03/17 15:07:01, jkarlin wrote: > Also remove from doomed_reports_? Seems like we should have a test for this. If it's not in pending_reports_, it can't be in doomed_reports_. I'll add a DCHECK. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.cc:107: for (const auto& jt : it->second) { On 2017/03/17 15:07:01, jkarlin wrote: > prefer s/jt/endpoint_and_client/ then you can say endpoint_and_client.second and > it's clear what it's referring to. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.cc:118: if (!endpoint.SchemeIsCryptographic()) On 2017/03/17 15:07:01, jkarlin wrote: > You could LOG() this, or return an error that's required to be checked by the > caller. I'm expecting the caller to be checking it already. This should probably actually be just a DCHECK. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... File net/reporting/reporting_cache.h (right): https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.h:133: // Map from origin to map from endpoint to unique_ptr-to-Client. On 2017/03/17 15:07:02, jkarlin wrote: > This comment is just spelling out the type and seems unnecessary. Instead, > describe its purpose. E.g., Stores all of the clients, arranged by origin and > endpoint. Done. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.h:134: std::map<url::Origin, std::map<GURL, std::unique_ptr<ReportingClient>>> On 2017/03/17 15:07:02, jkarlin wrote: > Prefer unordered_map, unless you need the ordering? I don't, but GURL and url::Origin don't have hash functions. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.h:138: std::map<const ReportingReport*, std::unique_ptr<ReportingReport>> reports_; On 2017/03/17 15:07:02, jkarlin wrote: > prefer unordered_map, unless you need the ordering? Done. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.h:138: std::map<const ReportingReport*, std::unique_ptr<ReportingReport>> reports_; On 2017/03/17 15:07:02, jkarlin wrote: > Ditto. Perhaps: owns all active reports, indexed by raw pointer. Done. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.h:142: std::set<const ReportingReport*> pending_reports_; On 2017/03/17 15:07:02, jkarlin wrote: > prefer unordered_set Done. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache.h:146: std::set<const ReportingReport*> doomed_reports_; On 2017/03/17 15:07:02, jkarlin wrote: > prefer unordered_set Done. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... File net/reporting/reporting_cache_unittest.cc (right): https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:32: const base::TimeTicks kExpires2(kExpires1 + base::TimeDelta::FromDays(7)); On 2017/03/17 15:07:03, jkarlin wrote: > Note that the above still need to be POD *mutter, grumble, move into ReportingCacheTest* (I don't think it's worth declaring them as const char[] kWhatever and then going right on to declare GURL kWhatever(kWhatever) or something.) https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... File net/reporting/reporting_client.h (right): https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_client.h:20: enum Subdomains { EXCLUDE_SUBDOMAINS = 0, INCLUDE_SUBDOMAINS = 1 }; On 2017/03/17 15:07:03, jkarlin wrote: > Prefer 'enum class'. Then you can rename the values to EXCLUDE and INCLUDE since > they'll always be prefixed by Subdomains:: ...huh, I had no idea that was a thing. Cool. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_client.h:22: ReportingClient(); On 2017/03/17 15:07:03, jkarlin wrote: > Any reason to allow the empty constructor? Hm, not anymore. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_client.h:39: Subdomains subdomains; On 2017/03/17 15:07:03, jkarlin wrote: > Needs a default value. Done. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_client.h:47: // Copy and assign allowed. On 2017/03/17 15:07:03, jkarlin wrote: > Probably best to declare the copy and assignment operations and = default them. > Though I can't find anywhere in the style guide that strictly requires that. So > I guess this is optional. Actually I don't think I need them anymore. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... File net/reporting/reporting_report.h (right): https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_report.h:24: ReportingReport(); On 2017/03/17 15:07:03, jkarlin wrote: > Any reason to allow this constructor? Done. https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... File net/reporting/reporting_test_util.cc (right): https://codereview.chromium.org/2708503002/diff/200001/net/reporting/reportin... net/reporting/reporting_test_util.cc:20: cache->GetClients(&clients); On 2017/03/17 15:07:03, jkarlin wrote: > Is a GetClientsForOriginAndEndpoint in order? Or is that only useful for > testing? How about GetClientsForOrigin? It's only useful for testing; I'll switch to GetClientsForOrigin.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Reporting: Create cache. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. This implements the Reporting cache, which holds report delivery configuration and queued reports. ========== to ========== 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. ==========
Description was changed from ========== 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. ========== to ========== 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 ==========
lgtm with nit https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... File net/reporting/reporting_cache_unittest.cc (right): https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:34: const base::TimeTicks kExpires2 = kExpires1 + base::TimeDelta::FromDays(7); The above members need _ suffixes.
Some comments on reviewing reporting_cache* https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... File net/reporting/reporting_cache.cc (right): https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... net/reporting/reporting_cache.cc:36: reports_[report.get()] = std::move(report); The dcheck can be done on the return value of insert. Should be more efficient than 2 operations. auto return_val = reports_.insert(std::pair<const ReportingReport*, std::unique_ptr<ReportingReport>(report.get(), std::move(report))); DCHECK_EQ(return_val.second, true); https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... net/reporting/reporting_cache.cc:52: pending_reports_.insert(report); The dcheck can be done on the return value of insert. https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... net/reporting/reporting_cache.cc:62: pending_reports_.erase(report); dcheck can be done on the return value of erase. https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... File net/reporting/reporting_cache.h (right): https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... net/reporting/reporting_cache.h:33: // endpoint failures/retry-after are tracked elsewhere. "are tracked elsewhere" => "are tracked in <name of class>" https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... net/reporting/reporting_cache.h:43: // |Report|. || are used for argument names so may be just say "relevant fields in the report to be created"
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, shivanisha. https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... File net/reporting/reporting_cache.cc (right): https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... net/reporting/reporting_cache.cc:36: reports_[report.get()] = std::move(report); On 2017/03/23 19:11:59, shivanisha wrote: > The dcheck can be done on the return value of insert. Should be more efficient > than 2 operations. > auto return_val = > reports_.insert(std::pair<const ReportingReport*, > std::unique_ptr<ReportingReport>(report.get(), std::move(report))); > DCHECK_EQ(return_val.second, true); Done. https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... net/reporting/reporting_cache.cc:52: pending_reports_.insert(report); On 2017/03/23 19:11:59, shivanisha wrote: > The dcheck can be done on the return value of insert. Done. https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... net/reporting/reporting_cache.cc:62: pending_reports_.erase(report); On 2017/03/23 19:11:59, shivanisha wrote: > dcheck can be done on the return value of erase. Done. https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... File net/reporting/reporting_cache.h (right): https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... net/reporting/reporting_cache.h:33: // endpoint failures/retry-after are tracked elsewhere. On 2017/03/23 19:11:59, shivanisha wrote: > "are tracked elsewhere" => "are tracked in <name of class>" Done. https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... net/reporting/reporting_cache.h:43: // |Report|. On 2017/03/23 19:11:59, shivanisha wrote: > || are used for argument names so may be just say "relevant fields in the report > to be created" Done. https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... File net/reporting/reporting_cache_unittest.cc (right): https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... net/reporting/reporting_cache_unittest.cc:34: const base::TimeTicks kExpires2 = kExpires1 + base::TimeDelta::FromDays(7); On 2017/03/23 15:05:37, jkarlin wrote: > The above members need _ suffixes. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/03/23 at 19:24:04, juliatuttle wrote: > PTAL, shivanisha. > > https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... > File net/reporting/reporting_cache.cc (right): > > https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... > net/reporting/reporting_cache.cc:36: reports_[report.get()] = std::move(report); > On 2017/03/23 19:11:59, shivanisha wrote: > > The dcheck can be done on the return value of insert. Should be more efficient > > than 2 operations. > > auto return_val = > > reports_.insert(std::pair<const ReportingReport*, > > std::unique_ptr<ReportingReport>(report.get(), std::move(report))); > > DCHECK_EQ(return_val.second, true); > > Done. > > https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... > net/reporting/reporting_cache.cc:52: pending_reports_.insert(report); > On 2017/03/23 19:11:59, shivanisha wrote: > > The dcheck can be done on the return value of insert. > > Done. > > https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... > net/reporting/reporting_cache.cc:62: pending_reports_.erase(report); > On 2017/03/23 19:11:59, shivanisha wrote: > > dcheck can be done on the return value of erase. > > Done. > > https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... > File net/reporting/reporting_cache.h (right): > > https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... > net/reporting/reporting_cache.h:33: // endpoint failures/retry-after are tracked elsewhere. > On 2017/03/23 19:11:59, shivanisha wrote: > > "are tracked elsewhere" => "are tracked in <name of class>" > > Done. > > https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... > net/reporting/reporting_cache.h:43: // |Report|. > On 2017/03/23 19:11:59, shivanisha wrote: > > || are used for argument names so may be just say "relevant fields in the report > > to be created" > > Done. > > https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... > File net/reporting/reporting_cache_unittest.cc (right): > > https://codereview.chromium.org/2708503002/diff/220001/net/reporting/reportin... > net/reporting/reporting_cache_unittest.cc:34: const base::TimeTicks kExpires2 = kExpires1 + base::TimeDelta::FromDays(7); > On 2017/03/23 15:05:37, jkarlin wrote: > > The above members need _ suffixes. > > Done. lgtm
The CQ bit was checked by juliatuttle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2708503002/#ps240001 (title: "Make requested changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
juliatuttle@chromium.org changed reviewers: + rdsmith@chromium.org
PTAL, rdsmith.
Initial comments. I'm going to focus my comments on the interface, and rely on Shivani's review for the implementation and testing, unless you or she request that I specifically look at implementation or testing code. 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:33: // endpoint failures/retry-after are tracked in ReportingEndpointManager. Willing to have a README.md file with a quick sketch of the goals and a link to the spec and design doc? (This is based on the spec reference above, and a concern that future readers of the code would have to do unreasonable archaeology back to this CL to find the spec link.) https://codereview.chromium.org/2708503002/diff/240001/net/reporting/reportin... net/reporting/reporting_cache.h:33: // endpoint failures/retry-after are tracked in ReportingEndpointManager. Request: Go into a bit more detail in this comment about the semantics of pending reports. 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 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? https://codereview.chromium.org/2708503002/diff/240001/net/reporting/reportin... net/reporting/reporting_cache.h:100: // have been made to |SetClient| or |RemoveEndpoint| in between. As implied above, this type of guarantee bothers me, because it requires all consumers of this routine to implicitly be aware of all other uses of the cache. Based on planned consumer usage, can you make the restriction stricter (i.e. only valid if used synchronously--do not store), or does that bork a planned use?
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1490627277337850,
"parent_rev": "9cd48d5cbc285e39f3a6401ecd40134e0f2d5bc7", "commit_rev":
"5868433300e998d6aee22dd603d95c5e42830776"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/5868433300e998d6aee22dd603d9... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/5868433300e998d6aee22dd603d9...
Message was sent while issue was closed.
On 2017/03/27 16:23:38, commit-bot: I haz the power wrote: > Committed patchset #13 (id:240001) as > https://chromium.googlesource.com/chromium/src/+/5868433300e998d6aee22dd603d9... ...wait, how did that work? Don't I need a second //net OWNER?
Message was sent while issue was closed.
These are apparently going in a followup CL! 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:33: // endpoint failures/retry-after are tracked in ReportingEndpointManager. On 2017/03/27 16:13:25, Randy Smith (Not in Mondays) wrote: > Request: Go into a bit more detail in this comment about the semantics of > pending reports. Done. https://codereview.chromium.org/2708503002/diff/240001/net/reporting/reportin... net/reporting/reporting_cache.h:33: // endpoint failures/retry-after are tracked in ReportingEndpointManager. On 2017/03/27 16:13:25, Randy Smith (Not in Mondays) wrote: > Willing to have a README.md file with a quick sketch of the goals and a link to > the spec and design doc? (This is based on the spec reference above, and a > concern that future readers of the code would have to do unreasonable > archaeology back to this CL to find the spec link.) Yeah, sure, sounds great. 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: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. Do you want the weak pointer just as a precaution against crashes, or as the actual lifetime management? https://codereview.chromium.org/2708503002/diff/240001/net/reporting/reportin... net/reporting/reporting_cache.h:100: // have been made to |SetClient| or |RemoveEndpoint| in between. On 2017/03/27 16:13:25, Randy Smith (Not in Mondays) wrote: > As implied above, this type of guarantee bothers me, because it requires all > consumers of this routine to implicitly be aware of all other uses of the cache. > Based on planned consumer usage, can you make the restriction stricter (i.e. > only valid if used synchronously--do not store), or does that bork a planned > use? Oh, sure, only used synchronously would be great.
Message was sent while issue was closed.
On 2017/03/27 16:39:03, Julia Tuttle wrote: > On 2017/03/27 16:23:38, commit-bot: I haz the power wrote: > > Committed patchset #13 (id:240001) as > > > https://chromium.googlesource.com/chromium/src/+/5868433300e998d6aee22dd603d9... > > ...wait, how did that work? Don't I need a second //net OWNER? No, if you're an owner you just need a committer to l-g-t-m and you can land.
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. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
