Moved video stub implementation to RectangleUpdateDecoder.

Moved video stub implementation to RectangleUpdateDecoder.


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

[kxing, 2012-08-23 19:55:12]

Could someone PTAL?

I'll rename ChromotingStats to ChromotingVideoStats in a separate CL.

[Wez, 2012-08-23 22:10:47]

http://codereview.chromium.org/10867039/diff/2001/remoting/client/chromoting_...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/10867039/diff/2001/remoting/client/chromoting_...
remoting/client/chromoting_client.cc:74: weak_ptr_, shutdown_task));
nit: Not related to this CL as such but does the caller stay alive until
|shutdown_task| runs? If so then we don't need to bounce via OnDisconnected()
and we can remove it and the weak_ptr/factory_.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/chromoting_...
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/10867039/diff/2001/remoting/client/chromoting_...
remoting/client/chromoting_client.h:60: ChromotingStats* GetStats();
nit: Rename to ChromotingVideoStats and GetVideoStats().

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
File remoting/client/rectangle_update_decoder.cc (right):

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:288: void
RectangleUpdateDecoder::DispatchPacket() {
nit: Rename to ProcessNextPacket?

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
File remoting/client/rectangle_update_decoder.h (right):

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:45: // Creates an update decoder on
|task_runner_|, outputting to |consumer|.
nit: Update this comment to clarify what the two TaskRunners are used for.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:58: void
DecodePacket(scoped_ptr<VideoPacket> packet, const base::Closure& done);
This doesn't need to be part of the public interface any more.  You could also
potentially fold it together with DispatchPacket.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:73: void DropAllPackets();
nit: Move this API up to near Initialize & DecodePacket and document what it
does.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:115: // The current dimentions of the
frame consumer view.
typo: dimentions -> dimensions

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:137: // Keep track of the last
sequence number bounced back from the host.
nit: last -> most recent

[Sergey Ulanov, 2012-08-23 22:22:08]

I think there are some opportunities to simplify this code. Particularly we no
longer need to keep list of pending packets. Though it might be better to fix it
in a separate CL.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
File remoting/client/rectangle_update_decoder.cc (right):

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:27: scoped_ptr<VideoPacket> packet,
const base::Closure& done)
nit: one argument per line please when arguments are wrapped.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:28: : packet(packet.release()),
done(done) {
nit: move done initializer to a separate line

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:69: if
(!decode_task_runner_->BelongsToCurrentThread()) {
Better to PostTask() in DispatchPacket() and here just DCHECK that you are on
the right thread.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:240: const base::Closure& done) {
nit: indentation

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:278: void
RectangleUpdateDecoder::DropAllPackets() {
I think we no longer need this method. (we needed it previously because the done
task was destroying the packets, but that's not the case anymore).

If you remove this method, then you no longer need received_packets_ list - you
can just queue pending packets in the task queue of the decoder thread. So
instead of this method you will just need StopDecoding() method that just sets a
flag and then DecodePacket() will do nothing once that flag is set.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:309:
base::Bind(&RectangleUpdateDecoder::OnPacketDone, base::Unretained(this),
Why do you need Unretained() here? This object is ref-counted isn't it. Not sure
if it's safe here

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:314: base::Time decode_start) {
nit: indentation

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
File remoting/client/rectangle_update_decoder.h (right):

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:58: void
DecodePacket(scoped_ptr<VideoPacket> packet, const base::Closure& done);
Should this be private now?

[kxing, 2012-08-24 17:05:22]

Could someone PTAL again?

http://codereview.chromium.org/10867039/diff/2001/remoting/client/chromoting_...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/10867039/diff/2001/remoting/client/chromoting_...
remoting/client/chromoting_client.cc:74: weak_ptr_, shutdown_task));
On 2012/08/23 22:10:48, Wez wrote:
> nit: Not related to this CL as such but does the caller stay alive until
> |shutdown_task| runs? If so then we don't need to bounce via OnDisconnected()
> and we can remove it and the weak_ptr/factory_.

Not sure. I'll let someone do that in a separate CL.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/chromoting_...
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/10867039/diff/2001/remoting/client/chromoting_...
remoting/client/chromoting_client.h:60: ChromotingStats* GetStats();
On 2012/08/23 22:10:48, Wez wrote:
> nit: Rename to ChromotingVideoStats and GetVideoStats().

I'll do that in a separate CL.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
File remoting/client/rectangle_update_decoder.cc (right):

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:27: scoped_ptr<VideoPacket> packet,
const base::Closure& done)
On 2012/08/23 22:22:09, sergeyu wrote:
> nit: one argument per line please when arguments are wrapped.

Done.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:28: : packet(packet.release()),
done(done) {
On 2012/08/23 22:22:09, sergeyu wrote:
> nit: move done initializer to a separate line

Done.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:69: if
(!decode_task_runner_->BelongsToCurrentThread()) {
On 2012/08/23 22:22:09, sergeyu wrote:
> Better to PostTask() in DispatchPacket() and here just DCHECK that you are on
> the right thread.

Done.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:240: const base::Closure& done) {
On 2012/08/23 22:22:09, sergeyu wrote:
> nit: indentation

Done.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:278: void
RectangleUpdateDecoder::DropAllPackets() {
On 2012/08/23 22:22:09, sergeyu wrote:
> I think we no longer need this method. (we needed it previously because the
done
> task was destroying the packets, but that's not the case anymore).
> 
> If you remove this method, then you no longer need received_packets_ list -
you
> can just queue pending packets in the task queue of the decoder thread. So
> instead of this method you will just need StopDecoding() method that just sets
a
> flag and then DecodePacket() will do nothing once that flag is set.

I'll let someone do this in a separate CL.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:288: void
RectangleUpdateDecoder::DispatchPacket() {
On 2012/08/23 22:10:48, Wez wrote:
> nit: Rename to ProcessNextPacket?

Done.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:309:
base::Bind(&RectangleUpdateDecoder::OnPacketDone, base::Unretained(this),
On 2012/08/23 22:22:09, sergeyu wrote:
> Why do you need Unretained() here? This object is ref-counted isn't it. Not
sure
> if it's safe here

Done.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.cc:314: base::Time decode_start) {
On 2012/08/23 22:22:09, sergeyu wrote:
> nit: indentation

Done.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
File remoting/client/rectangle_update_decoder.h (right):

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:45: // Creates an update decoder on
|task_runner_|, outputting to |consumer|.
On 2012/08/23 22:10:48, Wez wrote:
> nit: Update this comment to clarify what the two TaskRunners are used for.

Done.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:58: void
DecodePacket(scoped_ptr<VideoPacket> packet, const base::Closure& done);
On 2012/08/23 22:22:09, sergeyu wrote:
> Should this be private now?

Done.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:58: void
DecodePacket(scoped_ptr<VideoPacket> packet, const base::Closure& done);
On 2012/08/23 22:10:48, Wez wrote:
> This doesn't need to be part of the public interface any more.  You could also
> potentially fold it together with DispatchPacket.

Done.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:73: void DropAllPackets();
On 2012/08/23 22:10:48, Wez wrote:
> nit: Move this API up to near Initialize & DecodePacket and document what it
> does.

Done.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:115: // The current dimentions of the
frame consumer view.
On 2012/08/23 22:10:48, Wez wrote:
> typo: dimentions -> dimensions

Done.

http://codereview.chromium.org/10867039/diff/2001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:137: // Keep track of the last
sequence number bounced back from the host.
On 2012/08/23 22:10:48, Wez wrote:
> nit: last -> most recent

Done.

[Sergey Ulanov, 2012-08-24 18:44:12]

lgtm

http://codereview.chromium.org/10867039/diff/7001/remoting/client/rectangle_u...
File remoting/client/rectangle_update_decoder.h (right):

http://codereview.chromium.org/10867039/diff/7001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:10: #include "base/callback.h"
Don't need this include - you already have callback_forward.h included here.
Move it to .cc file.

[Sergey Ulanov, 2012-08-24 18:45:40]

http://codereview.chromium.org/10867039/diff/7001/remoting/client/rectangle_u...
File remoting/client/rectangle_update_decoder.h (right):

http://codereview.chromium.org/10867039/diff/7001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:130: // processed.
Please add TODO(sergeyu) to simplify this code and remove this list.

[kxing, 2012-08-24 19:35:50]

I'll commit this soon, unless someone objects.

http://codereview.chromium.org/10867039/diff/7001/remoting/client/rectangle_u...
File remoting/client/rectangle_update_decoder.h (right):

http://codereview.chromium.org/10867039/diff/7001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:10: #include "base/callback.h"
On 2012/08/24 18:44:12, sergeyu wrote:
> Don't need this include - you already have callback_forward.h included here.
> Move it to .cc file.

Done.

http://codereview.chromium.org/10867039/diff/7001/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:130: // processed.
On 2012/08/24 18:45:40, sergeyu wrote:
> Please add TODO(sergeyu) to simplify this code and remove this list.

Done.

[Wez, 2012-08-24 19:43:30]

LGTM w/ one nit, and many thanks for taking care of this clean-up! :)

http://codereview.chromium.org/10867039/diff/4002/remoting/client/rectangle_u...
File remoting/client/rectangle_update_decoder.h (right):

http://codereview.chromium.org/10867039/diff/4002/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:142: int64 last_sequence_number_;
nit: latest_sequence_number_

[kxing, 2012-08-24 19:54:42]

I'll commit this.

http://codereview.chromium.org/10867039/diff/4002/remoting/client/rectangle_u...
File remoting/client/rectangle_update_decoder.h (right):

http://codereview.chromium.org/10867039/diff/4002/remoting/client/rectangle_u...
remoting/client/rectangle_update_decoder.h:142: int64 last_sequence_number_;
On 2012/08/24 19:43:30, Wez wrote:
> nit: latest_sequence_number_

Done.

[commit-bot: I haz the power, 2012-08-24 19:54:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kxing@chromium.org/10867039/11001

[commit-bot: I haz the power, 2012-08-24 21:55:13]

Change committed as 153305