chromeos: Implement the new D-Bus client for SessionManagerClient.

chromeos: Implement the new D-Bus client for SessionManagerClient.

This is part 1 of the LoginLibrary to SessionManagerClient migration.
The new files are added but not used from Chrome yet.

The new code is based on login_library.cc and chromeos_login.cc,
and the new code will replace the them.

For code review, please refer to the files below:

http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chromeos/cros/login_library.h?view=markup
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chromeos/cros/login_library.cc?view=markup
http://git.chromium.org/gitweb/?p=chromiumos/platform/cros.git;a=blob;f=chromeos_login.h;hb=HEAD
http://git.chromium.org/gitweb/?p=chromiumos/platform/cros.git;a=blob;f=chromeos_login.cc;hb=HEAD

Per suggestion from cmasone, we'll be using SessionManagerBlah rather
than LoginManagerBlah for new code, as the former is more appropriate.

BUG=chromium-os:16555
TEST=chrome and tests build

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

[stevenjb, 2011-10-17 18:32:56]

You might want to have someone familiar with the original session/login manager
code look at this also.

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
File chrome/browser/chromeos/dbus/session_manager_client.cc (right):

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.cc:51: virtual void
AddObserver(Observer* observer) {
OVERRIDE

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.cc:56: virtual void
RemoveObserver(Observer* observer) {
OVERRIDE

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
File chrome/browser/chromeos/dbus/session_manager_client.h (right):

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.h:25: // Callend when the
owner key is set.
Called

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.h:36: typedef
base::Callback<void(bool)> StorePolicyCallback;
nit: move typedefs above virtuals or near where they are used.
Document callback args.

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.h:44: // Restarts a job
pointed by |pid| with the provided command line.
nit: s/pointed by/pointed to by/ [or "referenced by"]

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.h:63: // Attempts to store
the policy blob |prop| asynchronously.
|prop| or |policy_blob|?

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.h:64: // Takes |len| because
|prop| may have embedded NULL characters.
std::string, so no more |len|

[satorux1, 2011-10-17 21:24:44]

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
File chrome/browser/chromeos/dbus/session_manager_client.cc (right):

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.cc:51: virtual void
AddObserver(Observer* observer) {
On 2011/10/17 18:32:56, Steven Bennetts wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.cc:56: virtual void
RemoveObserver(Observer* observer) {
On 2011/10/17 18:32:56, Steven Bennetts wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
File chrome/browser/chromeos/dbus/session_manager_client.h (right):

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.h:25: // Callend when the
owner key is set.
On 2011/10/17 18:32:56, Steven Bennetts wrote:
> Called

Done.

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.h:36: typedef
base::Callback<void(bool)> StorePolicyCallback;
On 2011/10/17 18:32:56, Steven Bennetts wrote:
> nit: move typedefs above virtuals or near where they are used.
> Document callback args.

Done.

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.h:44: // Restarts a job
pointed by |pid| with the provided command line.
On 2011/10/17 18:32:56, Steven Bennetts wrote:
> nit: s/pointed by/pointed to by/ [or "referenced by"]

Done.

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.h:63: // Attempts to store
the policy blob |prop| asynchronously.
On 2011/10/17 18:32:56, Steven Bennetts wrote:
> |prop| or |policy_blob|?

Done.

http://codereview.chromium.org/8295019/diff/1/chrome/browser/chromeos/dbus/se...
chrome/browser/chromeos/dbus/session_manager_client.h:64: // Takes |len| because
|prop| may have embedded NULL characters.
On 2011/10/17 18:32:56, Steven Bennetts wrote:
> std::string, so no more |len|

Done.

[satorux1, 2011-10-17 21:30:35]

+cmasone who originally wrote this code.

[Chris Masone, 2011-10-18 16:32:41]

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/session_manager_client.cc (right):

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.cc:64:
login_manager::kSessionManagerEmitLoginPromptReady);
For speed purposes, we were calling this and not bothering to wait for a
response to come back from dbus, because we didn't care about the response.  Is
this doing the same?

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.cc:76:
login_manager::kSessionManagerEmitLoginPromptVisible);
Same comment as above

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.cc:152:
login_manager::kSessionManagerStorePolicy);
Shouldn't policy_blob be put in the message here?

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.cc:166: <<
login_manager::kSessionManagerEmitLoginPromptReady;
The particular error responses from DBus are useful for debugging.  Are they not
exposed?

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/session_manager_client.h (right):

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.h:45: virtual void
RestartEntd() = 0;
This is deprecated; we can either remove it now, or just note with a comment,
your choice.

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.h:61: virtual void
RetrievePolicy(RetrievePolicyCallback callback) = 0;
When I wrote this originally, davemoore indicated that he felt it shouldn't be
called RetrievePolicy, because it doesn't actually retrieve the policy :-)  It
asynchronously kicks off a request to retrieve the policy, and expects a
callback to fire upon success/failure.

That's why we settled on RequestRetrievePolicy.  We weren't super happy with the
name, but we felt it was more accurate.  If naming all asynchronous calls thusly
is a convention you're following in all this new code, that's fine.

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.h:63: // Used for
StorePolicyCallback. Takes a bollean indicating whether the
nit: boolean

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.h:69: virtual void
StorePolicy(const std::string& policy_bob,
Same comment as above.

[satorux1, 2011-10-18 17:49:44]

Chris, thank you for the great comments!

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/session_manager_client.cc (right):

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.cc:64:
login_manager::kSessionManagerEmitLoginPromptReady);
On 2011/10/18 16:32:41, Chris Masone wrote:
> For speed purposes, we were calling this and not bothering to wait for a
> response to come back from dbus, because we didn't care about the response. 
Is
> this doing the same?

Yes. CallMethod() is asynchronous. CallMethodAndBlock() is synchronous, but we
don't plan to use the latter for the code in this directory.

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.cc:76:
login_manager::kSessionManagerEmitLoginPromptVisible);
On 2011/10/18 16:32:41, Chris Masone wrote:
> Same comment as above

Done.

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.cc:152:
login_manager::kSessionManagerStorePolicy);
On 2011/10/18 16:32:41, Chris Masone wrote:
> Shouldn't policy_blob be put in the message here?

You are absolutely right. You saved me from harshly getting yelled at a later
time. Fixed.

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.cc:166: <<
login_manager::kSessionManagerEmitLoginPromptReady;
On 2011/10/18 16:32:41, Chris Masone wrote:
> The particular error responses from DBus are useful for debugging.  Are they
not
> exposed?

Good question. In case of a method call error, the error message is emitted from
dbus/object_proxy.cc, and NULL is passed to the callback.

The relevant piece of code is:

  } else if (dbus_message_get_type(response_message) ==
             DBUS_MESSAGE_TYPE_ERROR) {
    // This will take |response_message| and release (unref) it.                
 
    scoped_ptr<dbus::ErrorResponse> error_response(
        dbus::ErrorResponse::FromRawMessage(response_message));
    // Error message may contain the error message as string.                   
 
    dbus::MessageReader reader(error_response.get());
    std::string error_message;
    reader.PopString(&error_message);
    LOG(ERROR) << "Failed to call method: " << error_response->GetErrorName()
               << ": " << error_message;
    // We don't give the error message to the callback.                         
 
    response_callback.Run(NULL);

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/session_manager_client.h (right):

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.h:45: virtual void
RestartEntd() = 0;
On 2011/10/18 16:32:41, Chris Masone wrote:
> This is deprecated; we can either remove it now, or just note with a comment,
> your choice.

RestartEntd() is called from browser/chromeos/enterprise_extension_observer.cc.
Can we remove this code now?

For now, added a comment saying it's deprecated.

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.h:61: virtual void
RetrievePolicy(RetrievePolicyCallback callback) = 0;
On 2011/10/18 16:32:41, Chris Masone wrote:
> When I wrote this originally, davemoore indicated that he felt it shouldn't be
> called RetrievePolicy, because it doesn't actually retrieve the policy :-)  It
> asynchronously kicks off a request to retrieve the policy, and expects a
> callback to fire upon success/failure.
> 
> That's why we settled on RequestRetrievePolicy.  We weren't super happy with
the
> name, but we felt it was more accurate.  If naming all asynchronous calls
thusly
> is a convention you're following in all this new code, that's fine.

Good point. I expect all D-Bus method calls to be asynchronous in the D-Bus code
in Chrome, so I thought we wouldn't need to add something like 'Request' for
asynchronous methods. Otherwise, all method names end up starting with
'Request'. :)

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.h:63: // Used for
StorePolicyCallback. Takes a bollean indicating whether the
On 2011/10/18 16:32:41, Chris Masone wrote:
> nit: boolean

Done.

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.h:69: virtual void
StorePolicy(const std::string& policy_bob,
On 2011/10/18 16:32:41, Chris Masone wrote:
> Same comment as above.

Done.

[Chris Masone, 2011-10-18 17:55:44]

LGTM

I do notice that the mocks aren't added to any gyp files yet, but that doesn't
really matter for now anyway.

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/session_manager_client.cc (right):

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.cc:64:
login_manager::kSessionManagerEmitLoginPromptReady);
On 2011/10/18 17:49:44, satorux1 wrote:
> On 2011/10/18 16:32:41, Chris Masone wrote:
> > For speed purposes, we were calling this and not bothering to wait for a
> > response to come back from dbus, because we didn't care about the response. 
> Is
> > this doing the same?
> 
> Yes. CallMethod() is asynchronous. CallMethodAndBlock() is synchronous, but we
> don't plan to use the latter for the code in this directory.

cool.

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.cc:166: <<
login_manager::kSessionManagerEmitLoginPromptReady;
On 2011/10/18 17:49:44, satorux1 wrote:
> On 2011/10/18 16:32:41, Chris Masone wrote:
> > The particular error responses from DBus are useful for debugging.  Are they
> not
> > exposed?
> 
> Good question. In case of a method call error, the error message is emitted
from
> dbus/object_proxy.cc, and NULL is passed to the callback.
> 
> The relevant piece of code is:
> 
>   } else if (dbus_message_get_type(response_message) ==
>              DBUS_MESSAGE_TYPE_ERROR) {
>     // This will take |response_message| and release (unref) it.              
 
>  
>     scoped_ptr<dbus::ErrorResponse> error_response(
>         dbus::ErrorResponse::FromRawMessage(response_message));
>     // Error message may contain the error message as string.                 
 
>  
>     dbus::MessageReader reader(error_response.get());
>     std::string error_message;
>     reader.PopString(&error_message);
>     LOG(ERROR) << "Failed to call method: " << error_response->GetErrorName()
>                << ": " << error_message;
>     // We don't give the error message to the callback.                       
 
>  
>     response_callback.Run(NULL);

okey doke.

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/session_manager_client.h (right):

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.h:45: virtual void
RestartEntd() = 0;
On 2011/10/18 17:49:44, satorux1 wrote:
> On 2011/10/18 16:32:41, Chris Masone wrote:
> > This is deprecated; we can either remove it now, or just note with a
comment,
> > your choice.
> 
> RestartEntd() is called from
browser/chromeos/enterprise_extension_observer.cc.
> Can we remove this code now?
> 

I believe so, but kmixter and rginda would know for sure.  That can certainly
happen in a follow on CL, if it's appropriate.  I know that entd is no longer
running by default in R16, but it's still installed in case we need to backtrack
for some reason.  Perhaps we should leave it in til the Chromium R16 branch
point, if that hasn't happened already.

> For now, added a comment saying it's deprecated.

works for me!

http://codereview.chromium.org/8295019/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/session_manager_client.h:61: virtual void
RetrievePolicy(RetrievePolicyCallback callback) = 0;
On 2011/10/18 17:49:44, satorux1 wrote:
> On 2011/10/18 16:32:41, Chris Masone wrote:
> > When I wrote this originally, davemoore indicated that he felt it shouldn't
be
> > called RetrievePolicy, because it doesn't actually retrieve the policy :-) 
It
> > asynchronously kicks off a request to retrieve the policy, and expects a
> > callback to fire upon success/failure.
> > 
> > That's why we settled on RequestRetrievePolicy.  We weren't super happy with
> the
> > name, but we felt it was more accurate.  If naming all asynchronous calls
> thusly
> > is a convention you're following in all this new code, that's fine.
> 
> Good point. I expect all D-Bus method calls to be asynchronous in the D-Bus
code
> in Chrome, so I thought we wouldn't need to add something like 'Request' for
> asynchronous methods. Otherwise, all method names end up starting with
> 'Request'. :)

Fair enough

[stevenjb, 2011-10-18 18:02:37]

lgtm

http://codereview.chromium.org/8295019/diff/12001/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/session_manager_client.h (right):

http://codereview.chromium.org/8295019/diff/12001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/session_manager_client.h:68: // Attempts to store
the policy blob |policy_blob| asynchronously.
nit: could just be "Attempts to store |policy_blob| ..." since it's self
descriptive.

[satorux1, 2011-10-18 18:26:42]

The mock will be used in the next patch!

http://codereview.chromium.org/8295019/diff/12001/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/session_manager_client.h (right):

http://codereview.chromium.org/8295019/diff/12001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/session_manager_client.h:68: // Attempts to store
the policy blob |policy_blob| asynchronously.
On 2011/10/18 18:02:37, Steven Bennetts wrote:
> nit: could just be "Attempts to store |policy_blob| ..." since it's self
> descriptive.

Done.