Test using a global, replacable TaskQueueImpl factory.

Test using a global, replacable TaskQueueImpl factory.

BUG=none

[perkj_webrtc, 2017-06-14 15:48:02]

The CQ bit was checked by perkj@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 15:48:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[perkj_webrtc, 2017-06-14 15:51:54]

perkj@webrtc.org changed reviewers:
+ kjellander@webrtc.org, mbonadei@webrtc.org, tommi@webrtc.org

[perkj_webrtc, 2017-06-14 15:51:55]

I created a TQ PIMPL out of the linux implementation for discussion.

The implementation is added to all executables at the moment. But we probably
want to also add it to the peerconnection library as well and not all
executables if we want to do this.

[commit-bot: I haz the power, 2017-06-14 15:52:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 15:52:15]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/26469)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/25437)

[perkj_webrtc, 2017-08-18 10:07:48]

The CQ bit was checked by perkj@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-18 10:07:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[perkj_webrtc, 2017-08-18 10:17:53]

perkj@webrtc.org changed reviewers:
+ kwiberg@chromium.org, nisse@webrtc.org
- kjellander@webrtc.org, mbonadei@webrtc.org

[perkj_webrtc, 2017-08-18 10:17:54]

New discussions and new cls....

Latest approach still only touch the linux/android implementation at the moment
and I send this to continue the discussion.

TaskQueue API is unchanged.
On linux TaskQueue owns an impl. The impl is created from a global factory that
can be changed before first use.

Please take a look and see if you think this is acceptable.

[perkj_webrtc, 2017-08-18 10:28:19]

perkj@webrtc.org changed reviewers:
+ kwiberg@webrtc.org
- kwiberg@chromium.org

[commit-bot: I haz the power, 2017-08-18 10:40:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-18 10:40:31]

Dry run: Try jobs failed on following builders:
  linux_tsan2 on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/25046)

[nisse-webrtc, 2017-08-18 11:45:45]

I find it acceptable. Even though it would be preferable to

1. Use a single way to override override the implementation for internal
projects and for non-linux platforms.

2. Do it at link time. I'm probably naïve, but as far as I understand, it would
work to 

  a) put task_queue in its own gn target, 

  b) have a gn flag to say whether or not main webrtc 
     targets should depend on that target (if true,
     webrtc's task queue will be linked into any
     application linking with webrtc)

  c) unconditionally set that flag to always false for 
     internal projects, and

  d) let internal projects which actually want webrtc's 
     task queue implementation add a an explicit link
     dependency in their own build files.

https://codereview.webrtc.org/2936213003/diff/20001/webrtc/rtc_base/task_queu...
File webrtc/rtc_base/task_queue_impl_factory.cc (right):

https://codereview.webrtc.org/2936213003/diff/20001/webrtc/rtc_base/task_queu...
webrtc/rtc_base/task_queue_impl_factory.cc:24: // TODO(perkj): if OnceInit
exist... DOn't allow someone to call Set again.
If you can put a static variable in a single method, its initializer will be run
only once and (given recent enough C++), in a threadsafe manner.

Say, a private static method like

TaskQueueImplFactory*
Magic(TaskQueueFactory *init) {
  static TaskQueueFactory *the_factory =
    init ? init : new TaskQueueLibEventFactory();
  return the_factory;
}

Get() {
  return Magic(nullptr);
}

Set(TaskQueueImplFactory* init) {
  RTC_CHECK(init);
  RTC_CHECK_EQ(GetMagic(init), init);
}

This has the unintended effect of allowing multiple Set as long as they all pass
the same factory pointer, but I guess that's ok.

https://codereview.webrtc.org/2936213003/diff/20001/webrtc/rtc_base/task_queu...
File webrtc/rtc_base/task_queue_impl_factory.h (right):

https://codereview.webrtc.org/2936213003/diff/20001/webrtc/rtc_base/task_queu...
webrtc/rtc_base/task_queue_impl_factory.h:24: virtual
scoped_refptr<TaskQueueImpl> CreateImpl(
Naming: I'd prefer any of CreateTaskQueueImpl, CreateTQ or Create over
CreateImpl. "Impl" adds very little meaning per character.

[danilchap, 2017-08-18 11:57:54]

Description was changed from

==========
Test of how a TaskQueue PIMPL implementation could look like.

BUG=none
==========

to

==========
Test of how a TaskQueue PIMPL implementation could look like.

BUG=none
==========

[kwiberg-webrtc, 2017-08-21 09:21:55]

https://codereview.webrtc.org/2936213003/diff/20001/webrtc/rtc_base/task_queu...
File webrtc/rtc_base/task_queue_impl_factory.cc (right):

https://codereview.webrtc.org/2936213003/diff/20001/webrtc/rtc_base/task_queu...
webrtc/rtc_base/task_queue_impl_factory.cc:22: void
TaskQueueImplFactory::Set(TaskQueueImplFactory* factory) {
Is it really a good idea for Set() to not take ownership of the factory? It
would seem safer for Set() to take a unique_ptr, and then explicitly leak the
factory by storing it in the static raw pointer on line 16. (And document that
this is what Set() does!)

Another alternative would be to let the factory be a function pointer instead of
an interface. That way, there is no per-instance state, and storing it in a raw
pointer becomes unproblematic. It doesn't look like you really need any state...

https://codereview.webrtc.org/2936213003/diff/20001/webrtc/rtc_base/task_queu...
webrtc/rtc_base/task_queue_impl_factory.cc:26: return;
If the contract is that you're only allowed to call this once, you should
CHECK() that.

https://codereview.webrtc.org/2936213003/diff/20001/webrtc/rtc_base/task_queu...
webrtc/rtc_base/task_queue_impl_factory.cc:31: if (taskqueue_factory)
Don't read this variable without taking the lock.

https://codereview.webrtc.org/2936213003/diff/20001/webrtc/rtc_base/task_queu...
File webrtc/rtc_base/task_queue_impl_factory.h (right):

https://codereview.webrtc.org/2936213003/diff/20001/webrtc/rtc_base/task_queu...
webrtc/rtc_base/task_queue_impl_factory.h:38: class TaskQueueLibEventFactory :
public TaskQueueImplFactory {
Does this class definition need to be here, in a header? It's usually possible
to put interface implementations in a .cc file together with the definition of
the function that instantiates them.

https://codereview.webrtc.org/2936213003/diff/20001/webrtc/rtc_base/task_queu...
webrtc/rtc_base/task_queue_impl_factory.h:41: ~TaskQueueLibEventFactory()
override = default;
Do you need these two?

https://codereview.webrtc.org/2936213003/diff/20001/webrtc/rtc_base/task_queu...
webrtc/rtc_base/task_queue_impl_factory.h:44: TaskQueueLibEventFactory&
operator=(const TaskQueueLibEventFactory&) = delete;
I don't think you need these two. Copying or moving isn't useful, but not
harmful either.

[perkj_webrtc, 2017-09-15 11:51:13]

Description was changed from

==========
Test of how a TaskQueue PIMPL implementation could look like.

BUG=none
==========

to

==========
Test using a global, replacable TaskQueueImpl factory.

BUG=none
==========