ContentCapture Refactorying: Move the node set to DocumentSession

Moved sent_nodes and changed_nodes to DocumentSession, it makes
the code more clear, and improves the performance a little bit,
because the node is regroup to a number of small one according
to the document.

This patch also ignores the content change of the unsent node,
adds test for this case.

Bug: 1137463
Change-Id: Id8146bdbd374d09ba27e22851e13ede4f1511b2d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2469220Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: Michael Bai <michaelbai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818086}

third_party/blink/renderer/core/content_capture/content_capture_test.cc

@@ -194,7 +194,8 @@ class ContentCaptureTest : public PageTestBase,
         "<p id='p6'>6</p>"
         "<p id='p7'>7</p>"
         "<p id='p8'>8</p>"
-        "<div id='d1'></div>");
+        "<div id='d1'></div>"
+        "<p id='invisible'>invisible</p>");
     platform()->SetAutoAdvanceNowToPendingTasks(false);
     // TODO(michaelbai): ContentCaptureManager should be get from LocalFrame.
     content_capture_manager_ =
@@ -279,6 +280,8 @@ class ContentCaptureTest : public PageTestBase,
   const Vector<cc::NodeInfo>& NodeIds() const { return node_ids_; }
   const Vector<Persistent<Node>> Nodes() const { return nodes_; }
 
+  Node& invisible_node() const { return *invisible_node_; }
+
  private:
   void ResetResult() {
     GetWebContentCaptureClient()->ResetResults();
@@ -298,10 +301,13 @@ class ContentCaptureTest : public PageTestBase,
       node_ids_.push_back(
           cc::NodeInfo(DOMNodeIds::IdForNode(node), GetRect(layout_object)));
     }
+    invisible_node_ = GetElementById("invisible")->firstChild();
+    DCHECK(invisible_node_.Get());
   }
 
   Vector<Persistent<Node>> nodes_;
   Vector<cc::NodeInfo> node_ids_;
+  Persistent<Node> invisible_node_;
   std::unique_ptr<WebContentCaptureClientTestHelper> content_capture_client_;
   Persistent<ContentCaptureManagerTestHelper> content_capture_manager_;
   Persistent<ContentCaptureLocalFrameClientHelper> local_frame_client_;
@@ -373,6 +379,30 @@ TEST_P(ContentCaptureTest, NodeOnlySendOnce) {
   EXPECT_TRUE(GetWebContentCaptureClient()->RemovedData().empty());
 }
 
+TEST_P(ContentCaptureTest, UnsentNode) {
+  // Send all nodes expect |invisible_node_|.
+  RunContentCaptureTask();
+  EXPECT_FALSE(GetWebContentCaptureClient()->Data().empty());
+  EXPECT_EQ(GetExpectedSecondResultSize(),
+            GetWebContentCaptureClient()->Data().size());
+
+  // Simulates the |invisible_node_| being changed, and verifies no content
+  // change because |invisible_node_| wasn't captured.
+  GetContentCaptureManager()->OnNodeTextChanged(invisible_node());
+  RunContentCaptureTask();
+  EXPECT_TRUE(GetWebContentCaptureClient()->Data().empty());
+  EXPECT_TRUE(GetWebContentCaptureClient()->UpdatedData().empty());
+  EXPECT_TRUE(GetWebContentCaptureClient()->RemovedData().empty());
+
+  // Simulates the |invisible_node_| being removed, and verifies no content
+  // change because |invisible_node_| wasn't captured.
+  GetContentCaptureManager()->OnLayoutTextWillBeDestroyed(invisible_node());
+  RunContentCaptureTask();
+  EXPECT_TRUE(GetWebContentCaptureClient()->Data().empty());
+  EXPECT_TRUE(GetWebContentCaptureClient()->UpdatedData().empty());
+  EXPECT_TRUE(GetWebContentCaptureClient()->RemovedData().empty());
+}
+
 TEST_P(ContentCaptureTest, RemoveNodeBeforeSendingOut) {
   // Capture the content, but didn't send them.
   GetContentCaptureTask()->SetTaskStopState(

third_party/blink/renderer/core/content_capture/task_session.cc

@@ -11,31 +11,21 @@
 
 namespace blink {
 
-TaskSession::DocumentSession::DocumentSession(
-    const Document& document,
-    HeapHashSet<WeakMember<const Node>>& sent_nodes,
-    SentNodeCountCallback& callback)
-    : document_(&document), sent_nodes_(&sent_nodes), callback_(callback) {}
+TaskSession::DocumentSession::DocumentSession(const Document& document,
+                                              SentNodeCountCallback& callback)
+    : document_(&document), callback_(callback) {}
 
 TaskSession::DocumentSession::~DocumentSession() {
   if (callback_.has_value())
     callback_.value().Run(total_sent_nodes_);
 }
 
-void TaskSession::DocumentSession::AddCapturedNode(Node& node,
-                                                   const gfx::Rect& rect) {
-  // Replace the previous rect if any.
-  captured_content_.Set(WeakMember<Node>(&node), rect);
-}
-
-void TaskSession::DocumentSession::AddDetachedNode(int64_t id) {
-  detached_nodes_.emplace_back(id);
-}
-
-void TaskSession::DocumentSession::AddChangedNode(Node& node,
-                                                  const gfx::Rect& rect) {
-  // Replace the previous rect if any.
-  changed_content_.Set(WeakMember<Node>(&node), rect);
+bool TaskSession::DocumentSession::AddDetachedNode(const Node& node) {
+  if (sent_nodes_.Contains(&node)) {
+    detached_nodes_.emplace_back(reinterpret_cast<int64_t>(&node));
+    return true;
+  }
+  return false;
 }
 
 WebVector<int64_t> TaskSession::DocumentSession::MoveDetachedNodes() {
@@ -46,8 +36,8 @@ ContentHolder* TaskSession::DocumentSession::GetNextUnsentNode() {
   while (!captured_content_.IsEmpty()) {
     auto node = captured_content_.begin()->key;
     const gfx::Rect rect = captured_content_.Take(node);
-    if (node && node->GetLayoutObject() && !sent_nodes_->Contains(node)) {
-      sent_nodes_->insert(WeakMember<const Node>(node));
+    if (node && node->GetLayoutObject() && !sent_nodes_.Contains(node)) {
+      sent_nodes_.insert(WeakMember<const Node>(node));
       total_sent_nodes_++;
       return MakeGarbageCollected<ContentHolder>(node, rect);
     }
@@ -67,16 +57,40 @@ ContentHolder* TaskSession::DocumentSession::GetNextChangedNode() {
   return nullptr;
 }
 
+bool TaskSession::DocumentSession::AddChangedNode(Node& node) {
+  // No need to save the node that hasn't been sent because it will be captured
+  // once being on screen.
+  if (sent_nodes_.Contains(&node)) {
+    changed_nodes_.insert(WeakMember<Node>(&node));
+    return true;
+  }
+  return false;
+}
+
+void TaskSession::DocumentSession::OnContentCaptured(
+    Node& node,
+    const gfx::Rect& visual_rect) {
+  if (changed_nodes_.Take(&node)) {
+    changed_content_.Set(WeakMember<Node>(&node), visual_rect);
+  } else if (!sent_nodes_.Contains(&node)) {
+    captured_content_.Set(WeakMember<Node>(&node), visual_rect);
+  }  // else |node| has been sent and unchanged.
+}
+
 void TaskSession::DocumentSession::Trace(Visitor* visitor) const {
   visitor->Trace(captured_content_);
-  visitor->Trace(document_);
   visitor->Trace(changed_content_);
+  visitor->Trace(document_);
+  visitor->Trace(sent_nodes_);
+  visitor->Trace(changed_nodes_);
 }
 
 void TaskSession::DocumentSession::Reset() {
   changed_content_.clear();
   captured_content_.clear();
   detached_nodes_.Clear();
+  sent_nodes_.clear();
+  changed_nodes_.clear();
 }
 
 TaskSession::TaskSession() = default;
@@ -106,43 +120,27 @@ void TaskSession::GroupCapturedContentByDocument(
   // later replace the previous.
   for (const auto& i : captured_content) {
     if (Node* node = DOMNodeIds::NodeForId(i.node_id)) {
-      if (changed_nodes_.Take(node)) {
-        // The changed node might not be sent.
-        if (sent_nodes_.Contains(node)) {
-          EnsureDocumentSession(node->GetDocument())
-              .AddChangedNode(*node, i.visual_rect);
-        } else {
-          EnsureDocumentSession(node->GetDocument())
-              .AddCapturedNode(*node, i.visual_rect);
-        }
-        continue;
-      }
-      if (!sent_nodes_.Contains(node)) {
-        EnsureDocumentSession(node->GetDocument())
-            .AddCapturedNode(*node, i.visual_rect);
-      }
+      EnsureDocumentSession(node->GetDocument())
+          .OnContentCaptured(*node, i.visual_rect);
     }
   }
 }
 
 void TaskSession::OnNodeDetached(const Node& node) {
-  if (sent_nodes_.Contains(&node)) {
-    EnsureDocumentSession(node.GetDocument())
-        .AddDetachedNode(reinterpret_cast<int64_t>(&node));
+  if (EnsureDocumentSession(node.GetDocument()).AddDetachedNode(node))
     has_unsent_data_ = true;
-  }
 }
 
 void TaskSession::OnNodeChanged(Node& node) {
-  changed_nodes_.insert(WeakMember<Node>(&node));
+  if (EnsureDocumentSession(node.GetDocument()).AddChangedNode(node))
+    has_unsent_data_ = true;
 }
 
 TaskSession::DocumentSession& TaskSession::EnsureDocumentSession(
     const Document& doc) {
   DocumentSession* doc_session = GetDocumentSession(doc);
   if (!doc_session) {
-    doc_session =
-        MakeGarbageCollected<DocumentSession>(doc, sent_nodes_, callback_);
+    doc_session = MakeGarbageCollected<DocumentSession>(doc, callback_);
     to_document_session_.insert(&doc, doc_session);
   }
   return *doc_session;
@@ -157,8 +155,6 @@ TaskSession::DocumentSession* TaskSession::GetDocumentSession(
 }
 
 void TaskSession::Trace(Visitor* visitor) const {
-  visitor->Trace(sent_nodes_);
-  visitor->Trace(changed_nodes_);
   visitor->Trace(to_document_session_);
 }
 

third_party/blink/renderer/core/content_capture/task_session.h

@@ -50,12 +50,16 @@ class TaskSession final : public GarbageCollected<TaskSession> {
     using SentNodeCountCallback = base::RepeatingCallback<void(size_t)>;
 
     DocumentSession(const Document& document,
-                    HeapHashSet<WeakMember<const Node>>& sent_nodes,
                     SentNodeCountCallback& call_back);
     ~DocumentSession();
-    void AddCapturedNode(Node& node, const gfx::Rect& rect);
-    void AddDetachedNode(int64_t id);
-    void AddChangedNode(Node& node, const gfx::Rect& rect);
+    // Add the given |node| to changed node set if the node was sent, return
+    // true if succeed.
+    bool AddChangedNode(Node& node);
+    // Add the given |node| to detached node set if the node was sent, return
+    // true if succeed.
+    bool AddDetachedNode(const Node& node);
+    // Invoked on the content of this document is captured.
+    void OnContentCaptured(Node& node, const gfx::Rect& visual_rect);
     bool HasUnsentData() const {
       return HasUnsentCapturedContent() || HasUnsentChangedContent() ||
              HasUnsentDetachedNodes();
@@ -83,15 +87,19 @@ class TaskSession final : public GarbageCollected<TaskSession> {
     void Trace(Visitor*) const;
 
    private:
-    // The captured content that belongs to this document.
+    // The list of captured content that needs to be sent.
     HeapHashMap<WeakMember<Node>, gfx::Rect> captured_content_;
+    // The list of changed nodes that needs to be sent.
+    HeapHashMap<WeakMember<Node>, gfx::Rect> changed_content_;
     // The list of content id of node that has been detached from the
-    // LayoutTree.
+    // LayoutTree and needs to be sent.
     WebVector<int64_t> detached_nodes_;
+
     WeakMember<const Document> document_;
-    HeapHashSet<WeakMember<const Node>>* sent_nodes_;
-    // The list of changed nodes that needs to be sent.
-    HeapHashMap<WeakMember<Node>, gfx::Rect> changed_content_;
+    // A set of weak reference of the node that has been sent.
+    HeapHashSet<WeakMember<const Node>> sent_nodes_;
+    // A set of node whose value has been changed since last capture.
+    HeapHashSet<WeakMember<Node>> changed_nodes_;
 
     bool first_data_has_sent_ = false;
     // This is for the metrics to record the total node that has been sent.
@@ -131,12 +139,6 @@ class TaskSession final : public GarbageCollected<TaskSession> {
   DocumentSession& EnsureDocumentSession(const Document& doc);
   DocumentSession* GetDocumentSession(const Document& document) const;
 
-  // A set of weak reference of the node that has been sent.
-  HeapHashSet<WeakMember<const Node>> sent_nodes_;
-
-  // The list of node whose value has changed.
-  HeapHashSet<WeakMember<Node>> changed_nodes_;
-
   // This owns the DocumentSession which is released along with Document.
   HeapHashMap<WeakMember<const Document>, Member<DocumentSession>>
       to_document_session_;