Implement HitTestAggregator

components/viz/service/hit_test/hit_test_aggregator.cc

Index: components/viz/service/hit_test/hit_test_aggregator.cc
diff --git a/components/viz/service/hit_test/hit_test_aggregator.cc b/components/viz/service/hit_test/hit_test_aggregator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c36a4ecb1086982ef377e57b4269e2b4511f7f2b
--- /dev/null
+++ b/components/viz/service/hit_test/hit_test_aggregator.cc
@@ -0,0 +1,196 @@
+// Copyright 2017 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/viz/service/hit_test/hit_test_aggregator.h"
+#include "components/viz/common/hit_test/display_hit_test_data.h"
+
+namespace viz {
+namespace hit_test {
+
+namespace {
+// TODO(gklassen): Review and select appropriate sizes ( based on
+// telemetry / UMA? ).
+constexpr int kInitialSize = 1024;
+constexpr int kIncrementalSize = 1024;
+
+bool ValidateHitTestRegion(
+    const hit_test::mojom::HitTestRegionPtr& hit_test_region) {
+  if (hit_test_region->flags == hit_test::mojom::kHitTestChildSurface) {
+    if (!hit_test_region->surface_id.is_valid())
+      return false;
+  }
+  return true;
+}
+bool ValidateHitTestData(const hit_test::mojom::HitTestDataPtr& hit_test_data) {
+  for (auto& region : hit_test_data->regions) {
+    if (!ValidateHitTestRegion(region))
+      return false;
+  }
+  return true;
+}
+
+}  // namespace
+
+HitTestAggregator::HitTestAggregator()
+    : active_region_count_(0), weak_ptr_factory_(this) {
+  AllocateDisplayHitTestData();
+}
+HitTestAggregator::~HitTestAggregator() {}
+
+void HitTestAggregator::SubmitHitTestData(
+    hit_test::mojom::HitTestDataPtr hit_test_data) {
+  DCHECK(ValidateHitTestData(hit_test_data));
+  // TODO(gklassen): Runtime validation that hit_test_data is valid.
+  // TODO(gklassen): Inform FrameSink that the hit_test_data is invalid.
+  // TODO(gklassen): FrameSink needs to inform the host of a difficult renderer.
+  pending_[hit_test_data->surface_id] = std::move(hit_test_data);
+}
+
+bool HitTestAggregator::OnSurfaceDamaged(const cc::SurfaceId& surface_id,
+                                         const cc::BeginFrameAck& ack) {
+  return false;
+}
+
+void HitTestAggregator::OnSurfaceDiscarded(const cc::SurfaceId& surface_id) {
+  pending_.erase(surface_id);
+  active_.erase(surface_id);
+}
+
+void HitTestAggregator::OnSurfaceWillDraw(const cc::SurfaceId& surface_id) {
+  auto pending_search = pending_.find(surface_id);
+  if (pending_search == pending_.end()) {
+    // Have already activated pending hit_test_data objects for this surface.
+    return;
+  }
+  hit_test::mojom::HitTestData* hit_test_data = pending_search->second.get();
+
+  // Update the region count.
+  auto active_search = active_.find(surface_id);
+  if (active_search != active_.end()) {
+    hit_test::mojom::HitTestData* old_hit_test_data =
+        active_search->second.get();
+    active_region_count_ -= old_hit_test_data->regions.size();
+  }
+  active_region_count_ += hit_test_data->regions.size();
+  DCHECK_GE(active_region_count_, 0);
+
+  active_[surface_id] = std::move(pending_[surface_id]);
+  pending_.erase(surface_id);
+}
+
+void HitTestAggregator::AllocateDisplayHitTestData() {
+  AllocateDisplayHitTestData(kInitialSize);
+  Swap();
+  AllocateDisplayHitTestData(kInitialSize);
+}
+
+void HitTestAggregator::AllocateDisplayHitTestData(int size) {
+  size_t num_bytes = size * sizeof(DisplayHitTestRegion);

[sadrul, 2017/06/27 04:23:35]

Since |size| here depends on the input coming in from untrusted clients, should
this use base::CheckedNumeric<> to check for overflow?

[gklassen, 2017/06/27 21:46:33]

Good call.  A check has been added in the code that checks the size before
allocation. 

Done. 

+  write_handle_ = mojo::SharedBufferHandle::Create(num_bytes);
+  write_size_ = size;
+
+  mojo::ScopedSharedBufferMapping mapping = write_handle_->Map(num_bytes);
+  DisplayHitTestRegion* region = (DisplayHitTestRegion*)mapping.get();
+  region[0].child_count = kEndOfList;
+}
+
+void HitTestAggregator::PostTaskAggregate(cc::SurfaceId display_surface_id) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&HitTestAggregator::Aggregate,
+                     weak_ptr_factory_.GetWeakPtr(), display_surface_id));
+}
+
+void HitTestAggregator::Aggregate(cc::SurfaceId display_surface_id) {
+  // Size check.
+  int size = write_size_;
+  int max_size = active_region_count_ + active_.size() + 1;
+  if (max_size > size) {
+    while (size < max_size)
+      size += kIncrementalSize;

[sadrul, 2017/06/27 04:23:35]

We probably don't need the while loop?

[gklassen, 2017/06/27 21:46:33]

Done.

+    AllocateDisplayHitTestData(size);
+  }
+
+  AppendRoot(display_surface_id);
+}
+
+void HitTestAggregator::AppendRoot(cc::SurfaceId surface_id) {
+  auto search = active_.find(surface_id);
+  if (search == active_.end()) {
+    // Referenced surface not found ( it may be late ).
+    return;
+  }
+  hit_test::mojom::HitTestData* hit_test_data = search->second.get();
+
+  mojo::ScopedSharedBufferMapping mapping = MapWriteBuffer();

[sadrul, 2017/06/27 04:23:34]

Is there a reason we need to map/unmap? Can we just map and keep the mapped
memory around?

[gklassen, 2017/06/27 21:46:33]

It needs to be mapped here to initialize the list.  I had considered keeping the
mapped reference, but we really only need it at initialization and when we
aggregate the list so this approach seemed to work well and avoids the needs to
manage ( and swap ) another pointer.  

[gklassen, 2017/06/27 21:52:38]

On second thought this is a good suggestion.  

Done. 

+  DisplayHitTestRegion* regions = (DisplayHitTestRegion*)mapping.get();
+
+  regions[0].frame_sink_id = hit_test_data->surface_id.frame_sink_id();
+  regions[0].flags = hit_test_data->flags;
+  regions[0].rect = hit_test_data->bounds;
+  regions[0].transform = hit_test_data->transform;
+
+  int index = 1;
+  for (auto& region : hit_test_data->regions)
+    index = AppendRegion(regions, region, index);

[sadrul, 2017/06/27 04:23:35]

Can we DCHECK/CHECK that |index| remains within bounds at each step?

[gklassen, 2017/06/27 21:46:33]

Done.

+
+  DCHECK_GE(index, 1);
+  regions[0].child_count = index - 1;
+  regions[index].child_count = kEndOfList;
+}
+
+int HitTestAggregator::AppendRegion(
+    DisplayHitTestRegion* regions,
+    const hit_test::mojom::HitTestRegionPtr& region,
+    int index) {
+  DisplayHitTestRegion* element = &regions[index];
+
+  element->frame_sink_id = region->surface_id.frame_sink_id();
+  element->flags = region->flags;
+  element->rect = region->rect;
+  element->transform = region->transform;
+
+  int parent_index = index++;
+
+  if (region->flags == hit_test::mojom::kHitTestChildSurface) {
+    auto search = active_.find(region->surface_id);
+    if (search == active_.end()) {
+      // Surface HitTestData not found - it may be late.
+      // Don't include this region so that it doesn't receive events.
+      // TODO(gklassen): create test case for this case.
+      return parent_index;
+    }
+
+    // Rather than add a node in the tree for this hit_test_data element
+    // we can simplify the tree by merging the flags and transform into
+    // the kHitTestChildSurface element.
+    hit_test::mojom::HitTestData* hit_test_data = search->second.get();
+    // TODO(gklassen): Add test case to validate this.
+    if (!hit_test_data->transform.IsIdentity())
+      element->transform.PreconcatTransform(hit_test_data->transform);
+
+    element->flags |= hit_test_data->flags;
+
+    for (auto& child_region : hit_test_data->regions)
+      index = AppendRegion(regions, child_region, index);
+  }
+  DCHECK_GE(index - parent_index - 1, 0);
+  element->child_count = index - parent_index - 1;
+  return index;
+}
+
+void HitTestAggregator::Swap() {
+  std::swap(read_handle_, write_handle_);
+  std::swap(read_size_, write_size_);
+}
+
+mojo::ScopedSharedBufferMapping HitTestAggregator::MapReadBuffer() {
+  return read_handle_->Map(read_size_ * sizeof(DisplayHitTestRegion));
+}
+mojo::ScopedSharedBufferMapping HitTestAggregator::MapWriteBuffer() {
+  return write_handle_->Map(write_size_ * sizeof(DisplayHitTestRegion));
+}
+
+}  // namespace hit_test
+}  // namespace viz