Skip to content

Commit

Permalink
[Notes] Use UnguessableToken instead of guid
Browse files Browse the repository at this point in the history 
Change-Id: I612294d2890ad5abf98030e5a242dc636cb30564
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3563635
Reviewed-by: Guillaume Jenkins <gujen@google.com>
Commit-Queue: Gayane Petrosyan <gayane@google.com>
Cr-Commit-Position: refs/heads/main@{#988025}
  • Loading branch information
Gayane Petrosyan authored and Chromium LUCI CQ committed Apr 1, 2022
1 parent d42fa41 commit da6cd3d
Show file tree
Hide file tree
Showing 10 changed files with 177 additions and 155 deletions.
23 changes: 13 additions & 10 deletions components/user_notes/browser/user_note_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,23 @@ base::SafeRef<UserNoteService> UserNoteService::GetSafeRef() {
return weak_ptr_factory_.GetSafeRef();
}

void UserNoteService::OnNoteInstanceAddedToPage(const std::string& guid,
UserNotesManager* manager) {
void UserNoteService::OnNoteInstanceAddedToPage(
const base::UnguessableToken& id,
UserNotesManager* manager) {
DCHECK(IsUserNotesEnabled());
const auto& entry_it = model_map_.find(guid);
const auto& entry_it = model_map_.find(id);
DCHECK(entry_it != model_map_.end())
<< "A note instance without backing model was added to a page";

entry_it->second.managers.insert(manager);
}

void UserNoteService::OnNoteInstanceRemovedFromPage(const std::string& guid,
UserNotesManager* manager) {
void UserNoteService::OnNoteInstanceRemovedFromPage(
const base::UnguessableToken& id,
UserNotesManager* manager) {
DCHECK(IsUserNotesEnabled());

const auto& entry_it = model_map_.find(guid);
const auto& entry_it = model_map_.find(id);
DCHECK(entry_it != model_map_.end())
<< "A note model was destroyed before all its instances";

Expand All @@ -44,22 +46,23 @@ void UserNoteService::OnNoteInstanceRemovedFromPage(const std::string& guid,

// If there are no longer any pages displaying this model, destroy it.
if (entry_it->second.managers.empty()) {
model_map_.erase(guid);
model_map_.erase(id);
}
}

void UserNoteService::OnNoteFocused(const std::string& guid) {
void UserNoteService::OnNoteFocused(const base::UnguessableToken& id) {
DCHECK(IsUserNotesEnabled());
NOTIMPLEMENTED();
}

void UserNoteService::OnNoteCreationDone(const std::string& guid,
void UserNoteService::OnNoteCreationDone(const base::UnguessableToken& id,
const std::string& note_content) {
DCHECK(IsUserNotesEnabled());
NOTIMPLEMENTED();
}

void UserNoteService::OnNoteCreationCancelled(const std::string& guid) {
void UserNoteService::OnNoteCreationCancelled(
const base::UnguessableToken& id) {
DCHECK(IsUserNotesEnabled());
NOTIMPLEMENTED();
}
Expand Down
18 changes: 11 additions & 7 deletions components/user_notes/browser/user_note_service.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "base/gtest_prod_util.h"
#include "base/memory/safe_ref.h"
#include "base/memory/weak_ptr.h"
#include "base/unguessable_token.h"
#include "components/user_notes/interfaces/user_note_service_delegate.h"
#include "components/user_notes/interfaces/user_notes_ui_delegate.h"
#include "components/user_notes/model/user_note.h"
Expand All @@ -36,22 +37,22 @@ class UserNoteService : public KeyedService, public UserNotesUIDelegate {

// Called by |UserNotesManager| objects when a |UserNoteInstance| is added to
// the page they're attached to. Updates the model map to add a ref to the
// given |UserNotesManager| for the note with the specified GUID.
void OnNoteInstanceAddedToPage(const std::string& guid,
// given |UserNotesManager| for the note with the specified ID.
void OnNoteInstanceAddedToPage(const base::UnguessableToken& id,
UserNotesManager* manager);

// Same as |OnNoteInstanceAddedToPage|, except for when a note is removed from
// a page. Updates the model map to remove the ref to the given
// |UserNotesManager|. If this is the last page where the note was displayed,
// also deletes the model from the model map.
void OnNoteInstanceRemovedFromPage(const std::string& guid,
void OnNoteInstanceRemovedFromPage(const base::UnguessableToken& id,
UserNotesManager* manager);

// UserNotesUIDelegate implementation.
void OnNoteFocused(const std::string& guid) override;
void OnNoteCreationDone(const std::string& guid,
void OnNoteFocused(const base::UnguessableToken& id) override;
void OnNoteCreationDone(const base::UnguessableToken& id,
const std::string& note_content) override;
void OnNoteCreationCancelled(const std::string& guid) override;
void OnNoteCreationCancelled(const base::UnguessableToken& id) override;

private:
struct ModelMapEntry {
Expand All @@ -74,7 +75,10 @@ class UserNoteService : public KeyedService, public UserNotesUIDelegate {
// instance of that note, which is necessary to clean up the models when
// they're no longer in use and to remove notes from affected web pages when
// they're deleted by the user.
std::unordered_map<std::string, ModelMapEntry> model_map_;
std::unordered_map<base::UnguessableToken,
ModelMapEntry,
base::UnguessableTokenHash>
model_map_;

std::unique_ptr<UserNoteServiceDelegate> delegate_;
base::WeakPtrFactory<UserNoteService> weak_ptr_factory_{this};
Expand Down
70 changes: 36 additions & 34 deletions components/user_notes/browser/user_note_service_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@ namespace user_notes {

namespace {

const std::string kNoteId1 = "note-id-1";
const std::string kNoteId2 = "note-id-2";

// This is a hack to use a null |Page| in the tests instead of creating a
// mock, which is a relatively high effort task. Passing |nullptr| to this
// function and using the return value in a constructor requiring a |Page&|
Expand Down Expand Up @@ -51,30 +48,34 @@ class UserNoteServiceDelegateMockImpl : public UserNoteServiceDelegate {
class UserNoteServiceTest : public testing::Test {
public:
UserNoteServiceTest() {
// Create 2 note ids.
note_ids_.push_back(base::UnguessableToken::Create());
note_ids_.push_back(base::UnguessableToken::Create());

scoped_feature_list_.InitAndEnableFeature(user_notes::kUserNotes);
note_service_ = std::make_unique<UserNoteService>(
std::make_unique<UserNoteServiceDelegateMockImpl>());
auto note1 = std::make_unique<UserNote>(kNoteId1, GetTestUserNoteMetadata(),
GetTestUserNoteBody(),
GetTestUserNotePageTarget());
auto note2 = std::make_unique<UserNote>(kNoteId2, GetTestUserNoteMetadata(),
GetTestUserNoteBody(),
GetTestUserNotePageTarget());
auto note1 = std::make_unique<UserNote>(
note_ids_[0], GetTestUserNoteMetadata(), GetTestUserNoteBody(),
GetTestUserNotePageTarget());
auto note2 = std::make_unique<UserNote>(
note_ids_[1], GetTestUserNoteMetadata(), GetTestUserNoteBody(),
GetTestUserNotePageTarget());
UserNoteService::ModelMapEntry entry1(std::move(note1));
UserNoteService::ModelMapEntry entry2(std::move(note2));
note_service_->model_map_.emplace(kNoteId1, std::move(entry1));
note_service_->model_map_.emplace(kNoteId2, std::move(entry2));
note_service_->model_map_.emplace(note_ids_[0], std::move(entry1));
note_service_->model_map_.emplace(note_ids_[1], std::move(entry2));
}

int ManagerCountForId(const std::string& id) {
int ManagerCountForId(const base::UnguessableToken& id) {
const auto& entry_it = note_service_->model_map_.find(id);
if (entry_it == note_service_->model_map_.end()) {
return -1;
}
return entry_it->second.managers.size();
}

bool DoesModelExist(const std::string& id) {
bool DoesModelExist(const base::UnguessableToken& id) {
const auto& entry_it = note_service_->model_map_.find(id);
return entry_it != note_service_->model_map_.end();
}
Expand All @@ -84,26 +85,27 @@ class UserNoteServiceTest : public testing::Test {
protected:
base::test::ScopedFeatureList scoped_feature_list_;
std::unique_ptr<UserNoteService> note_service_;
std::vector<base::UnguessableToken> note_ids_;
};

TEST_F(UserNoteServiceTest, AddNoteIntancesToModelMap) {
// Verify initial state.
EXPECT_EQ(ModelMapSize(), 2);
EXPECT_EQ(ManagerCountForId(kNoteId1), 0);
EXPECT_EQ(ManagerCountForId(kNoteId2), 0);
EXPECT_EQ(ManagerCountForId(note_ids_[0]), 0);
EXPECT_EQ(ManagerCountForId(note_ids_[1]), 0);

// Simulate note instances being created in managers.
auto m1 =
UserNotesManager::CreateForTest(NullPage(), note_service_->GetSafeRef());
auto m2 =
UserNotesManager::CreateForTest(NullPage(), note_service_->GetSafeRef());
note_service_->OnNoteInstanceAddedToPage(kNoteId1, m1.get());
note_service_->OnNoteInstanceAddedToPage(kNoteId1, m2.get());
note_service_->OnNoteInstanceAddedToPage(kNoteId2, m1.get());
note_service_->OnNoteInstanceAddedToPage(note_ids_[0], m1.get());
note_service_->OnNoteInstanceAddedToPage(note_ids_[0], m2.get());
note_service_->OnNoteInstanceAddedToPage(note_ids_[1], m1.get());

EXPECT_EQ(ModelMapSize(), 2);
EXPECT_EQ(ManagerCountForId(kNoteId1), 2);
EXPECT_EQ(ManagerCountForId(kNoteId2), 1);
EXPECT_EQ(ManagerCountForId(note_ids_[0]), 2);
EXPECT_EQ(ManagerCountForId(note_ids_[1]), 1);
}

TEST_F(UserNoteServiceTest, RemoveNoteIntancesFromModelMap) {
Expand All @@ -112,34 +114,34 @@ TEST_F(UserNoteServiceTest, RemoveNoteIntancesFromModelMap) {
UserNotesManager::CreateForTest(NullPage(), note_service_->GetSafeRef());
auto m2 =
UserNotesManager::CreateForTest(NullPage(), note_service_->GetSafeRef());
note_service_->OnNoteInstanceAddedToPage(kNoteId1, m1.get());
note_service_->OnNoteInstanceAddedToPage(kNoteId1, m2.get());
note_service_->OnNoteInstanceAddedToPage(kNoteId2, m1.get());
note_service_->OnNoteInstanceAddedToPage(note_ids_[0], m1.get());
note_service_->OnNoteInstanceAddedToPage(note_ids_[0], m2.get());
note_service_->OnNoteInstanceAddedToPage(note_ids_[1], m1.get());

// Verify initial state.
EXPECT_EQ(ModelMapSize(), 2);
EXPECT_EQ(ManagerCountForId(kNoteId1), 2);
EXPECT_EQ(ManagerCountForId(kNoteId2), 1);
EXPECT_EQ(ManagerCountForId(note_ids_[0]), 2);
EXPECT_EQ(ManagerCountForId(note_ids_[1]), 1);

// Simulate a note instance being removed from a page. Its ref should be
// removed from the model map, but only for the removed note.
note_service_->OnNoteInstanceRemovedFromPage(kNoteId1, m1.get());
note_service_->OnNoteInstanceRemovedFromPage(note_ids_[0], m1.get());
EXPECT_EQ(ModelMapSize(), 2);
EXPECT_EQ(ManagerCountForId(kNoteId1), 1);
EXPECT_EQ(ManagerCountForId(kNoteId2), 1);
EXPECT_EQ(ManagerCountForId(note_ids_[0]), 1);
EXPECT_EQ(ManagerCountForId(note_ids_[1]), 1);

// Simulate the last instance of a note being removed from its page. Its model
// should be cleaned up from the model map.
note_service_->OnNoteInstanceRemovedFromPage(kNoteId1, m2.get());
note_service_->OnNoteInstanceRemovedFromPage(note_ids_[0], m2.get());
EXPECT_EQ(ModelMapSize(), 1);
EXPECT_EQ(ManagerCountForId(kNoteId2), 1);
EXPECT_FALSE(DoesModelExist(kNoteId1));
EXPECT_EQ(ManagerCountForId(note_ids_[1]), 1);
EXPECT_FALSE(DoesModelExist(note_ids_[0]));

// Repeat with the other note instance.
note_service_->OnNoteInstanceRemovedFromPage(kNoteId2, m1.get());
note_service_->OnNoteInstanceRemovedFromPage(note_ids_[1], m1.get());
EXPECT_EQ(ModelMapSize(), 0);
EXPECT_FALSE(DoesModelExist(kNoteId1));
EXPECT_FALSE(DoesModelExist(kNoteId2));
EXPECT_FALSE(DoesModelExist(note_ids_[0]));
EXPECT_FALSE(DoesModelExist(note_ids_[1]));
}

} // namespace user_notes
19 changes: 10 additions & 9 deletions components/user_notes/browser/user_notes_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,14 @@ UserNotesManager::UserNotesManager(content::Page& page,

UserNotesManager::~UserNotesManager() {
for (const auto& entry_it : instance_map_) {
service_->OnNoteInstanceRemovedFromPage(entry_it.second->model().guid(),
service_->OnNoteInstanceRemovedFromPage(entry_it.second->model().id(),
this);
}
}

UserNoteInstance* UserNotesManager::GetNoteInstance(const std::string& guid) {
const auto& entry_it = instance_map_.find(guid);
UserNoteInstance* UserNotesManager::GetNoteInstance(
const base::UnguessableToken id) {
const auto& entry_it = instance_map_.find(id);
if (entry_it == instance_map_.end()) {
return nullptr;
}
Expand All @@ -47,23 +48,23 @@ const std::vector<UserNoteInstance*> UserNotesManager::GetAllNoteInstances() {
return notes;
}

void UserNotesManager::RemoveNote(const std::string& guid) {
const auto& entry_it = instance_map_.find(guid);
void UserNotesManager::RemoveNote(const base::UnguessableToken id) {
const auto& entry_it = instance_map_.find(id);
DCHECK(entry_it != instance_map_.end())
<< "Attempted to remove a note instance from a page where it didn't "
"exist";

service_->OnNoteInstanceRemovedFromPage(guid, this);
service_->OnNoteInstanceRemovedFromPage(id, this);
instance_map_.erase(entry_it);
}

void UserNotesManager::AddNoteInstance(std::unique_ptr<UserNoteInstance> note) {
DCHECK(instance_map_.find(note->model().guid()) == instance_map_.end())
DCHECK(instance_map_.find(note->model().id()) == instance_map_.end())
<< "Attempted to add a note instance for the same note to the same page "
"more than once";

service_->OnNoteInstanceAddedToPage(note->model().guid(), this);
instance_map_.emplace(note->model().guid(), std::move(note));
service_->OnNoteInstanceAddedToPage(note->model().id(), this);
instance_map_.emplace(note->model().id(), std::move(note));
}

} // namespace user_notes
11 changes: 7 additions & 4 deletions components/user_notes/browser/user_notes_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#include "base/gtest_prod_util.h"
#include "base/memory/safe_ref.h"
#include "base/unguessable_token.h"
#include "components/user_notes/browser/user_note_instance.h"
#include "components/user_notes/browser/user_note_service.h"
#include "content/public/browser/page_user_data.h"
Expand Down Expand Up @@ -39,15 +40,15 @@ class UserNotesManager : public content::PageUserData<UserNotesManager> {
UserNotesManager(const UserNotesManager&) = delete;
UserNotesManager& operator=(const UserNotesManager&) = delete;

// Returns the note instance for the given GUID, or nullptr if this page does
// Returns the note instance for the given ID, or nullptr if this page does
// not have an instance of that note.
UserNoteInstance* GetNoteInstance(const std::string& guid);
UserNoteInstance* GetNoteInstance(const base::UnguessableToken id);

// Returns all note instances for the |Page| this object is attached to.
const std::vector<UserNoteInstance*> GetAllNoteInstances();

// Destroys the note instance associated with the given GUID.
void RemoveNote(const std::string& guid);
void RemoveNote(const base::UnguessableToken id);

// Stores the given note instance into this object's note instance container.
void AddNoteInstance(std::unique_ptr<UserNoteInstance> note);
Expand All @@ -69,7 +70,9 @@ class UserNotesManager : public content::PageUserData<UserNotesManager> {
base::SafeRef<UserNoteService> service_;

// The list of note instances displayed in this page, mapped by their note ID.
std::unordered_map<std::string, std::unique_ptr<UserNoteInstance>>
std::unordered_map<base::UnguessableToken,
std::unique_ptr<UserNoteInstance>,
base::UnguessableTokenHash>
instance_map_;
};

Expand Down

0 comments on commit da6cd3d

Please sign in to comment.