Commit d3967817 authored by Lily Chen's avatar Lily Chen Committed by Commit Bot

Reporting: Separately observe reports and clients in ReportingObserver

The ReportingCache stores unsent and pending reports, as well as client
configurations. Previously, observers were notified upon any change to
any of the cached data. This change differentiates changes to the cached
reports from changes to the cached clients, and notifies observers
appropriately.

This will make it easier to add a ReportingStore class which will
observe the ReportingCache and persist the changed data to disk.

Also renames what was originally ReportingObserver to
ReportingCacheObserver to avoid confusion with the JavaScript
ReportingObserver interface
(https://w3c.github.io/reporting/#interface-reporting-observer).

Bug: none
Change-Id: I4afd530b703f04da0bcc53e8ad2d615cb4ed0238
Reviewed-on: https://chromium-review.googlesource.com/c/1362191Reviewed-by: default avatarMisha Efimov <mef@chromium.org>
Commit-Queue: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614799}
parent 35a89996
......@@ -1772,6 +1772,8 @@ component("net") {
"reporting/reporting_browsing_data_remover.h",
"reporting/reporting_cache.cc",
"reporting/reporting_cache.h",
"reporting/reporting_cache_observer.cc",
"reporting/reporting_cache_observer.h",
"reporting/reporting_client.cc",
"reporting/reporting_client.h",
"reporting/reporting_context.cc",
......@@ -1788,8 +1790,6 @@ component("net") {
"reporting/reporting_header_parser.h",
"reporting/reporting_network_change_observer.cc",
"reporting/reporting_network_change_observer.h",
"reporting/reporting_observer.cc",
"reporting/reporting_observer.h",
"reporting/reporting_policy.cc",
"reporting/reporting_policy.h",
"reporting/reporting_report.cc",
......
......@@ -97,7 +97,7 @@ class ReportingCacheImpl : public ReportingCache {
RemoveReportInternal(to_evict);
}
context_->NotifyCacheUpdated();
context_->NotifyCachedReportsUpdated();
}
void GetReports(
......@@ -195,7 +195,7 @@ class ReportingCacheImpl : public ReportingCache {
reports_[report]->attempts++;
}
context_->NotifyCacheUpdated();
context_->NotifyCachedReportsUpdated();
}
void IncrementEndpointDeliveries(const url::Origin& origin,
......@@ -227,7 +227,7 @@ class ReportingCacheImpl : public ReportingCache {
}
}
context_->NotifyCacheUpdated();
context_->NotifyCachedReportsUpdated();
}
void RemoveAllReports(ReportingReport::Outcome outcome) override {
......@@ -244,7 +244,7 @@ class ReportingCacheImpl : public ReportingCache {
for (const ReportingReport* report : reports_to_remove)
RemoveReportInternal(report);
context_->NotifyCacheUpdated();
context_->NotifyCachedReportsUpdated();
}
void SetClient(const url::Origin& origin,
......@@ -282,7 +282,7 @@ class ReportingCacheImpl : public ReportingCache {
RemoveClient(to_evict);
}
context_->NotifyCacheUpdated();
context_->NotifyCachedClientsUpdated();
}
void MarkClientUsed(const ReportingClient* client) override {
......@@ -407,7 +407,7 @@ class ReportingCacheImpl : public ReportingCache {
for (const ReportingClient* client : clients_to_remove)
RemoveClient(client);
context_->NotifyCacheUpdated();
context_->NotifyCachedClientsUpdated();
}
void RemoveClientForOriginAndEndpoint(const url::Origin& origin,
......@@ -416,7 +416,7 @@ class ReportingCacheImpl : public ReportingCache {
GetClientByOriginAndEndpoint(origin, endpoint);
RemoveClient(client);
context_->NotifyCacheUpdated();
context_->NotifyCachedClientsUpdated();
}
void RemoveClientsForEndpoint(const GURL& endpoint) override {
......@@ -431,7 +431,7 @@ class ReportingCacheImpl : public ReportingCache {
RemoveClient(client);
if (!clients_to_remove.empty())
context_->NotifyCacheUpdated();
context_->NotifyCachedClientsUpdated();
}
void RemoveAllClients() override {
......@@ -439,7 +439,7 @@ class ReportingCacheImpl : public ReportingCache {
wildcard_clients_.clear();
client_metadata_.clear();
context_->NotifyCacheUpdated();
context_->NotifyCachedClientsUpdated();
}
ClientStatistics GetStatisticsForOriginAndEndpoint(
......
......@@ -2,14 +2,16 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "net/reporting/reporting_observer.h"
#include "net/reporting/reporting_cache_observer.h"
namespace net {
void ReportingObserver::OnCacheUpdated() {}
void ReportingCacheObserver::OnReportsUpdated() {}
ReportingObserver::ReportingObserver() = default;
void ReportingCacheObserver::OnClientsUpdated() {}
ReportingObserver::~ReportingObserver() = default;
ReportingCacheObserver::ReportingCacheObserver() = default;
ReportingCacheObserver::~ReportingCacheObserver() = default;
} // namespace net
......@@ -2,27 +2,31 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef NET_REPORTING_REPORTING_OBSERVER_H_
#define NET_REPORTING_REPORTING_OBSERVER_H_
#ifndef NET_REPORTING_REPORTING_CACHE_OBSERVER_H_
#define NET_REPORTING_REPORTING_CACHE_OBSERVER_H_
#include "base/macros.h"
#include "net/base/net_export.h"
namespace net {
class NET_EXPORT ReportingObserver {
class NET_EXPORT ReportingCacheObserver {
public:
// Called whenever any change is made to the ReportingCache.
virtual void OnCacheUpdated();
// Called whenever any change is made to the reports in the ReportingCache.
virtual void OnReportsUpdated();
// Called whenever any change is made to the client entries in the
// ReportingCache.
virtual void OnClientsUpdated();
protected:
ReportingObserver();
ReportingCacheObserver();
~ReportingObserver();
~ReportingCacheObserver();
DISALLOW_COPY_AND_ASSIGN(ReportingObserver);
DISALLOW_COPY_AND_ASSIGN(ReportingCacheObserver);
};
} // namespace net
#endif // NET_REPORTING_REPORTING_OBSERVER_H_
#endif // NET_REPORTING_REPORTING_CACHE_OBSERVER_H_
......@@ -12,8 +12,8 @@
#include "base/test/values_test_util.h"
#include "base/time/time.h"
#include "base/values.h"
#include "net/reporting/reporting_cache_observer.h"
#include "net/reporting/reporting_client.h"
#include "net/reporting/reporting_observer.h"
#include "net/reporting/reporting_report.h"
#include "net/reporting/reporting_test_util.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -23,16 +23,24 @@
namespace net {
namespace {
class TestReportingObserver : public ReportingObserver {
class TestReportingCacheObserver : public ReportingCacheObserver {
public:
TestReportingObserver() : cache_update_count_(0) {}
TestReportingCacheObserver()
: cached_reports_update_count_(0), cached_clients_update_count_(0) {}
void OnCacheUpdated() override { ++cache_update_count_; }
void OnReportsUpdated() override { ++cached_reports_update_count_; }
void OnClientsUpdated() override { ++cached_clients_update_count_; }
int cache_update_count() const { return cache_update_count_; }
int cached_reports_update_count() const {
return cached_reports_update_count_;
}
int cached_clients_update_count() const {
return cached_clients_update_count_;
}
private:
int cache_update_count_;
int cached_reports_update_count_;
int cached_clients_update_count_;
};
class ReportingCacheTest : public ReportingTestBase {
......@@ -43,12 +51,12 @@ class ReportingCacheTest : public ReportingTestBase {
policy.max_client_count = 5;
UsePolicy(policy);
context()->AddObserver(&observer_);
context()->AddCacheObserver(&observer_);
}
~ReportingCacheTest() override { context()->RemoveObserver(&observer_); }
~ReportingCacheTest() override { context()->RemoveCacheObserver(&observer_); }
TestReportingObserver* observer() { return &observer_; }
TestReportingCacheObserver* observer() { return &observer_; }
size_t report_count() {
std::vector<const ReportingReport*> reports;
......@@ -134,7 +142,7 @@ class ReportingCacheTest : public ReportingTestBase {
const base::TimeTicks kExpires2_ = kExpires1_ + base::TimeDelta::FromDays(7);
private:
TestReportingObserver observer_;
TestReportingCacheObserver observer_;
};
TEST_F(ReportingCacheTest, Reports) {
......@@ -144,7 +152,7 @@ TEST_F(ReportingCacheTest, Reports) {
cache()->AddReport(kUrl1_, kUserAgent_, kGroup1_, kType_,
std::make_unique<base::DictionaryValue>(), 0, kNow_, 0);
EXPECT_EQ(1, observer()->cache_update_count());
EXPECT_EQ(1, observer()->cached_reports_update_count());
cache()->GetReports(&reports);
ASSERT_EQ(1u, reports.size());
......@@ -161,7 +169,7 @@ TEST_F(ReportingCacheTest, Reports) {
EXPECT_FALSE(cache()->IsReportDoomedForTesting(report));
cache()->IncrementReportsAttempts(reports);
EXPECT_EQ(2, observer()->cache_update_count());
EXPECT_EQ(2, observer()->cached_reports_update_count());
cache()->GetReports(&reports);
ASSERT_EQ(1u, reports.size());
......@@ -170,7 +178,7 @@ TEST_F(ReportingCacheTest, Reports) {
EXPECT_EQ(1, report->attempts);
cache()->RemoveReports(reports, ReportingReport::Outcome::UNKNOWN);
EXPECT_EQ(3, observer()->cache_update_count());
EXPECT_EQ(3, observer()->cached_reports_update_count());
cache()->GetReports(&reports);
EXPECT_TRUE(reports.empty());
......@@ -181,14 +189,14 @@ TEST_F(ReportingCacheTest, RemoveAllReports) {
std::make_unique<base::DictionaryValue>(), 0, kNow_, 0);
cache()->AddReport(kUrl1_, kUserAgent_, kGroup1_, kType_,
std::make_unique<base::DictionaryValue>(), 0, kNow_, 0);
EXPECT_EQ(2, observer()->cache_update_count());
EXPECT_EQ(2, observer()->cached_reports_update_count());
std::vector<const ReportingReport*> reports;
cache()->GetReports(&reports);
EXPECT_EQ(2u, reports.size());
cache()->RemoveAllReports(ReportingReport::Outcome::UNKNOWN);
EXPECT_EQ(3, observer()->cache_update_count());
EXPECT_EQ(3, observer()->cached_reports_update_count());
cache()->GetReports(&reports);
EXPECT_TRUE(reports.empty());
......@@ -197,7 +205,7 @@ TEST_F(ReportingCacheTest, RemoveAllReports) {
TEST_F(ReportingCacheTest, RemovePendingReports) {
cache()->AddReport(kUrl1_, kUserAgent_, kGroup1_, kType_,
std::make_unique<base::DictionaryValue>(), 0, kNow_, 0);
EXPECT_EQ(1, observer()->cache_update_count());
EXPECT_EQ(1, observer()->cached_reports_update_count());
std::vector<const ReportingReport*> reports;
cache()->GetReports(&reports);
......@@ -212,7 +220,7 @@ TEST_F(ReportingCacheTest, RemovePendingReports) {
cache()->RemoveReports(reports, ReportingReport::Outcome::UNKNOWN);
EXPECT_TRUE(cache()->IsReportPendingForTesting(reports[0]));
EXPECT_TRUE(cache()->IsReportDoomedForTesting(reports[0]));
EXPECT_EQ(2, observer()->cache_update_count());
EXPECT_EQ(2, observer()->cached_reports_update_count());
// After removing report, future calls to GetReports should not return it.
std::vector<const ReportingReport*> visible_reports;
......@@ -228,7 +236,7 @@ TEST_F(ReportingCacheTest, RemovePendingReports) {
TEST_F(ReportingCacheTest, RemoveAllPendingReports) {
cache()->AddReport(kUrl1_, kUserAgent_, kGroup1_, kType_,
std::make_unique<base::DictionaryValue>(), 0, kNow_, 0);
EXPECT_EQ(1, observer()->cache_update_count());
EXPECT_EQ(1, observer()->cached_reports_update_count());
std::vector<const ReportingReport*> reports;
cache()->GetReports(&reports);
......@@ -243,7 +251,7 @@ TEST_F(ReportingCacheTest, RemoveAllPendingReports) {
cache()->RemoveAllReports(ReportingReport::Outcome::UNKNOWN);
EXPECT_TRUE(cache()->IsReportPendingForTesting(reports[0]));
EXPECT_TRUE(cache()->IsReportDoomedForTesting(reports[0]));
EXPECT_EQ(2, observer()->cache_update_count());
EXPECT_EQ(2, observer()->cached_reports_update_count());
// After removing report, future calls to GetReports should not return it.
std::vector<const ReportingReport*> visible_reports;
......@@ -327,7 +335,7 @@ TEST_F(ReportingCacheTest, GetReportsAsValue) {
TEST_F(ReportingCacheTest, Endpoints) {
SetClient(kOrigin1_, kEndpoint1_, false, kGroup1_, kExpires1_);
EXPECT_EQ(1, observer()->cache_update_count());
EXPECT_EQ(1, observer()->cached_clients_update_count());
const ReportingClient* client =
FindClientInCache(cache(), kOrigin1_, kEndpoint1_);
......@@ -339,7 +347,7 @@ TEST_F(ReportingCacheTest, Endpoints) {
EXPECT_EQ(kExpires1_, client->expires);
SetClient(kOrigin1_, kEndpoint1_, true, kGroup2, kExpires2_);
EXPECT_EQ(2, observer()->cache_update_count());
EXPECT_EQ(2, observer()->cached_clients_update_count());
client = FindClientInCache(cache(), kOrigin1_, kEndpoint1_);
ASSERT_TRUE(client);
......@@ -350,7 +358,7 @@ TEST_F(ReportingCacheTest, Endpoints) {
EXPECT_EQ(kExpires2_, client->expires);
cache()->RemoveClients(std::vector<const ReportingClient*>{client});
EXPECT_EQ(3, observer()->cache_update_count());
EXPECT_EQ(3, observer()->cached_clients_update_count());
client = FindClientInCache(cache(), kOrigin1_, kEndpoint1_);
EXPECT_FALSE(client);
......@@ -374,10 +382,10 @@ TEST_F(ReportingCacheTest, RemoveClientForOriginAndEndpoint) {
SetClient(kOrigin1_, kEndpoint1_, false, kGroup1_, kExpires1_);
SetClient(kOrigin1_, kEndpoint2_, false, kGroup2, kExpires1_);
SetClient(kOrigin2_, kEndpoint1_, false, kGroup1_, kExpires1_);
EXPECT_EQ(3, observer()->cache_update_count());
EXPECT_EQ(3, observer()->cached_clients_update_count());
cache()->RemoveClientForOriginAndEndpoint(kOrigin1_, kEndpoint1_);
EXPECT_EQ(4, observer()->cache_update_count());
EXPECT_EQ(4, observer()->cached_clients_update_count());
std::vector<const ReportingClient*> clients;
cache()->GetClientsForOriginAndGroup(kOrigin1_, kGroup1_, &clients);
......@@ -394,10 +402,10 @@ TEST_F(ReportingCacheTest, RemoveClientsForEndpoint) {
SetClient(kOrigin1_, kEndpoint1_, false, kGroup1_, kExpires1_);
SetClient(kOrigin1_, kEndpoint2_, false, kGroup2, kExpires1_);
SetClient(kOrigin2_, kEndpoint1_, false, kGroup1_, kExpires1_);
EXPECT_EQ(3, observer()->cache_update_count());
EXPECT_EQ(3, observer()->cached_clients_update_count());
cache()->RemoveClientsForEndpoint(kEndpoint1_);
EXPECT_EQ(4, observer()->cache_update_count());
EXPECT_EQ(4, observer()->cached_clients_update_count());
std::vector<const ReportingClient*> clients;
cache()->GetClientsForOriginAndGroup(kOrigin1_, kGroup1_, &clients);
......@@ -461,10 +469,10 @@ TEST_F(ReportingCacheTest, GetClientsAsValue) {
TEST_F(ReportingCacheTest, RemoveAllClients) {
SetClient(kOrigin1_, kEndpoint1_, false, kGroup1_, kExpires1_);
SetClient(kOrigin2_, kEndpoint2_, false, kGroup1_, kExpires1_);
EXPECT_EQ(2, observer()->cache_update_count());
EXPECT_EQ(2, observer()->cached_clients_update_count());
cache()->RemoveAllClients();
EXPECT_EQ(3, observer()->cache_update_count());
EXPECT_EQ(3, observer()->cached_clients_update_count());
std::vector<const ReportingClient*> clients;
cache()->GetClients(&clients);
......
......@@ -17,12 +17,12 @@
#include "net/base/backoff_entry.h"
#include "net/base/rand_callback.h"
#include "net/reporting/reporting_cache.h"
#include "net/reporting/reporting_cache_observer.h"
#include "net/reporting/reporting_delegate.h"
#include "net/reporting/reporting_delivery_agent.h"
#include "net/reporting/reporting_endpoint_manager.h"
#include "net/reporting/reporting_garbage_collector.h"
#include "net/reporting/reporting_network_change_observer.h"
#include "net/reporting/reporting_observer.h"
#include "net/reporting/reporting_policy.h"
#include "net/reporting/reporting_uploader.h"
......@@ -55,19 +55,24 @@ std::unique_ptr<ReportingContext> ReportingContext::Create(
ReportingContext::~ReportingContext() = default;
void ReportingContext::AddObserver(ReportingObserver* observer) {
DCHECK(!observers_.HasObserver(observer));
observers_.AddObserver(observer);
void ReportingContext::AddCacheObserver(ReportingCacheObserver* observer) {
DCHECK(!cache_observers_.HasObserver(observer));
cache_observers_.AddObserver(observer);
}
void ReportingContext::RemoveObserver(ReportingObserver* observer) {
DCHECK(observers_.HasObserver(observer));
observers_.RemoveObserver(observer);
void ReportingContext::RemoveCacheObserver(ReportingCacheObserver* observer) {
DCHECK(cache_observers_.HasObserver(observer));
cache_observers_.RemoveObserver(observer);
}
void ReportingContext::NotifyCacheUpdated() {
for (auto& observer : observers_)
observer.OnCacheUpdated();
void ReportingContext::NotifyCachedReportsUpdated() {
for (auto& observer : cache_observers_)
observer.OnReportsUpdated();
}
void ReportingContext::NotifyCachedClientsUpdated() {
for (auto& observer : cache_observers_)
observer.OnClientsUpdated();
}
ReportingContext::ReportingContext(const ReportingPolicy& policy,
......
......@@ -22,12 +22,12 @@ class TickClock;
namespace net {
class ReportingCache;
class ReportingCacheObserver;
class ReportingDelegate;
class ReportingDeliveryAgent;
class ReportingEndpointManager;
class ReportingGarbageCollector;
class ReportingNetworkChangeObserver;
class ReportingObserver;
class ReportingUploader;
class URLRequestContext;
......@@ -57,10 +57,11 @@ class NET_EXPORT ReportingContext {
return garbage_collector_.get();
}
void AddObserver(ReportingObserver* observer);
void RemoveObserver(ReportingObserver* observer);
void AddCacheObserver(ReportingCacheObserver* observer);
void RemoveCacheObserver(ReportingCacheObserver* observer);
void NotifyCacheUpdated();
void NotifyCachedReportsUpdated();
void NotifyCachedClientsUpdated();
protected:
ReportingContext(const ReportingPolicy& policy,
......@@ -77,8 +78,8 @@ class NET_EXPORT ReportingContext {
const base::TickClock* tick_clock_;
std::unique_ptr<ReportingUploader> uploader_;
base::ObserverList<ReportingObserver, /* check_empty= */ true>::Unchecked
observers_;
base::ObserverList<ReportingCacheObserver, /* check_empty= */ true>::Unchecked
cache_observers_;
std::unique_ptr<ReportingDelegate> delegate_;
......
......@@ -17,9 +17,9 @@
#include "base/timer/timer.h"
#include "base/values.h"
#include "net/reporting/reporting_cache.h"
#include "net/reporting/reporting_cache_observer.h"
#include "net/reporting/reporting_delegate.h"
#include "net/reporting/reporting_endpoint_manager.h"
#include "net/reporting/reporting_observer.h"
#include "net/reporting/reporting_report.h"
#include "net/reporting/reporting_uploader.h"
#include "url/gurl.h"
......@@ -52,26 +52,28 @@ void SerializeReports(const std::vector<const ReportingReport*>& reports,
}
class ReportingDeliveryAgentImpl : public ReportingDeliveryAgent,
public ReportingObserver {
public ReportingCacheObserver {
public:
ReportingDeliveryAgentImpl(ReportingContext* context)
: context_(context),
timer_(std::make_unique<base::OneShotTimer>()),
weak_factory_(this) {
context_->AddObserver(this);
context_->AddCacheObserver(this);
}
// ReportingDeliveryAgent implementation:
~ReportingDeliveryAgentImpl() override { context_->RemoveObserver(this); }
~ReportingDeliveryAgentImpl() override {
context_->RemoveCacheObserver(this);
}
void SetTimerForTesting(std::unique_ptr<base::OneShotTimer> timer) override {
DCHECK(!timer_->IsRunning());
timer_ = std::move(timer);
}
// ReportingObserver implementation:
void OnCacheUpdated() override {
// ReportingCacheObserver implementation:
void OnReportsUpdated() override {
if (CacheHasReports() && !timer_->IsRunning()) {
SendReports();
StartTimer();
......
......@@ -11,8 +11,8 @@
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "net/reporting/reporting_cache.h"
#include "net/reporting/reporting_cache_observer.h"
#include "net/reporting/reporting_context.h"
#include "net/reporting/reporting_observer.h"
#include "net/reporting/reporting_policy.h"
#include "net/reporting/reporting_report.h"
......@@ -21,17 +21,17 @@ namespace net {
namespace {
class ReportingGarbageCollectorImpl : public ReportingGarbageCollector,
public ReportingObserver {
public ReportingCacheObserver {
public:
ReportingGarbageCollectorImpl(ReportingContext* context)
: context_(context), timer_(std::make_unique<base::OneShotTimer>()) {
context_->AddObserver(this);
context_->AddCacheObserver(this);
}
// ReportingGarbageCollector implementation:
~ReportingGarbageCollectorImpl() override {
context_->RemoveObserver(this);
context_->RemoveCacheObserver(this);
}
void SetTimerForTesting(std::unique_ptr<base::OneShotTimer> timer) override {
......@@ -39,7 +39,7 @@ class ReportingGarbageCollectorImpl : public ReportingGarbageCollector,
}
// ReportingObserver implementation:
void OnCacheUpdated() override {
void OnReportsUpdated() override {
if (timer_->IsRunning())
return;
......@@ -67,12 +67,12 @@ class ReportingGarbageCollectorImpl : public ReportingGarbageCollector,
}
// Don't restart the timer on the garbage collector's own updates.
context_->RemoveObserver(this);
context_->RemoveCacheObserver(this);
context_->cache()->RemoveReports(failed_reports,
ReportingReport::Outcome::ERASED_FAILED);
context_->cache()->RemoveReports(expired_reports,
ReportingReport::Outcome::ERASED_EXPIRED);
context_->AddObserver(this);
context_->AddCacheObserver(this);
}
ReportingContext* context_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment