Various improvements to the Chromoting Android app

Various improvements to the Chromoting Android app

This commit introduces several changes:
- Renames the ChromotingJni class to ChromotingJniRuntime and stores a reference to the instance in the other classes
- Resolves a memory leak in JniFrameConsumer::ApplyBuffer by wrapping the buffer in a scoped_ptr
- Make the canvas activity display full screen, hiding the status bar and action bar to devote more space to the remote desktop
- Clarify the error message that is displayed when no hosts are registered with the logged in account

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213397

[solb, 2013-07-22 22:49:13]

Here's my response to your latest comments on
https://codereview.chromium.org/19297003.

[Wez, 2013-07-23 03:53:06]

https://codereview.chromium.org/19967007/diff/1/remoting/client/jni/jni_frame...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/19967007/diff/1/remoting/client/jni/jni_frame...
remoting/client/jni/jni_frame_consumer.cc:77: delete buffer;
Rather than explicitly delete, assign |buffer| to a scoped_ptr<> at function
scope, and explicitly release() it to DrawBuffer()

https://codereview.chromium.org/19967007/diff/1/remoting/client/jni/jni_frame...
File remoting/client/jni/jni_frame_consumer.h (right):

https://codereview.chromium.org/19967007/diff/1/remoting/client/jni/jni_frame...
remoting/client/jni/jni_frame_consumer.h:48: ChromotingJni* jni_environment_;
nit: Suggest java_interface_ or jni_interface_

https://codereview.chromium.org/19967007/diff/1/remoting/resources/strings.xml
File remoting/resources/strings.xml (right):

https://codereview.chromium.org/19967007/diff/1/remoting/resources/strings.xm...
remoting/resources/strings.xml:28: <string name="error_unexpected_response">No
hosts registered with that account</string>
Was this supposed to be in this CL?

[solb, 2013-07-23 19:01:10]

https://codereview.chromium.org/19967007/diff/1/remoting/client/jni/jni_frame...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/19967007/diff/1/remoting/client/jni/jni_frame...
remoting/client/jni/jni_frame_consumer.cc:77: delete buffer;
On 2013/07/23 03:53:06, Wez wrote:
> Rather than explicitly delete, assign |buffer| to a scoped_ptr<> at function
> scope, and explicitly release() it to DrawBuffer()

Done.

https://codereview.chromium.org/19967007/diff/1/remoting/client/jni/jni_frame...
File remoting/client/jni/jni_frame_consumer.h (right):

https://codereview.chromium.org/19967007/diff/1/remoting/client/jni/jni_frame...
remoting/client/jni/jni_frame_consumer.h:48: ChromotingJni* jni_environment_;
On 2013/07/23 03:53:06, Wez wrote:
> nit: Suggest java_interface_ or jni_interface_

I'm renaming ChromotingJni to ChromotingJniRuntime and calling this reference
jni_runtime_.

[Wez, 2013-07-23 19:51:38]

nit: The sense of your CL description differs between the 1st & 2nd and 3rd &
4th entries.

[Wez, 2013-07-23 19:59:07]

https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:25:
DCHECK(ChromotingJniRuntime::GetInstance()->
This seems to be using the GetInstance() model, too; why not just pass the
ChromotingJniRuntime in at construct-time?

https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/jni_f...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:78: buffer_scoped.reset();
nit: Add a comment to explin why you do this rather than let scoped_ptr<> clean
it up when it goes out of scope

[solb, 2013-07-23 20:50:28]

https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.cc:25:
DCHECK(ChromotingJniRuntime::GetInstance()->
On 2013/07/23 19:59:07, Wez wrote:
> This seems to be using the GetInstance() model, too; why not just pass the
> ChromotingJniRuntime in at construct-time?

Done.

https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/jni_f...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:78: buffer_scoped.reset();
On 2013/07/23 19:59:07, Wez wrote:
> nit: Add a comment to explin why you do this rather than let scoped_ptr<>
clean
> it up when it goes out of scope

Now that you mention it, I guess I'll do just that. ;)

[Wez, 2013-07-23 20:53:01]

https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/jni_f...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/jni_f...
remoting/client/jni/jni_frame_consumer.cc:78: buffer_scoped.reset();
On 2013/07/23 20:50:28, solb wrote:
> On 2013/07/23 19:59:07, Wez wrote:
> > nit: Add a comment to explin why you do this rather than let scoped_ptr<>
> clean
> > it up when it goes out of scope
> 
> Now that you mention it, I guess I'll do just that. ;)

Oh, I assumed it was deliberate, to avoid temporarily hosing memory by having
two buffers allocated?

[solb, 2013-07-23 20:59:50]

On 2013/07/23 20:53:01, Wez wrote:
>
https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/jni_f...
> File remoting/client/jni/jni_frame_consumer.cc (right):
> 
>
https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/jni_f...
> remoting/client/jni/jni_frame_consumer.cc:78: buffer_scoped.reset();
> On 2013/07/23 20:50:28, solb wrote:
> > On 2013/07/23 19:59:07, Wez wrote:
> > > nit: Add a comment to explin why you do this rather than let scoped_ptr<>
> > clean
> > > it up when it goes out of scope
> > 
> > Now that you mention it, I guess I'll do just that. ;)
> 
> Oh, I assumed it was deliberate, to avoid temporarily hosing memory by having
> two buffers allocated?

It was more a matter of automatically replacing the delete call with a reset()
one, although it did exhibit the behavior you describe. Should I put it back to
decrease the probability of running out of memory (which is probably of
diminishing concern on newer devices)?

[Wez, 2013-07-23 21:05:19]

On 2013/07/23 20:59:50, solb wrote:
> On 2013/07/23 20:53:01, Wez wrote:
> >
>
https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/jni_f...
> > File remoting/client/jni/jni_frame_consumer.cc (right):
> > 
> >
>
https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/jni_f...
> > remoting/client/jni/jni_frame_consumer.cc:78: buffer_scoped.reset();
> > On 2013/07/23 20:50:28, solb wrote:
> > > On 2013/07/23 19:59:07, Wez wrote:
> > > > nit: Add a comment to explin why you do this rather than let
scoped_ptr<>
> > > clean
> > > > it up when it goes out of scope
> > > 
> > > Now that you mention it, I guess I'll do just that. ;)
> > 
> > Oh, I assumed it was deliberate, to avoid temporarily hosing memory by
having
> > two buffers allocated?
> 
> It was more a matter of automatically replacing the delete call with a reset()
> one, although it did exhibit the behavior you describe. Should I put it back
to
> decrease the probability of running out of memory (which is probably of
> diminishing concern on newer devices)?

Alas, desktops keep getting bigger just as devices keep getting more RAM - I'd
suggest keeping it in with a comment to explain why. :)

[solb, 2013-07-23 21:20:02]

On 2013/07/23 21:05:19, Wez wrote:
> On 2013/07/23 20:59:50, solb wrote:
> > On 2013/07/23 20:53:01, Wez wrote:
> > >
> >
>
https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/jni_f...
> > > File remoting/client/jni/jni_frame_consumer.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/19967007/diff/13001/remoting/client/jni/jni_f...
> > > remoting/client/jni/jni_frame_consumer.cc:78: buffer_scoped.reset();
> > > On 2013/07/23 20:50:28, solb wrote:
> > > > On 2013/07/23 19:59:07, Wez wrote:
> > > > > nit: Add a comment to explin why you do this rather than let
> scoped_ptr<>
> > > > clean
> > > > > it up when it goes out of scope
> > > > 
> > > > Now that you mention it, I guess I'll do just that. ;)
> > > 
> > > Oh, I assumed it was deliberate, to avoid temporarily hosing memory by
> having
> > > two buffers allocated?
> > 
> > It was more a matter of automatically replacing the delete call with a
reset()
> > one, although it did exhibit the behavior you describe. Should I put it back
> to
> > decrease the probability of running out of memory (which is probably of
> > diminishing concern on newer devices)?
> 
> Alas, desktops keep getting bigger just as devices keep getting more RAM - I'd
> suggest keeping it in with a comment to explain why. :)

Done.

[Wez, 2013-07-24 00:13:27]

lgtm

https://codereview.chromium.org/19967007/diff/29002/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/19967007/diff/29002/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.h:40:
ChromotingJniInstance(ChromotingJniRuntime* jni_runtime,
nit: Clarify the lifetime requirement on |jni_runtime|

https://codereview.chromium.org/19967007/diff/29002/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.h:100: ChromotingJniRuntime*
jni_runtime_;
nit: Add a comment clarifying that the instance uses this to call back to Java.

[solb, 2013-07-24 00:46:09]

https://codereview.chromium.org/19967007/diff/29002/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/19967007/diff/29002/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.h:40:
ChromotingJniInstance(ChromotingJniRuntime* jni_runtime,
On 2013/07/24 00:13:28, Wez wrote:
> nit: Clarify the lifetime requirement on |jni_runtime|

Done.

https://codereview.chromium.org/19967007/diff/29002/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.h:100: ChromotingJniRuntime*
jni_runtime_;
On 2013/07/24 00:13:28, Wez wrote:
> nit: Add a comment clarifying that the instance uses this to call back to
Java.

Done.

[commit-bot: I haz the power, 2013-07-24 03:54:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/19967007/65001

[commit-bot: I haz the power, 2013-07-24 10:42:34]

Change committed as 213397