cc: Adding BeginFrameObserverMap.

cc: Adding BeginFrameObserverMap.

BeginFrameObserverMap is like base::observer_list except it allows the easy
storing of data about each observer. Like base::observer_list it is safe to
Add/Remove observers while iterating over the map.

R=brianderson,danakj
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[mithro-old, 2015-09-11 04:22:01]

Hi Dana,

I continued to bang on the ObserverList stuff in
https://codereview.chromium.org/1026233002/ for a while before I realized what
we really needed is an ObserverMap object.

In our use case we want to be able to store an arbitrary bunch of data about
each Observer. Initially it will just be the NeedsBeginFrame state, but in the
future it will also include stats about how the observer is performing.

Can you take a look at this and see what you think?

Tim 'mithro' Ansell

[danakj, 2015-09-11 17:56:09]

https://codereview.chromium.org/1337803002/diff/20001/cc/base/observer_map.h
File cc/base/observer_map.h (right):

https://codereview.chromium.org/1337803002/diff/20001/cc/base/observer_map.h#...
cc/base/observer_map.h:13: template <class ObserverType, class ObserverDataType>
Do you need to use this with multiple ObserverTypes and ObserverDataTypes? Can
we not use templates and just put this beside the class that wants to use it,
specifically for it?

Let's not try to make something too general. Previous guidance from base/ owners
has been to avoid making general base-like things outside of base and keep it
specific to your use case.

[mithro-old, 2015-09-11 19:03:57]

https://codereview.chromium.org/1337803002/diff/20001/cc/base/observer_map.h
File cc/base/observer_map.h (right):

https://codereview.chromium.org/1337803002/diff/20001/cc/base/observer_map.h#...
cc/base/observer_map.h:13: template <class ObserverType, class ObserverDataType>
On 2015/09/11 17:56:09, danakj wrote:
> Do you need to use this with multiple ObserverTypes and ObserverDataTypes? Can
> we not use templates and just put this beside the class that wants to use it,
> specifically for it?

For our use case we probably only have one ObserverType (BeginFrameObserver) but
definitely do want different ObserverDataTypes for different BeginFrameSource
objects.

Having it templated on the ObserverType (rather than specific to
BeginFrameObserver) does make the tests slightly simpler.

I can move this to cc/scheduler/begin_frame_observer_map.xx if preferred. I
don't think the implementation would actually change if I moved it however?

> Let's not try to make something too general. Previous guidance from base/
owners
> has been to avoid making general base-like things outside of base and keep it
> specific to your use case.

Okay.

[mithro-old, 2015-09-11 19:08:17]

I was also wondering if I should be using base/collections/scoped_ptr_hash_map.h
rather than a base::hash_map for this. There doesn't appear to be a
scoped_ptr_hash_set?

[danakj, 2015-09-11 19:40:17]

On Fri, Sep 11, 2015 at 12:08 PM, <mithro@mithis.com> wrote:

> I was also wondering if I should be using
> base/collections/scoped_ptr_hash_map.h
> rather than a base::hash_map for this. There doesn't appear to be a
> scoped_ptr_hash_set?
>

The map does not own the pointers in it, so there's no scoped_ptrs. I think
hash_map is right.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-09-11 19:41:34]

On Fri, Sep 11, 2015 at 12:03 PM, <mithro@mithis.com> wrote:

>
>
> https://codereview.chromium.org/1337803002/diff/20001/cc/base/observer_map.h
> File cc/base/observer_map.h (right):
>
>
>
https://codereview.chromium.org/1337803002/diff/20001/cc/base/observer_map.h#...
> cc/base/observer_map.h:13: template <class ObserverType, class
> ObserverDataType>
> On 2015/09/11 17:56:09, danakj wrote:
>
>> Do you need to use this with multiple ObserverTypes and
>>
> ObserverDataTypes? Can
>
>> we not use templates and just put this beside the class that wants to
>>
> use it,
>
>> specifically for it?
>>
>
> For our use case we probably only have one ObserverType
> (BeginFrameObserver) but definitely do want different ObserverDataTypes
> for different BeginFrameSource objects.
>
> Having it templated on the ObserverType (rather than specific to
> BeginFrameObserver) does make the tests slightly simpler.
>
> I can move this to cc/scheduler/begin_frame_observer_map.xx if
> preferred. I don't think the implementation would actually change if I
> moved it however?


Sure, I'd just like it to be as non-general/specific as possible. Like, do
we need the Compact functionality? Do we need the ability to add an
observer mid iteration and have it execute? The fewer contracts the better.


>
>
> Let's not try to make something too general. Previous guidance from
>>
> base/ owners
>
>> has been to avoid making general base-like things outside of base and
>>
> keep it
>
>> specific to your use case.
>>
>
> Okay.
>
> https://codereview.chromium.org/1337803002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2015-09-11 22:05:44]

I changed the generic ObserverMap to the BeginFrameObserverMap but didn't change
any of the implementation.

I can change ObserverType of BeginFrameObserver...

The map *does* own the ObserverDataType objects but not the ObserverType.

Tim 'mithro' Ansell

[brianderson, 2015-09-12 00:31:30]

Generally agree with Dana's sentiment to only add what we need.

If we add a "WillLoseBeginFrameSource" on the Observer, we'll definitely need
RemoveObserver to support re-entrancy. Not sure we'd need AddObserver to be
re-entrant though.