Make AudioPlayer optional in ChromotingClient

Make AudioPlayer optional in ChromotingClient

Passing in a null pointer now disables the audio stream.

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

[solb, 2013-07-09 19:20:46]

Clients that don't support audio can now simply pass a null AudioPlayer pointer
into ChromotingClient's constructor.

[Wez, 2013-07-09 20:08:07]

https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
remoting/client/chromoting_client.cc:44: if (audio_player.get())
nit: if (audio_player)

https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
remoting/client/chromoting_client.cc:44: if (audio_player.get())
This if has a multi-line statement, so use {}

https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
remoting/client/chromoting_client.cc:49: // Otherwise, it will be left as a null
pointer.
What does this comment mean?

https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
File remoting/client/chromoting_client.h (right):

https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
remoting/client/chromoting_client.h:45: // A null pointer in |audio_player|
disables the audio stream.
nit: We should have a comment here that clarifies the ClientContext etc object
lifetime requirements... for this CL, though, I'd suggest "|audio_player| may be
null, in which case audio will not be requested."

https://codereview.chromium.org/18452006/diff/1/remoting/protocol/connection_...
File remoting/protocol/connection_to_host.h (right):

https://codereview.chromium.org/18452006/diff/1/remoting/protocol/connection_...
remoting/protocol/connection_to_host.h:81: // |audio_stub| disables the audio
stream.
nit: See comment on ChromotingClient; we should document the requirements for
all the non-obvious parameters, really.

[solb, 2013-07-09 20:23:57]

On 2013/07/09 20:08:07, Wez wrote:
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> File remoting/client/chromoting_client.cc (right):
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> remoting/client/chromoting_client.cc:44: if (audio_player.get())
> nit: if (audio_player)
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> remoting/client/chromoting_client.cc:44: if (audio_player.get())
> This if has a multi-line statement, so use {}
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> remoting/client/chromoting_client.cc:49: // Otherwise, it will be left as a
null
> pointer.
> What does this comment mean?
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> File remoting/client/chromoting_client.h (right):
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> remoting/client/chromoting_client.h:45: // A null pointer in |audio_player|
> disables the audio stream.
> nit: We should have a comment here that clarifies the ClientContext etc object
> lifetime requirements... for this CL, though, I'd suggest "|audio_player| may
be
> null, in which case audio will not be requested."
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/protocol/connection_...
> File remoting/protocol/connection_to_host.h (right):
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/protocol/connection_...
> remoting/protocol/connection_to_host.h:81: // |audio_stub| disables the audio
> stream.
> nit: See comment on ChromotingClient; we should document the requirements for
> all the non-obvious parameters, really.

Done.

[solb, 2013-07-09 22:25:46]

On 2013/07/09 20:23:57, solb wrote:
> On 2013/07/09 20:08:07, Wez wrote:
> >
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> > File remoting/client/chromoting_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> > remoting/client/chromoting_client.cc:44: if (audio_player.get())
> > nit: if (audio_player)
> > 
> >
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> > remoting/client/chromoting_client.cc:44: if (audio_player.get())
> > This if has a multi-line statement, so use {}
> > 
> >
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> > remoting/client/chromoting_client.cc:49: // Otherwise, it will be left as a
> null
> > pointer.
> > What does this comment mean?
> > 
> >
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> > File remoting/client/chromoting_client.h (right):
> > 
> >
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> > remoting/client/chromoting_client.h:45: // A null pointer in |audio_player|
> > disables the audio stream.
> > nit: We should have a comment here that clarifies the ClientContext etc
object
> > lifetime requirements... for this CL, though, I'd suggest "|audio_player|
may
> be
> > null, in which case audio will not be requested."
> > 
> >
>
https://codereview.chromium.org/18452006/diff/1/remoting/protocol/connection_...
> > File remoting/protocol/connection_to_host.h (right):
> > 
> >
>
https://codereview.chromium.org/18452006/diff/1/remoting/protocol/connection_...
> > remoting/protocol/connection_to_host.h:81: // |audio_stub| disables the
audio
> > stream.
> > nit: See comment on ChromotingClient; we should document the requirements
for
> > all the non-obvious parameters, really.
> 
> Done.

ping: Wez

[Wez, 2013-07-09 22:41:38]

lgtm

[commit-bot: I haz the power, 2013-07-09 22:41:58]

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

[solb, 2013-07-10 00:00:31]

With those stub classes gone, here's a very basic version of
ChromotingJNIInstance and its accompanying JNI entry points instead.

https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
File remoting/client/chromoting_client.cc (right):

https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
remoting/client/chromoting_client.cc:44: if (audio_player.get())
On 2013/07/09 20:08:07, Wez wrote:
> nit: if (audio_player)

Done.

https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
remoting/client/chromoting_client.cc:44: if (audio_player.get())
On 2013/07/09 20:08:07, Wez wrote:
> This if has a multi-line statement, so use {}

Done.

https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
remoting/client/chromoting_client.cc:49: // Otherwise, it will be left as a null
pointer.
On 2013/07/09 20:08:07, Wez wrote:
> What does this comment mean?

Done.

https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
File remoting/client/chromoting_client.h (right):

https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
remoting/client/chromoting_client.h:45: // A null pointer in |audio_player|
disables the audio stream.
On 2013/07/09 20:08:07, Wez wrote:
> nit: We should have a comment here that clarifies the ClientContext etc object
> lifetime requirements... for this CL, though, I'd suggest "|audio_player| may
be
> null, in which case audio will not be requested."

Done.

https://codereview.chromium.org/18452006/diff/1/remoting/protocol/connection_...
File remoting/protocol/connection_to_host.h (right):

https://codereview.chromium.org/18452006/diff/1/remoting/protocol/connection_...
remoting/protocol/connection_to_host.h:81: // |audio_stub| disables the audio
stream.
On 2013/07/09 20:08:07, Wez wrote:
> nit: See comment on ChromotingClient; we should document the requirements for
> all the non-obvious parameters, really.

Done.

[solb, 2013-07-10 00:01:47]

On 2013/07/10 00:00:31, solb wrote:
> With those stub classes gone, here's a very basic version of
> ChromotingJNIInstance and its accompanying JNI entry points instead.
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> File remoting/client/chromoting_client.cc (right):
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> remoting/client/chromoting_client.cc:44: if (audio_player.get())
> On 2013/07/09 20:08:07, Wez wrote:
> > nit: if (audio_player)
> 
> Done.
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> remoting/client/chromoting_client.cc:44: if (audio_player.get())
> On 2013/07/09 20:08:07, Wez wrote:
> > This if has a multi-line statement, so use {}
> 
> Done.
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> remoting/client/chromoting_client.cc:49: // Otherwise, it will be left as a
null
> pointer.
> On 2013/07/09 20:08:07, Wez wrote:
> > What does this comment mean?
> 
> Done.
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> File remoting/client/chromoting_client.h (right):
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/client/chromoting_cl...
> remoting/client/chromoting_client.h:45: // A null pointer in |audio_player|
> disables the audio stream.
> On 2013/07/09 20:08:07, Wez wrote:
> > nit: We should have a comment here that clarifies the ClientContext etc
object
> > lifetime requirements... for this CL, though, I'd suggest "|audio_player|
may
> be
> > null, in which case audio will not be requested."
> 
> Done.
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/protocol/connection_...
> File remoting/protocol/connection_to_host.h (right):
> 
>
https://codereview.chromium.org/18452006/diff/1/remoting/protocol/connection_...
> remoting/protocol/connection_to_host.h:81: // |audio_stub| disables the audio
> stream.
> On 2013/07/09 20:08:07, Wez wrote:
> > nit: See comment on ChromotingClient; we should document the requirements
for
> > all the non-obvious parameters, really.
> 
> Done.

Sorry; that was meant for https://codereview.chromium.org/18856012.  Please
check it out at your convenience.

[commit-bot: I haz the power, 2013-07-10 02:00:59]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-07-10 02:41:08]

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

[commit-bot: I haz the power, 2013-07-10 07:00:03]

Change committed as 210769