Make WeakPtr use SequenceChecker instead of ThreadChecker

Make WeakPtr use SequenceChecker instead of ThreadChecker

Add TODO to let WeakPtrFactory/SupportsWeakPtr be bound to a
SequencedTaskRunner and not just a single thread.

Add ThreadTaskRunnerHandle::GetIfExists().

BUG=165590

[akalin, 2012-12-13 01:48:00]

+darin for review and base/
+scherkus for webkit/media

[akalin, 2012-12-13 01:48:19]

On 2012/12/13 01:48:00, akalin wrote:
> +darin for review and base/
> +scherkus for webkit/media

This CL is based off https://codereview.chromium.org/11550031/

[scherkus (not reviewing), 2012-12-13 03:33:49]

lgtm
On Dec 12, 2012 5:48 PM, <akalin@chromium.org> wrote:

> Reviewers: darin, scherkus,
>
> Message:
> +darin for review and base/
> +scherkus for webkit/media
>
> Description:
> Make WeakPtr use SequenceChecker instead of ThreadChecker
>
> Add TODO to let WeakPtrFactory/SupportsWeakPtr be bound to a
> SequencedTaskRunner and not just a single thread.
>
> Add ThreadTaskRunnerHandle::**GetIfExists().
>
> BUG=165590
>
>
> Please review this at
https://codereview.chromium.**org/11564003/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M base/memory/weak_ptr.h
>   M base/memory/weak_ptr.cc
>   M base/memory/weak_ptr_unittest.**cc
>   M base/thread_task_runner_**handle.h
>   M base/thread_task_runner_**handle.cc
>   M webkit/media/webmediaplayer_**ms.h
>
>
> Index: base/memory/weak_ptr.cc
> diff --git a/base/memory/weak_ptr.cc b/base/memory/weak_ptr.cc
> index 9dec8fd6db9c29956673a79326efa4**b22af2f3ad..**
> e4e43b1d1b21e7ae5f6a0484d0835e**ae26d88c12 100644
> --- a/base/memory/weak_ptr.cc
> +++ b/base/memory/weak_ptr.cc
> @@ -3,25 +3,33 @@
>  // found in the LICENSE file.
>
>  #include "base/memory/weak_ptr.h"
> +#include "base/single_thread_task_**runner.h"
> +#include "base/thread_task_runner_**handle.h"
>
>  namespace base {
>  namespace internal {
>
> -WeakReference::Flag::Flag() : is_valid_(true) {
> +WeakReference::Flag::Flag()
> +    : sequence_checker_(**ThreadTaskRunnerHandle::**GetIfExists()),
> +      is_valid_(true) {
>  }
>
>  void WeakReference::Flag::**Invalidate() {
>    // The flag being invalidated with a single ref implies that there are
> no
>    // weak pointers in existence. Allow deletion on other thread in this
> case.
> -  DCHECK(thread_checker_.**CalledOnValidThread() || HasOneRef());
> +  DCHECK(sequence_checker_.**CalledInSequence() || HasOneRef());
>    is_valid_ = false;
>  }
>
>  bool WeakReference::Flag::IsValid() const {
> -  DCHECK(thread_checker_.**CalledOnValidThread());
> +  DCHECK(sequence_checker_.**CalledInSequence());
>    return is_valid_;
>  }
>
> +void WeakReference::Flag::**DetachFromThread() {
> +  sequence_checker_.**ChangeSequence(**ThreadTaskRunnerHandle::**
> GetIfExists());
> +}
> +
>  WeakReference::Flag::~Flag() {
>  }
>
> Index: base/memory/weak_ptr.h
> diff --git a/base/memory/weak_ptr.h b/base/memory/weak_ptr.h
> index 7006fb63e0981468e30ac2c631e8d6**c1ed645ef7..**
> 21951982428555f2e777b6205704bc**d10ac64f2c 100644
> --- a/base/memory/weak_ptr.h
> +++ b/base/memory/weak_ptr.h
> @@ -74,8 +74,8 @@
>  #include "base/base_export.h"
>  #include "base/logging.h"
>  #include "base/memory/ref_counted.h"
> +#include "base/sequence_checker.h"
>  #include "base/template_util.h"
> -#include "base/threading/thread_**checker.h"
>
>  namespace base {
>
> @@ -97,14 +97,14 @@ class BASE_EXPORT WeakReference {
>      void Invalidate();
>      bool IsValid() const;
>
> -    void DetachFromThread() { thread_checker_.**DetachFromThread(); }
> +    void DetachFromThread();
>
>     private:
>      friend class base::RefCountedThreadSafe<**Flag>;
>
>      ~Flag();
>
> -    ThreadChecker thread_checker_;
> +    SequenceChecker sequence_checker_;
>      bool is_valid_;
>    };
>
> @@ -245,6 +245,9 @@ class WeakPtr : public internal::WeakPtrBase {
>    T* ptr_;
>  };
>
> +// TODO(akalin): Make WeakPtrFactory and SupportsWeakPtr support
> +// binding to a SequencedTaskRunner instead of just a single thread.
> +
>  // A class may extend from SupportsWeakPtr to expose weak pointers to
> itself.
>  // This is useful in cases where you want others to be able to get a weak
>  // pointer to your class.  It also has the property that you don't need to
> Index: base/memory/weak_ptr_unittest.**cc
> diff --git a/base/memory/weak_ptr_**unittest.cc b/base/memory/weak_ptr_**
> unittest.cc
> index d5f8057cc5b88069b35d640dcc130f**e81eaa41e8..**
> 3fc5ba15f62aea9aa378f656a1ac29**a43208e9d3 100644
> --- a/base/memory/weak_ptr_**unittest.cc
> +++ b/base/memory/weak_ptr_**unittest.cc
> @@ -137,8 +137,6 @@ class BackgroundThread : public Thread {
>    }
>  };
>
> -}  // namespace
> -
>  TEST(WeakPtrFactoryTest, Basic) {
>    int data;
>    WeakPtrFactory<int> factory(&data);
> @@ -474,4 +472,6 @@ TEST(WeakPtrDeathTest, NonOwnerThreadDeletesObject) {
>
>  #endif
>
> +}  // namespace
> +
>  }  // namespace base
> Index: base/thread_task_runner_**handle.cc
> diff --git a/base/thread_task_runner_**handle.cc
> b/base/thread_task_runner_**handle.cc
> index d1b07aaaf012f9f63a7282de5fb758**b6a8503e7d..**
> 8c073880c76ea2e5b0a5f5fa37ebb1**83f71f4816 100644
> --- a/base/thread_task_runner_**handle.cc
> +++ b/base/thread_task_runner_**handle.cc
> @@ -4,6 +4,8 @@
>
>  #include "base/thread_task_runner_**handle.h"
>
> +#include <cstddef>
> +
>  #include "base/lazy_instance.h"
>  #include "base/single_thread_task_**runner.h"
>  #include "base/threading/thread_local.**h"
> @@ -18,10 +20,17 @@
base::LazyInstance<base::**ThreadLocalPointer<**ThreadTaskRunnerHandle>
> >
>  }  // namespace
>
>  // static
> -scoped_refptr<**SingleThreadTaskRunner> ThreadTaskRunnerHandle::Get() {
> +scoped_refptr<**SingleThreadTaskRunner>
ThreadTaskRunnerHandle::**GetIfExists()
> {
>    ThreadTaskRunnerHandle* current = lazy_tls_ptr.Pointer()->Get();
> -  DCHECK(current);
> -  return current->task_runner_;
> +  return current ? current->task_runner_ : NULL;
> +}
> +
> +// static
> +scoped_refptr<**SingleThreadTaskRunner> ThreadTaskRunnerHandle::Get() {
> +  const scoped_refptr<**SingleThreadTaskRunner>& thread_task_runner =
> +      GetIfExists();
> +  CHECK(thread_task_runner.get()**);
> +  return thread_task_runner;
>  }
>
>  ThreadTaskRunnerHandle::**ThreadTaskRunnerHandle(
> Index: base/thread_task_runner_**handle.h
> diff --git a/base/thread_task_runner_**handle.h b/base/thread_task_runner_
> **handle.h
> index 599d15bb52ffce3187aaeb575c2c80**5ca9aa6f5f..**
> 1608984892dbb6c22a0334740dce61**94dd674e79 100644
> --- a/base/thread_task_runner_**handle.h
> +++ b/base/thread_task_runner_**handle.h
> @@ -19,7 +19,12 @@ class SingleThreadTaskRunner;
>  // constructor.
>  class BASE_EXPORT ThreadTaskRunnerHandle {
>   public:
> -  // Gets the SingleThreadTaskRunner for the current thread.
> +  // Gets the SingleThreadTaskRunner for the current thread, or NULL
> +  // if one doesn't exist.
> +  static scoped_refptr<**SingleThreadTaskRunner> GetIfExists();
> +
> +  // CHECKs that there is a SingleThreadTaskRunner for the current
> +  // thread and returns it.
>    static scoped_refptr<**SingleThreadTaskRunner> Get();
>
>    ThreadTaskRunnerHandle(
> Index: webkit/media/webmediaplayer_**ms.h
> diff --git a/webkit/media/webmediaplayer_**ms.h
> b/webkit/media/webmediaplayer_**ms.h
> index 8d33581312aaf13bf9cc3e2ec4cb72**7b8cbf48e0..**
> 5d77b7f01a2872bc8b291c7a516b69**2a889f2209 100644
> --- a/webkit/media/webmediaplayer_**ms.h
> +++ b/webkit/media/webmediaplayer_**ms.h
> @@ -9,6 +9,7 @@
>  #include "base/memory/scoped_ptr.h"
>  #include "base/memory/weak_ptr.h"
>  #include "base/synchronization/lock.h"
> +#include "base/threading/thread_**checker.h"
>  #include "googleurl/src/gurl.h"
>  #include "media/filters/skcanvas_video_**renderer.h"
>  #include "skia/ext/platform_canvas.h"
>
>
>

[akalin, 2013-02-16 01:17:06]

Darin's probably busy, so -darin and +jar for base/.

[jar (doing other things), 2013-02-16 02:37:27]

https://codereview.chromium.org/11564003/diff/8001/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/11564003/diff/8001/base/memory/weak_ptr.cc#ne...
base/memory/weak_ptr.cc:20: DCHECK(sequence_checker_.CalledInSequence() ||
HasOneRef());
nit: Although the semantics may be as intended (to replace CalledOnValidThread),
this method name is very confusing.

Can you suggest a better name, or perhaps even (worst case) have another method
that CalledOnValidThread() or maybe CalledOnConsistentThread()?

https://codereview.chromium.org/11564003/diff/8001/base/memory/weak_ptr.cc#ne...
base/memory/weak_ptr.cc:30:
sequence_checker_.ChangeSequence(ThreadTaskRunnerHandle::GetIfExists());
nit: This "ChangeSequence" also seems confusing :-(.

Maybe it would help if at least the slot was called something like:
consistent_thread_checker_ 
or (though even this bothers me):
valide_thread_checker_

In truth, I  again feel like the method name does not help the reader understand
what this is doing, or why we're calling it :-(.

Suggestions?

https://codereview.chromium.org/11564003/diff/8001/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

https://codereview.chromium.org/11564003/diff/8001/base/memory/weak_ptr.h#new...
base/memory/weak_ptr.h:248: // TODO(akalin): Make WeakPtrFactory and
SupportsWeakPtr support
nit: Can you add a bug number.

https://codereview.chromium.org/11564003/diff/8001/base/thread_task_runner_ha...
File base/thread_task_runner_handle.cc (right):

https://codereview.chromium.org/11564003/diff/8001/base/thread_task_runner_ha...
base/thread_task_runner_handle.cc:33: return thread_task_runner;
nit: List methods after constructors and destructors.

https://codereview.chromium.org/11564003/diff/8001/base/thread_task_runner_ha...
File base/thread_task_runner_handle.h (right):

https://codereview.chromium.org/11564003/diff/8001/base/thread_task_runner_ha...
base/thread_task_runner_handle.h:28: static
scoped_refptr<SingleThreadTaskRunner> Get();
nit: List constructors and destructors before methods on lines 22-28 (even
though it was previously out of order).

[akalin, 2013-10-07 23:39:47]

closing inactive/obsolete CLs