MessageLoopProxy cleanups in remoting client.

MessageLoopProxy cleanups in remoting client.

Replaced all usage of MessageLoopProxy with SingleThreadTaskRunner.
Also includes some other minor cleanups, e.g. removed 
ClientContext::network_message_loop().


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141589

[Wez, 2012-05-26 00:18:40]

Mostly looks good; a few nits & things to clarify, though.

http://codereview.chromium.org/10454018/diff/1/remoting/base/scoped_thread_pr...
File remoting/base/scoped_thread_proxy.h (right):

http://codereview.chromium.org/10454018/diff/1/remoting/base/scoped_thread_pr...
remoting/base/scoped_thread_proxy.h:22: // ScopedThreadProxy is a task runner
proxy that cancels all pending
nit: We should consider renaming this ScopedTaskRunner.  However, I've got a CL
laying around to remove this class entirely, so probably not worth making the
change now.

http://codereview.chromium.org/10454018/diff/1/remoting/base/scoped_thread_pr...
remoting/base/scoped_thread_proxy.h:41: //         :
thread_proxy_(base::ThreadTaskRunnerHandle::Get()) {}
Where are you getting ThreadTaskRunnerHandle from? I don't see an include for
it?

http://codereview.chromium.org/10454018/diff/1/remoting/client/chromoting_cli...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/10454018/diff/1/remoting/client/chromoting_cli...
remoting/client/chromoting_client.cc:71: if
(!task_runner()->BelongsToCurrentThread()) {
Which other threads does the client get Stop()ped from?

http://codereview.chromium.org/10454018/diff/1/remoting/client/chromoting_cli...
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/10454018/diff/1/remoting/client/chromoting_cli...
remoting/client/chromoting_client.h:49: void Stop(const base::Closure&
shutdown_task);
Add a comment clarifying the threading properties of Start(), Stop(), etc.

http://codereview.chromium.org/10454018/diff/1/remoting/client/frame_consumer...
File remoting/client/frame_consumer_proxy.cc (right):

http://codereview.chromium.org/10454018/diff/1/remoting/client/frame_consumer...
remoting/client/frame_consumer_proxy.cc:64:
DCHECK(frame_consumer_task_runner_->BelongsToCurrentThread());
This DCHECK() looks wrong. FrameConsumerProxy is ref-counted precisely because
it posts tasks to itself on a different thread, so the tear-down thread can't be
guaranteed.

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/chromot...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_scriptable_object.h:123: ChromotingInstance*
instance,
nit: Add a comment explaining that the instance must out-live the scriptable
object?

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_scriptable_object.h:124:
base::SingleThreadTaskRunner* plugin_task_runner);
nit: scoped_refptr<>?

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_scriptable_object.h:124:
base::SingleThreadTaskRunner* plugin_task_runner);
nit: Clarify that we only need to pass the plugin TaskRunner here because each
plugin has its own TaskRunner, while plugins may share a main thread, so getting
the thread-current TaskRunner won't work?

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_scriptable_object.h:124:
base::SingleThreadTaskRunner* plugin_task_runner);
ChromotingScriptableObject doesn't actually need a SingleThreadTaskRunner, just
a TaskRunner.

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_xmpp_proxy.h (right):

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_xmpp_proxy.h:26: base::SingleThreadTaskRunner*
callback_task_runner);
nit: scoped_refptr<>?

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_xmpp_proxy.h:26: base::SingleThreadTaskRunner*
callback_task_runner);
nit: Clarify the roles of the two TaskRunners.

http://codereview.chromium.org/10454018/diff/1/remoting/host/host_user_interf...
File remoting/host/host_user_interface.h (right):

http://codereview.chromium.org/10454018/diff/1/remoting/host/host_user_interf...
remoting/host/host_user_interface.h:53: base::MessageLoopProxy*
ui_message_loop() const;
Any reason not to convert these as well, and avoid the need for the
forward-define above?  MessageLoopProxy *is* a SingleThreadTaskRunner, after
all.

[Sergey Ulanov, 2012-05-30 01:07:03]

http://codereview.chromium.org/10454018/diff/1/remoting/base/scoped_thread_pr...
File remoting/base/scoped_thread_proxy.h (right):

http://codereview.chromium.org/10454018/diff/1/remoting/base/scoped_thread_pr...
remoting/base/scoped_thread_proxy.h:22: // ScopedThreadProxy is a task runner
proxy that cancels all pending
On 2012/05/26 00:18:40, Wez wrote:
> nit: We should consider renaming this ScopedTaskRunner.  However, I've got a
CL
> laying around to remove this class entirely, so probably not worth making the
> change now.

Ok, I will hold this CL until you land yours.

http://codereview.chromium.org/10454018/diff/1/remoting/base/scoped_thread_pr...
remoting/base/scoped_thread_proxy.h:41: //         :
thread_proxy_(base::ThreadTaskRunnerHandle::Get()) {}
On 2012/05/26 00:18:40, Wez wrote:
> Where are you getting ThreadTaskRunnerHandle from? I don't see an include for
> it?

This is a comment, so I don't need an include for it :)

http://codereview.chromium.org/10454018/diff/1/remoting/client/chromoting_cli...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/10454018/diff/1/remoting/client/chromoting_cli...
remoting/client/chromoting_client.cc:71: if
(!task_runner()->BelongsToCurrentThread()) {
On 2012/05/26 00:18:40, Wez wrote:
> Which other threads does the client get Stop()ped from?

This is no longer necessary. Replaced with a DCHECK.

http://codereview.chromium.org/10454018/diff/1/remoting/client/chromoting_cli...
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/10454018/diff/1/remoting/client/chromoting_cli...
remoting/client/chromoting_client.h:49: void Stop(const base::Closure&
shutdown_task);
On 2012/05/26 00:18:40, Wez wrote:
> Add a comment clarifying the threading properties of Start(), Stop(), etc.

Done.

http://codereview.chromium.org/10454018/diff/1/remoting/client/frame_consumer...
File remoting/client/frame_consumer_proxy.cc (right):

http://codereview.chromium.org/10454018/diff/1/remoting/client/frame_consumer...
remoting/client/frame_consumer_proxy.cc:64:
DCHECK(frame_consumer_task_runner_->BelongsToCurrentThread());
On 2012/05/26 00:18:40, Wez wrote:
> This DCHECK() looks wrong. FrameConsumerProxy is ref-counted precisely because
> it posts tasks to itself on a different thread, so the tear-down thread can't
be
> guaranteed.

Agree. Removed it.

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/chromot...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_scriptable_object.h:123: ChromotingInstance*
instance,
On 2012/05/26 00:18:40, Wez wrote:
> nit: Add a comment explaining that the instance must out-live the scriptable
> object?

Done.

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_scriptable_object.h:124:
base::SingleThreadTaskRunner* plugin_task_runner);
On 2012/05/26 00:18:40, Wez wrote:
> ChromotingScriptableObject doesn't actually need a SingleThreadTaskRunner,
just
> a TaskRunner.

It assumes that the task runner runs all tasks on the plugin thread, so it's
still better to use SingleThreadTaskRunner here.

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_scriptable_object.h:124:
base::SingleThreadTaskRunner* plugin_task_runner);
On 2012/05/26 00:18:40, Wez wrote:
> nit: scoped_refptr<>?

It's really necessary in case, though still might be good idea for readablity.
In any case, I think it's better to fix it in a separate CL - it's out of scope
for this one.

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_scriptable_object.h:124:
base::SingleThreadTaskRunner* plugin_task_runner);
On 2012/05/26 00:18:40, Wez wrote:
> nit: Clarify that we only need to pass the plugin TaskRunner here because each
> plugin has its own TaskRunner, while plugins may share a main thread, so
getting
> the thread-current TaskRunner won't work?

Potentially we can set ThreadTaskRunnerHandle for ppapi plugin. We just don't do
it currently. In any case this code is going to go away soon, so no need to fix
it.

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/pepper_...
File remoting/client/plugin/pepper_xmpp_proxy.h (right):

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_xmpp_proxy.h:26: base::SingleThreadTaskRunner*
callback_task_runner);
On 2012/05/26 00:18:40, Wez wrote:
> nit: scoped_refptr<>?

Will fix in a separate CL.

http://codereview.chromium.org/10454018/diff/1/remoting/client/plugin/pepper_...
remoting/client/plugin/pepper_xmpp_proxy.h:26: base::SingleThreadTaskRunner*
callback_task_runner);
On 2012/05/26 00:18:40, Wez wrote:
> nit: Clarify the roles of the two TaskRunners.

Done.

http://codereview.chromium.org/10454018/diff/1/remoting/host/host_user_interf...
File remoting/host/host_user_interface.h (right):

http://codereview.chromium.org/10454018/diff/1/remoting/host/host_user_interf...
remoting/host/host_user_interface.h:53: base::MessageLoopProxy*
ui_message_loop() const;
On 2012/05/26 00:18:40, Wez wrote:
> Any reason not to convert these as well, and avoid the need for the
> forward-define above?  MessageLoopProxy *is* a SingleThreadTaskRunner, after
> all.

This CL is for remoting/client. /host will be fixed in a separate CL.

[Wez, 2012-05-30 20:56:45]

http://codereview.chromium.org/10454018/diff/1/remoting/base/scoped_thread_pr...
File remoting/base/scoped_thread_proxy.h (right):

http://codereview.chromium.org/10454018/diff/1/remoting/base/scoped_thread_pr...
remoting/base/scoped_thread_proxy.h:22: // ScopedThreadProxy is a task runner
proxy that cancels all pending
On 2012/05/30 01:07:07, sergeyu wrote:
> On 2012/05/26 00:18:40, Wez wrote:
> > nit: We should consider renaming this ScopedTaskRunner.  However, I've got a
> CL
> > laying around to remove this class entirely, so probably not worth making
the
> > change now.
> 
> Ok, I will hold this CL until you land yours.

CL 10454018 has landed; let me know when you're ready for me to review this
again!

[Sergey Ulanov, 2012-06-01 18:35:14]

On 2012/05/30 20:56:45, Wez wrote:
>
http://codereview.chromium.org/10454018/diff/1/remoting/base/scoped_thread_pr...
> File remoting/base/scoped_thread_proxy.h (right):
> 
>
http://codereview.chromium.org/10454018/diff/1/remoting/base/scoped_thread_pr...
> remoting/base/scoped_thread_proxy.h:22: // ScopedThreadProxy is a task runner
> proxy that cancels all pending
> On 2012/05/30 01:07:07, sergeyu wrote:
> > On 2012/05/26 00:18:40, Wez wrote:
> > > nit: We should consider renaming this ScopedTaskRunner.  However, I've got
a
> > CL
> > > laying around to remove this class entirely, so probably not worth making
> the
> > > change now.
> > 
> > Ok, I will hold this CL until you land yours.
> 
> CL 10454018 has landed; let me know when you're ready for me to review this
> again!

This CL is ready now.

[Sergey Ulanov, 2012-06-08 00:53:52]

ping.
I've merged with the change that removed ScopedThreadProxy.

[Wez, 2012-06-11 22:32:44]

LGTM w/ some nits & suggested follow-ups.

http://codereview.chromium.org/10454018/diff/18001/remoting/client/chromoting...
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/10454018/diff/18001/remoting/client/chromoting...
remoting/client/chromoting_client.h:82: base::SingleThreadTaskRunner*
task_runner();
nit: It would be simpler to just take a scoped_refptr<> to the TaskRunner rather
than reach it through ClientContext every time.

http://codereview.chromium.org/10454018/diff/18001/remoting/client/chromoting...
remoting/client/chromoting_client.h:82: base::SingleThreadTaskRunner*
task_runner();
nit: Where is SingleThreadTaskRunner defined in the #includes? I think you
probably still need a forward-define here to satisy IWYU?

http://codereview.chromium.org/10454018/diff/18001/remoting/client/client_con...
File remoting/client/client_context.h (right):

http://codereview.chromium.org/10454018/diff/18001/remoting/client/client_con...
remoting/client/client_context.h:22: ClientContext(base::SingleThreadTaskRunner*
main_task_runner);
nit: Clarify what this thread is used for, e.g. is it the network thread, the UI
thread, etc?

http://codereview.chromium.org/10454018/diff/18001/remoting/client/frame_cons...
File remoting/client/frame_consumer_proxy.h (right):

http://codereview.chromium.org/10454018/diff/18001/remoting/client/frame_cons...
remoting/client/frame_consumer_proxy.h:30:
scoped_refptr<base::SingleThreadTaskRunner> frame_consumer_task_runner);
nit: See comment below re re-naming.

http://codereview.chromium.org/10454018/diff/18001/remoting/client/frame_cons...
remoting/client/frame_consumer_proxy.h:49:
scoped_refptr<base::SingleThreadTaskRunner> frame_consumer_task_runner_;
nit: You could just call this |task_runner_|, really; that it's the
FrameConsumer's task runner is pretty obvious from comments & usage.

http://codereview.chromium.org/10454018/diff/18001/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_xmpp_proxy.cc (right):

http://codereview.chromium.org/10454018/diff/18001/remoting/client/plugin/pep...
remoting/client/plugin/pepper_xmpp_proxy.cc:31: void
PepperXmppProxy::DetachCallback() {
nit: Are you missing a thread-check here?

http://codereview.chromium.org/10454018/diff/18001/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_xmpp_proxy.h (right):

http://codereview.chromium.org/10454018/diff/18001/remoting/client/plugin/pep...
remoting/client/plugin/pepper_xmpp_proxy.h:27: // attacked with
AttachCallback().
nit: atacked -> registered

[Sergey Ulanov, 2012-06-11 23:35:56]

http://codereview.chromium.org/10454018/diff/18001/remoting/client/chromoting...
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/10454018/diff/18001/remoting/client/chromoting...
remoting/client/chromoting_client.h:82: base::SingleThreadTaskRunner*
task_runner();
On 2012/06/11 22:32:44, Wez wrote:
> nit: Where is SingleThreadTaskRunner defined in the #includes? I think you
> probably still need a forward-define here to satisy IWYU?

Done.

http://codereview.chromium.org/10454018/diff/18001/remoting/client/chromoting...
remoting/client/chromoting_client.h:82: base::SingleThreadTaskRunner*
task_runner();
On 2012/06/11 22:32:44, Wez wrote:
> nit: It would be simpler to just take a scoped_refptr<> to the TaskRunner
rather
> than reach it through ClientContext every time.

Done.

http://codereview.chromium.org/10454018/diff/18001/remoting/client/client_con...
File remoting/client/client_context.h (right):

http://codereview.chromium.org/10454018/diff/18001/remoting/client/client_con...
remoting/client/client_context.h:22: ClientContext(base::SingleThreadTaskRunner*
main_task_runner);
On 2012/06/11 22:32:44, Wez wrote:
> nit: Clarify what this thread is used for, e.g. is it the network thread, the
UI
> thread, etc?

Done.

http://codereview.chromium.org/10454018/diff/18001/remoting/client/frame_cons...
File remoting/client/frame_consumer_proxy.h (right):

http://codereview.chromium.org/10454018/diff/18001/remoting/client/frame_cons...
remoting/client/frame_consumer_proxy.h:30:
scoped_refptr<base::SingleThreadTaskRunner> frame_consumer_task_runner);
On 2012/06/11 22:32:44, Wez wrote:
> nit: See comment below re re-naming.

Done.

http://codereview.chromium.org/10454018/diff/18001/remoting/client/frame_cons...
remoting/client/frame_consumer_proxy.h:49:
scoped_refptr<base::SingleThreadTaskRunner> frame_consumer_task_runner_;
On 2012/06/11 22:32:44, Wez wrote:
> nit: You could just call this |task_runner_|, really; that it's the
> FrameConsumer's task runner is pretty obvious from comments & usage.

Done.

http://codereview.chromium.org/10454018/diff/18001/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_xmpp_proxy.cc (right):

http://codereview.chromium.org/10454018/diff/18001/remoting/client/plugin/pep...
remoting/client/plugin/pepper_xmpp_proxy.cc:31: void
PepperXmppProxy::DetachCallback() {
On 2012/06/11 22:32:44, Wez wrote:
> nit: Are you missing a thread-check here?

Done.

http://codereview.chromium.org/10454018/diff/18001/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_xmpp_proxy.h (right):

http://codereview.chromium.org/10454018/diff/18001/remoting/client/plugin/pep...
remoting/client/plugin/pepper_xmpp_proxy.h:27: // attacked with
AttachCallback().
On 2012/06/11 22:32:44, Wez wrote:
> nit: atacked -> registered

Done.

[commit-bot: I haz the power, 2012-06-11 23:36:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10454018/22002

[commit-bot: I haz the power, 2012-06-12 01:52:55]

Change committed as 141589