Refactor VideoStub interface to accept ownership of video packets.

Refactor VideoStub interface to accept ownership of video packets.

Previously VideoStub::ProcessVideoPacket() did not accept ownership of
video packets. Instead the done callback was responsible for deleting
the packet.


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

[Sergey Ulanov, 2012-03-24 00:13:04]

Alpha: ping

[Alpha Left Google, 2012-03-26 20:29:18]

http://codereview.chromium.org/9827006/diff/1/remoting/client/chromoting_clie...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/9827006/diff/1/remoting/client/chromoting_clie...
remoting/client/chromoting_client.cc:23: : packet(packet.release()), done(done)
{
This line seems odd to me, can we use different variable names for parameter and
member? Also is .release() necessary here? It seems complicated to me since we
first call .Pass() then .release(), is there a simpler way?

http://codereview.chromium.org/9827006/diff/1/remoting/protocol/message_reade...
File remoting/protocol/message_reader.cc (right):

http://codereview.chromium.org/9827006/diff/1/remoting/protocol/message_reade...
remoting/protocol/message_reader.cc:84: std::vector<CompoundBuffer*>
new_messages;
The ownership of CompoundBuffer is unclear here. I think your intention is to
let OnNewData() to delete the message by scoped_ptr.

Since we are transferring ownership of CompoundBuffer to
|message_received_callback_| anyway I would prefer to use
vector<scoped_ptr<CompoundBuffer> > and then .Pass below to be explicit about
the intention.

http://codereview.chromium.org/9827006/diff/1/remoting/protocol/message_reade...
File remoting/protocol/message_reader_unittest.cc (right):

http://codereview.chromium.org/9827006/diff/1/remoting/protocol/message_reade...
remoting/protocol/message_reader_unittest.cc:101: std::vector<CompoundBuffer*>
messages_;
use vector<scoped_ptr<CompoundBuffer> > also.

[Sergey Ulanov, 2012-03-26 20:42:13]

http://codereview.chromium.org/9827006/diff/1/remoting/client/chromoting_clie...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/9827006/diff/1/remoting/client/chromoting_clie...
remoting/client/chromoting_client.cc:23: : packet(packet.release()), done(done)
{
On 2012/03/26 20:29:18, Alpha wrote:
> This line seems odd to me, can we use different variable names for parameter
and
> member? Also is .release() necessary here? It seems complicated to me since we
> first call .Pass() then .release(), is there a simpler way?

done(done) was here before. 

We need to release() here instead of Pass() because this struct stores pointer
instead of scoped_ptr<>. Can't use scoped_ptr<> because the struct needs to be
copyable so that it can be stored in STL containers.

http://codereview.chromium.org/9827006/diff/1/remoting/protocol/message_reade...
File remoting/protocol/message_reader.cc (right):

http://codereview.chromium.org/9827006/diff/1/remoting/protocol/message_reade...
remoting/protocol/message_reader.cc:84: std::vector<CompoundBuffer*>
new_messages;
On 2012/03/26 20:29:18, Alpha wrote:
> The ownership of CompoundBuffer is unclear here. I think your intention is to
> let OnNewData() to delete the message by scoped_ptr.

Yes this code gets ownership of these messages and then passes to
message_received_callback_. 
> 
> Since we are transferring ownership of CompoundBuffer to
> |message_received_callback_| anyway I would prefer to use
> vector<scoped_ptr<CompoundBuffer> > and then .Pass below to be explicit about
> the intention.

Can't use scoped_ptr<> here because it's not compatible with STL containers
(it's not copyable) :-(.

http://codereview.chromium.org/9827006/diff/1/remoting/protocol/message_reade...
File remoting/protocol/message_reader_unittest.cc (right):

http://codereview.chromium.org/9827006/diff/1/remoting/protocol/message_reade...
remoting/protocol/message_reader_unittest.cc:101: std::vector<CompoundBuffer*>
messages_;
On 2012/03/26 20:29:18, Alpha wrote:
> use vector<scoped_ptr<CompoundBuffer> > also.

Same problem here. vector<scoped_ptr<CompoundBuffer> > would not compile.

[Alpha Left Google, 2012-03-27 18:02:31]

lgtm

[commit-bot: I haz the power, 2012-03-27 22:47:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9827006/14001

[commit-bot: I haz the power, 2012-03-27 23:35:55]

Try job failure for 9827006-14001 (retry) on win for step "update" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-03-27 23:36:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9827006/21002

[commit-bot: I haz the power, 2012-03-27 23:52:21]

Try job failure for 9827006-21002 on win for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-03-28 00:58:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9827006/21002

[commit-bot: I haz the power, 2012-03-28 03:57:59]

Change committed as 129355