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..cc4a971e055ea8c7e51dc2527ed94ebbd4881e49
--- /dev/null
+++ b/components/viz/service/hit_test/hit_test_aggregator.cc
@@ -0,0 +1,220 @@
+// 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/aggregated_hit_test_region.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;
+constexpr int kMaxRegionsPerSurface = 1024;
+constexpr int kMaxSize = 100 * 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 ValidateHitTestRegionList(
+    const hit_test::mojom::HitTestRegionListPtr& hit_test_region_list) {
+  if (hit_test_region_list->regions.size() > kMaxRegionsPerSurface)
+    return false;
+  for (auto& region : hit_test_region_list->regions) {
+    if (!ValidateHitTestRegion(region))
+      return false;
+  }
+  return true;
+}
+
+}  // namespace
+
+HitTestAggregator::HitTestAggregator()
+    : active_region_count_(0), weak_ptr_factory_(this) {

[danakj, 2017/07/13 15:39:38]

you can assign the 0 in the .h file where var is declared. this makes it easier
to remember to init things.

[gklassen, 2017/07/13 19:59:14]

Done.

+  AllocateHitTestRegionArray();
+}
+
+HitTestAggregator::~HitTestAggregator() = default;
+
+void HitTestAggregator::SubmitHitTestRegionList(
+    hit_test::mojom::HitTestRegionListPtr hit_test_region_list) {
+  DCHECK(ValidateHitTestRegionList(hit_test_region_list));
+  // TODO(gklassen): Runtime validation that hit_test_region_list is valid.
+  // TODO(gklassen): Inform FrameSink that the hit_test_region_list is invalid.
+  // TODO(gklassen): FrameSink needs to inform the host of a difficult renderer.
+  pending_[hit_test_region_list->surface_id] = std::move(hit_test_region_list);
+}
+
+bool HitTestAggregator::OnSurfaceDamaged(const SurfaceId& surface_id,
+                                         const cc::BeginFrameAck& ack) {
+  return false;
+}
+
+void HitTestAggregator::OnSurfaceDiscarded(const SurfaceId& surface_id) {
+  // Update the region count.
+  auto active_search = active_.find(surface_id);
+  if (active_search != active_.end()) {
+    hit_test::mojom::HitTestRegionList* old_hit_test_data =
+        active_search->second.get();
+    active_region_count_ -= old_hit_test_data->regions.size();
+  }
+  DCHECK_GE(active_region_count_, 0);
+
+  pending_.erase(surface_id);
+  active_.erase(surface_id);
+}
+
+void HitTestAggregator::OnSurfaceWillDraw(const SurfaceId& surface_id) {
+  auto pending_search = pending_.find(surface_id);
+  if (pending_search == pending_.end()) {
+    // Have already activated pending hit_test_region_list objects for this
+    // surface.
+    return;
+  }
+  hit_test::mojom::HitTestRegionList* hit_test_region_list =
+      pending_search->second.get();
+
+  // Update the region count.
+  auto active_search = active_.find(surface_id);
+  if (active_search != active_.end()) {
+    hit_test::mojom::HitTestRegionList* old_hit_test_data =
+        active_search->second.get();
+    active_region_count_ -= old_hit_test_data->regions.size();
+  }
+  active_region_count_ += hit_test_region_list->regions.size();
+  DCHECK_GE(active_region_count_, 0);
+
+  active_[surface_id] = std::move(pending_[surface_id]);
+  pending_.erase(surface_id);
+}
+
+void HitTestAggregator::AllocateHitTestRegionArray() {
+  AllocateHitTestRegionArray(kInitialSize);
+  Swap();
+  AllocateHitTestRegionArray(kInitialSize);
+}
+
+void HitTestAggregator::AllocateHitTestRegionArray(int size) {
+  size_t num_bytes = size * sizeof(AggregatedHitTestRegion);
+  write_handle_ = mojo::SharedBufferHandle::Create(num_bytes);
+  write_size_ = size;
+  write_buffer_ = write_handle_->Map(num_bytes);
+
+  AggregatedHitTestRegion* region =
+      (AggregatedHitTestRegion*)write_buffer_.get();
+  region[0].child_count = kEndOfList;
+}
+
+void HitTestAggregator::PostTaskAggregate(SurfaceId display_surface_id) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&HitTestAggregator::Aggregate,
+                     weak_ptr_factory_.GetWeakPtr(), display_surface_id));
+}
+
+void HitTestAggregator::Aggregate(SurfaceId display_surface_id) {
+  // Check to ensure that enough memory has been allocated.
+  int size = write_size_;
+  int max_size = active_region_count_ + active_.size() + 1;
+  if (max_size > kMaxSize)
+    max_size = kMaxSize;
+
+  if (max_size > size) {
+    size = (1 + max_size / kIncrementalSize) * kIncrementalSize;
+    AllocateHitTestRegionArray(size);
+  }
+
+  AppendRoot(display_surface_id);
+}
+
+void HitTestAggregator::AppendRoot(SurfaceId surface_id) {
+  auto search = active_.find(surface_id);
+  CHECK(search != active_.end());

[danakj, 2017/07/13 15:39:38]

do you mean DCHECK?

See
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[gklassen, 2017/07/13 19:59:14]

The use of CHECK was intentional and based on conversation with others - but
perhaps without alot of thought - is DCHECK more appropriate?  The root surface
should be in the list when this is called. 

[danakj, 2017/07/14 15:27:37]

Unless code outside your control is going to sometimes trigger it, or you're
tracking down a known bug, or protecting against a security problem in
difficult-to-control code, I don't think CHECK is right. I don't think this code
matches any of thsoe.

[gklassen, 2017/07/14 16:54:45]

Thank you for the explanation and detail - it's very helpful. 

Done.

+
+  hit_test::mojom::HitTestRegionList* hit_test_region_list =
+      search->second.get();
+
+  AggregatedHitTestRegion* regions =
+      (AggregatedHitTestRegion*)write_buffer_.get();

[danakj, 2017/07/13 15:39:38]

Use c++ casts. https://google.github.io/styleguide/cppguide.html#Casting

[gklassen, 2017/07/13 19:59:14]

Good call.  Thanks.  Done.

+
+  regions[0].frame_sink_id = hit_test_region_list->surface_id.frame_sink_id();
+  regions[0].flags = hit_test_region_list->flags;
+  regions[0].rect = hit_test_region_list->bounds;
+  regions[0].transform = hit_test_region_list->transform;
+
+  int index = 1;
+  for (const auto& region : hit_test_region_list->regions) {
+    if (index >= write_size_ - 1)
+      break;
+    index = AppendRegion(regions, region, index);
+  }
+
+  DCHECK_GE(index, 1);
+  regions[0].child_count = index - 1;
+  regions[index].child_count = kEndOfList;
+}
+
+int HitTestAggregator::AppendRegion(
+    AggregatedHitTestRegion* regions,
+    const hit_test::mojom::HitTestRegionPtr& region,
+    int index) {
+  AggregatedHitTestRegion* 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 (index >= write_size_ - 1) {
+    element->child_count = 0;
+    return index;
+  }
+
+  if (region->flags == hit_test::mojom::kHitTestChildSurface) {
+    auto search = active_.find(region->surface_id);
+    if (search == active_.end()) {
+      // Surface HitTestRegionList not found - it may be late.
+      // Don't include this region so that it doesn't receive events.
+      return parent_index;
+    }
+
+    // Rather than add a node in the tree for this hit_test_region_list element
+    // we can simplify the tree by merging the flags and transform into
+    // the kHitTestChildSurface element.
+    hit_test::mojom::HitTestRegionList* hit_test_region_list =
+        search->second.get();
+    if (!hit_test_region_list->transform.IsIdentity())
+      element->transform.PreconcatTransform(hit_test_region_list->transform);
+
+    element->flags |= hit_test_region_list->flags;
+
+    for (const auto& child_region : hit_test_region_list->regions) {
+      index = AppendRegion(regions, child_region, index);
+      if (index >= write_size_ - 1)
+        break;
+    }
+  }
+  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_);

[danakj, 2017/07/13 15:39:38]

use ADL lookup for swap, with using std::swap:
https://stackoverflow.com/questions/28130671/how-does-using-stdswap-enable-adl

[gklassen, 2017/07/13 19:59:14]

Done.

+  std::swap(read_size_, write_size_);
+  std::swap(read_buffer_, write_buffer_);
+}
+
+}  // namespace hit_test
+}  // namespace viz