Add JingleThreadWrapper.

Add JingleThreadWrapper.

JingleThreadWrapper wraps chromium thread with a talk_base::Thread interface.

BUG=None
TEST=Unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79576

[Alpha Left Google, 2011-03-26 01:06:01]

Looks good with comments about commenting in the code. operations for priority
queue appear to be correct, but I'd prefer using data structure like set that
already does everything for you..

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.cc
File jingle/glue/thread_wrapper.cc (right):

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.c...
jingle/glue/thread_wrapper.cc:72: for (std::list<talk_base::Message>::iterator
it = messages_.begin();
These two loops look really similar, please add comments.

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.c...
jingle/glue/thread_wrapper.cc:96:
delayed_messages_.erase(delayed_messages_.end() - 1);
why not just use delayed_messages_.back()?

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.c...
jingle/glue/thread_wrapper.cc:103: std::make_heap(delayed_messages_.begin(),
delayed_messages_.end());
where is the comparator for messages, add comment here.

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.c...
jingle/glue/thread_wrapper.cc:139:
delayed_messages_.erase(delayed_messages_.end() - 1);
why not use delayed_messages_.back()?

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.c...
jingle/glue/thread_wrapper.cc:150: NOTREACHED();
These methods are NOTREACHED(), why? Explain please.

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.h
File jingle/glue/thread_wrapper.h (right):

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.h...
jingle/glue/thread_wrapper.h:16: class JingleThreadWrapper
Add a short comment here to explain what this is doing.

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.h...
jingle/glue/thread_wrapper.h:56: friend class
base::RefCountedThreadSafe<JingleThreadWrapper>;
Because we need comments saying this has to be used with scoped_refptr<>.

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.h...
jingle/glue/thread_wrapper.h:75: DelayedMessagesHeap delayed_messages_;
I would suggest you use STL set so you don't need to manipulate the data
structure yourself, but using priority queue can give you constant time for
accessing front().  Also using set require you to write a custom comparator.

http://codereview.chromium.org/6747017/diff/3002/jingle/jingle.gyp
File jingle/jingle.gyp (right):

http://codereview.chromium.org/6747017/diff/3002/jingle/jingle.gyp#newcode12
jingle/jingle.gyp:12: 'target_name': 'jingle_glue',
avoid using the same target name, this has confused one of win/mac/linux before.

[Sergey Ulanov, 2011-03-26 01:44:24]

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.cc
File jingle/glue/thread_wrapper.cc (right):

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.c...
jingle/glue/thread_wrapper.cc:72: for (std::list<talk_base::Message>::iterator
it = messages_.begin();
On 2011/03/26 01:06:01, Alpha wrote:
> These two loops look really similar, please add comments.

Done.

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.c...
jingle/glue/thread_wrapper.cc:96:
delayed_messages_.erase(delayed_messages_.end() - 1);
On 2011/03/26 01:06:01, Alpha wrote:
> why not just use delayed_messages_.back()?
erase() needs iterator. back() returns reference to last element.

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.c...
jingle/glue/thread_wrapper.cc:103: std::make_heap(delayed_messages_.begin(),
delayed_messages_.end());
On 2011/03/26 01:06:01, Alpha wrote:
> where is the comparator for messages, add comment here.
It's defined in talk_base::DelayedMessage. Anyway added explicit comparator for
set.

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.c...
jingle/glue/thread_wrapper.cc:139:
delayed_messages_.erase(delayed_messages_.end() - 1);
On 2011/03/26 01:06:01, Alpha wrote:
> why not use delayed_messages_.back()?
back() returns reference to last element. erase() needs iterator.

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.c...
jingle/glue/thread_wrapper.cc:150: NOTREACHED();
On 2011/03/26 01:06:01, Alpha wrote:
> These methods are NOTREACHED(), why? Explain please.
This is mentioned in the header. Added more comments here.
In short: I'm implementing stuff that is used by p2p part of libjingle.

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.h
File jingle/glue/thread_wrapper.h (right):

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.h...
jingle/glue/thread_wrapper.h:16: class JingleThreadWrapper
On 2011/03/26 01:06:01, Alpha wrote:
> Add a short comment here to explain what this is doing.

Done.

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.h...
jingle/glue/thread_wrapper.h:56: friend class
base::RefCountedThreadSafe<JingleThreadWrapper>;
On 2011/03/26 01:06:01, Alpha wrote:
> Because we need comments saying this has to be used with scoped_refptr<>.

Don't needs this. removed.

http://codereview.chromium.org/6747017/diff/3002/jingle/glue/thread_wrapper.h...
jingle/glue/thread_wrapper.h:75: DelayedMessagesHeap delayed_messages_;
On 2011/03/26 01:06:01, Alpha wrote:
> I would suggest you use STL set so you don't need to manipulate the data
> structure yourself, but using priority queue can give you constant time for
> accessing front().  Also using set require you to write a custom comparator.
Changed this to set.

[Sergey Ulanov, 2011-03-26 01:45:51]

http://codereview.chromium.org/6747017/diff/3002/jingle/jingle.gyp
File jingle/jingle.gyp (right):

http://codereview.chromium.org/6747017/diff/3002/jingle/jingle.gyp#newcode12
jingle/jingle.gyp:12: 'target_name': 'jingle_glue',
On 2011/03/26 01:06:01, Alpha wrote:
> avoid using the same target name, this has confused one of win/mac/linux
before.
Do we have jingle_glue target anywhere else? There is chromoting_jingle_glue, so
I don' think this is a problem

[Alpha Left Google, 2011-03-28 16:24:08]

LGTM.