Skip to content

Commit

Permalink
Remove unnecessary DeactivatedSource struct
Browse files Browse the repository at this point in the history
It wraps a single-valued Reason enum, so for the time being it is
unnecessary. If additional deactivation reasons are added in the future,
the struct can be restored.

Change-Id: I804c75efd37079e396bc39a8d0d92e5385c22cca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3638671
Quick-Run: Andrew Paseltiner <apaseltiner@chromium.org>
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Reviewed-by: Nan Lin <linnan@chromium.org>
Commit-Queue: Nan Lin <linnan@chromium.org>
Auto-Submit: Andrew Paseltiner <apaseltiner@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1001830}
  • Loading branch information
Andrew Paseltiner authored and Chromium LUCI CQ committed May 11, 2022
1 parent bf0b2cd commit ef9d60d
Show file tree
Hide file tree
Showing 16 changed files with 33 additions and 116 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -272,8 +272,7 @@ IN_PROC_BROWSER_TEST_F(AttributionInternalsWebUiBrowserTest,
.BuildStored()}));

manager_.NotifySourceDeactivated(
DeactivatedSource(SourceBuilder(now + base::Hours(3)).BuildStored(),
DeactivatedSource::Reason::kReplacedByNewerSource));
SourceBuilder(now + base::Hours(3)).BuildStored());

// This shouldn't result in a row, as registration succeeded.
manager_.NotifySourceHandled(SourceBuilder(now).Build(),
Expand Down Expand Up @@ -630,9 +629,8 @@ IN_PROC_BROWSER_TEST_F(AttributionInternalsWebUiBrowserTest,
.WillByDefault(InvokeCallback(
{SourceBuilder(now).SetSourceEventId(5).BuildStored()}));

manager_.NotifySourceDeactivated(DeactivatedSource(
SourceBuilder(now + base::Hours(2)).SetSourceEventId(6).BuildStored(),
DeactivatedSource::Reason::kReplacedByNewerSource));
manager_.NotifySourceDeactivated(
SourceBuilder(now + base::Hours(2)).SetSourceEventId(6).BuildStored());

EXPECT_CALL(manager_, ClearData)
.WillOnce([](base::Time delete_begin, base::Time delete_end,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,17 +273,10 @@ void AttributionInternalsHandlerImpl::OnReportsChanged(
}

void AttributionInternalsHandlerImpl::OnSourceDeactivated(
const DeactivatedSource& deactivated_source) {
Attributability attributability;
switch (deactivated_source.reason) {
case DeactivatedSource::Reason::kReplacedByNewerSource:
attributability = Attributability::kReplacedByNewerSource;
break;
}

auto source =
WebUISource(deactivated_source.source.common_info(), attributability,
deactivated_source.source.dedup_keys());
const StoredSource& deactivated_source) {
auto source = WebUISource(deactivated_source.common_info(),
Attributability::kReplacedByNewerSource,
deactivated_source.dedup_keys());

for (auto& observer : observers_) {
observer->OnSourceRejectedOrDeactivated(source.Clone());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,7 @@ class AttributionInternalsHandlerImpl
// AttributionObserver:
void OnSourcesChanged() override;
void OnReportsChanged(AttributionReport::ReportType report_type) override;
void OnSourceDeactivated(
const DeactivatedSource& deactivated_source) override;
void OnSourceDeactivated(const StoredSource& deactivated_source) override;
void OnSourceHandled(const StorableSource& source,
StorableSource::Result result) override;
void OnReportSent(const AttributionReport& report,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -827,7 +827,7 @@ void AttributionManagerImpl::NotifyReportsChanged(
}

void AttributionManagerImpl::NotifySourceDeactivated(
const DeactivatedSource& source) {
const StoredSource& source) {
for (auto& observer : observers_)
observer.OnSourceDeactivated(source);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ class AttributionStorage;
class AttributionStorageDelegate;
class CreateReportResult;
class StoragePartitionImpl;
class StoredSource;

struct DeactivatedSource;
struct SendResult;

// UI thread class that manages the lifetime of the underlying attribution
Expand Down Expand Up @@ -158,7 +158,7 @@ class CONTENT_EXPORT AttributionManagerImpl : public AttributionManager {

void NotifySourcesChanged();
void NotifyReportsChanged(AttributionReport::ReportType report_type);
void NotifySourceDeactivated(const DeactivatedSource& source);
void NotifySourceDeactivated(const StoredSource& source);
void NotifyReportSent(bool is_debug_report, AttributionReport, SendResult);

bool IsReportAllowed(const AttributionReport&) const;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1103,10 +1103,8 @@ TEST_F(AttributionManagerImplTest, HandleSource_NotifiesObservers) {

EXPECT_CALL(observer, OnSourcesChanged);
EXPECT_CALL(observer, OnReportsChanged).Times(0);
EXPECT_CALL(observer,
OnSourceDeactivated(DeactivatedSource{
builder.SetDefaultFilterData().BuildStored(),
DeactivatedSource::Reason::kReplacedByNewerSource}));
EXPECT_CALL(observer, OnSourceDeactivated(
builder.SetDefaultFilterData().BuildStored()));
}

attribution_manager_->HandleSource(source);
Expand Down
4 changes: 2 additions & 2 deletions content/browser/attribution_reporting/attribution_observer.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ namespace content {

class AttributionTrigger;
class CreateReportResult;
class StoredSource;

struct DeactivatedSource;
struct SendResult;

// Observes events in the Attribution Reporting API. Observers are registered on
Expand All @@ -35,7 +35,7 @@ class AttributionObserver : public base::CheckedObserver {

// Called when a source is deactivated. Note that this isn't called when a
// source reaches the attribution limit.
virtual void OnSourceDeactivated(const DeactivatedSource& source) {}
virtual void OnSourceDeactivated(const StoredSource& source) {}

// Called when a report is sent, regardless of success, but not for attempts
// that will be retried.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,18 +58,4 @@ CreateReportResult& CreateReportResult::operator=(const CreateReportResult&) =
CreateReportResult& CreateReportResult::operator=(CreateReportResult&&) =
default;

DeactivatedSource::DeactivatedSource(StoredSource source, Reason reason)
: source(std::move(source)), reason(reason) {}

DeactivatedSource::~DeactivatedSource() = default;

DeactivatedSource::DeactivatedSource(const DeactivatedSource&) = default;

DeactivatedSource::DeactivatedSource(DeactivatedSource&&) = default;

DeactivatedSource& DeactivatedSource::operator=(const DeactivatedSource&) =
default;

DeactivatedSource& DeactivatedSource::operator=(DeactivatedSource&&) = default;

} // namespace content
19 changes: 0 additions & 19 deletions content/browser/attribution_reporting/attribution_observer_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,11 @@
#include "base/time/time.h"
#include "content/browser/attribution_reporting/attribution_report.h"
#include "content/browser/attribution_reporting/attribution_trigger.h"
#include "content/browser/attribution_reporting/stored_source.h"
#include "content/common/content_export.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

namespace content {

struct CONTENT_EXPORT DeactivatedSource {
enum class Reason {
kReplacedByNewerSource,
};

DeactivatedSource(StoredSource source, Reason reason);
~DeactivatedSource();

DeactivatedSource(const DeactivatedSource&);
DeactivatedSource(DeactivatedSource&&);

DeactivatedSource& operator=(const DeactivatedSource&);
DeactivatedSource& operator=(DeactivatedSource&&);

StoredSource source;
Reason reason;
};

class CONTENT_EXPORT CreateReportResult {
public:
CreateReportResult(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ using StoreSourceResult = ::content::AttributionStorage::StoreSourceResult;

StoreSourceResult::StoreSourceResult(
StorableSource::Result status,
std::vector<DeactivatedSource> deactivated_sources,
std::vector<StoredSource> deactivated_sources,
absl::optional<base::Time> min_fake_report_time)
: status(status),
deactivated_sources(std::move(deactivated_sources)),
Expand Down
6 changes: 2 additions & 4 deletions content/browser/attribution_reporting/attribution_storage.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ class AttributionTrigger;
class CreateReportResult;
class StoredSource;

struct DeactivatedSource;

// This class provides an interface for persisting attribution data to
// disk, and performing queries on it. AttributionStorage should initialize
// itself. Calls to a AttributionStorage instance that failed to initialize
Expand All @@ -35,7 +33,7 @@ class AttributionStorage {
struct CONTENT_EXPORT StoreSourceResult {
explicit StoreSourceResult(
StorableSource::Result status,
std::vector<DeactivatedSource> deactivated_sources = {},
std::vector<StoredSource> deactivated_sources = {},
absl::optional<base::Time> min_fake_report_time = absl::nullopt);

~StoreSourceResult();
Expand All @@ -47,7 +45,7 @@ class AttributionStorage {
StoreSourceResult& operator=(StoreSourceResult&&);

StorableSource::Result status;
std::vector<DeactivatedSource> deactivated_sources;
std::vector<StoredSource> deactivated_sources;
// The earliest report time for any fake reports stored alongside the
// source, if any.
absl::optional<base::Time> min_fake_report_time;
Expand Down
14 changes: 6 additions & 8 deletions content/browser/attribution_reporting/attribution_storage_sql.cc
Original file line number Diff line number Diff line change
Expand Up @@ -395,12 +395,12 @@ AttributionStorageSql::~AttributionStorageSql() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
}

absl::optional<std::vector<DeactivatedSource>>
absl::optional<std::vector<StoredSource>>
AttributionStorageSql::DeactivateSources(
const std::string& serialized_conversion_destination,
const std::string& serialized_reporting_origin,
int return_limit) {
std::vector<DeactivatedSource> deactivated_sources;
std::vector<StoredSource> deactivated_sources;

if (return_limit != 0) {
// Get at most `return_limit` sources that will be deactivated. We do this
Expand Down Expand Up @@ -428,9 +428,7 @@ AttributionStorageSql::DeactivateSources(
if (!source_data.has_value())
return absl::nullopt;

deactivated_sources.emplace_back(
std::move(source_data->source),
DeactivatedSource::Reason::kReplacedByNewerSource);
deactivated_sources.push_back(std::move(source_data->source));
}
if (!get_statement.Succeeded())
return absl::nullopt;
Expand Down Expand Up @@ -458,10 +456,10 @@ AttributionStorageSql::DeactivateSources(

for (auto& deactivated_source : deactivated_sources) {
absl::optional<std::vector<uint64_t>> dedup_keys =
ReadDedupKeys(deactivated_source.source.source_id());
ReadDedupKeys(deactivated_source.source_id());
if (!dedup_keys.has_value())
return absl::nullopt;
deactivated_source.source.SetDedupKeys(std::move(*dedup_keys));
deactivated_source.SetDedupKeys(std::move(*dedup_keys));
}

return deactivated_sources;
Expand Down Expand Up @@ -528,7 +526,7 @@ AttributionStorage::StoreSourceResult AttributionStorageSql::StoreSource(
// In the case where we get a new source for a given <reporting_origin,
// conversion_destination> we should mark all active, converted impressions
// with the matching <reporting_origin, conversion_destination> as not active.
absl::optional<std::vector<DeactivatedSource>> deactivated_sources =
absl::optional<std::vector<StoredSource>> deactivated_sources =
DeactivateSources(serialized_conversion_destination,
serialized_reporting_origin,
deactivated_source_return_limit);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,10 @@ class CONTENT_EXPORT AttributionStorageSql : public AttributionStorage {

// Deactivates active, converted sources with the given conversion destination
// and reporting origin. Returns at most `limit` of those, or null on error.
[[nodiscard]] absl::optional<std::vector<DeactivatedSource>>
DeactivateSources(const std::string& serialized_conversion_destination,
const std::string& serialized_reporting_origin,
int return_limit) VALID_CONTEXT_REQUIRED(sequence_checker_);
[[nodiscard]] absl::optional<std::vector<StoredSource>> DeactivateSources(
const std::string& serialized_conversion_destination,
const std::string& serialized_reporting_origin,
int return_limit) VALID_CONTEXT_REQUIRED(sequence_checker_);

// Returns false on failure.
[[nodiscard]] bool DeleteSources(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1655,9 +1655,7 @@ TEST_F(AttributionStorageTest, StoreSource_ReturnsDeactivatedSources) {

builder1.SetDedupKeys({13});
EXPECT_THAT(storage()->StoreSource(builder2.Build()).deactivated_sources,
ElementsAre(DeactivatedSource(
builder1.SetDefaultFilterData().BuildStored(),
DeactivatedSource::Reason::kReplacedByNewerSource)));
ElementsAre(builder1.SetDefaultFilterData().BuildStored()));

EXPECT_THAT(storage()->GetActiveSources(),
ElementsAre(builder2.SetDefaultFilterData().BuildStored()));
Expand Down Expand Up @@ -1691,9 +1689,7 @@ TEST_F(AttributionStorageTest, StoreSource_ReturnsDeactivatedSources_Limited) {
->StoreSource(builder3.Build(),
/*deactivated_source_return_limit=*/1)
.deactivated_sources,
ElementsAre(DeactivatedSource(
builder1.SetDefaultFilterData().BuildStored(),
DeactivatedSource::Reason::kReplacedByNewerSource)));
ElementsAre(builder1.SetDefaultFilterData().BuildStored()));
EXPECT_THAT(storage()->GetActiveSources(),
ElementsAre(builder3.SetDefaultFilterData().BuildStored()));
}
Expand Down
25 changes: 1 addition & 24 deletions content/browser/attribution_reporting/attribution_test_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ void MockAttributionManager::NotifyReportsChanged(
}

void MockAttributionManager::NotifySourceDeactivated(
const DeactivatedSource& source) {
const StoredSource& source) {
for (auto& observer : observers_)
observer.OnSourceDeactivated(source);
}
Expand Down Expand Up @@ -876,14 +876,6 @@ bool operator==(const SendResult& a, const SendResult& b) {
return tie(a) == tie(b);
}

bool operator==(const DeactivatedSource& a, const DeactivatedSource& b) {
const auto tie = [](const DeactivatedSource& deactivated_source) {
return std::make_tuple(deactivated_source.source,
deactivated_source.reason);
};
return tie(a) == tie(b);
}

bool operator==(const AttributionAggregatableTriggerData& a,
const AttributionAggregatableTriggerData& b) {
const auto tie = [](const AttributionAggregatableTriggerData& trigger_data) {
Expand Down Expand Up @@ -988,15 +980,6 @@ std::ostream& operator<<(std::ostream& out,
return out;
}

std::ostream& operator<<(std::ostream& out, DeactivatedSource::Reason reason) {
switch (reason) {
case DeactivatedSource::Reason::kReplacedByNewerSource:
out << "kReplacedByNewerSource";
break;
}
return out;
}

std::ostream& operator<<(std::ostream& out, RateLimitResult result) {
switch (result) {
case RateLimitResult::kAllowed:
Expand Down Expand Up @@ -1237,12 +1220,6 @@ std::ostream& operator<<(std::ostream& out, const SendResult& info) {
<< ",http_response_code=" << info.http_response_code << "}";
}

std::ostream& operator<<(std::ostream& out,
const DeactivatedSource& deactivated_source) {
return out << "{source=" << deactivated_source.source
<< ",reason=" << deactivated_source.reason << "}";
}

std::ostream& operator<<(std::ostream& out, StorableSource::Result status) {
switch (status) {
case StorableSource::Result::kSuccess:
Expand Down
11 changes: 2 additions & 9 deletions content/browser/attribution_reporting/attribution_test_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ class MockAttributionManager : public AttributionManager {

void NotifySourcesChanged();
void NotifyReportsChanged(AttributionReport::ReportType report_type);
void NotifySourceDeactivated(const DeactivatedSource& source);
void NotifySourceDeactivated(const StoredSource& source);
void NotifySourceHandled(const StorableSource& source,
StorableSource::Result result);
void NotifyReportSent(const AttributionReport& report,
Expand Down Expand Up @@ -391,7 +391,7 @@ class MockAttributionObserver : public AttributionObserver {

MOCK_METHOD(void,
OnSourceDeactivated,
(const DeactivatedSource& source),
(const StoredSource& source),
(override));

MOCK_METHOD(void,
Expand Down Expand Up @@ -665,8 +665,6 @@ bool operator==(const AttributionReport& a, const AttributionReport& b);

bool operator==(const SendResult& a, const SendResult& b);

bool operator==(const DeactivatedSource& a, const DeactivatedSource& b);

bool operator==(const AttributionAggregatableTriggerData& a,
const AttributionAggregatableTriggerData& b);

Expand All @@ -679,8 +677,6 @@ std::ostream& operator<<(std::ostream& out,
std::ostream& operator<<(std::ostream& out,
AttributionTrigger::AggregatableResult status);

std::ostream& operator<<(std::ostream& out, DeactivatedSource::Reason reason);

std::ostream& operator<<(std::ostream& out, RateLimitResult result);

std::ostream& operator<<(std::ostream& out, AttributionSourceType source_type);
Expand Down Expand Up @@ -732,9 +728,6 @@ std::ostream& operator<<(std::ostream& out,
std::ostream& operator<<(std::ostream& out,
StoredSource::ActiveState active_state);

std::ostream& operator<<(std::ostream& out,
const DeactivatedSource& deactivated_source);

std::ostream& operator<<(std::ostream& out, StorableSource::Result status);

std::ostream& operator<<(
Expand Down

0 comments on commit ef9d60d

Please sign in to comment.