Skip to content

Commit

Permalink
[Shortcut] Add observer for shortcut registry cache.
Browse files Browse the repository at this point in the history
This CL adds an observer for shortcut registry cache.

BUG=1412708

Change-Id: If2ae1d0e80ef1ec38bca3bca27575342141ad6e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4566014
Reviewed-by: Tim Sergeant <tsergeant@chromium.org>
Commit-Queue: Maggie Cai <mxcai@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1150402}
  • Loading branch information
Maggie Cai authored and Chromium LUCI CQ committed May 30, 2023
1 parent 77b19d7 commit b23400c
Show file tree
Hide file tree
Showing 9 changed files with 203 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ source_set("unit_tests") {

deps = [
":shortcut",
"//base",
"//testing/gtest",
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ Shortcut::Shortcut(const std::string& host_app_id, const std::string& local_id)

Shortcut::~Shortcut() = default;

bool Shortcut::operator==(const Shortcut& rhs) const {
return this->shortcut_id == rhs.shortcut_id &&
this->host_app_id == rhs.host_app_id &&
this->local_id == rhs.local_id && this->name == rhs.name &&
this->shortcut_source == rhs.shortcut_source;
}

std::unique_ptr<Shortcut> Shortcut::Clone() const {
auto shortcut = std::make_unique<Shortcut>(host_app_id, local_id);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ struct COMPONENT_EXPORT(SHORTCUT) Shortcut {
Shortcut(Shortcut&&) = delete;
Shortcut& operator=(Shortcut&&) = delete;

bool operator==(const Shortcut&) const;

~Shortcut();

std::unique_ptr<Shortcut> Clone() const;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,27 @@

#include "base/check.h"
#include "base/containers/contains.h"
#include "base/observer_list.h"
#include "components/services/app_service/public/cpp/shortcut/shortcut_update.h"

namespace apps {

ShortcutRegistryCache::ShortcutRegistryCache() = default;

ShortcutRegistryCache::~ShortcutRegistryCache() = default;
ShortcutRegistryCache::~ShortcutRegistryCache() {
for (auto& obs : observers_) {
obs.OnShortcutRegistryCacheWillBeDestroyed(this);
}
CHECK(observers_.empty());
}

void ShortcutRegistryCache::AddObserver(Observer* observer) {
observers_.AddObserver(observer);
}

void ShortcutRegistryCache::RemoveObserver(Observer* observer) {
observers_.RemoveObserver(observer);
}

void ShortcutRegistryCache::UpdateShortcut(ShortcutPtr delta) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
Expand All @@ -24,12 +38,19 @@ void ShortcutRegistryCache::UpdateShortcut(ShortcutPtr delta) {
is_updating_ = true;
const ShortcutId shortcut_id = delta->shortcut_id;

if (HasShortcut(shortcut_id)) {
ShortcutUpdate::Merge(states_[shortcut_id].get(), delta.get());
Shortcut* state =
HasShortcut(shortcut_id) ? states_[shortcut_id].get() : nullptr;

for (auto& obs : observers_) {
obs.OnShortcutUpdated(ShortcutUpdate(state, delta.get()));
}

if (state) {
ShortcutUpdate::Merge(state, delta.get());
} else {
states_.emplace(shortcut_id, delta->Clone());
}
// TODO(crbug.com/1412708): Update observer.

is_updating_ = false;
}

Expand All @@ -54,4 +75,4 @@ std::vector<ShortcutPtr> ShortcutRegistryCache::GetAllShortcuts() {
return shortcuts;
}

} // namespace apps
} // namespace apps
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@
#include "base/containers/contains.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/stack_allocated.h"
#include "base/observer_list.h"
#include "base/observer_list_types.h"
#include "base/sequence_checker.h"
#include "components/services/app_service/public/cpp/shortcut/shortcut.h"
#include "components/services/app_service/public/cpp/shortcut/shortcut_update.h"

namespace apps {

Expand All @@ -34,13 +37,31 @@ class COMPONENT_EXPORT(SHORTCUT) ShortcutRegistryCache {
STACK_ALLOCATED();
};

class COMPONENT_EXPORT(SHORTCUT) Observer : public base::CheckedObserver {
public:
// Called when a shortcut been updated (including added). `update` contains
// the shortcut updating information to let the clients know which shortcut
// has been updated and what changes have been made.
virtual void OnShortcutUpdated(const ShortcutUpdate& update) {}

// Called when the ShortcutRegistryCache object (the thing that this
// observer observes) will be destroyed. In response, the observer, |this|,
// should call "cache->RemoveObserver(this)", whether directly or indirectly
// (e.g. via base::ScopedObservation::Reset).
virtual void OnShortcutRegistryCacheWillBeDestroyed(
ShortcutRegistryCache* cache) = 0;
};

ShortcutRegistryCache();

ShortcutRegistryCache(const ShortcutRegistryCache&) = delete;
ShortcutRegistryCache& operator=(const ShortcutRegistryCache&) = delete;

~ShortcutRegistryCache();

void AddObserver(Observer* observer);
void RemoveObserver(Observer* observer);

// Apply the shortcut update `delta` to an existing shortcut, or create a new
// shortcut if it doesn't exists.
void UpdateShortcut(ShortcutPtr delta);
Expand All @@ -67,6 +88,8 @@ class COMPONENT_EXPORT(SHORTCUT) ShortcutRegistryCache {
// TODO(crbug.com/1412708): Handle observer updates if proved to be necessary.
bool is_updating_ = false;

base::ObserverList<Observer> observers_;

SEQUENCE_CHECKER(sequence_checker_);
};
using ShortcutView = ShortcutRegistryCache::ShortcutView;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,43 @@
#include <memory>
#include <utility>

#include "base/scoped_observation.h"
#include "components/services/app_service/public/cpp/shortcut/shortcut.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace apps {

class ShortcutRegistryCacheTest : public testing::Test {
class ShortcutRegistryCacheTest : public testing::Test,
public ShortcutRegistryCache::Observer {
public:
ShortcutRegistryCache& cache() { return cache_; }

protected:
void ExpectShortcutUpdate(std::unique_ptr<ShortcutUpdate> update) {
expected_update_ = std::move(update);
if (!obs_.IsObserving()) {
obs_.Observe(&cache());
}
on_shortcut_updated_called_ = false;
}
bool OnShortcutUpdatedCalled() { return on_shortcut_updated_called_; }

private:
void OnShortcutUpdated(const ShortcutUpdate& update) override {
on_shortcut_updated_called_ = true;
EXPECT_EQ(update, *expected_update_);
}

void OnShortcutRegistryCacheWillBeDestroyed(
ShortcutRegistryCache* cache) override {
obs_.Reset();
}
ShortcutRegistryCache cache_;
std::unique_ptr<ShortcutUpdate> expected_update_;
bool on_shortcut_updated_called_ = false;
base::ScopedObservation<ShortcutRegistryCache,
ShortcutRegistryCache::Observer>
obs_{this};
};

TEST_F(ShortcutRegistryCacheTest, AddShortcut) {
Expand Down Expand Up @@ -75,4 +101,34 @@ TEST_F(ShortcutRegistryCacheTest, UpdateShortcut) {
EXPECT_EQ(stored_shortcut->host_app_id, host_app_id);
EXPECT_EQ(stored_shortcut->local_id, local_id);
}

TEST_F(ShortcutRegistryCacheTest, Observer) {
std::string host_app_id = "host_app_id";
std::string local_id = "local_id";
auto shortcut = std::make_unique<Shortcut>(host_app_id, local_id);
ShortcutId shortcut_id = shortcut->shortcut_id;
shortcut->name = "name";
shortcut->shortcut_source = ShortcutSource::kUser;
ExpectShortcutUpdate(
std::make_unique<ShortcutUpdate>(nullptr, shortcut.get()));
cache().UpdateShortcut(std::move(shortcut));
EXPECT_TRUE(OnShortcutUpdatedCalled());

auto shortcut_delta = std::make_unique<Shortcut>(host_app_id, local_id);
shortcut_delta->name = "new name";
shortcut_delta->shortcut_source = ShortcutSource::kDeveloper;
std::unique_ptr<Shortcut> current_state =
cache().GetShortcut(shortcut_id)->Clone();
ExpectShortcutUpdate(std::make_unique<ShortcutUpdate>(current_state.get(),
shortcut_delta.get()));
cache().UpdateShortcut(std::move(shortcut_delta));
EXPECT_TRUE(OnShortcutUpdatedCalled());

auto shortcut_nochange = std::make_unique<Shortcut>(host_app_id, local_id);
current_state = cache().GetShortcut(shortcut_id)->Clone();
ExpectShortcutUpdate(std::make_unique<ShortcutUpdate>(
current_state.get(), shortcut_nochange.get()));
cache().UpdateShortcut(std::move(shortcut_nochange));
EXPECT_TRUE(OnShortcutUpdatedCalled());
}
} // namespace apps
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.

#include "components/services/app_service/public/cpp/shortcut/shortcut_update.h"
#include <type_traits>

#include "base/check.h"
#include "base/check_op.h"
Expand All @@ -23,6 +24,25 @@ ShortcutUpdate::ShortcutUpdate(const Shortcut* state, const Shortcut* delta)
}
}

bool ShortcutUpdate::operator==(const ShortcutUpdate& rhs) const {
bool states_are_same = false;
bool deltas_are_same = false;
if (!this->state_ && !rhs.state_) {
states_are_same = true;
}
if (this->state_ && rhs.state_) {
states_are_same = *(this->state_) == *(rhs.state_);
}

if (!this->delta_ && !rhs.delta_) {
deltas_are_same = true;
}
if (this->delta_ && rhs.delta_) {
deltas_are_same = *(this->delta_) == *(rhs.delta_);
}
return states_are_same && deltas_are_same;
}

void ShortcutUpdate::Merge(Shortcut* state, const Shortcut* delta) {
CHECK(state);
if (!delta) {
Expand Down Expand Up @@ -83,4 +103,17 @@ bool ShortcutUpdate::ShortcutSourceChanged() const {
ShortcutSource::kUnknown);
}

// For logging and debug purposes.
COMPONENT_EXPORT(SHORTCUT)
std::ostream& operator<<(std::ostream& out,
const ShortcutUpdate& shortcut_update) {
out << "ShortcutId: " << shortcut_update.ShortcutId() << std::endl;
out << "HostAppId: " << shortcut_update.HostAppId() << std::endl;
out << "LocalId: " << shortcut_update.LocalId() << std::endl;
out << "Name: " << shortcut_update.Name() << std::endl;
out << "ShortcutSource: " << EnumToString(shortcut_update.ShortcutSource())
<< std::endl;
return out;
}

} // namespace apps
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ class COMPONENT_EXPORT(SHORTCUT) ShortcutUpdate {
ShortcutUpdate(const ShortcutUpdate&) = delete;
ShortcutUpdate& operator=(const ShortcutUpdate&) = delete;

bool operator==(const ShortcutUpdate&) const;

const ShortcutId& ShortcutId() const;
const std::string& HostAppId() const;
const std::string& LocalId() const;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "components/services/app_service/public/cpp/shortcut/shortcut_update.h"

#include <memory>

#include "components/services/app_service/public/cpp/shortcut/shortcut.h"
#include "testing/gtest/include/gtest/gtest.h"

Expand Down Expand Up @@ -91,4 +93,54 @@ TEST_F(ShortcutUpdateTest, Merge) {
EXPECT_EQ(shortcut_state.local_id, local_id_);
}

TEST_F(ShortcutUpdateTest, Equal) {
std::unique_ptr<Shortcut> state_1 = std::make_unique<Shortcut>("a", "b");
state_1->name = "name";
std::unique_ptr<Shortcut> state_same_as_1 =
std::make_unique<Shortcut>("a", "b");
state_same_as_1->name = "name";
std::unique_ptr<Shortcut> state_2 = std::make_unique<Shortcut>("a", "b");
state_2->name = "different name";

std::unique_ptr<Shortcut> delta_1 = std::make_unique<Shortcut>("a", "b");
delta_1->name = "new name";
std::unique_ptr<Shortcut> delta_same_as_1 =
std::make_unique<Shortcut>("a", "b");
delta_same_as_1->name = "new name";
std::unique_ptr<Shortcut> delta_2 = std::make_unique<Shortcut>("a", "b");
delta_2->name = "new new name";

// Test nullptr handling.
EXPECT_EQ(ShortcutUpdate(nullptr, delta_1.get()),
ShortcutUpdate(nullptr, delta_1.get()));
EXPECT_NE(ShortcutUpdate(nullptr, delta_1.get()),
ShortcutUpdate(state_1.get(), nullptr));
EXPECT_NE(ShortcutUpdate(nullptr, delta_1.get()),
ShortcutUpdate(state_1.get(), delta_1.get()));
EXPECT_NE(ShortcutUpdate(state_1.get(), nullptr),
ShortcutUpdate(nullptr, delta_1.get()));
EXPECT_EQ(ShortcutUpdate(state_1.get(), nullptr),
ShortcutUpdate(state_1.get(), nullptr));
EXPECT_NE(ShortcutUpdate(state_1.get(), nullptr),
ShortcutUpdate(state_1.get(), delta_1.get()));
EXPECT_NE(ShortcutUpdate(state_1.get(), delta_1.get()),
ShortcutUpdate(nullptr, delta_1.get()));
EXPECT_NE(ShortcutUpdate(state_1.get(), delta_1.get()),
ShortcutUpdate(state_1.get(), nullptr));
EXPECT_EQ(ShortcutUpdate(state_1.get(), delta_1.get()),
ShortcutUpdate(state_1.get(), delta_1.get()));

// Test deep equal.
EXPECT_EQ(ShortcutUpdate(state_1.get(), delta_1.get()),
ShortcutUpdate(state_same_as_1.get(), delta_same_as_1.get()));

// Test not equal.
EXPECT_NE(ShortcutUpdate(state_1.get(), delta_1.get()),
ShortcutUpdate(state_2.get(), delta_1.get()));
EXPECT_NE(ShortcutUpdate(state_1.get(), delta_1.get()),
ShortcutUpdate(state_1.get(), delta_2.get()));
EXPECT_NE(ShortcutUpdate(state_1.get(), delta_1.get()),
ShortcutUpdate(state_2.get(), delta_2.get()));
}

} // namespace apps

0 comments on commit b23400c

Please sign in to comment.