Implement HitTestAggregator

components/viz/service/hit_test/hit_test_aggregator.h

Index: components/viz/service/hit_test/hit_test_aggregator.h
diff --git a/components/viz/service/hit_test/hit_test_aggregator.h b/components/viz/service/hit_test/hit_test_aggregator.h
new file mode 100644
index 0000000000000000000000000000000000000000..f04baf7f19a5bd70d075736c45e94c84cd58cfd0
--- /dev/null
+++ b/components/viz/service/hit_test/hit_test_aggregator.h
@@ -0,0 +1,118 @@
+// 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.
+
+#ifndef COMPONENTS_VIZ_SERVICE_HIT_TEST_HIT_TEST_AGGREGATOR_H_
+#define COMPONENTS_VIZ_SERVICE_HIT_TEST_HIT_TEST_AGGREGATOR_H_
+
+#include "cc/surfaces/surface_observer.h"
+#include "components/viz/common/aggregated_hit_test_region.h"
+#include "components/viz/common/surface_id.h"
+#include "components/viz/service/viz_service_export.h"
+#include "services/viz/public/interfaces/hit_test_region_list.mojom.h"
+
+namespace viz {
+namespace hit_test {

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

just use viz namespace here.

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

Done.

+
+namespace test {
+class HitTestAggregatorTest;

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

tests are found in the same namespace as the class usually, so just viz.

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

Done.

+}
+
+// HitTestAggregator collects HitTestRegionList objects from surfaces and
+// aggregates them into a DisplayHitTesData structue made available in
+// shared memory to enable efficient hit testing across processes.
+//
+// This is intended to be created in the viz or GPU process. For mus+ash this
+// will be true after the mus process split.
+

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

nit: remove whitespace

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

Done.

+class VIZ_SERVICE_EXPORT HitTestAggregator : public cc::SurfaceObserver {
+ public:
+  HitTestAggregator();
+  ~HitTestAggregator();
+
+  // Called when HitTestRegionList is submitted along with every call
+  // to SubmitCompositorFrame.  This is collected in pending_ until
+  // surfaces are aggregated and put on the display.
+  void SubmitHitTestRegionList(
+      hit_test::mojom::HitTestRegionListPtr hit_test_region_list);
+
+  // Called after surfaces have been aggregated into the DisplayFrame.
+  // In this call HitTestRegionList structures received from active surfaces
+  // are aggregated into the HitTestRegionList structure in
+  // shared memory used for event targetting.
+  void Aggregate(SurfaceId display_surface_id);
+
+  // Performs the work of Aggregate by creating a PostTask so that
+  // the work is not directly on the call.
+  void PostTaskAggregate(SurfaceId display_surface_id);
+
+  // Called at BeginFrame. Swaps buffers in shared memory.
+  void Swap();
+
+ private:
+  friend class test::HitTestAggregatorTest;

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

There are nicer ways to expose things to tests than a friend. A friend has 2
disadvantages:
- It loses all control of the class internals, you can't decide what to expose
and what not
- You have to name all the tests here, so when you add a test later sometimes
you have to add knowledge about it here.

You could instead put things you want to expose to tests in a protected block,
then in the test file make a subclass that exposes those things with "using".

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

Thank you.  I've looked at this a few times and there are many things that the
tests currently inspect that I would need to add to the protected block.  Given
the limited scope would it be ok to use the friend class here?

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

I wouldn't block you from this, but I just don't recommend it.

FWIW Another pattern instead of protected would be to friend a
HitTestAggregatorTestApi or something, and it exposes by pass-thru instead of
inheritance the things you want to expose. This again doesn't require updating
the production code when you add a new test suite. But it does lose control of
what the tests access, so it's a bit of a middle solution.

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

Thank you.  Using a friend class bothered Rob as well and so I've made the
change as you originally suggested and it is cleaner.  

Done. 

+
+  // Allocates memory for the AggregatedHitTestRegion array.
+  void AllocateHitTestRegionArray();
+  void AllocateHitTestRegionArray(int length);
+
+  // Appends the root element to the AggregatedHitTestRegion array.
+  void AppendRoot(SurfaceId surface_id);
+
+  // Appends a region to the HitTestRegionList structure to recursively
+  // build the tree.
+  int AppendRegion(AggregatedHitTestRegion* regions,
+                   const hit_test::mojom::HitTestRegionPtr& region,
+                   int index);

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

nit: i'd group index and regions together since it is an index into regions, and
maybe name it to say that as well

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

Done.

+
+  // cc::SurfaceObserver:
+  void OnSurfaceCreated(const cc::SurfaceInfo& surface_info) override {}
+  void OnSurfaceDestroyed(const SurfaceId& surface_id) override {}
+  bool OnSurfaceDamaged(const SurfaceId& surface_id,
+                        const cc::BeginFrameAck& ack) override;
+  void OnSurfaceDiscarded(const SurfaceId& surface_id) override;
+  void OnSurfaceDamageExpected(const SurfaceId& surface_id,
+                               const cc::BeginFrameArgs& args) override {}
+
+  // Called when a surface has been aggregated and added to the
+  // display frame.  HitTestRegionList objects are held but ignored until
+  // this happens.  HitTestRegionList for the surface is copied from |pending_|
+  // to |active_| in this method.
+  void OnSurfaceWillDraw(const SurfaceId& surface_id) override;
+
+  using HitTestRegionListMap =

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

nit: Now that we have auto, usually I think it's better to just write out the
type on the variables instead of using a type alias like this. It leaves less
guessing/indirection about performance characteristics with moves or inserts
etc.

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

Done.

+      std::map<SurfaceId, hit_test::mojom::HitTestRegionListPtr>;

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

How big is this going to be, what is the usage pattern going to be like?

See base/containers/README.md for some options. std::map isn't always the best
choice.

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

Thank you.  After review std::map still seems like a suitable choice - at least
until we can review size and usage patterns in context at which time
base::flat\_map and base::small\_map may offer some improvement.

+
+  // The collection of received HitTestRegionList objects that have not yet
+  // been added to the DisplayFrame (OnSurfaceWillDraw has not been called).
+  HitTestRegionListMap pending_;
+
+  // The collection of HitTestRegionList objects that have been added to the
+  // DisplayFrame (OnSurfaceWillDraw has been called).
+  HitTestRegionListMap active_;
+
+  // Keeps track of the number of regions in the active list
+  // so that we know when we exceed the available length.
+  int active_region_count_;
+
+  mojo::ScopedSharedBufferHandle read_handle_;
+  mojo::ScopedSharedBufferHandle write_handle_;
+
+  // The number of elements allocated.
+  int read_size_;
+  int write_size_;
+
+  mojo::ScopedSharedBufferMapping read_buffer_;
+  mojo::ScopedSharedBufferMapping write_buffer_;
+
+  // Handles the case when this object is deleted after
+  // the PostTaskAggregation call is scheduled but before invocation.
+  base::WeakPtrFactory<HitTestAggregator> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(HitTestAggregator);
+};
+
+}  // namespace hit_test
+}  // namespace viz
+
+#endif  // COMPONENTS_VIZ_SERVICE_HIT_TEST_HIT_TEST_AGGREGATOR_H_