Replace PluginThreadTaskRunner with a simpler MainPluginThreadTaskRunner.

Replace PluginThreadTaskRunner with a simpler MainPluginThreadTaskRunner.

PluginThreadTaskRunner was designed to work with NPAPI and PPAPI, but
it's no longer necessary. Replacing it with MainPluginThreadTaskRunner
that's Pepper-specific and is much simpler.

BUG=544275
Committed: https://crrev.com/815bc8f7727e03d4e66d19462bfca9d943be1198
Cr-Commit-Position: refs/heads/master@{#355200}

[Sergey Ulanov, 2015-10-16 22:10:57]

sergeyu@chromium.org changed reviewers:
+ dcaiafa@chromium.org

[Sergey Ulanov, 2015-10-20 14:51:58]

ping

[Sergey Ulanov, 2015-10-20 22:57:24]

sergeyu@chromium.org changed reviewers:
+ wez@chromium.org

[Sergey Ulanov, 2015-10-20 22:57:24]

+wez, can you please take a look?

[Wez, 2015-10-20 23:05:48]

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_instance.h (right):

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.h:246:
scoped_refptr<MainPluginThreadTaskRunner> plugin_task_runner_;
Does the ChromotingInstance need to know that this is a
MainPluginThreadTaskRunner, or could it hold it as a SingleThreadTaskRunner or
even TaskRunner, to avoid spreading that implementation detail?

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/main...
File remoting/client/plugin/main_plugin_thread_task_runner.cc (right):

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/main...
remoting/client/plugin/main_plugin_thread_task_runner.cc:20:
core_->CallOnMainThread(delay.InMilliseconds(),
Any risk of e.g. spinning due to this fine-grained to millisecond-grained
conversion?

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/main...
File remoting/client/plugin/main_plugin_thread_task_runner.h (right):

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/main...
remoting/client/plugin/main_plugin_thread_task_runner.h:22: class
MainPluginThreadTaskRunner : public base::SingleThreadTaskRunner {
nit: I'd suggest calling this PepperMainThreadTaskRunner, since it's
PPAPI-specific, and that would fit the naming of the other PPAPI-specific code.

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/main...
remoting/client/plugin/main_plugin_thread_task_runner.h:39: void RunTask(int32_t
result, const base::Closure& closure);
nit: Not at all obvious from the name & parameters what this actually does!

[Sergey Ulanov, 2015-10-20 23:28:05]

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_instance.h (right):

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.h:246:
scoped_refptr<MainPluginThreadTaskRunner> plugin_task_runner_;
On 2015/10/20 23:05:48, Wez wrote:
> Does the ChromotingInstance need to know that this is a
> MainPluginThreadTaskRunner, or could it hold it as a SingleThreadTaskRunner or
> even TaskRunner, to avoid spreading that implementation detail?

Done.

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/main...
File remoting/client/plugin/main_plugin_thread_task_runner.cc (right):

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/main...
remoting/client/plugin/main_plugin_thread_task_runner.cc:20:
core_->CallOnMainThread(delay.InMilliseconds(),
On 2015/10/20 23:05:48, Wez wrote:
> Any risk of e.g. spinning due to this fine-grained to millisecond-grained
> conversion?

Here CallOnMainThread() is guaranteed to be called once and only once per
PostDelayedTask() call, so there is no possibility of spinning in this class
(like it was before, see the linked bug). But the calling code may still repost
the task if it's called too early. Now I replaced InMilliseconds() with
InMillisecondsRoundedUp() to avoid that problem.

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/main...
File remoting/client/plugin/main_plugin_thread_task_runner.h (right):

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/main...
remoting/client/plugin/main_plugin_thread_task_runner.h:22: class
MainPluginThreadTaskRunner : public base::SingleThreadTaskRunner {
On 2015/10/20 23:05:48, Wez wrote:
> nit: I'd suggest calling this PepperMainThreadTaskRunner, since it's
> PPAPI-specific, and that would fit the naming of the other PPAPI-specific
code.

Done.

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/main...
remoting/client/plugin/main_plugin_thread_task_runner.h:39: void RunTask(int32_t
result, const base::Closure& closure);
On 2015/10/20 23:05:48, Wez wrote:
> nit: Not at all obvious from the name & parameters what this actually does!

Added comment and renamed second parameter.

[Wez, 2015-10-20 23:43:52]

lgtm

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/main...
File remoting/client/plugin/main_plugin_thread_task_runner.cc (right):

https://codereview.chromium.org/1410923004/diff/1/remoting/client/plugin/main...
remoting/client/plugin/main_plugin_thread_task_runner.cc:20:
core_->CallOnMainThread(delay.InMilliseconds(),
On 2015/10/20 at 23:28:05, Sergey Ulanov wrote:
> On 2015/10/20 23:05:48, Wez wrote:
> > Any risk of e.g. spinning due to this fine-grained to millisecond-grained
> > conversion?
> 
> Here CallOnMainThread() is guaranteed to be called once and only once per
PostDelayedTask() call, so there is no possibility of spinning in this class
(like it was before, see the linked bug). But the calling code may still repost
the task if it's called too early. Now I replaced InMilliseconds() with
InMillisecondsRoundedUp() to avoid that problem.

Right - spinning callers are as bad as spinning in the task-runner itself!

https://codereview.chromium.org/1410923004/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/pepper_main_thread_task_runner.cc (right):

https://codereview.chromium.org/1410923004/diff/20001/remoting/client/plugin/...
remoting/client/plugin/pepper_main_thread_task_runner.cc:14:
PepperMainThreadTaskRunner::~PepperMainThreadTaskRunner() {}
nit: Move this to match the ordering in the header.

https://codereview.chromium.org/1410923004/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/pepper_main_thread_task_runner.h (right):

https://codereview.chromium.org/1410923004/diff/20001/remoting/client/plugin/...
remoting/client/plugin/pepper_main_thread_task_runner.h:39: // Helpers that runs
the specified |task|. The |result| argument is ignored,
Helpers->Helper.

Suggest "Helper that allows a base::Closure to be used as a
pp::CompletionCallback, by ignoring the completion result."

[Sergey Ulanov, 2015-10-20 23:49:16]

https://codereview.chromium.org/1410923004/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/pepper_main_thread_task_runner.cc (right):

https://codereview.chromium.org/1410923004/diff/20001/remoting/client/plugin/...
remoting/client/plugin/pepper_main_thread_task_runner.cc:14:
PepperMainThreadTaskRunner::~PepperMainThreadTaskRunner() {}
On 2015/10/20 23:43:52, Wez wrote:
> nit: Move this to match the ordering in the header.

Done.

https://codereview.chromium.org/1410923004/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/pepper_main_thread_task_runner.h (right):

https://codereview.chromium.org/1410923004/diff/20001/remoting/client/plugin/...
remoting/client/plugin/pepper_main_thread_task_runner.h:39: // Helpers that runs
the specified |task|. The |result| argument is ignored,
On 2015/10/20 23:43:52, Wez wrote:
> Helpers->Helper.
> 
> Suggest "Helper that allows a base::Closure to be used as a
> pp::CompletionCallback, by ignoring the completion result."

Done.

[Sergey Ulanov, 2015-10-20 23:49:46]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-10-20 23:49:47]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1410923004/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-10-20 23:49:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1410923004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1410923004/40001

[commit-bot: I haz the power, 2015-10-21 00:22:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-21 00:22:48]

Patchset 3 (id:??) landed as
https://crrev.com/815bc8f7727e03d4e66d19462bfca9d943be1198
Cr-Commit-Position: refs/heads/master@{#355200}