wm: Change the DispatcherClient interface.

wm: Change the DispatcherClient interface.

Instead of having RunWithDispatcher() to start a nested message-loop, and a
corresponding QuitNestedMessageLoop() to terminate it, have a DispatcherRunLoop
object that is similar to base::RunLoop for starting a nested message loop for
a specified DispatcherClient.

BUG=none
R=sky@chromium.org

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

[sadrul, 2014-05-27 05:05:00]

The test failures on the bot look related to this change. I am going to take a
look at that and ping back for review when they are fixed.

[sadrul, 2014-05-27 09:44:45]

Tests fixed. Ready for review.

[sky, 2014-05-27 17:19:37]

https://codereview.chromium.org/280483003/diff/150001/ui/wm/public/dispatcher...
File ui/wm/public/dispatcher_client.h (right):

https://codereview.chromium.org/280483003/diff/150001/ui/wm/public/dispatcher...
ui/wm/public/dispatcher_client.h:20: base::Closure* quit_closure) = 0;
It seems more natural to use a scoping object for this. Basically the same thing
as RunLoop. You create the RunLoop, run it, then destroy or quit it to cancel
the runloop. If you did that you could make RunWithDispatcher protected and
friend the RunLoop class.

[sadrul, 2014-05-27 17:37:14]

https://codereview.chromium.org/280483003/diff/150001/ui/wm/public/dispatcher...
File ui/wm/public/dispatcher_client.h (right):

https://codereview.chromium.org/280483003/diff/150001/ui/wm/public/dispatcher...
ui/wm/public/dispatcher_client.h:20: base::Closure* quit_closure) = 0;
On 2014/05/27 17:19:38, sky wrote:
> It seems more natural to use a scoping object for this. Basically the same
thing
> as RunLoop. You create the RunLoop, run it, then destroy or quit it to cancel
> the runloop. If you did that you could make RunWithDispatcher protected and
> friend the RunLoop class.

Do you mean something like:

  DispatcherClient* client = ...;
  DispatcherRunLoop run_loop = client->CreateNestedRunLoop();
  quit_closure_ = run_loop.QuitClosure();
  run_loop.Run();

?

[sky, 2014-05-27 18:04:57]

On Tue, May 27, 2014 at 10:37 AM,  <sadrul@chromium.org> wrote:
>
>
https://codereview.chromium.org/280483003/diff/150001/ui/wm/public/dispatcher...
> File ui/wm/public/dispatcher_client.h (right):
>
>
https://codereview.chromium.org/280483003/diff/150001/ui/wm/public/dispatcher...
> ui/wm/public/dispatcher_client.h:20: base::Closure* quit_closure) = 0;
> On 2014/05/27 17:19:38, sky wrote:
>>
>> It seems more natural to use a scoping object for this. Basically the
>
> same thing
>>
>> as RunLoop. You create the RunLoop, run it, then destroy or quit it to
>
> cancel
>>
>> the runloop. If you did that you could make RunWithDispatcher
>
> protected and
>>
>> friend the RunLoop class.
>
>
> Do you mean something like:
>
>   DispatcherClient* client = ...;
>   DispatcherRunLoop run_loop = client->CreateNestedRunLoop();
>   quit_closure_ = run_loop.QuitClosure();
>   run_loop.Run();

Or perhaps:

DispatcherRunLoop run_loop(client);
run_loop.Run();
...
run_loop.Quit();


>
> ?
>
> https://codereview.chromium.org/280483003/

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

[sadrul, 2014-05-28 21:22:39]

Done

[sky, 2014-05-28 23:11:12]

https://codereview.chromium.org/280483003/diff/220001/ui/wm/core/nested_accel...
File ui/wm/core/nested_accelerator_controller.cc (right):

https://codereview.chromium.org/280483003/diff/220001/ui/wm/core/nested_accel...
ui/wm/core/nested_accelerator_controller.cc:37: run_loop->QuitClosure());
This is more of what I was thinking, but having to express an API in terms of
closures feels awkward. Is there a reason you don't want real Run/Quit methods
DispatcherClient that DispatcherRunLoop calls?

[sadrul, 2014-05-29 00:25:38]

https://codereview.chromium.org/280483003/diff/220001/ui/wm/core/nested_accel...
File ui/wm/core/nested_accelerator_controller.cc (right):

https://codereview.chromium.org/280483003/diff/220001/ui/wm/core/nested_accel...
ui/wm/core/nested_accelerator_controller.cc:37: run_loop->QuitClosure());
On 2014/05/28 23:11:12, sky wrote:
> This is more of what I was thinking, but having to express an API in terms of
> closures feels awkward. Is there a reason you don't want real Run/Quit methods
> DispatcherClient that DispatcherRunLoop calls?

I think that makes it possible for the wrong dispatcher RunLoop to be terminated
if Quit() is called for the same RunLoop multiple times. It also makes it
difficult to get to the QuitClosure before already calling Run(). The
alternative is to not have QuitClosure, and pass around a pointer to the
dispatcher RunLoop instead. But the closure thing feels better I think.

[sky, 2014-05-29 14:50:39]

On Wed, May 28, 2014 at 5:25 PM,  <sadrul@chromium.org> wrote:
>
>
https://codereview.chromium.org/280483003/diff/220001/ui/wm/core/nested_accel...
> File ui/wm/core/nested_accelerator_controller.cc (right):
>
>
https://codereview.chromium.org/280483003/diff/220001/ui/wm/core/nested_accel...
> ui/wm/core/nested_accelerator_controller.cc:37:
> run_loop->QuitClosure());
> On 2014/05/28 23:11:12, sky wrote:
>>
>> This is more of what I was thinking, but having to express an API in
>
> terms of
>>
>> closures feels awkward. Is there a reason you don't want real Run/Quit
>
> methods
>>
>> DispatcherClient that DispatcherRunLoop calls?
>
>
> I think that makes it possible for the wrong dispatcher RunLoop to be
> terminated if Quit() is called for the same RunLoop multiple times. It
> also makes it difficult to get to the QuitClosure before already calling
> Run(). The alternative is to not have QuitClosure, and pass around a
> pointer to the dispatcher RunLoop instead. But the closure thing feels
> better I think.

If Quit only makes sense once could the DispatcherRunLoop track
whether it has invoked Quit?

  -Scott
>
> https://codereview.chromium.org/280483003/

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

[sadrul, 2014-05-29 19:01:54]

On 2014/05/29 14:50:39, sky wrote:
> On Wed, May 28, 2014 at 5:25 PM,  <mailto:sadrul@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/280483003/diff/220001/ui/wm/core/nested_accel...
> > File ui/wm/core/nested_accelerator_controller.cc (right):
> >
> >
>
https://codereview.chromium.org/280483003/diff/220001/ui/wm/core/nested_accel...
> > ui/wm/core/nested_accelerator_controller.cc:37:
> > run_loop->QuitClosure());
> > On 2014/05/28 23:11:12, sky wrote:
> >>
> >> This is more of what I was thinking, but having to express an API in
> >
> > terms of
> >>
> >> closures feels awkward. Is there a reason you don't want real Run/Quit
> >
> > methods
> >>
> >> DispatcherClient that DispatcherRunLoop calls?
> >
> >
> > I think that makes it possible for the wrong dispatcher RunLoop to be
> > terminated if Quit() is called for the same RunLoop multiple times. It
> > also makes it difficult to get to the QuitClosure before already calling
> > Run(). The alternative is to not have QuitClosure, and pass around a
> > pointer to the dispatcher RunLoop instead. But the closure thing feels
> > better I think.
> 
> If Quit only makes sense once could the DispatcherRunLoop track
> whether it has invoked Quit?

That would work I think, yes. The issue of not having a QuitClosure() still
remains though. Do you think the alternative to not having that is acceptable?
(I don't have strong preference either way)

[sky, 2014-05-29 19:35:11]

One more proposal. Make DispatcherRunLoop pure virtual with
DispatcherClient vending the appropriate instance. This way the
DispatcherClient can make the DispatcherRunLoop contain the real
state. Any better?

On Thu, May 29, 2014 at 12:01 PM,  <sadrul@chromium.org> wrote:
> On 2014/05/29 14:50:39, sky wrote:
>
>> On Wed, May 28, 2014 at 5:25 PM,  <mailto:sadrul@chromium.org> wrote:
>> >
>> >
>
>
>
https://codereview.chromium.org/280483003/diff/220001/ui/wm/core/nested_accel...
>>
>> > File ui/wm/core/nested_accelerator_controller.cc (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/280483003/diff/220001/ui/wm/core/nested_accel...
>>
>> > ui/wm/core/nested_accelerator_controller.cc:37:
>> > run_loop->QuitClosure());
>> > On 2014/05/28 23:11:12, sky wrote:
>> >>
>> >> This is more of what I was thinking, but having to express an API in
>> >
>> > terms of
>> >>
>> >> closures feels awkward. Is there a reason you don't want real Run/Quit
>> >
>> > methods
>> >>
>> >> DispatcherClient that DispatcherRunLoop calls?
>> >
>> >
>> > I think that makes it possible for the wrong dispatcher RunLoop to be
>> > terminated if Quit() is called for the same RunLoop multiple times. It
>> > also makes it difficult to get to the QuitClosure before already calling
>> > Run(). The alternative is to not have QuitClosure, and pass around a
>> > pointer to the dispatcher RunLoop instead. But the closure thing feels
>> > better I think.
>
>
>> If Quit only makes sense once could the DispatcherRunLoop track
>> whether it has invoked Quit?
>
>
> That would work I think, yes. The issue of not having a QuitClosure() still
> remains though. Do you think the alternative to not having that is
> acceptable?
> (I don't have strong preference either way)
>
> https://codereview.chromium.org/280483003/

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

[sadrul, 2014-05-29 20:03:50]

On 2014/05/29 19:35:11, sky wrote:
> One more proposal. Make DispatcherRunLoop pure virtual with
> DispatcherClient vending the appropriate instance. This way the
> DispatcherClient can make the DispatcherRunLoop contain the real
> state. Any better?

I considered that at one point (https://codereview.chromium.org/280483003/#msg5)
It's doable; the only downside is that we would need each platform (i.e. ash,
desktop) to provide implementations for both the DispatcherRunLoop and the
DispatcherClient. It's not necessarily bad; but doesn't feel much cleaner
either.

> 
> On Thu, May 29, 2014 at 12:01 PM,  <mailto:sadrul@chromium.org> wrote:
> > On 2014/05/29 14:50:39, sky wrote:
> >
> >> On Wed, May 28, 2014 at 5:25 PM,  <mailto:sadrul@chromium.org> wrote:
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/280483003/diff/220001/ui/wm/core/nested_accel...
> >>
> >> > File ui/wm/core/nested_accelerator_controller.cc (right):
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/280483003/diff/220001/ui/wm/core/nested_accel...
> >>
> >> > ui/wm/core/nested_accelerator_controller.cc:37:
> >> > run_loop->QuitClosure());
> >> > On 2014/05/28 23:11:12, sky wrote:
> >> >>
> >> >> This is more of what I was thinking, but having to express an API in
> >> >
> >> > terms of
> >> >>
> >> >> closures feels awkward. Is there a reason you don't want real Run/Quit
> >> >
> >> > methods
> >> >>
> >> >> DispatcherClient that DispatcherRunLoop calls?
> >> >
> >> >
> >> > I think that makes it possible for the wrong dispatcher RunLoop to be
> >> > terminated if Quit() is called for the same RunLoop multiple times. It
> >> > also makes it difficult to get to the QuitClosure before already calling
> >> > Run(). The alternative is to not have QuitClosure, and pass around a
> >> > pointer to the dispatcher RunLoop instead. But the closure thing feels
> >> > better I think.
> >
> >
> >> If Quit only makes sense once could the DispatcherRunLoop track
> >> whether it has invoked Quit?
> >
> >
> > That would work I think, yes. The issue of not having a QuitClosure() still
> > remains though. Do you think the alternative to not having that is
> > acceptable?
> > (I don't have strong preference either way)
> >
> > https://codereview.chromium.org/280483003/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-05-29 20:51:42]

Fair enough, what you have it is. LGTM

[sadrul, 2014-05-29 22:49:04]

Committed patchset #11 manually as r273634 (presubmit successful).