Move UpdateStreamEncoding value into the BeginUpdateStreamMessage since we...

Move common ChromotingView functionality into base class so that the code can
be shared between PepperView and X11View.

Have View create appropriate decoder based on encoding of the update stream.

Add state to decoder to keep track of whether Begin/End has been called in the
correct order.

Add unittests for new routines.

BUG=none
TEST=remoting unit tests


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

[Alpha Left Google, 2010-08-10 22:35:56]

This is quite different than the original design.

The original design was that we can have multuple encoding regions in each
update stream. So we can have one backing store and multiple different decoders
in each view. To dynamically choose a decoder instead of hard-coding we can do
it in HandleUpdateStreamPacket() in the view classes instead.

http://codereview.chromium.org/3124005/diff/13002/15001
File remoting/base/protocol/chromotocol.proto (right):

http://codereview.chromium.org/3124005/diff/13002/15001#newcode38
remoting/base/protocol/chromotocol.proto:38: EncodingInvalid = -1;
I don't think we need to have invalid here.

http://codereview.chromium.org/3124005/diff/13002/15004
File remoting/client/plugin/pepper_view.cc (left):

http://codereview.chromium.org/3124005/diff/13002/15004#oldcode179
remoting/client/plugin/pepper_view.cc:179: 
Can't we initialize the decoder here?

so we can do
if (!decoder_) {
  DCHECK(msg->has_begin_rect());
  decoder_ = CreateEncoder(msg->encoding());
}

[garykac, 2010-08-10 22:48:45]

http://codereview.chromium.org/3124005/diff/13002/15001
File remoting/base/protocol/chromotocol.proto (right):

http://codereview.chromium.org/3124005/diff/13002/15001#newcode38
remoting/base/protocol/chromotocol.proto:38: EncodingInvalid = -1;
On 2010/08/10 22:35:56, Alpha wrote:
> I don't think we need to have invalid here.
> 

Since "None" really means "uncompressed", we currently have no way to
distinguish between Invalid and Uncompressed.

I plan on writing unittests that will use this value.

http://codereview.chromium.org/3124005/diff/13002/15004
File remoting/client/plugin/pepper_view.cc (left):

http://codereview.chromium.org/3124005/diff/13002/15004#oldcode179
remoting/client/plugin/pepper_view.cc:179: 
On 2010/08/10 22:35:56, Alpha wrote:
> Can't we initialize the decoder here?
> 
> so we can do
> if (!decoder_) {
>   DCHECK(msg->has_begin_rect());
>   decoder_ = CreateEncoder(msg->encoding());
> }
> 

HandleBeginUpdateStream() calls BeginDecode() on the decoder, so it needs to be
around at "begin" time.

We could move all the BeginUpdateStream code in here, but then there's no point
in having a separate "begin" message.

[Alpha Left Google, 2010-08-10 22:53:34]

On 2010/08/10 22:48:45, garykac wrote:
> http://codereview.chromium.org/3124005/diff/13002/15001
> File remoting/base/protocol/chromotocol.proto (right):
> 
> http://codereview.chromium.org/3124005/diff/13002/15001#newcode38
> remoting/base/protocol/chromotocol.proto:38: EncodingInvalid = -1;
> On 2010/08/10 22:35:56, Alpha wrote:
> > I don't think we need to have invalid here.
> > 
> 
> Since "None" really means "uncompressed", we currently have no way to
> distinguish between Invalid and Uncompressed.
> 
> I plan on writing unittests that will use this value.
> 
> http://codereview.chromium.org/3124005/diff/13002/15004
> File remoting/client/plugin/pepper_view.cc (left):
> 
> http://codereview.chromium.org/3124005/diff/13002/15004#oldcode179
> remoting/client/plugin/pepper_view.cc:179: 
> On 2010/08/10 22:35:56, Alpha wrote:
> > Can't we initialize the decoder here?
> > 
> > so we can do
> > if (!decoder_) {
> >   DCHECK(msg->has_begin_rect());
> >   decoder_ = CreateEncoder(msg->encoding());
> > }
> > 
> 
> HandleBeginUpdateStream() calls BeginDecode() on the decoder, so it needs to
be
> around at "begin" time.
> 
> We could move all the BeginUpdateStream code in here, but then there's no
point
> in having a separate "begin" message.

I think it's better to move the logic to HandleUpdateStreamPacket().

HandleBeginUpdateStream() is supposed to prepare the backing store, for example
it'll lock the backing store for writing. So you can keep allocating the frame_
there in HandleBeginUpdateStream() as illustration.

[garykac, 2010-08-17 22:47:08]

Updated based on comments and discussion.

[Alpha Left Google, 2010-08-20 05:17:07]

http://codereview.chromium.org/3124005/diff/31010/36003
File remoting/client/chromoting_view.cc (right):

http://codereview.chromium.org/3124005/diff/31010/36003#newcode20
remoting/client/chromoting_view.cc:20: CHECK(encoding != EncodingInvalid);
We should use DCHECK instead of CHECK unless we are quite sure this won't happen
in runtime. Like in this case having CHECK is too strict as user can craft the
message to kill the host.

http://codereview.chromium.org/3124005/diff/31010/36003#newcode24
remoting/client/chromoting_view.cc:24: if (decoder_type_ != EncodingInvalid &&
decoder_started_) {
Having both a runtime check and CHECK seems redundant, getting rid one of those?

I still don't see much use of EncodingInvalid but okay..

http://codereview.chromium.org/3124005/diff/31010/36003#newcode29
remoting/client/chromoting_view.cc:29: CHECK(decoder_.get());
We don't need to check for the object being allocated, it'll crash anyway when
we try to run it.

http://codereview.chromium.org/3124005/diff/31010/36003#newcode30
remoting/client/chromoting_view.cc:30: CHECK(decoder_type_ == encoding);
Same here having CHECK is too strict pariticular this is something can be
crafted by the user.

http://codereview.chromium.org/3124005/diff/31010/36003#newcode34
remoting/client/chromoting_view.cc:34: if (decoder_type_ != encoding ||
!decoder_.get()) {
The above section already check that decoder_type_ == encoding why the check
here?

http://codereview.chromium.org/3124005/diff/31010/36003#newcode44
remoting/client/chromoting_view.cc:44: CHECK(decoder_.get());
I think these checks are redundant..

http://codereview.chromium.org/3124005/diff/31010/36003#newcode57
remoting/client/chromoting_view.cc:57: decoder_started_ = true;
I think this variable should go into decoder instead.

http://codereview.chromium.org/3124005/diff/31010/36003#newcode62
remoting/client/chromoting_view.cc:62: CHECK(decoder_started_);
Move this variable into Decoder and we can check there and should use CHECK in
this case.

http://codereview.chromium.org/3124005/diff/31010/36003#newcode71
remoting/client/chromoting_view.cc:71: decoder_started_ = false;
Move this into Decoder too.

http://codereview.chromium.org/3124005/diff/31010/36004
File remoting/client/chromoting_view.h (right):

http://codereview.chromium.org/3124005/diff/31010/36004#newcode52
remoting/client/chromoting_view.h:52: // (2) Make sure the |frame_| has been
initialized
nit: full stop at the end.

http://codereview.chromium.org/3124005/diff/31010/36004#newcode82
remoting/client/chromoting_view.h:82: bool SetupDecoder(UpdateStreamEncoding
encoding);
Why return a boolean in this method?

There should be another method like: bool HasDecoderStarted(UpdateStreamEncoding
encodig) const; that checks if decoder has started or not.

The HasDecoderStarted() method should then ask decoder_ for started().

http://codereview.chromium.org/3124005/diff/31010/36004#newcode100
remoting/client/chromoting_view.h:100: 
nit: |decoder_|

http://codereview.chromium.org/3124005/diff/31010/36004#newcode102
remoting/client/chromoting_view.h:102: bool decoder_started_;
Why not add a method to Decoder like bool started() const;

Having one started variable doesn't seem to handle the case of multiple
decoders.

[garykac, 2010-08-24 00:44:58]

http://codereview.chromium.org/3124005/diff/31010/36003
File remoting/client/chromoting_view.cc (right):

http://codereview.chromium.org/3124005/diff/31010/36003#newcode20
remoting/client/chromoting_view.cc:20: CHECK(encoding != EncodingInvalid);
On 2010/08/20 05:17:07, Alpha wrote:
> We should use DCHECK instead of CHECK unless we are quite sure this won't
happen
> in runtime. Like in this case having CHECK is too strict as user can craft the
> message to kill the host.

Changed to LOG(ERROR) with a return false.

http://codereview.chromium.org/3124005/diff/31010/36003#newcode24
remoting/client/chromoting_view.cc:24: if (decoder_type_ != EncodingInvalid &&
decoder_started_) {
On 2010/08/20 05:17:07, Alpha wrote:
> Having both a runtime check and CHECK seems redundant, getting rid one of
those?
> 
> I still don't see much use of EncodingInvalid but okay..

They're not doing the same thing.  One is checking |encoding| and the other is
checking |decoder_type_|.

The first check is verifying that the value passed in is valid -- i.e., that you
never request a decoder with the Invalid type. This could happen if the value in
the UpdateStreamPacket was uninitialized.

The code here is checking to see if we already have a decoder setup and started.
If so, we simply return that previously allocated decoder.

http://codereview.chromium.org/3124005/diff/31010/36003#newcode29
remoting/client/chromoting_view.cc:29: CHECK(decoder_.get());
On 2010/08/20 05:17:07, Alpha wrote:
> We don't need to check for the object being allocated, it'll crash anyway when
> we try to run it.

Code has changed - this is now a LOG(ERROR)

http://codereview.chromium.org/3124005/diff/31010/36003#newcode30
remoting/client/chromoting_view.cc:30: CHECK(decoder_type_ == encoding);
On 2010/08/20 05:17:07, Alpha wrote:
> Same here having CHECK is too strict pariticular this is something can be
> crafted by the user.

Done.

http://codereview.chromium.org/3124005/diff/31010/36003#newcode34
remoting/client/chromoting_view.cc:34: if (decoder_type_ != encoding ||
!decoder_.get()) {
On 2010/08/20 05:17:07, Alpha wrote:
> The above section already check that decoder_type_ == encoding why the check
> here?
> 

If the requested decoder doesn't match the one that we already have allocated,
then we need to allocate a new one.

http://codereview.chromium.org/3124005/diff/31010/36003#newcode44
remoting/client/chromoting_view.cc:44: CHECK(decoder_.get());
On 2010/08/20 05:17:07, Alpha wrote:
> I think these checks are redundant..

See above.

http://codereview.chromium.org/3124005/diff/31010/36003#newcode57
remoting/client/chromoting_view.cc:57: decoder_started_ = true;
On 2010/08/20 05:17:07, Alpha wrote:
> I think this variable should go into decoder instead.

Done.

http://codereview.chromium.org/3124005/diff/31010/36003#newcode62
remoting/client/chromoting_view.cc:62: CHECK(decoder_started_);
On 2010/08/20 05:17:07, Alpha wrote:
> Move this variable into Decoder and we can check there and should use CHECK in
> this case.

Done.

http://codereview.chromium.org/3124005/diff/31010/36003#newcode71
remoting/client/chromoting_view.cc:71: decoder_started_ = false;
On 2010/08/20 05:17:07, Alpha wrote:
> Move this into Decoder too.

Done.

http://codereview.chromium.org/3124005/diff/31010/36004
File remoting/client/chromoting_view.h (right):

http://codereview.chromium.org/3124005/diff/31010/36004#newcode52
remoting/client/chromoting_view.h:52: // (2) Make sure the |frame_| has been
initialized
On 2010/08/20 05:17:07, Alpha wrote:
> nit: full stop at the end.

Done.

http://codereview.chromium.org/3124005/diff/31010/36004#newcode82
remoting/client/chromoting_view.h:82: bool SetupDecoder(UpdateStreamEncoding
encoding);
On 2010/08/20 05:17:07, Alpha wrote:
> Why return a boolean in this method?
> 
> There should be another method like: bool
HasDecoderStarted(UpdateStreamEncoding
> encodig) const; that checks if decoder has started or not.
> 
> The HasDecoderStarted() method should then ask decoder_ for started().
> 

I added an IsStarted() method. For (testing) convenience, this method returns
that value.

http://codereview.chromium.org/3124005/diff/31010/36004#newcode100
remoting/client/chromoting_view.h:100: 
On 2010/08/20 05:17:07, Alpha wrote:
> nit: |decoder_|

Done.

http://codereview.chromium.org/3124005/diff/31010/36004#newcode102
remoting/client/chromoting_view.h:102: bool decoder_started_;
On 2010/08/20 05:17:07, Alpha wrote:
> Why not add a method to Decoder like bool started() const;
> 
> Having one started variable doesn't seem to handle the case of multiple
> decoders.

Done.

[Alpha Left Google, 2010-08-24 01:16:33]

LGTM.