Added daemon process NPAPI.

Added daemon process NPAPI.

This provides a framework that we can implement per-platform to allow the
Me2Me daemon to installed, started and stopped by the web-app.

I've gone with a polling-based API for now, as it simplifies some of
the logic. We can revisit this if necessary to make it more sophisticated.

BUG=None
TEST=None until we have some implementations.


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

[Sergey Ulanov, 2012-02-02 23:41:24]

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi.h (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:5: #ifndef REMOTING_DAEMON_NPAPI_H_
nit: REMOTING_HOST_PLUGIN_DAEMONG_NPAPI_H_

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:10: class DaemonNpapi {
Not sure if this is a right name for this class - it doesn't seem to be NPAPI
specific. Maybe DaemonController?

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:14: enum State {
nit: This should be declared before the constructor. Types go first.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:53: #endif
nit: // REMOTING_...

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi_linux.cc (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_linux.cc:5: #include
"remoting/host/plugin/daemon_npapi.h"
nit: empty line after this include

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_linux.cc:23:
DaemonNpapiLinux::DaemonNpapiLinux() {
nit: should this be in the nameless namespace where the class is declared.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_linux.cc:36: remoting::DaemonNpapi*
remoting::DaemonNpapi::Create() {
nit put this in namespace remoting { }?

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/host_script_object.h (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/host_script_object.h:111: //   string pin
We'll probably need a bunch of other parameters beside PIN, e.g. auth tokens.
Maybe pass JSON blob with configuration?

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/remoting.gy...
remoting/remoting.gyp:289: 'host/plugin/daemon_npapi_linux.cc',
daemon_npapi.h should also be here.

[alexeypa (please no reviews), 2012-02-03 00:26:05]

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi.h (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:40: virtual void Start(const char* pin) = 0;
nit: typically it is pretty bad idea to pass a string as a char*. I would wrap
it to std::string or something else.

Potentially there is also a i18n issue. If, say, in the future the PIN will not
be numeric or, for instance we decide to support arabic digits
(http://unicode.org/glossary/#arabic_indic_digits).

[Jamie, 2012-02-03 00:47:40]

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi.h (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:5: #ifndef REMOTING_DAEMON_NPAPI_H_
On 2012/02/02 23:41:24, sergeyu wrote:
> nit: REMOTING_HOST_PLUGIN_DAEMONG_NPAPI_H_

Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:10: class DaemonNpapi {
On 2012/02/02 23:41:24, sergeyu wrote:
> Not sure if this is a right name for this class - it doesn't seem to be NPAPI
> specific. Maybe DaemonController?

Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:14: enum State {
On 2012/02/02 23:41:24, sergeyu wrote:
> nit: This should be declared before the constructor. Types go first.

I think it's clearer placed next to where it's used, but I'll adhere to the
style guide.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:40: virtual void Start(const char* pin) = 0;
On 2012/02/03 00:26:05, alexeypa wrote:
> nit: typically it is pretty bad idea to pass a string as a char*. I would wrap
> it to std::string or something else.

Both seem reasonably common in Chrome. I prefer const char* because it avoids an
unnecessary string ctor if the caller passes in a C-style string.

> Potentially there is also a i18n issue. If, say, in the future the PIN will
not
> be numeric or, for instance we decide to support arabic digits
> (http://unicode.org/glossary/#arabic_indic_digits).

The string is UTF8-encoded, so I don't think there are i18n issues.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:53: #endif
On 2012/02/02 23:41:24, sergeyu wrote:
> nit: // REMOTING_...

Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi_linux.cc (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_linux.cc:5: #include
"remoting/host/plugin/daemon_npapi.h"
On 2012/02/02 23:41:24, sergeyu wrote:
> nit: empty line after this include

Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_linux.cc:23:
DaemonNpapiLinux::DaemonNpapiLinux() {
On 2012/02/02 23:41:24, sergeyu wrote:
> nit: should this be in the nameless namespace where the class is declared.

For consistency with other uses of a similar pattern, done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_linux.cc:36: remoting::DaemonNpapi*
remoting::DaemonNpapi::Create() {
On 2012/02/02 23:41:24, sergeyu wrote:
> nit put this in namespace remoting { }?

Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/host_script_object.h (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/host_script_object.h:111: //   string pin
On 2012/02/02 23:41:24, sergeyu wrote:
> We'll probably need a bunch of other parameters beside PIN, e.g. auth tokens.
> Maybe pass JSON blob with configuration?

I think I'd rather add these as needed in a more strongly-typed way. You're
right that the PIN is not going to be sufficient long term, but until we know
what else will be needed, it's a good start.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/remoting.gy...
remoting/remoting.gyp:289: 'host/plugin/daemon_npapi_linux.cc',
On 2012/02/02 23:41:24, sergeyu wrote:
> daemon_npapi.h should also be here.

Good catch! Done.

[Sergey Ulanov, 2012-02-03 01:08:24]

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi.h (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:40: virtual void Start(const char* pin) = 0;
On 2012/02/03 00:47:41, Jamie wrote:
> On 2012/02/03 00:26:05, alexeypa wrote:
> > nit: typically it is pretty bad idea to pass a string as a char*. I would
wrap
> > it to std::string or something else.
> 
> Both seem reasonably common in Chrome. I prefer const char* because it avoids
an
> unnecessary string ctor if the caller passes in a C-style string.

Chromoting uses string class in most cases, and I think it's better to be
consistent with the rest of remoting code. Also, string is more efficient when
you copy it from one object to another, i.e. if the caller does have string, and
the method being called needs to save it, then it would not need to duplicate
the the content of the string (it just increases ref-counter), while with char*
the data would have to be duplicated.
You can also use base::StringPiece class if you worry about efficiency, but I
don't think it is really necessary here.

> 
> > Potentially there is also a i18n issue. If, say, in the future the PIN will
> not
> > be numeric or, for instance we decide to support arabic digits
> > (http://unicode.org/glossary/#arabic_indic_digits).
> 
> The string is UTF8-encoded, so I don't think there are i18n issues.

[Wez, 2012-02-03 01:25:36]

LGTM with nits.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi.h (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:10: class DaemonNpapi {
On 2012/02/02 23:41:24, sergeyu wrote:
> Not sure if this is a right name for this class - it doesn't seem to be NPAPI
> specific. Maybe DaemonController?

Agreed.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:19: STATE_NOT_INSTALLED = 0,
nit: Do we need explicit values for each enum value?

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:28: STATE_START_FAILED = 3
Perhaps have a STATE_NOT_IMPLEMENTED for now, for Linux & Mac to return, so we
can avoid presenting the UI on those platforms for now?

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:40: virtual void Start(const char* pin) = 0;
Why do we want to set the PIN when we start the Daemon process?  What if we find
the daemon is installed & configured but is not running?  How do we then start
it?  How do we change the PIN of an already-installed daemon?

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:40: virtual void Start(const char* pin) = 0;
On 2012/02/03 00:26:05, alexeypa wrote:
> nit: typically it is pretty bad idea to pass a string as a char*. I would wrap
> it to std::string or something else.
> 
> Potentially there is also a i18n issue. If, say, in the future the PIN will
not
> be numeric or, for instance we decide to support arabic digits
> (http://unicode.org/glossary/#arabic_indic_digits).

Surely wrapping it as std::string doesn't help - it's more to do with the
character encoding convention, which in Chrome is UTF-8 for the most part, and
is stated explicitly earlier in this method's description.

I think the preference is for APIs to use const base::StringPiece& , although
most remoting/ code uses const std::string&.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi_linux.cc (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_linux.cc:30: void
DaemonNpapiLinux::Start(const char* pin) {
nit: NOTIMPLEMENTED()?

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_linux.cc:33: void DaemonNpapiLinux::Stop() {
nit: Ditto.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi_mac.cc (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_mac.cc:5: #include
"remoting/host/plugin/daemon_npapi.h"
nit: newline after this

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_mac.cc:30: void DaemonNpapiMac::Start(const
char* pin) {
nit: NOTIMPLEMENTED().

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_mac.cc:33: void DaemonNpapiMac::Stop() {
nit: Ditto.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi_win.cc (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_win.cc:5: #include
"remoting/host/plugin/daemon_npapi.h"
nit: newline.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_win.cc:23: DaemonNpapiWin::DaemonNpapiWin() {
Why aren't these defined as well as declared in the anonymous namespace?

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_win.cc:30: void DaemonNpapiWin::Start(const
char* pin) {
nit: NOTIMPLEMENTED

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/host_script_object.cc:570: std::string(pin.UTF8Characters,
pin.UTF8Length).c_str());
Definitely cleaner for Start() to accept a std::string or base::StringPiece
here.

[Jamie, 2012-02-03 01:40:15]

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi.h (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:19: STATE_NOT_INSTALLED = 0,
On 2012/02/03 01:25:36, Wez wrote:
> nit: Do we need explicit values for each enum value?

When the JS side of this is implemented, we'll need to keep enum values in sync,
and I like to specify them explicitly as a signal to that effect. Once I know
what JS file the values will be duplicated in, I'll add a comment here as well.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:28: STATE_START_FAILED = 3
On 2012/02/03 01:25:36, Wez wrote:
> Perhaps have a STATE_NOT_IMPLEMENTED for now, for Linux & Mac to return, so we
> can avoid presenting the UI on those platforms for now?

Good idea. Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi.h:40: virtual void Start(const char* pin) = 0;
On 2012/02/03 01:25:36, Wez wrote:
> Why do we want to set the PIN when we start the Daemon process?  What if we
find
> the daemon is installed & configured but is not running?  How do we then start
> it?

According to the latest mocks, you can't start the daemon without specifying a
PIN.

>  How do we change the PIN of an already-installed daemon?

Call Start again. There's a comment to that effect.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi_linux.cc (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_linux.cc:30: void
DaemonNpapiLinux::Start(const char* pin) {
On 2012/02/03 01:25:36, Wez wrote:
> nit: NOTIMPLEMENTED()?

Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_linux.cc:33: void DaemonNpapiLinux::Stop() {
On 2012/02/03 01:25:36, Wez wrote:
> nit: Ditto.

Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi_mac.cc (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_mac.cc:5: #include
"remoting/host/plugin/daemon_npapi.h"
On 2012/02/03 01:25:36, Wez wrote:
> nit: newline after this

Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_mac.cc:30: void DaemonNpapiMac::Start(const
char* pin) {
On 2012/02/03 01:25:36, Wez wrote:
> nit: NOTIMPLEMENTED().

Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_mac.cc:33: void DaemonNpapiMac::Stop() {
On 2012/02/03 01:25:36, Wez wrote:
> nit: Ditto.

Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/daemon_npapi_win.cc (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_win.cc:5: #include
"remoting/host/plugin/daemon_npapi.h"
On 2012/02/03 01:25:36, Wez wrote:
> nit: newline.

Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_win.cc:23: DaemonNpapiWin::DaemonNpapiWin() {
On 2012/02/03 01:25:36, Wez wrote:
> Why aren't these defined as well as declared in the anonymous namespace?

Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/daemon_npapi_win.cc:30: void DaemonNpapiWin::Start(const
char* pin) {
On 2012/02/03 01:25:36, Wez wrote:
> nit: NOTIMPLEMENTED

Done.

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/9316078/diff/3002/remoting/host/plugin...
remoting/host/plugin/host_script_object.cc:570: std::string(pin.UTF8Characters,
pin.UTF8Length).c_str());
On 2012/02/03 01:25:36, Wez wrote:
> Definitely cleaner for Start() to accept a std::string or base::StringPiece
> here.

Done.

[Jamie, 2012-02-06 23:50:53]

Please take another look. Conflating Start with state setters won't work in the
long term when we have more state than just a PIN so the new model is that the
caller needs to set state as appropriate before calling other functions in this
API, which may return an error if this is not done.

[Wez, 2012-02-07 00:39:52]

lgtm

https://chromiumcodereview.appspot.com/9316078/diff/8001/remoting/host/plugin...
File remoting/host/plugin/daemon_controller.h (right):

https://chromiumcodereview.appspot.com/9316078/diff/8001/remoting/host/plugin...
remoting/host/plugin/daemon_controller.h:49: // TODO(jamiewalch): More
state-setting methods needed here. Sufficient
nit: They're not _needed_, unless they're... well... _needed_... But yes, the
calls should document the state-setting requirements.

[commit-bot: I haz the power, 2012-02-07 01:06:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/9316078/8001

[commit-bot: I haz the power, 2012-02-07 05:35:21]

Change committed as 120723