[webkit-reviews] review granted: [Bug 31639] Add asserts to RefCounted to make sure ref/deref happens on the right thread. : [Attachment 44423] Updated per David's comments.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dmitry Titov <dimich at chromium.org>'s
request for review:
Bug 31639: Add asserts to RefCounted to make sure ref/deref happens on the
right thread.
https://bugs.webkit.org/show_bug.cgi?id=31639

Attachment 44423: Updated per David's comments.
https://bugs.webkit.org/attachment.cgi?id=44423&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
The only thing I don't like about this is the name of
disableThreadVerification. I would prefer a name that describes the threading
regime being used rather than a simple "disable" call. But since the number of
callers is extremely small at this time, it seems we can change this later.

I'd eventually like to see the cross-thread string use be turned on, not in the
allocation of the StringImpl, but at the call sites. I know the class is used
cross thread "everywhere", but I think eventually we could do that. There are
tons of strings where it would be good to have the main thread assertion work.

> +	   // Start thread verification as soon as the ref count gets to 2.
> +	   // The class gets created with a ref count of 1 and then passed
> +	   // to another thread where to ref count get increased.  This
> +	   // is a heuristic but it seems to always work and has helped
> +	   // find some bugs.

We use one space after a period, nottwo.

> +    ThreadVerifier()
> +	   : m_isActive(false)
> +	   , m_isThreadVerificationDisabled(false)
> +	   , m_isOwnedByMainThread(false)
> +	   , m_owningThread(0)
> +    { }

We'd normally put the braces on separate lines.

> @@ -44,7 +44,7 @@ IconRecord::IconRecord(const String& url)
>      , m_stamp(0)
>      , m_dataSet(false)
>  {
> -
> +    disableThreadVerification();
>  }

I'd like to see a comment at all the disableThreadVerification call sites about
why this needs to be done.