[webkit-reviews] review granted: [Bug 23175] String and UString should be able to share a UChar* buffer. : [Attachment 27266] Part 3: Add CrossThreadRefCounted.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Levin <levin at chromium.org>'s
request for review:
Bug 23175: String and UString should be able to share a UChar* buffer.
https://bugs.webkit.org/show_bug.cgi?id=23175

Attachment 27266: Part 3: Add CrossThreadRefCounted.
https://bugs.webkit.org/attachment.cgi?id=27266&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // ThreadSafeShared can have a siginificant perf impact when used in low
level classes

Typo here in the spelling of "significant".

> +	   // data is freed using *fastFree*.

I believe this comment is wrong. I think the data is deallocated with delete,
not fastFree.

> +	   // Used to make an instance that can be used on another thread.
> +	   PassRefPtr<CrossThreadRefCounted<T> > copy();

I think this naming is getting increasingly unclear. The functions that copy
objects to be used on other threads probably need a name that somehow talks
about threads rather then just the word "copy".

> +	   bool isShared() const { return !m_refCounter.hasOneRef() ||
dataAccessMustBeThreadSafe(); }

I don't understand the semantics of this function. When would you call it and
what would you do based on the result.

> +	   bool dataAccessMustBeThreadSafe() const { return
m_threadSafeRefCounter && !m_threadSafeRefCounter->hasOneRef(); }

Same question.

> +	   ~CrossThreadRefCounted()
> +	   {
> +	       if (m_data && !m_threadSafeRefCounter)
> +		   delete m_data;
> +	   }

The null check of m_data is redundant with what the compiler will already do
automatically, so I suggest omitting it.

> +#if USE(LOCKFREE_THREADSAFESHARED)
> +	   if (atomicDecrement(&m_refCount) <= 0)
> +#else
> +	   {
> +	       MutexLocker locker(m_mutex);
> +	       --m_refCount;
> +	   }
> +	   if (m_refCount <= 0)
> +#endif
> +	       return true;

I think this would read more clearly if the return statement was repeated
isnide the #if.

r=me