[UserNotes] Start FrameUserNoteChange::Apply implementation

Implements the first part of the FrameUserNoteChange::Apply method, up
until the point where async requests to the renderer must be made. The
async infra is set up but calls to Mojo will be added in the follow-up
CL.

Detailed changes:
  - Added a ref to UserNoteService inside FrameUserNoteChanges. The ref
    is needed to get refs to the note models (and eventually to get
    temporary instances from the service for the note authoring flow).
    Updated the CalculateNoteChanges method and unit tests to account
    for the new parameter in the constructor.
  - Apply() now takes a callback as param (the initial plan was to have
    Apply() call a method on the service as a callback), which will
    make it easier for callers to deal with the 2 possible flows for
    Apply(): storage updates and navigations. It also greatly
    simplifies tests.
  - Added a getter for a note model in the service, needed to construct
    NoteInstance objects.
  - Added unit tests for Apply() (test cases for the note authoring
    workflow will follow when note authoring is implemented).
  - Made other small fixes such as UserNoteManager methods now correctly
    taking params by ref instead of value and using a nullptr as the
    delegate in the tab helper tests.

Bug: 1321920
Change-Id: I6ae2703ad3f202eb2060bc06799092cc55e586b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3622099
Reviewed-by: Caroline Rising <corising@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: Guillaume Jenkins <gujen@google.com>
Cr-Commit-Position: refs/heads/main@{#1001112}

chrome/browser/user_notes/user_notes_tab_helper_unittest.cc

@@ -43,8 +43,8 @@ MATCHER(HasUserNoteManager, "") {
 
 class MockUserNoteService : public UserNoteService {
  public:
-  MockUserNoteService()
-      : UserNoteService(std::unique_ptr<UserNoteServiceDelegate>()) {}
+  // A service delegate is not needed for these tests, so pass nullptr.
+  MockUserNoteService() : UserNoteService(/*delegate=*/nullptr) {}
 
   MOCK_METHOD(void,
               OnFrameNavigated,

components/user_notes/browser/BUILD.gn

@@ -31,6 +31,7 @@ static_library("browser") {
 source_set("unit_tests") {
   testonly = true
   sources = [
+    "frame_user_note_changes_unittest.cc",
     "user_note_base_test.cc",
     "user_note_base_test.h",
     "user_note_manager_unittest.cc",
@@ -43,6 +44,7 @@ source_set("unit_tests") {
     "//base/test:test_support",
     "//components/user_notes:features",
     "//components/user_notes/interfaces",
+    "//components/user_notes/model",
     "//components/user_notes/model:unit_tests",
     "//content/public/browser",
     "//content/test:test_support",

components/user_notes/browser/frame_user_note_changes.cc

@@ -4,42 +4,84 @@
 
 #include "components/user_notes/browser/frame_user_note_changes.h"
 
-#include "base/notreached.h"
+#include "base/barrier_closure.h"
+#include "components/user_notes/browser/user_note_manager.h"
+#include "components/user_notes/model/user_note.h"
+#include "components/user_notes/model/user_note_target.h"
 #include "content/public/browser/render_frame_host.h"
 
 namespace user_notes {
 
-FrameUserNoteChanges::FrameUserNoteChanges(content::RenderFrameHost* rfh,
-                                           const ChangeList& notes_added,
-                                           const ChangeList& notes_modified,
-                                           const ChangeList& notes_removed)
-    : rfh_(rfh),
+FrameUserNoteChanges::FrameUserNoteChanges(
+    base::SafeRef<UserNoteService> service,
+    content::RenderFrameHost* rfh,
+    const ChangeList& notes_added,
+    const ChangeList& notes_modified,
+    const ChangeList& notes_removed)
+    : service_(service),
+      rfh_(rfh),
       notes_added_(notes_added),
       notes_modified_(notes_modified),
       notes_removed_(notes_removed) {
   DCHECK(!notes_added_.empty() || !notes_modified_.empty() ||
          !notes_removed_.empty());
+  DCHECK(rfh_);
 }
 
-FrameUserNoteChanges::FrameUserNoteChanges(content::RenderFrameHost* rfh,
-                                           ChangeList&& notes_added,
-                                           ChangeList&& notes_modified,
-                                           ChangeList&& notes_removed)
-    : rfh_(rfh),
+FrameUserNoteChanges::FrameUserNoteChanges(
+    base::SafeRef<UserNoteService> service,
+    content::RenderFrameHost* rfh,
+    ChangeList&& notes_added,
+    ChangeList&& notes_modified,
+    ChangeList&& notes_removed)
+    : service_(service),
+      rfh_(rfh),
       notes_added_(std::move(notes_added)),
       notes_modified_(std::move(notes_modified)),
       notes_removed_(std::move(notes_removed)) {
   DCHECK(!notes_added_.empty() || !notes_modified_.empty() ||
          !notes_removed_.empty());
+  DCHECK(rfh_);
 }
 
 FrameUserNoteChanges::FrameUserNoteChanges(FrameUserNoteChanges&& other) =
     default;
 
 FrameUserNoteChanges::~FrameUserNoteChanges() = default;
 
-void FrameUserNoteChanges::Apply() {
-  NOTIMPLEMENTED();
+void FrameUserNoteChanges::Apply(base::OnceClosure callback) {
+  UserNoteManager* manager = UserNoteManager::GetForPage(rfh_->GetPage());
+  DCHECK(manager);
+
+  // Removed notes can be synchronously deleted from the note manager. There is
+  // no need to wait for the async removal of the page highlights on the
+  // renderer side.
+  for (const base::UnguessableToken& note_id : notes_removed_) {
+    manager->RemoveNote(note_id);
+  }
+
+  // For added notes, the async highlight creation on the renderer side must be
+  // awaited, because the order in which notes are shown in the Notes UI depends
+  // on the order of the corresponding highlights in the page. Use a barrier
+  // closure to wait until all note highlights have been created in the page.
+  base::RepeatingClosure barrier =
+      base::BarrierClosure(notes_added_.size(), std::move(callback));
+  for (const base::UnguessableToken& note_id : notes_added_) {
+    const UserNote* note = service_->GetNoteModel(note_id);
+    DCHECK(note);
+
+    // TODO(gujen): Support the note authoring workflow, in which case a
+    // temporary note instance will already exist in the service's temporary
+    // map, which we can synchronously move to the note manager here.
+
+    std::unique_ptr<UserNoteInstance> instance_unique = MakeNoteInstance(note);
+    manager->AddNoteInstance(std::move(instance_unique), barrier);
+  }
+}
+
+std::unique_ptr<UserNoteInstance> FrameUserNoteChanges::MakeNoteInstance(
+    const UserNote* note_model) const {
+  return std::make_unique<UserNoteInstance>(note_model->GetSafeRef());
 }
 
 }  // namespace user_notes

components/user_notes/browser/frame_user_note_changes.h

@@ -7,44 +7,57 @@
 
 #include <vector>
 
+#include "base/callback.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/raw_ptr.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"
 
 namespace content {
 class RenderFrameHost;
 }  // namespace content
 
 namespace user_notes {
 
-typedef std::vector<base::UnguessableToken> ChangeList;
-
 // A container to represent changes to a frame's displayed User Notes. Includes
 // the logic to apply the changes in the associated frame.
 class FrameUserNoteChanges {
  public:
-  FrameUserNoteChanges(content::RenderFrameHost* rfh,
+  using ChangeList = std::vector<base::UnguessableToken>;
+
+  FrameUserNoteChanges(base::SafeRef<UserNoteService> service,
+                       content::RenderFrameHost* rfh,
                        const ChangeList& notes_added,
                        const ChangeList& notes_modified,
                        const ChangeList& notes_removed);
-  FrameUserNoteChanges(content::RenderFrameHost* rfh,
+  FrameUserNoteChanges(base::SafeRef<UserNoteService> service,
+                       content::RenderFrameHost* rfh,
                        ChangeList&& notes_added,
                        ChangeList&& notes_modified,
                        ChangeList&& notes_removed);
   FrameUserNoteChanges(const FrameUserNoteChanges&) = delete;
   FrameUserNoteChanges& operator=(const FrameUserNoteChanges&) = delete;
   FrameUserNoteChanges(FrameUserNoteChanges&& other);
-  ~FrameUserNoteChanges();
+  virtual ~FrameUserNoteChanges();
+
+  // Kicks off the asynchronous logic to add and remove highlights in the frame
+  // as necessary. Invokes the provided callback after the changes have fully
+  // propagated to the note manager and the new notes have had their highlights
+  // created in the web page.
+  void Apply(base::OnceClosure callback);
 
-  // Kicks off the asynchronous logic to propagate the note changes to the
-  // frame, namely creating and removing highlights as necessary. Then, notifies
-  // the note service that the changes have been fully applied for this frame so
-  // it can request the UI to refresh itself as needed.
-  void Apply();
+ protected:
+  // Called by `Apply()` to construct a new note instance pointing to the
+  // provided model. Can be overridden by tests to construct a mocked instance.
+  virtual std::unique_ptr<UserNoteInstance> MakeNoteInstance(
+      const UserNote* note_model) const;
 
  private:
   FRIEND_TEST_ALL_PREFIXES(UserNoteUtilsTest, CalculateNoteChanges);
 
+  base::SafeRef<UserNoteService> service_;
   raw_ptr<content::RenderFrameHost> rfh_;
   ChangeList notes_added_;
   ChangeList notes_modified_;

components/user_notes/browser/frame_user_note_changes_unittest.cc

@@ -0,0 +1,167 @@
+// Copyright 2022 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/user_notes/browser/frame_user_note_changes.h"
+
+#include <memory>
+#include <vector>
+
+#include "base/memory/safe_ref.h"
+#include "base/unguessable_token.h"
+#include "components/user_notes/browser/user_note_base_test.h"
+#include "components/user_notes/browser/user_note_instance.h"
+#include "components/user_notes/model/user_note.h"
+#include "content/public/browser/render_frame_host.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;
+using testing::Mock;
+
+namespace user_notes {
+
+namespace {
+
+// A mock for a note instance that synchronously invokes the callback when
+// initializing the text highlight.
+class MockUserNoteInstance : public UserNoteInstance {
+ public:
+  explicit MockUserNoteInstance(base::SafeRef<UserNote> model_ref)
+      : UserNoteInstance(model_ref) {}
+
+  MOCK_METHOD(void,
+              InitializeHighlightIfNeeded,
+              (base::OnceClosure callback),
+              (override));
+};
+
+// Partially mock the object under test so calls to `MakeNoteInstance` can be
+// intercepted, allowing the tests to create mocked instances.
+class MockFrameUserNoteChanges : public FrameUserNoteChanges {
+ public:
+  MockFrameUserNoteChanges(
+      base::SafeRef<UserNoteService> service,
+      content::RenderFrameHost* rfh,
+      const FrameUserNoteChanges::ChangeList& notes_added,
+      const FrameUserNoteChanges::ChangeList& notes_modified,
+      const FrameUserNoteChanges::ChangeList& notes_removed)
+      : FrameUserNoteChanges(service,
+                             rfh,
+                             notes_added,
+                             notes_modified,
+                             notes_removed) {}
+
+  MOCK_METHOD(std::unique_ptr<UserNoteInstance>,
+              MakeNoteInstance,
+              (const UserNote* note_model),
+              (const override));
+};
+
+void MockInitializeHighlightIfNeeded(base::OnceClosure callback) {
+  std::move(callback).Run();
+}
+
+std::unique_ptr<UserNoteInstance> MockMakeNoteInstance(
+    const UserNote* note_model) {
+  auto instance_mock =
+      std::make_unique<MockUserNoteInstance>(note_model->GetSafeRef());
+
+  EXPECT_CALL(*instance_mock, InitializeHighlightIfNeeded(_))
+      .Times(1)
+      .WillOnce(&MockInitializeHighlightIfNeeded);
+
+  return instance_mock;
+}
+
+}  // namespace
+
+class FrameUserNoteChangesTest : public UserNoteBaseTest {};
+
+// Tests that added notes correctly kick off highlight initialization on the
+// renderer side, and new instances are correctly added to the note manager.
+TEST_F(FrameUserNoteChangesTest, ApplyAddedNotes) {
+  AddNewNotesToService(3);
+  UserNoteManager* m = ConfigureNewManager();
+  AddNewInstanceToManager(m, note_ids_[0]);
+
+  std::vector<base::UnguessableToken> added({note_ids_[1], note_ids_[2]});
+  std::vector<base::UnguessableToken> modified;
+  std::vector<base::UnguessableToken> removed;
+
+  auto mock_changes = std::make_unique<MockFrameUserNoteChanges>(
+      note_service_->GetSafeRef(), web_contents_list_[0]->GetMainFrame(), added,
+      modified, removed);
+
+  EXPECT_CALL(*mock_changes, MakeNoteInstance(_))
+      .Times(2)
+      .WillRepeatedly(&MockMakeNoteInstance);
+
+  mock_changes->Apply(base::BindOnce([] {}));
+
+  // The mocks ensure the callback is invoked synchronously, so verifications
+  // can happen immediately.
+  EXPECT_EQ(InstanceMapSize(m), 3u);
+  EXPECT_TRUE(m->GetNoteInstance(note_ids_[0]));
+  EXPECT_TRUE(m->GetNoteInstance(note_ids_[1]));
+  EXPECT_TRUE(m->GetNoteInstance(note_ids_[2]));
+}
+
+// Tests that modified notes don't impact instances in the note manager.
+TEST_F(FrameUserNoteChangesTest, ApplyModifiedNotes) {
+  AddNewNotesToService(3);
+  UserNoteManager* m = ConfigureNewManager();
+  AddNewInstanceToManager(m, note_ids_[0]);
+  AddNewInstanceToManager(m, note_ids_[1]);
+  AddNewInstanceToManager(m, note_ids_[2]);
+
+  std::vector<base::UnguessableToken> added;
+  std::vector<base::UnguessableToken> modified({note_ids_[0], note_ids_[2]});
+  std::vector<base::UnguessableToken> removed;
+
+  auto mock_changes = std::make_unique<MockFrameUserNoteChanges>(
+      note_service_->GetSafeRef(), web_contents_list_[0]->GetMainFrame(), added,
+      modified, removed);
+
+  EXPECT_CALL(*mock_changes, MakeNoteInstance(_)).Times(0);
+
+  mock_changes->Apply(base::BindOnce([] {}));
+
+  // The mocks ensure the callback is invoked synchronously, so verifications
+  // can happen immediately.
+  EXPECT_EQ(InstanceMapSize(m), 3u);
+  EXPECT_TRUE(m->GetNoteInstance(note_ids_[0]));
+  EXPECT_TRUE(m->GetNoteInstance(note_ids_[1]));
+  EXPECT_TRUE(m->GetNoteInstance(note_ids_[2]));
+}
+
+// Tests that removed notes correctly have their instances removed from the
+// note manager.
+TEST_F(FrameUserNoteChangesTest, ApplyRemovedNotes) {
+  AddNewNotesToService(3);
+  UserNoteManager* m = ConfigureNewManager();
+  AddNewInstanceToManager(m, note_ids_[0]);
+  AddNewInstanceToManager(m, note_ids_[1]);
+  AddNewInstanceToManager(m, note_ids_[2]);
+
+  std::vector<base::UnguessableToken> added;
+  std::vector<base::UnguessableToken> modified;
+  std::vector<base::UnguessableToken> removed({note_ids_[0], note_ids_[2]});
+
+  auto mock_changes = std::make_unique<MockFrameUserNoteChanges>(
+      note_service_->GetSafeRef(), web_contents_list_[0]->GetMainFrame(), added,
+      modified, removed);
+
+  EXPECT_CALL(*mock_changes, MakeNoteInstance(_)).Times(0);
+
+  mock_changes->Apply(base::BindOnce([] {}));
+
+  // The mocks ensure the callback is invoked synchronously, so verifications
+  // can happen immediately.
+  EXPECT_EQ(InstanceMapSize(m), 1u);
+  EXPECT_TRUE(m->GetNoteInstance(note_ids_[1]));
+  EXPECT_FALSE(m->GetNoteInstance(note_ids_[0]));
+  EXPECT_FALSE(m->GetNoteInstance(note_ids_[2]));
+}
+
+}  // namespace user_notes

components/user_notes/browser/user_note_base_test.cc

@@ -86,8 +86,8 @@ size_t UserNoteBaseTest::ManagerCountForId(
   return entry_it->second.managers.size();
 }
 
-bool UserNoteBaseTest::DoesModelExist(const base::UnguessableToken& id) {
-  const auto& entry_it = note_service_->model_map_.find(id);
+bool UserNoteBaseTest::DoesModelExist(const base::UnguessableToken& note_id) {
+  const auto& entry_it = note_service_->model_map_.find(note_id);
   return entry_it != note_service_->model_map_.end();
 }
 

components/user_notes/browser/user_note_base_test.h

@@ -37,11 +37,11 @@ class UserNoteBaseTest : public content::RenderViewHostTestHarness {
   void AddNewInstanceToManager(UserNoteManager* manager,
                                base::UnguessableToken note_id);
 
-  size_t ManagerCountForId(const base::UnguessableToken& id);
+  size_t ManagerCountForId(const base::UnguessableToken& note_id);
 
-  bool DoesModelExist(const base::UnguessableToken& id);
+  bool DoesModelExist(const base::UnguessableToken& note_id);
 
-  bool DoesManagerExistForId(const base::UnguessableToken& id,
+  bool DoesManagerExistForId(const base::UnguessableToken& note_id,
                              UserNoteManager* manager);
 
   size_t ModelMapSize();