cc: Making BeginFrameSources support multiple BeginFrameObservers.

cc: Making BeginFrameSources support multiple BeginFrameObservers.

This is needed by the UnifiedBeginFrame and DisplayScheduler projects.

Originally based on Simon Hong's work at 
  https://codereview.chromium.org/845393002/ - "cc: Create ProxyBeginFrameSource",
and then further reworked with Brian Anderson in 
  https://codereview.chromium.org/1318603012/

BUG=448140
R=brianderson@chromium.org,simonhong@chromium.org
TEST=cc_unittests
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[mithro-old, 2015-03-24 05:08:06]

Hi Simon / Brian,

This is an alternative solution to https://codereview.chromium.org/1022483003/

The main problem with this patch is that iterating over an observer_list_
requires a bunch of methods to become non-const.

This also doesn't provide any feedback on if a BeginFrameSource has any
observers. I'm not sure if that is actually useful functionality though.

Tim 'mithro' Ansell

[danakj, 2015-03-25 18:12:54]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-03-25 18:12:55]

https://codereview.chromium.org/1026233002/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/1026233002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:199: base::trace_event::TracedValue* dict) {
What's with all the const changes? AsValueInto can't be related to using an
ObserverList?

[brianderson, 2015-03-25 23:21:06]

Looks like most of this patch didn't get uploaded?

[mithro-old, 2015-03-26 00:54:49]

Hi,

This patch enables multi observers but doesn't add tests for it. Wanted to know
which of the two patches was preferred before going ahead fixing up tests.

Tim 'mithro' Ansell

https://codereview.chromium.org/1026233002/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/1026233002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:199: base::trace_event::TracedValue* dict) {
On 2015/03/25 18:12:55, danakj wrote:
> What's with all the const changes? AsValueInto can't be related to using an
> ObserverList?

The AsValueInto loops over each of the observers. Iterating over an ObserverList
is non const operation.

[simonhong, 2015-03-26 01:09:20]

On 2015/03/26 00:54:49, mithro wrote:
> Hi,
> 
> This patch enables multi observers but doesn't add tests for it. Wanted to
know
> which of the two patches was preferred before going ahead fixing up tests.
> 
> Tim 'mithro' Ansell
> 
>
https://codereview.chromium.org/1026233002/diff/1/cc/scheduler/begin_frame_so...
> File cc/scheduler/begin_frame_source.cc (right):
> 
>
https://codereview.chromium.org/1026233002/diff/1/cc/scheduler/begin_frame_so...
> cc/scheduler/begin_frame_source.cc:199: base::trace_event::TracedValue* dict)
{
> On 2015/03/25 18:12:55, danakj wrote:
> > What's with all the const changes? AsValueInto can't be related to using an
> > ObserverList?
> 
> The AsValueInto loops over each of the observers. Iterating over an
ObserverList
> is non const operation.

IMO, tim's other cl (BeginFrameObserverMultiplexer) is much closer to its usage
because
it just handles multiple observers.

[brianderson, 2015-09-04 19:48:18]

Hi Dana,

As an OWNER of src/base, can you take a look at this method and tell me what you
think? 

This CL show the use case for adding a "const iterator" to the observer_list
class at
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list.h.
I think when modifying that class it should be possible to do the same thing as
here without the nasty casts, but I have yet to actually try an implementation.
We probably want to be a little stricter on the checking when adding to base as
we are less confident in what the users can do (plus people do nasty things like
const_casts :P).

We currently don't want to block this CL on waiting for something to get into
base but do want to make sure we can replace the what ever we use here with what
does end up into base.

Thanks for your review!

Tim 'mithro' Ansell

https://codereview.chromium.org/1026233002/diff/20001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/1026233002/diff/20001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:112:
BeginFrameSourceBase::getConstObserverListIterator() const {
Please pay special attention here.

[mithro-old, 2015-09-04 19:51:48]

Hi Dana,

Sorry that was me (Tim 'mithro' Ansell) and not Brian commenting. We are
currently pair coding on his machine and I accidently used the wrong browser
window.

Tim 'mithro' Ansell

[danakj, 2015-09-04 23:00:54]

I don't get why you need this. Maybe you can explain some? For example:

#include "base/observer_list.h"

class Foo {
 public:
  void Me() {
    LOG(ERROR) << "HI";
  }
};

TEST(Dana, kj) {
  Foo a;
  Foo b;

  base::ObserverList<const Foo> list;
  list.AddObserver(&a);
  list.AddObserver(&b);
  FOR_EACH_OBSERVER(const Foo, list, Me());
}

This fails:
../../cc/layers/layer_unittest.cc:1348:3: error: member function 'Me' not
viable: 'this' argument has type 'const Foo', but function is not marked
const

But if Me() is const, then it works. ie, you can only call const methods in
the list here.

On Fri, Sep 4, 2015 at 12:48 PM, <brianderson@chromium.org> wrote:

> Hi Dana,
>
> As an OWNER of src/base, can you take a look at this method and tell me
> what you
> think?
>
> This CL show the use case for adding a "const iterator" to the
> observer_list
> class at
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list.h
> .
> I think when modifying that class it should be possible to do the same
> thing as
> here without the nasty casts, but I have yet to actually try an
> implementation.
> We probably want to be a little stricter on the checking when adding to
> base as
> we are less confident in what the users can do (plus people do nasty
> things like
> const_casts :P).
>
> We currently don't want to block this CL on waiting for something to get
> into
> base but do want to make sure we can replace the what ever we use here
> with what
> does end up into base.
>
> Thanks for your review!
>
> Tim 'mithro' Ansell
>
>
>
>
https://codereview.chromium.org/1026233002/diff/20001/cc/scheduler/begin_fram...
> File cc/scheduler/begin_frame_source.cc (right):
>
>
>
https://codereview.chromium.org/1026233002/diff/20001/cc/scheduler/begin_fram...
> cc/scheduler/begin_frame_source.cc:112:
> BeginFrameSourceBase::getConstObserverListIterator() const {
> Please pay special attention here.
>
> https://codereview.chromium.org/1026233002/
>

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

[mithro-old, 2015-09-08 22:05:30]

Hi Dana,

This is what we are trying to do;
-------------------------------------------------------
class FooObserver {
  // Normal notify structure
  void Notify(ThingWhichHasObservers o) {
    o.RemoveObserver(this);
  }

  // Property or other method that the observer has.
  int GetId() const {
    return my_id_;
  }
}

class ThingWhichHasObservers {
 base::ObserverList<Foo> my_observers;

 // Normal notify
 void NotifyObservers() {
   FOR_EACH_OBSERVER(Foo, my_observers, Notify(this));
  }

 // Get some information about the observer
 int LargestObserverId() const {
    int current_largest_id = -1;
    auto it = base::ObserverList<const Foo>::Iterator(my_observers_);
    const FooObserver* obs;
    while ((obs = it.GetNext()) != NULL) {
      int ob_id = obs->GetId();
      if (ob_id > current_largest_id)
        current_largest_id = ob_id;
    }
    return ob_id;
  }
 }
-------------------------------------------------------

The important part is that inside a **const method** on
ThingWhichHasObservers we want to be able to loop over the observer
list and call const methods on the observers.

As observers are normally allowed to modify the observer list while
you are iterating over them (so they can remove themselves after being
notified) the ObserverList::Iterator currently only works on non-const
ObserverLists. This means you can't loop over ObserverLists in a const
method!

If you can guarantee that the method you are calling on the Observers
will not modify the observer list - such as only calling const
methods, then it should be fine to iterate over the list. That is what
we are doing here.

Does that make more sense?

Tim 'mithro' Ansell

[danakj, 2015-09-08 23:12:09]

On the one hand:

Sure I think we could add a FOR_EACH_OBSERVER_CONST.

(1) It would use a ConstIterator that had a weakptr to a const
ObserverList. (template the Iterator?)
(2) The macro would have a pointer to a const ObserverType.

I think that would be safe. If you tried to use this in a non-const method,
it would still only allow you to call const methods on observers, due to 2.

On the other hand:

It's really easy to do something like:

OtherThing::Walk() const {
  FOR_EACH_OBSERVER_CONST(MyObserver, observer_list_, Foo());
}

MyObserver::Foo() const {
  other_thing_->Bar();
}

OtherThing::Bar() {
  observer_list_->Remove(my_observer_);
}

And then what was the point? ie. constness is very.. unsafe, and mostly to
reason about a single method's behaviour. This part:

> If you can guarantee that the method you are calling on the Observers
> will not modify the observer list - such as only calling const
> methods, then it should be fine to iterate over the list. That is what
> we are doing here.

I don't think it can be so, that guarantee.

So.. FOR_EACH_OBSERVER_CONST can't actually be a promise that has anything
to do with the observer list. So it's kind of weird to make it a part of
the ObserverList. Can you do it more simply like..

OtherThing::Walk() const {
  ObserverList* list = const_cast<ObserverList*>(&observer_list_);
  FOR_EACH_OBSERVER(const MyObserver, list, Foo());
}

Or, why did you need such a complicated way to iterate the list as it is
now?



On Tue, Sep 8, 2015 at 3:05 PM, <mithro@mithis.com> wrote:

> Hi Dana,
>
> This is what we are trying to do;
> -------------------------------------------------------
> class FooObserver {
>   // Normal notify structure
>   void Notify(ThingWhichHasObservers o) {
>     o.RemoveObserver(this);
>   }
>
>   // Property or other method that the observer has.
>   int GetId() const {
>     return my_id_;
>   }
> }
>
> class ThingWhichHasObservers {
>  base::ObserverList<Foo> my_observers;
>
>  // Normal notify
>  void NotifyObservers() {
>    FOR_EACH_OBSERVER(Foo, my_observers, Notify(this));
>   }
>
>  // Get some information about the observer
>  int LargestObserverId() const {
>     int current_largest_id = -1;
>     auto it = base::ObserverList<const Foo>::Iterator(my_observers_);
>     const FooObserver* obs;
>     while ((obs = it.GetNext()) != NULL) {
>       int ob_id = obs->GetId();
>       if (ob_id > current_largest_id)
>         current_largest_id = ob_id;
>     }
>     return ob_id;
>   }
>  }
> -------------------------------------------------------
>
> The important part is that inside a **const method** on
> ThingWhichHasObservers we want to be able to loop over the observer
> list and call const methods on the observers.
>
> As observers are normally allowed to modify the observer list while
> you are iterating over them (so they can remove themselves after being
> notified) the ObserverList::Iterator currently only works on non-const
> ObserverLists. This means you can't loop over ObserverLists in a const
> method!
>
> If you can guarantee that the method you are calling on the Observers
> will not modify the observer list - such as only calling const
> methods, then it should be fine to iterate over the list. That is what
> we are doing here.
>
> Does that make more sense?
>
> Tim 'mithro' Ansell
>
> https://codereview.chromium.org/1026233002/
>

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

[mithro-old, 2015-09-10 18:10:56]

Hi Brian,

Can you take a look at this and tell me what you think? I have a follow up patch
which extends DidFinishFrame to include observer information.

Tim 'mithro' Ansell

[danakj, 2015-09-10 21:30:23]

So, we could add a templated method to iterate ObserverList and let you run a
lambda in it for each observer instead of calling a method on the observers:

template<typename ObserverType, typename ObserverListType, typename Func>
inline void ForEachObserver(ObserverListType* list, Func func) {
  if (!list->might_have_observers())
    return;
  typename ObserverListType::Iterator it(list);
  ObserverType* obs;
  while ((obs = it.GetNext()) != nullptr)
    func(obs);
}


Then the const problem ends up like:

  base::ObserverList<BeginFrameObserver>* mutable_list_ =
      const_cast<base::ObserverList<BeginFrameObserver>*>(&observer_list_);
  base::ForEachObserver<const BeginFrameObserver>(
      mutable_list_, [dict](const BeginFrameObserver* obs) {
        dict->BeginDictionary();
        obs->AsValueInto(dict);
        dict->EndDictionary();
      });

Which is definitely less crazy...

All this has left me wondering why we're trying to shoe-horn ObserverList in
here, as it doesn't seem to really fit anymore.

Would a simple vector of Foo*'s work better for all of out use cases/issues
here?

[mithro-old, 2015-09-10 22:32:16]

On 2015/09/10 21:30:23, danakj wrote:
> So, we could add a templated method to iterate ObserverList and let you run a
> lambda in it for each observer instead of calling a method on the observers:
> 
> template<typename ObserverType, typename ObserverListType, typename Func>
> inline void ForEachObserver(ObserverListType* list, Func func) {
>   if (!list->might_have_observers())
>     return;
>   typename ObserverListType::Iterator it(list);
>   ObserverType* obs;
>   while ((obs = it.GetNext()) != nullptr)
>     func(obs);
> }

Just a FYI, macros do support multi-line statement calls in them as shown by the
following gtest example;
  #ifndef NDEBUG
    EXPECT_DEATH({
                   obs.OnBeginFrame(CreateBeginFrameArgsForTesting(
                       BEGINFRAME_FROM_HERE, 50, 200, 300));
                 },
                 "");
  #endif

So the macro version could just be;
  OtherThing::Walk() const {
    FOR_EACH_OBSERVER_CONST(MyObserver, observer_list_, {
          dict->BeginDictionary();
          obs->AsValueInto(dict);
          dict->EndDictionary();
    });
  }

If we use a macro or a template I think is doesn't actually matter though.

> Then the const problem ends up like:
> 
>   base::ObserverList<BeginFrameObserver>* mutable_list_ =
>       const_cast<base::ObserverList<BeginFrameObserver>*>(&observer_list_);
>   base::ForEachObserver<const BeginFrameObserver>(
>       mutable_list_, [dict](const BeginFrameObserver* obs) {
>         dict->BeginDictionary();
>         obs->AsValueInto(dict);
>         dict->EndDictionary();
>       });
> 
> Which is definitely less crazy...

This seems "less safe" because if you drop the const from the BeginFrameObserver
in the "mutable_list_, [dict](const BeginFrameObserver* obs) {" line you can
accidently calling non-const methods on obs.

While the difference is probably analogous to the difference between pointing a
single barrel shotgun verse double barrel shotgun shotgun at your foot, it still
feels better too me.

> 
> All this has left me wondering why we're trying to shoe-horn ObserverList in
> here, as it doesn't seem to really fit anymore.
> 
> Would a simple vector of Foo*'s work better for all of out use cases/issues
> here?

The main reason for wanting to use ObserverList here is that we need to allow
observers to remove themselves in the OnBeginFrame callback we send them.

There are a couple of ways we could get this behaviour with a simple vector of
Foo*;
 * Make a copy of the vector before iterating over them
 * RemoveObserver sets the vector value to null and then we have a "compact"
method which actually removes the elements.

In the end we would just be implementing a more featureful version of
ObserverList?

Tim 'mithro' Ansell

[danakj, 2015-09-10 22:37:44]

On Thu, Sep 10, 2015 at 3:32 PM, <mithro@mithis.com> wrote:

> On 2015/09/10 21:30:23, danakj wrote:
>
>> So, we could add a templated method to iterate ObserverList and let you
>> run a
>> lambda in it for each observer instead of calling a method on the
>> observers:
>>
>
> template<typename ObserverType, typename ObserverListType, typename Func>
>> inline void ForEachObserver(ObserverListType* list, Func func) {
>>    if (!list->might_have_observers())
>>      return;
>>    typename ObserverListType::Iterator it(list);
>>    ObserverType* obs;
>>    while ((obs = it.GetNext()) != nullptr)
>>      func(obs);
>> }
>>
>
> Just a FYI, macros do support multi-line statement calls in them as shown
> by the
> following gtest example;
>   #ifndef NDEBUG
>     EXPECT_DEATH({
>                    obs.OnBeginFrame(CreateBeginFrameArgsForTesting(
>                        BEGINFRAME_FROM_HERE, 50, 200, 300));
>                  },
>                  "");
>   #endif
>

Ah ya cool.


>
> So the macro version could just be;
>   OtherThing::Walk() const {
>     FOR_EACH_OBSERVER_CONST(MyObserver, observer_list_, {
>           dict->BeginDictionary();
>           obs->AsValueInto(dict);
>           dict->EndDictionary();
>     });
>   }
>

I think that won't work though because the macro does "obs->func" which
would translate to "obs->{dict->BeginDi.... }" which isn't valid c++.


>
> If we use a macro or a template I think is doesn't actually matter though.
>
> Then the const problem ends up like:
>>
>
>    base::ObserverList<BeginFrameObserver>* mutable_list_ =
>>
>>  const_cast<base::ObserverList<BeginFrameObserver>*>(&observer_list_);
>>    base::ForEachObserver<const BeginFrameObserver>(
>>        mutable_list_, [dict](const BeginFrameObserver* obs) {
>>          dict->BeginDictionary();
>>          obs->AsValueInto(dict);
>>          dict->EndDictionary();
>>        });
>>
>
> Which is definitely less crazy...
>>
>
> This seems "less safe" because if you drop the const from the
> BeginFrameObserver
> in the "mutable_list_, [dict](const BeginFrameObserver* obs) {" line you
> can
> accidently calling non-const methods on obs.
>
> While the difference is probably analogous to the difference between
> pointing a
> single barrel shotgun verse double barrel shotgun shotgun at your foot, it
> still
> feels better too me.


It's perhaps "less safe" but I don't think either are safe anyway (as said
before). And I do think it's a lot easier to tell what's going on vs the
getConstIterator method which is pretty confusing to read, that method
mostly just makes me kinda sad.


>
>
>
> All this has left me wondering why we're trying to shoe-horn ObserverList
>> in
>> here, as it doesn't seem to really fit anymore.
>>
>
> Would a simple vector of Foo*'s work better for all of out use cases/issues
>> here?
>>
>
> The main reason for wanting to use ObserverList here is that we need to
> allow
> observers to remove themselves in the OnBeginFrame callback we send them.
>
> There are a couple of ways we could get this behaviour with a simple
> vector of
> Foo*;
>  * Make a copy of the vector before iterating over them
>  * RemoveObserver sets the vector value to null and then we have a
> "compact"
> method which actually removes the elements.
>
> In the end we would just be implementing a more featureful version of
> ObserverList?


Things in base are general but they don't all work for every situation.
Taking a copy of the vector first seems like it would be fine, and very
simple. I don't think it's making a more featureful ObserverList. It would
be strongly typed (not templated) and specific to this particular use case.


>
>
> Tim 'mithro' Ansell
>
> https://codereview.chromium.org/1026233002/
>

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

[brianderson, 2015-09-12 00:23:51]

Tests look good. Will take another look at this patch when it's rebased on
https://codereview.chromium.org/1337803002.

https://codereview.chromium.org/1026233002/diff/40001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/1026233002/diff/40001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:271: TEST(BeginFrameSourceBaseTest,
ObserverMulti) {
Looks like the same test as ObserverMultiSimple?

https://codereview.chromium.org/1026233002/diff/40001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:338: TEST(BeginFrameSourceBaseTest,
ObserverMultiRemoveOnBeginFrame) {
If you are going to add a WillLoseBeginFrameSource as part of this patch alo add
a test similar to this one.

BeginFrameObserverMap supports adding observers while triggering an observer. I
can't think of a case where that's needed. If it's not, remove it from
BeginFrameObserverMap. If it is, there should probably be a test in here for
that.