WIP Hittest Component.

components/viz/hit_test/hit_test_aggregator.cc

+// 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/hit_test/hit_test_aggregator.h"
+#include "base/logging.h"
+#include "display_hit_test_data.h"
+
+namespace viz {
+namespace hit_test {
+
+namespace {
+// TODO: Review and select appropriate initial size ( based on telemetry? ).

[rjkroege, 2017/06/09 15:47:08]

convention is TODO(gklassen): 

i.e.: every TODO is suppose to have an owner

[gklassen, 2017/06/12 16:30:41]

Done.  Thank you.  I like that. 

+constexpr int kInitialSize = 1024;
+}
+
+HitTestAggregator::HitTestAggregator() : weak_ptr_factory_(this) {
+  AllocateDisplayHitTestData();
+}
+HitTestAggregator::~HitTestAggregator() {}
+
+void HitTestAggregator::SubmitHitTestData(
+    hit_test::mojom::HitTestDataPtr hit_test_data) {
+  // TODO: Review what to do if the submitted data is invalid.
+  pending_[hit_test_data->surface_id_] = std::move(hit_test_data);
+}
+
+void HitTestAggregator::OnSurfaceDiscarded(const cc::SurfaceId& surface_id) {
+  pending_.erase(surface_id);
+  active_.erase(surface_id);
+}
+void HitTestAggregator::OnSurfaceWillDraw(const cc::SurfaceId& surface_id) {

[rjkroege, 2017/06/09 15:47:08]

I like a blank line between defns

[gklassen, 2017/06/12 16:30:41]

Me too.  Thanks for the catch. Done.

+  auto search = pending_.find(surface_id);
+  if (search == pending_.end()) {
+    // Have already activated pending hit_test_data objects for this surface.
+    return;
+  }
+  active_[surface_id] = std::move(pending_[surface_id]);

[rjkroege, 2017/06/09 15:47:08]

DCHECK that active_[surface_id] exists

[gklassen, 2017/06/12 16:30:41]

It may not exist though?  pending_ must exist but we runtime check that above.

+  pending_.erase(surface_id);
+}
+
+void HitTestAggregator::AllocateDisplayHitTestData() {
+  AllocateDisplayHitTestData(kInitialSize);
+}
+
+void HitTestAggregator::AllocateDisplayHitTestData(int size) {
+  int cb = sizeof(DisplayHitTestData) + size * sizeof(DisplayHitTestRegion);
+  display_hit_test_data_ = (DisplayHitTestData*)::operator new(cb);

[rjkroege, 2017/06/09 15:47:08]

i don't understand this line. And assuming that I do, c-style casts strongly
undesired in Chrome

[gklassen, 2017/06/12 16:30:41]

Agreed.  As discussed I've updated this code moderately to hopefully make it
clearer.  

+
+  display_hit_test_data_->size_ = size;
+  display_hit_test_data_->read_offset_ = 0;
+  display_hit_test_data_->regions_[0].child_count_ = kEndOfList;
+  display_hit_test_data_->regions_[size << 2].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) {
+  int index = GetBackIndex();
+  int last_index = Append(display_surface_id, index);
+  display_hit_test_data_->regions_[last_index].child_count_ = kEndOfList;
+}
+
+int HitTestAggregator::Append(cc::SurfaceId surface_id, int index) {
+  auto search = active_.find(surface_id);
+  if (search == active_.end()) {
+    // Referenced surface not found ( it may be late ).
+    return index;
+  }
+  hit_test::mojom::HitTestData* hit_test_data = search->second.get();
+
+  for (auto& region : hit_test_data->regions_) {
+    index = Append(region, index);
+  }
+  return index;
+}
+
+int HitTestAggregator::Append(const hit_test::mojom::HitTestRegionPtr& region,

[rjkroege, 2017/06/09 15:47:08]

You need to make certain that you handle what happens when you run out of space.
And all this code needs to test that it's not writing beyond its region.

+                              int index) {
+  DisplayHitTestRegion* element = &display_hit_test_data_->regions_[index];
+
+  element->frame_sink_id_ = region->surface_id_.frame_sink_id();

[rjkroege, 2017/06/09 15:47:08]

Observation: not very C++-ish. Some might complain later.

[gklassen, 2017/06/12 16:30:41]

Suggestions welcome - how can I make this more C++?

[rjkroege, 2017/06/12 18:13:00]

not clear. leave it

+  element->flags_ = region->flags_;
+  element->rect_ = region->rect_;
+  element->transform_ = region->transform_;
+
+  int parent_index = index++;
+
+  if (region->flags_ ==

[rjkroege, 2017/06/09 15:47:08]

have to &.

Multiple flags can be simultaneously defined

[gklassen, 2017/06/12 16:30:41]

Acknowledged.

[rjkroege, 2017/06/12 18:13:00]

but... you didn't change the code?

+      hit_test::mojom::HitTestRegionFlags::HIT_TEST_CHILD_SURFACE) {
+    index = Append(region->surface_id_, index);
+  }
+
+  element->child_count_ = index - parent_index - 1;
+  return index;
+}
+
+void HitTestAggregator::Swap() {
+  display_hit_test_data_->read_offset_ = GetBackIndex();
+}
+
+int HitTestAggregator::GetBackIndex() {
+  if (display_hit_test_data_->read_offset_ == 0)
+    return display_hit_test_data_->size_ / 2;
+  return 0;
+}
+
+DisplayHitTestRegion* HitTestAggregator::GetCurrentRegions() {
+  return display_hit_test_data_->regions_ +
+         display_hit_test_data_->read_offset_;
+}
+
+}  // namespace hit_test
+}  // namespace viz