viz: Refactor hit-test data submission trace flow.

Modifies GetActiveHitTestRegionList in hit_test_manager in order to
optionally return the ActiveFrameIndex. This way we dont have to look it
up again when checking for newly submitted hit-test data. Will also be
used in future changes of hit_test_aggregator for similar purposes.

R=riajiang@chromium.org

Change-Id: Ied4b0f690184017969ed42928230a3c5977b21c2
Reviewed-on: https://chromium-review.googlesource.com/c/1296892
Commit-Queue: Alexander Shah <zandershah@google.com>
Reviewed-by: Ria Jiang <riajiang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603498}

components/viz/service/hit_test/hit_test_aggregator.cc

@@ -58,16 +58,15 @@ void HitTestAggregator::SendHitTestData() {
 }
 
 base::Optional<int64_t> HitTestAggregator::GetTraceIdIfUpdated(
-    const SurfaceId& surface_id) {
+    const SurfaceId& surface_id,
+    uint64_t active_frame_index) {
   bool enabled;
   TRACE_EVENT_CATEGORY_GROUP_ENABLED(
       TRACE_DISABLED_BY_DEFAULT("viz.hit_testing_flow"), &enabled);
   if (!enabled)
     return base::nullopt;
 
-  int32_t active_frame_index =
-      hit_test_manager_->GetActiveFrameIndex(surface_id);
-  int32_t& frame_index = last_active_frame_index_[surface_id];
+  uint64_t& frame_index = last_active_frame_index_[surface_id.frame_sink_id()];
   if (frame_index == active_frame_index)
     return base::nullopt;
   frame_index = active_frame_index;
@@ -75,13 +74,17 @@ base::Optional<int64_t> HitTestAggregator::GetTraceIdIfUpdated(
 }
 
 void HitTestAggregator::AppendRoot(const SurfaceId& surface_id) {
+  uint64_t active_frame_index;
   const HitTestRegionList* hit_test_region_list =
       hit_test_manager_->GetActiveHitTestRegionList(
-          local_surface_id_lookup_delegate_, surface_id.frame_sink_id());
+          local_surface_id_lookup_delegate_, surface_id.frame_sink_id(),
+          &active_frame_index);
+
   if (!hit_test_region_list)
     return;
 
-  base::Optional<int64_t> trace_id = GetTraceIdIfUpdated(surface_id);
+  base::Optional<int64_t> trace_id =
+      GetTraceIdIfUpdated(surface_id, active_frame_index);
   TRACE_EVENT_WITH_FLOW1(
       TRACE_DISABLED_BY_DEFAULT("viz.hit_testing_flow"), "Event.Pipeline",
       TRACE_ID_GLOBAL(trace_id.value_or(-1)),
@@ -126,9 +129,11 @@ size_t HitTestAggregator::AppendRegion(size_t region_index,
 
     referenced_child_regions_.insert(region.frame_sink_id);
 
+    uint64_t active_frame_index;
     const HitTestRegionList* hit_test_region_list =
         hit_test_manager_->GetActiveHitTestRegionList(
-            local_surface_id_lookup_delegate_, region.frame_sink_id);
+            local_surface_id_lookup_delegate_, region.frame_sink_id,
+            &active_frame_index);
     if (!hit_test_region_list) {
       // Hit-test data not found with this FrameSinkId. This means that it
       // failed to find a surface corresponding to this FrameSinkId at surface
@@ -156,7 +161,8 @@ size_t HitTestAggregator::AppendRegion(size_t region_index,
                 region.frame_sink_id);
         SurfaceId surface_id(region.frame_sink_id, local_surface_id);
 
-        base::Optional<int64_t> trace_id = GetTraceIdIfUpdated(surface_id);
+        base::Optional<int64_t> trace_id =
+            GetTraceIdIfUpdated(surface_id, active_frame_index);
         TRACE_EVENT_WITH_FLOW1(
             TRACE_DISABLED_BY_DEFAULT("viz.hit_testing_flow"), "Event.Pipeline",
             TRACE_ID_GLOBAL(trace_id.value_or(-1)),

components/viz/service/hit_test/hit_test_aggregator.h

@@ -60,10 +60,11 @@ class VIZ_SERVICE_EXPORT HitTestAggregator {
                    int32_t child_count);
 
   // Returns the |trace_id| of the |begin_frame_ack| in the active frame for
-  // the given surface_id if it is different than when it was last queried.
+  // the given |surface_id| if it is different than when it was last queried.
   // This is used in order to ensure that the flow between receiving hit-test
   // data and aggregating is included only once per submission.
-  base::Optional<int64_t> GetTraceIdIfUpdated(const SurfaceId& surface_id);
+  base::Optional<int64_t> GetTraceIdIfUpdated(const SurfaceId& surface_id,
+                                              uint64_t active_frame_index);
 
   const HitTestManager* const hit_test_manager_;
 
@@ -89,7 +90,7 @@ class VIZ_SERVICE_EXPORT HitTestAggregator {
   // to detect cycles.
   base::flat_set<FrameSinkId> referenced_child_regions_;
 
-  base::flat_map<SurfaceId, int32_t> last_active_frame_index_;
+  base::flat_map<FrameSinkId, uint64_t> last_active_frame_index_;
 
   // Handles the case when this object is deleted after
   // the PostTaskAggregation call is scheduled but before invocation.

components/viz/service/hit_test/hit_test_manager.cc

@@ -68,7 +68,8 @@ void HitTestManager::SubmitHitTestRegionList(
 
 const HitTestRegionList* HitTestManager::GetActiveHitTestRegionList(
     LatestLocalSurfaceIdLookupDelegate* delegate,
-    const FrameSinkId& frame_sink_id) const {
+    const FrameSinkId& frame_sink_id,
+    uint64_t* store_active_frame_index) const {
   if (!delegate)
     return nullptr;
 
@@ -85,6 +86,8 @@ const HitTestRegionList* HitTestManager::GetActiveHitTestRegionList(
   Surface* surface = surface_manager_->GetSurfaceForId(surface_id);
   DCHECK(surface);
   uint64_t frame_index = surface->GetActiveFrameIndex();
+  if (store_active_frame_index)
+    *store_active_frame_index = frame_index;
 
   auto& frame_index_map = search->second;
   auto search2 = frame_index_map.find(frame_index);
@@ -94,11 +97,6 @@ const HitTestRegionList* HitTestManager::GetActiveHitTestRegionList(
   return &search2->second;
 }
 
-int32_t HitTestManager::GetActiveFrameIndex(const SurfaceId& id) const {
-  Surface* surface = surface_manager_->GetSurfaceForId(id);
-  return surface->GetActiveFrameIndex();
-}
-
 int64_t HitTestManager::GetTraceId(const SurfaceId& id) const {
   Surface* surface = surface_manager_->GetSurfaceForId(id);
   return surface->GetActiveFrame().metadata.begin_frame_ack.trace_id;

components/viz/service/hit_test/hit_test_manager.h

@@ -45,12 +45,13 @@ class VIZ_SERVICE_EXPORT HitTestManager : public SurfaceObserver {
   // Returns the HitTestRegionList corresponding to the given
   // |frame_sink_id| and the active CompositorFrame matched by frame_index.
   // The returned pointer is not stable and should not be stored or used after
-  // calling any non-const methods on this class.
+  // calling any non-const methods on this class. ActiveFrameIndex is stored
+  // if |store_active_frame_index| is given, which is used to detect updates.
   const HitTestRegionList* GetActiveHitTestRegionList(
       LatestLocalSurfaceIdLookupDelegate* delegate,
-      const FrameSinkId& frame_sink_id) const;
+      const FrameSinkId& frame_sink_id,
+      uint64_t* store_active_frame_index = nullptr) const;
 
-  int32_t GetActiveFrameIndex(const SurfaceId& id) const;
   int64_t GetTraceId(const SurfaceId& id) const;
 
  private: