WIP Hittest Component.

components/viz/hit_test/hit_test_aggregator.cc

Index: components/viz/hit_test/hit_test_aggregator.cc
diff --git a/components/viz/hit_test/hit_test_aggregator.cc b/components/viz/hit_test/hit_test_aggregator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4fbca4c331951d632a8eb6d7a0d8fa984ea90de7
--- /dev/null
+++ b/components/viz/hit_test/hit_test_aggregator.cc
@@ -0,0 +1,116 @@
+// 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 {
+constexpr int kInitialSize = 1024;

[rjkroege, 2017/06/07 21:19:05]

need a TODO() to figure out what a good initial size is

need a TODO() to figure out what to do about changing size.

[gklassen, 2017/06/08 20:33:21]

Done.

+}
+
+using namespace hit_test::mojom;

[rjkroege, 2017/06/07 21:19:05]

I'm not keen on this.

[gklassen, 2017/06/08 20:33:20]

Ok.  Removed.  Done.

+
+HitTestAggregator::HitTestAggregator() {
+  AllocateDisplayHitTestData();
+}
+HitTestAggregator::~HitTestAggregator() {}
+
+void HitTestAggregator::SubmitHitTestData(HitTestDataPtr hit_test_data) {
+  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) {
+  active_[surface_id] = std::move(pending_[surface_id]);

[rjkroege, 2017/06/07 21:19:05]

so...  when you get another OnSurfaceWillDraw re-using a specific SurfaceId in a
subsequent frame, you will find the surface_id missing from pending_? And the
code will be sad?

[gklassen, 2017/06/08 20:33:20]

Thanks.  Code added to explicitly check and handle this case. Done.

+}
+
+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);
+
+  display_hit_test_data_->size_ = size;

[rjkroege, 2017/06/07 21:19:04]

DisplayHitTestData could have a constructor?

[gklassen, 2017/06/08 20:33:21]

It's now a struct ( based on earlier discussion and recommendation ).

+  display_hit_test_data_->front_offset_ = 0;
+  display_hit_test_data_->regions_[0].child_count_ = kEndOfList;
+  display_hit_test_data_->regions_[size << 2].child_count_ = kEndOfList;

[rjkroege, 2017/06/07 21:19:05]

Why do you init it?

[gklassen, 2017/06/08 20:33:21]

It's using child_count == kEndOfList to indicate the end of the list.  It's a
struct so this seemed like the best place to init - but better suggestions are
welcome and appreciated.  I'm not sure if I like it either. 

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

struct declaration could init (is C++11-ism apparently)

+}
+
+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) {

[rjkroege, 2017/06/07 21:19:05]

completely private methods could be in an anonymous name space inside of this
class.

[gklassen, 2017/06/08 20:33:21]

Thanks.  This method currently accesses some member variables so I would need to
refactor.  Is it worth it?

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

Acknowledged.

+  auto search = active_.find(surface_id);
+  if (search == active_.end()) {
+    // referenced surface not found!

[rjkroege, 2017/06/07 21:19:05]

this indicates a bug in SurfaceAggregator yes? They should all exist.

[gklassen, 2017/06/08 20:33:21]

Based on earlier conversations I believe it's possible for a surface to call
SubmitCompositorFrame late - if so then this would happen and be expected.  The
hit_test_data for this surface would be empty as would the contents of the
frame.  

+    return index;
+  }
+  HitTestData* hit_test_data = search->second.get();
+
+  if (hit_test_data->regions_.size() == 0) {

[rjkroege, 2017/06/07 21:19:04]

we must have at least 1 region for valid data yes? DCHECK?

[gklassen, 2017/06/08 20:33:21]

A surface with no children may have no sub-regions no?

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

A HTD can have HTRs without any of them having the  HTR_CHILD_SURFACE

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

Right. 

+    // if there are no sub-regions then we don't need a bounds check entry
+    return index;
+  }
+
+  DisplayHitTestRegion* element = &display_hit_test_data_->regions_[index];
+
+  int parent_index = index++;
+
+  element->frame_sink_id_ = hit_test_data->surface_id_.frame_sink_id();
+  element->flags_ = HitTestRegionFlags::HIT_TEST_BOUNDS;

[rjkroege, 2017/06/07 21:19:04]

You have a separate region for the bounds? Why?

[gklassen, 2017/06/08 20:33:21]

After review and discussion this morning I think you are right that this isn't
necessary.

+  element->rect_ = hit_test_data->bounds_;
+
+  for (auto& region : hit_test_data->regions_) {
+    index = Append(region, index);
+  }
+
+  element->child_count_ = index - parent_index - 1;

[rjkroege, 2017/06/07 21:19:05]

Assumptions here should be wrapped in DCHECK

[gklassen, 2017/06/08 20:33:21]

Agreed.  Do you have suggestions for assumptions to check?

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

this one. I'll note all the others that occur to me

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

Done.

+  return index;
+}
+
+int HitTestAggregator::Append(const HitTestRegionPtr& region, int index) {
+  DisplayHitTestRegion* element = &display_hit_test_data_->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_ == 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_->front_offset_ = GetBackIndex();
+}
+
+int HitTestAggregator::GetBackIndex() {
+  return display_hit_test_data_->front_offset_ == 0

[rjkroege, 2017/06/07 21:19:05]

if is easier to read?

[gklassen, 2017/06/08 20:33:20]

I wrote it both ways.  I think I agree with you.  Done. 

+             ? display_hit_test_data_->size_ / 2
+             : 0;
+}
+
+DisplayHitTestRegion* HitTestAggregator::GetCurrentRegions() {
+  return display_hit_test_data_->regions_ +
+         display_hit_test_data_->front_offset_;
+}
+
+}  // namespace viz