Add WaitForCondition() and use it in ExtensionTestNotificationObserver.

Add WaitForCondition() and use it in ExtensionTestNotificationObserver.

BUG=none
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235331

[Avi (use Gerrit), 2013-10-23 19:21:52]

I see what you're getting at but don't see the point.

With notifications, it's generic. "I wait for NOTIFICATION_XXX and then do this
test". You're splitting it here so that we can reuse the part that waits on the
message loop (named here ConditionRunLoop).

But that part is tiny. ConditionRunLoop is tiny. In the new world
post-notifications, you're probably going to have to create a new class in many
situations to listen for the specific cases. So what are we saving by extracting
a tiny bit of functionality into ConditionRunLoop? I bet that in this
changelist, if you deleted the ConditionRunLoop class, saving 30 lines,
NotificationCallbackRunner would gain three lines.

https://codereview.chromium.org/30943005/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_test_notification_observer.cc (right):

https://codereview.chromium.org/30943005/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_test_notification_observer.cc:34:
base::RunLoop run_loop_;
MessageLoopRunner is probably a better choice.

[Bernhard Bauer, 2013-10-23 21:14:28]

On 2013/10/23 19:21:52, Avi wrote:
> I see what you're getting at but don't see the point.
> 
> With notifications, it's generic. "I wait for NOTIFICATION_XXX and then do
this
> test". You're splitting it here so that we can reuse the part that waits on
the
> message loop (named here ConditionRunLoop).
> 
> But that part is tiny. ConditionRunLoop is tiny. In the new world
> post-notifications, you're probably going to have to create a new class in
many
> situations to listen for the specific cases. So what are we saving by
extracting
> a tiny bit of functionality into ConditionRunLoop? I bet that in this
> changelist, if you deleted the ConditionRunLoop class, saving 30 lines,
> NotificationCallbackRunner would gain three lines.

If you remember the original discussion, Jeffrey's proposal was to extend a
generic class for this (i.e. WindowedNotificationObserver, which turned out not
to work because of Bartosz' change), to which you replied that generic
notification classes are going to go away together with notifications. Hence my
suggestion to keep the part that actually relies on notifications separate from
the rest, so it can be replaced more easily (which is something that will need
to happen no matter what I do). So what do you concretely suggest in this case?
Use a generic class that relies on notifications? Create separate
ExtensionPageActionCountObserver and ExtensionPageActionVisibilityObserver
classes? Or keep everything as it is, which encourages people to use
WindowedNotificationObserver in a wrong way?

[Avi (use Gerrit), 2013-10-23 21:21:53]

On 2013/10/23 21:14:28, Bernhard Bauer wrote:
> On 2013/10/23 19:21:52, Avi wrote:
> > I see what you're getting at but don't see the point.
> > 
> > With notifications, it's generic. "I wait for NOTIFICATION_XXX and then do
> this
> > test". You're splitting it here so that we can reuse the part that waits on
> the
> > message loop (named here ConditionRunLoop).
> > 
> > But that part is tiny. ConditionRunLoop is tiny. In the new world
> > post-notifications, you're probably going to have to create a new class in
> many
> > situations to listen for the specific cases. So what are we saving by
> extracting
> > a tiny bit of functionality into ConditionRunLoop? I bet that in this
> > changelist, if you deleted the ConditionRunLoop class, saving 30 lines,
> > NotificationCallbackRunner would gain three lines.
> 
> If you remember the original discussion, Jeffrey's proposal was to extend a
> generic class for this (i.e. WindowedNotificationObserver, which turned out
not
> to work because of Bartosz' change), to which you replied that generic
> notification classes are going to go away together with notifications. Hence
my
> suggestion to keep the part that actually relies on notifications separate
from
> the rest, so it can be replaced more easily (which is something that will need
> to happen no matter what I do). So what do you concretely suggest in this
case?
> Use a generic class that relies on notifications? Create separate
> ExtensionPageActionCountObserver and ExtensionPageActionVisibilityObserver
> classes? Or keep everything as it is, which encourages people to use
> WindowedNotificationObserver in a wrong way?

I don't see an alternative other than creating separate classes. :(

The other possibility is to build something atop the sigaction-type thing that
Cait and Albert were proposing but I'm not sure that will ever get off the
ground.

[Bernhard Bauer, 2013-10-23 21:42:54]

On Wed, Oct 23, 2013 at 11:21 PM, <avi@chromium.org> wrote:

> On 2013/10/23 21:14:28, Bernhard Bauer wrote:
>
>> On 2013/10/23 19:21:52, Avi wrote:
>> > I see what you're getting at but don't see the point.
>> >
>> > With notifications, it's generic. "I wait for NOTIFICATION_XXX and then
>> do
>> this
>> > test". You're splitting it here so that we can reuse the part that
>> waits on
>> the
>> > message loop (named here ConditionRunLoop).
>> >
>> > But that part is tiny. ConditionRunLoop is tiny. In the new world
>> > post-notifications, you're probably going to have to create a new class
>> in
>> many
>> > situations to listen for the specific cases. So what are we saving by
>> extracting
>> > a tiny bit of functionality into ConditionRunLoop? I bet that in this
>> > changelist, if you deleted the ConditionRunLoop class, saving 30 lines,
>> > NotificationCallbackRunner would gain three lines.
>>
>
>  If you remember the original discussion, Jeffrey's proposal was to extend
>> a
>> generic class for this (i.e. WindowedNotificationObserver, which turned
>> out
>>
> not
>
>> to work because of Bartosz' change), to which you replied that generic
>> notification classes are going to go away together with notifications.
>> Hence
>>
> my
>
>> suggestion to keep the part that actually relies on notifications separate
>>
> from
>
>> the rest, so it can be replaced more easily (which is something that will
>> need
>> to happen no matter what I do). So what do you concretely suggest in this
>>
> case?
>
>> Use a generic class that relies on notifications? Create separate
>> ExtensionPageActionCountObserv**er and ExtensionPageActionVisibilityO**
>> bserver
>> classes? Or keep everything as it is, which encourages people to use
>> WindowedNotificationObserver in a wrong way?
>>
>
> I don't see an alternative other than creating separate classes. :(
>

Right :-( But in that case, would you actually object to pulling as much
common functionality out of these classes as possible? That is basically
what I want to do with ConditionRunLoop.


> The other possibility is to build something atop the sigaction-type thing
> that
> Cait and Albert were proposing but I'm not sure that will ever get off the
> ground.
>

Is that the "Notifications 2.0" thing? Yeah, I have the feeling it's
eventually going to be something like that (although possibly not that
exact proposal).

https://codereview.chromium.**org/30943005/<https://codereview.chromium.org/3...
>

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

[Avi (use Gerrit), 2013-10-24 04:05:21]

> > I don't see an alternative other than creating separate classes. :(
> 
> Right :-( But in that case, would you actually object to pulling as much
> common functionality out of these classes as possible? That is basically
> what I want to do with ConditionRunLoop.

Right, but what you're pulling out is so minimal that I don't see the point.
It's basically running a MessageLoopRunner, which is itself a self-contained
object. I'm not going to call foul if you really want to, but I'm not
enthralled.

> Is that the "Notifications 2.0" thing? Yeah, I have the feeling it's
> eventually going to be something like that (although possibly not that
> exact proposal).

Yeah. There's great appeal to that in terms of modularity, especially for
something like this (waiting for notifications turns into waiting for a signal),
but there are a few people with experience with similar systems that have
hesitations. I'm waiting to see how it plays out.

[Bernhard Bauer, 2013-10-24 18:06:32]

On 2013/10/24 04:05:21, Avi wrote:
> > > I don't see an alternative other than creating separate classes. :(
> > 
> > Right :-( But in that case, would you actually object to pulling as much
> > common functionality out of these classes as possible? That is basically
> > what I want to do with ConditionRunLoop.
> 
> Right, but what you're pulling out is so minimal that I don't see the point.
> It's basically running a MessageLoopRunner, which is itself a self-contained
> object. I'm not going to call foul if you really want to, but I'm not
> enthralled.

I think it is useful. Sure, it's not a lot of code, but a couple of lines saved
adds up very quickly if we're going to need to add a lot of new classes.
test_utils.cc etc. is full of these small helper classes, and even
WindowedNotificationObserver itself is really just a NotificationRegistrary with
a MessageLoopRunner.

In addition, if we should end up not adding a class for every former
notification (because we use some generic solution), then we might even get by
without writing any new classes, by combining existing things! For example, in
this CL I was also able to rewrite WaitForExtensionViewsToLoad() (which waits
for one of two notifications repeatedly in a loop) using
NotificationCallbackRunner and ConditionRunLoop.

> > Is that the "Notifications 2.0" thing? Yeah, I have the feeling it's
> > eventually going to be something like that (although possibly not that
> > exact proposal).
> 
> Yeah. There's great appeal to that in terms of modularity, especially for
> something like this (waiting for notifications turns into waiting for a
signal),
> but there are a few people with experience with similar systems that have
> hesitations. I'm waiting to see how it plays out.

[Bernhard Bauer, 2013-10-24 18:06:54]

Jeffrey, want to take a look?

[Avi (use Gerrit), 2013-10-24 18:41:48]

If you find it useful I will not object.

MessageLoopRunner, though.

[Jeffrey Yasskin, 2013-10-24 23:49:22]

It would be great if we could have only one MessageLoop runner instead of both
RunLoop and MessageLoopRunner. But you can't fix that in this CL.

https://codereview.chromium.org/30943005/diff/270001/chrome/browser/extension...
File chrome/browser/extensions/extension_test_notification_observer.cc (right):

https://codereview.chromium.org/30943005/diff/270001/chrome/browser/extension...
chrome/browser/extensions/extension_test_notification_observer.cc:80: class
NotificationCallbackRunner : public content::NotificationObserver {
What if we had something like:

class NotificationSet : public content::NotificationObserver {
 public:
  NotificationSet* Add(int type, const content::NotificationSource& source =
content::NotificationService::AllSources());
  // Notified any time an Add()ed notification is received.
  // The details of the notification are dropped.
  CallbackList<void()>& callback_list();
}

Then we could write WaitForCondition(Callback<bool()>, NotificationSet*) to
replace the multi-notification case in WaitForExtensionViewsToLoad().

We could, instead, make WaitForCondition(Callback<bool()> a member of the
NotificationSet, which would replace the callback_list() and possibly the
inheritance from NotificationObserver, but might make the class a little less
general.

https://codereview.chromium.org/30943005/diff/270001/chrome/browser/extension...
chrome/browser/extensions/extension_test_notification_observer.cc:169:
WaitForConditionWithNotification(
I definitely like this since it removes the duplicated condition.

https://codereview.chromium.org/30943005/diff/270001/chrome/browser/extension...
chrome/browser/extensions/extension_test_notification_observer.cc:192:
NotificationCallbackRunner callback_runner(
I don't really like using NotificationCallbackRunner directly since it makes it
harder to expose a simple interface to other files, and I think we want this as
a general test utility.

[Bernhard Bauer, 2013-10-29 16:37:01]

https://codereview.chromium.org/30943005/diff/270001/chrome/browser/extension...
File chrome/browser/extensions/extension_test_notification_observer.cc (right):

https://codereview.chromium.org/30943005/diff/270001/chrome/browser/extension...
chrome/browser/extensions/extension_test_notification_observer.cc:80: class
NotificationCallbackRunner : public content::NotificationObserver {
On 2013/10/24 23:49:23, Jeffrey Yasskin wrote:
> What if we had something like:
> 
> class NotificationSet : public content::NotificationObserver {
>  public:
>   NotificationSet* Add(int type, const content::NotificationSource& source =
> content::NotificationService::AllSources());
>   // Notified any time an Add()ed notification is received.
>   // The details of the notification are dropped.
>   CallbackList<void()>& callback_list();
> }
> 
> Then we could write WaitForCondition(Callback<bool()>, NotificationSet*) to
> replace the multi-notification case in WaitForExtensionViewsToLoad().

Done. As an aside, the interface to CallbackList is Not Awesome.

> We could, instead, make WaitForCondition(Callback<bool()> a member of the
> NotificationSet, which would replace the callback_list() and possibly the
> inheritance from NotificationObserver, but might make the class a little less
> general.

Or we could add a method ConditionRunLoop::WaitWithCallbackList, which would
subscribe itself to the callback list, then wait on it?

[Bernhard Bauer, 2013-11-04 12:24:46]

On 2013/10/29 16:37:01, Bernhard Bauer wrote:
>
https://codereview.chromium.org/30943005/diff/270001/chrome/browser/extension...
> File chrome/browser/extensions/extension_test_notification_observer.cc
(right):
> 
>
https://codereview.chromium.org/30943005/diff/270001/chrome/browser/extension...
> chrome/browser/extensions/extension_test_notification_observer.cc:80: class
> NotificationCallbackRunner : public content::NotificationObserver {
> On 2013/10/24 23:49:23, Jeffrey Yasskin wrote:
> > What if we had something like:
> > 
> > class NotificationSet : public content::NotificationObserver {
> >  public:
> >   NotificationSet* Add(int type, const content::NotificationSource& source =
> > content::NotificationService::AllSources());
> >   // Notified any time an Add()ed notification is received.
> >   // The details of the notification are dropped.
> >   CallbackList<void()>& callback_list();
> > }
> > 
> > Then we could write WaitForCondition(Callback<bool()>, NotificationSet*) to
> > replace the multi-notification case in WaitForExtensionViewsToLoad().
> 
> Done. As an aside, the interface to CallbackList is Not Awesome.
> 
> > We could, instead, make WaitForCondition(Callback<bool()> a member of the
> > NotificationSet, which would replace the callback_list() and possibly the
> > inheritance from NotificationObserver, but might make the class a little
less
> > general.
> 
> Or we could add a method ConditionRunLoop::WaitWithCallbackList, which would
> subscribe itself to the callback list, then wait on it?

Ping?

[Jeffrey Yasskin, 2013-11-06 03:33:59]

Sorry for the delay. lgtm, but see if you like the suggestions below. Thanks for
the refactoring!

https://codereview.chromium.org/30943005/diff/270001/chrome/browser/extension...
File chrome/browser/extensions/extension_test_notification_observer.cc (right):

https://codereview.chromium.org/30943005/diff/270001/chrome/browser/extension...
chrome/browser/extensions/extension_test_notification_observer.cc:80: class
NotificationCallbackRunner : public content::NotificationObserver {
On 2013/10/29 16:37:02, Bernhard Bauer wrote:
> On 2013/10/24 23:49:23, Jeffrey Yasskin wrote:
> > What if we had something like:
> > 
> > class NotificationSet : public content::NotificationObserver {
> >  public:
> >   NotificationSet* Add(int type, const content::NotificationSource& source =
> > content::NotificationService::AllSources());
> >   // Notified any time an Add()ed notification is received.
> >   // The details of the notification are dropped.
> >   CallbackList<void()>& callback_list();
> > }
> > 
> > Then we could write WaitForCondition(Callback<bool()>, NotificationSet*) to
> > replace the multi-notification case in WaitForExtensionViewsToLoad().
> 
> Done. As an aside, the interface to CallbackList is Not Awesome.

:-/

> > We could, instead, make WaitForCondition(Callback<bool()> a member of the
> > NotificationSet, which would replace the callback_list() and possibly the
> > inheritance from NotificationObserver, but might make the class a little
less
> > general.
> 
> Or we could add a method ConditionRunLoop::WaitWithCallbackList, which would
> subscribe itself to the callback list, then wait on it? 

I'd actually suggest getting rid of ConditionRunLoop now that it's only used in
one place. See my comment on WaitForCondition.

https://codereview.chromium.org/30943005/diff/500001/chrome/browser/extension...
File chrome/browser/extensions/extension_test_notification_observer.cc (right):

https://codereview.chromium.org/30943005/diff/500001/chrome/browser/extension...
chrome/browser/extensions/extension_test_notification_observer.cc:123:
ConditionRunLoop run_loop(condition);
Since this is now the only use of ConditionRunLoop, I think you can inline it
here:

static void QuitIfCallbackTrue(MessageLoopRunner* run_loop,
                        const base::Callback<bool(void)>* condition) {
  if (condition->Run())
    runner->Quit();
}

if (condition.Run())
  return;
scoped_refptr<MessageLoopRunner> run_loop(new MessageLoopRunner);
scoped_ptr<base::CallbackList<void()>::Subscription> subscription =
    notification_set->callback_list().Add(
      base::Bind(&QuitIfCallbackTrue, base::Unretained(run_loop.get()),
                 base::Unretained(&condition)));
run_loop->Run();

https://codereview.chromium.org/30943005/diff/500001/chrome/browser/extension...
chrome/browser/extensions/extension_test_notification_observer.cc:133:
NotificationSet notification_set;
I think I'd weakly prefer duplicating these two lines at each call site, but
it's not a big deal.

[Bernhard Bauer, 2013-11-11 14:37:44]

Albert, can I get an OWNERS review for CallbackList?

https://codereview.chromium.org/30943005/diff/500001/chrome/browser/extension...
File chrome/browser/extensions/extension_test_notification_observer.cc (right):

https://codereview.chromium.org/30943005/diff/500001/chrome/browser/extension...
chrome/browser/extensions/extension_test_notification_observer.cc:123:
ConditionRunLoop run_loop(condition);
On 2013/11/06 03:33:59, Jeffrey Yasskin wrote:
> Since this is now the only use of ConditionRunLoop, I think you can inline it
> here:
> 
> static void QuitIfCallbackTrue(MessageLoopRunner* run_loop,
>                         const base::Callback<bool(void)>* condition) {
>   if (condition->Run())
>     runner->Quit();
> }
> 
> if (condition.Run())
>   return;
> scoped_refptr<MessageLoopRunner> run_loop(new MessageLoopRunner);
> scoped_ptr<base::CallbackList<void()>::Subscription> subscription =
>     notification_set->callback_list().Add(
>       base::Bind(&QuitIfCallbackTrue, base::Unretained(run_loop.get()),
>                  base::Unretained(&condition)));
> run_loop->Run();

Done!

https://codereview.chromium.org/30943005/diff/500001/chrome/browser/extension...
chrome/browser/extensions/extension_test_notification_observer.cc:133:
NotificationSet notification_set;
On 2013/11/06 03:33:59, Jeffrey Yasskin wrote:
> I think I'd weakly prefer duplicating these two lines at each call site, but
> it's not a big deal.

Hm... I kind of like having the short cut.

[awong, 2013-11-12 01:49:13]

Forwarding to Cait since she wrote this. Will rubber stamp if she says it's
good.

[Cait (Slow), 2013-11-12 17:04:07]

I'm ok with the addition of the WARN_UNUSED_RESULT (there was a lot of back and
forth in the original CL on whether or not one was needed there, but it
certainly doesn't hurt).

I'm a bit confused as to why this code requires a CallbackList though -- are
there plans to have it handle several callbacks at once in the future? 

BTW, if you have thoughts on how to improve the CallbackList API,  I'd love to
hear them..

https://codereview.chromium.org/30943005/diff/600001/chrome/browser/extension...
File chrome/browser/extensions/extension_test_notification_observer.cc (right):

https://codereview.chromium.org/30943005/diff/600001/chrome/browser/extension...
chrome/browser/extensions/extension_test_notification_observer.cc:105:
base::Bind(&MaybeQuit, base::Unretained(runner.get()), condition));
What's the reasoning behind using a CallbackList here? AFAICT, it only ever has
once callback in it...

[Bernhard Bauer, 2013-11-12 17:51:44]

> I'm ok with the addition of the WARN_UNUSED_RESULT (there was a lot of back
and
> forth in the original CL on whether or not one was needed there, but it
> certainly doesn't hurt).
> 
> I'm a bit confused as to why this code requires a CallbackList though -- are
> there plans to have it handle several callbacks at once in the future? 
> 
> BTW, if you have thoughts on how to improve the CallbackList API,  I'd love to
> hear them..

Well, it's mostly the fact that you have to keep the subscription around
(instead of passing in a callback bound to a WeakPtr and not having to deal with
it later), together with the template size
(scoped_ptr<base::CallbackList<void()>::Subscription> is quite a mouthful).

https://codereview.chromium.org/30943005/diff/600001/chrome/browser/extension...
File chrome/browser/extensions/extension_test_notification_observer.cc (right):

https://codereview.chromium.org/30943005/diff/600001/chrome/browser/extension...
chrome/browser/extensions/extension_test_notification_observer.cc:105:
base::Bind(&MaybeQuit, base::Unretained(runner.get()), condition));
On 2013/11/12 17:04:07, Cait Phillips wrote:
> What's the reasoning behind using a CallbackList here? AFAICT, it only ever
has
> once callback in it...

I think the idea was to use CallbackList in place of a generic event you can
subscribe to, but Jeffrey can probably explain better.

[Cait (Slow), 2013-11-12 18:26:30]

On 2013/11/12 17:51:44, Bernhard Bauer wrote:
> > I'm ok with the addition of the WARN_UNUSED_RESULT (there was a lot of back
> and
> > forth in the original CL on whether or not one was needed there, but it
> > certainly doesn't hurt).
> > 
> > I'm a bit confused as to why this code requires a CallbackList though -- are
> > there plans to have it handle several callbacks at once in the future? 
> > 
> > BTW, if you have thoughts on how to improve the CallbackList API,  I'd love
to
> > hear them..
> 
> Well, it's mostly the fact that you have to keep the subscription around
> (instead of passing in a callback bound to a WeakPtr and not having to deal
with
> it later), together with the template size
> (scoped_ptr<base::CallbackList<void()>::Subscription> is quite a mouthful).

Even if you pass a callback bound to a WeakPtr, it seems the preferred approach
is still to remove your callback on destruction (to avoid having a bunch of dead
weakptrs floating around). Which means that you have to hold onto something
(callback, subscription etc.)  to do the unregistering of your callback. Also, 
w/o the subscription, services managing CallbackLists need to expose both an add
and remove method.


> 
>
https://codereview.chromium.org/30943005/diff/600001/chrome/browser/extension...
> File chrome/browser/extensions/extension_test_notification_observer.cc
(right):
> 
>
https://codereview.chromium.org/30943005/diff/600001/chrome/browser/extension...
> chrome/browser/extensions/extension_test_notification_observer.cc:105:
> base::Bind(&MaybeQuit, base::Unretained(runner.get()), condition));
> On 2013/11/12 17:04:07, Cait Phillips wrote:
> > What's the reasoning behind using a CallbackList here? AFAICT, it only ever
> has
> > once callback in it...
> 
> I think the idea was to use CallbackList in place of a generic event you can
> subscribe to, but Jeffrey can probably explain better.

[Bernhard Bauer, 2013-11-12 18:44:46]

On 2013/11/12 18:26:30, Cait Phillips wrote:
> On 2013/11/12 17:51:44, Bernhard Bauer wrote:
> > > I'm ok with the addition of the WARN_UNUSED_RESULT (there was a lot of
back
> > and
> > > forth in the original CL on whether or not one was needed there, but it
> > > certainly doesn't hurt).
> > > 
> > > I'm a bit confused as to why this code requires a CallbackList though --
are
> > > there plans to have it handle several callbacks at once in the future? 
> > > 
> > > BTW, if you have thoughts on how to improve the CallbackList API,  I'd
love
> to
> > > hear them..
> > 
> > Well, it's mostly the fact that you have to keep the subscription around
> > (instead of passing in a callback bound to a WeakPtr and not having to deal
> with
> > it later), together with the template size
> > (scoped_ptr<base::CallbackList<void()>::Subscription> is quite a mouthful).
> 
> Even if you pass a callback bound to a WeakPtr, it seems the preferred
approach
> is still to remove your callback on destruction (to avoid having a bunch of
dead
> weakptrs floating around). Which means that you have to hold onto something
> (callback, subscription etc.)  to do the unregistering of your callback. Also,

> w/o the subscription, services managing CallbackLists need to expose both an
add
> and remove method.

Right, but here for example the CallbackList and the object the callback is
bound to are destroyed right after another, so the invalidated WeakPtr will not
stay around for long anyway. To look at it the other way, the only reason to use
a WeakPtr is if its pointee may be destroyed while the pointer is still around,
so any time someone uses it, they are fine with having the invalidated WeakPtr
around.

I know there are excellent reasons for doing things the way they are done right
now in CallbackList (and I have no problem with keeping the API as it is), but
there are some cases where the reasons don't really apply (for example because
we control the lifetime of both objects), and it would be neat to have a
simpler, fire-and-forget way for those cases. You did ask for my thoughts ;-)

> 
> 
> > 
> >
>
https://codereview.chromium.org/30943005/diff/600001/chrome/browser/extension...
> > File chrome/browser/extensions/extension_test_notification_observer.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/30943005/diff/600001/chrome/browser/extension...
> > chrome/browser/extensions/extension_test_notification_observer.cc:105:
> > base::Bind(&MaybeQuit, base::Unretained(runner.get()), condition));
> > On 2013/11/12 17:04:07, Cait Phillips wrote:
> > > What's the reasoning behind using a CallbackList here? AFAICT, it only
ever
> > has
> > > once callback in it...
> > 
> > I think the idea was to use CallbackList in place of a generic event you can
> > subscribe to, but Jeffrey can probably explain better.

[Cait (Slow), 2013-11-14 15:32:29]

On 2013/11/12 18:44:46, Bernhard Bauer wrote:
> On 2013/11/12 18:26:30, Cait Phillips wrote:
> > On 2013/11/12 17:51:44, Bernhard Bauer wrote:
> > > > I'm ok with the addition of the WARN_UNUSED_RESULT (there was a lot of
> back
> > > and
> > > > forth in the original CL on whether or not one was needed there, but it
> > > > certainly doesn't hurt).
> > > > 
> > > > I'm a bit confused as to why this code requires a CallbackList though --
> are
> > > > there plans to have it handle several callbacks at once in the future? 
> > > > 
> > > > BTW, if you have thoughts on how to improve the CallbackList API,  I'd
> love
> > to
> > > > hear them..
> > > 
> > > Well, it's mostly the fact that you have to keep the subscription around
> > > (instead of passing in a callback bound to a WeakPtr and not having to
deal
> > with
> > > it later), together with the template size
> > > (scoped_ptr<base::CallbackList<void()>::Subscription> is quite a
mouthful).
> > 
> > Even if you pass a callback bound to a WeakPtr, it seems the preferred
> approach
> > is still to remove your callback on destruction (to avoid having a bunch of
> dead
> > weakptrs floating around). Which means that you have to hold onto something
> > (callback, subscription etc.)  to do the unregistering of your callback.
Also,
> 
> > w/o the subscription, services managing CallbackLists need to expose both an
> add
> > and remove method.
> 
> Right, but here for example the CallbackList and the object the callback is
> bound to are destroyed right after another, so the invalidated WeakPtr will
not
> stay around for long anyway. To look at it the other way, the only reason to
use
> a WeakPtr is if its pointee may be destroyed while the pointer is still
around,
> so any time someone uses it, they are fine with having the invalidated WeakPtr
> around.
> 
> I know there are excellent reasons for doing things the way they are done
right
> now in CallbackList (and I have no problem with keeping the API as it is), but
> there are some cases where the reasons don't really apply (for example because
> we control the lifetime of both objects), and it would be neat to have a
> simpler, fire-and-forget way for those cases. You did ask for my thoughts ;-)
> 

This is partly why I'm questioning whether CallbackList is the right thing to
use here. I agree some sort of fire-and-forget approach for waiting for events
in tests would be very useful.
Seems like the general idea here is that you want to be able to have a callback
run when a notification is fired which checks a condition and if the condition
is not true, it keeps waiting. So why not just provide a single callback to
NotificationSet, and run it whenever a notification gets called, and reset is
when the NotificationSet goes away? Then the WaitForCondition methods wouldn't
need to hold onto anything...


> > 
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/30943005/diff/600001/chrome/browser/extension...
> > > File chrome/browser/extensions/extension_test_notification_observer.cc
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/30943005/diff/600001/chrome/browser/extension...
> > > chrome/browser/extensions/extension_test_notification_observer.cc:105:
> > > base::Bind(&MaybeQuit, base::Unretained(runner.get()), condition));
> > > On 2013/11/12 17:04:07, Cait Phillips wrote:
> > > > What's the reasoning behind using a CallbackList here? AFAICT, it only
> ever
> > > has
> > > > once callback in it...
> > > 
> > > I think the idea was to use CallbackList in place of a generic event you
can
> > > subscribe to, but Jeffrey can probably explain better.

[Cait (Slow), 2013-11-14 15:36:26]

so as not to block you, base/callbackList changes lgtm 

(regardless of whether you decide to use callbacks or CallbackLists)

[awong, 2013-11-14 15:40:36]

base OWNERS lgtm rubber-stamp

FWIW, I also don't quite understand why you are exposing a CallbackList
here.


On Thu, Nov 14, 2013 at 7:36 AM, <caitkp@chromium.org> wrote:

> so as not to block you, base/callbackList changes lgtm
>
> (regardless of whether you decide to use callbacks or CallbackLists)
>
> https://codereview.chromium.org/30943005/
>

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

[Jeffrey Yasskin (google), 2013-11-14 17:38:32]

On Tue, Nov 12, 2013 at 9:51 AM, <bauerb@chromium.org> wrote:
>> I'm ok with the addition of the WARN_UNUSED_RESULT (there was a lot of
>> back
>
> and
>>
>> forth in the original CL on whether or not one was needed there, but it
>> certainly doesn't hurt).
>
>
>> I'm a bit confused as to why this code requires a CallbackList though --
>> are
>> there plans to have it handle several callbacks at once in the future?
>
>
>> BTW, if you have thoughts on how to improve the CallbackList API, I'd
>> love to
>> hear them..
>
>
> Well, it's mostly the fact that you have to keep the subscription around
> (instead of passing in a callback bound to a WeakPtr and not having to
deal
> with
> it later), together with the template size
> (scoped_ptr<base::CallbackList<void()>::Subscription> is quite a
mouthful).
>
>
>
>
https://codereview.chromium.org/30943005/diff/600001/chrome/browser/extension...
> File chrome/browser/extensions/extension_test_notification_observer.cc
> (right):
>
>
https://codereview.chromium.org/30943005/diff/600001/chrome/browser/extension...
> chrome/browser/extensions/extension_test_notification_observer.cc:105:
> base::Bind(&MaybeQuit, base::Unretained(runner.get()), condition));
> On 2013/11/12 17:04:07, Cait Phillips wrote:
>>
>> What's the reasoning behind using a CallbackList here? AFAICT, it only
>
> ever has
>>
>> once callback in it...
>
>
> I think the idea was to use CallbackList in place of a generic event you
> can subscribe to, but Jeffrey can probably explain better.

I'm thinking about ways to build composable abstractions. Even though, in
this case, we only need one callback, there's no way to use something
that's optimized to that extent as a vocabulary type to communicate between
modules. With a CallbackList in NotificationSet, it can be used to bridge
between code that sends notifications and code that listens on certain
kinds of CallbackLists.

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

[commit-bot: I haz the power, 2013-11-15 10:01:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/30943005/600001

[commit-bot: I haz the power, 2013-11-15 12:52:19]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 13:05:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/30943005/600001

[commit-bot: I haz the power, 2013-11-15 15:10:05]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 15:43:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/30943005/600001

[commit-bot: I haz the power, 2013-11-15 15:54:42]

Change committed as 235331