chromeos: Add dbus MTPDClient.

chromeos: Add dbus MTPDClient.

BUG=chromium-os:29557
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150382

[Lei Zhang, 2012-08-03 03:20:43]

This is basically a poor copy of CrOSDiskClient, just like MTPD is a poor copy
of the cros disk daemon.

Due to some infrastructure issues that we _just_ resolved, I haven't been able
to upload the MTPD code to Gerrit for review. For now, there's a copy in
/home/thestig/mtpd/platform/mtpd. In particular, the dbus protocol is described
by /home/thestig/mtpd/platform/mtpd/mtpd.xml.

I also don't have the dbus constants in system_api/dbus/service_constants.h yet,
so please excuse the mess.

https://chromiumcodereview.appspot.com/10825170/diff/4001/chromeos/dbus/mtpd_...
File chromeos/dbus/mtpd_client.cc (right):

https://chromiumcodereview.appspot.com/10825170/diff/4001/chromeos/dbus/mtpd_...
chromeos/dbus/mtpd_client.cc:310: std::vector<FileEntry> file_entries;
note: I'm not sure if I did this right. Please look carefully.

[hashimoto, 2012-08-03 04:48:42]

Are you willing to make this checked in before M22 branch point as well as
changes below?
 - add a mock client
 - add this client to DBusThreadManger
 - actual user code of MTPDClient

http://codereview.chromium.org/10825170/diff/4001/chromeos/dbus/mtpd_client.cc
File chromeos/dbus/mtpd_client.cc (right):

http://codereview.chromium.org/10825170/diff/4001/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:314: LOG(ERROR) << "Invalid response: " <<
response->ToString();
Run error_callback

[Lei Zhang, 2012-08-03 04:52:47]

On 2012/08/03 04:48:42, hashimoto wrote:
> Are you willing to make this checked in before M22 branch point as well as
> changes below?
>  - add a mock client
>  - add this client to DBusThreadManger
>  - actual user code of MTPDClient
> 
> http://codereview.chromium.org/10825170/diff/4001/chromeos/dbus/mtpd_client.cc
> File chromeos/dbus/mtpd_client.cc (right):
> 
>
http://codereview.chromium.org/10825170/diff/4001/chromeos/dbus/mtpd_client.c...
> chromeos/dbus/mtpd_client.cc:314: LOG(ERROR) << "Invalid response: " <<
> response->ToString();
> Run error_callback

Yes, I will certainly try to do all of the above.

[Lei Zhang, 2012-08-03 04:53:50]

Should I add a mock client and add this to DBusThreadManger in this CL?

http://codereview.chromium.org/10825170/diff/4001/chromeos/dbus/mtpd_client.cc
File chromeos/dbus/mtpd_client.cc (right):

http://codereview.chromium.org/10825170/diff/4001/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:314: LOG(ERROR) << "Invalid response: " <<
response->ToString();
On 2012/08/03 04:48:42, hashimoto wrote:
> Run error_callback

Done.

[hashimoto, 2012-08-03 05:07:12]

LGTM assuming TODOs are going to be resolved as soon as possible.

On 2012/08/03 04:53:50, Lei Zhang wrote:
> Should I add a mock client and add this to DBusThreadManger in this CL?
It's up to you.  Anyway, they will not be a large change.
> 
> http://codereview.chromium.org/10825170/diff/4001/chromeos/dbus/mtpd_client.cc
> File chromeos/dbus/mtpd_client.cc (right):
> 
>
http://codereview.chromium.org/10825170/diff/4001/chromeos/dbus/mtpd_client.c...
> chromeos/dbus/mtpd_client.cc:314: LOG(ERROR) << "Invalid response: " <<
> response->ToString();
> On 2012/08/03 04:48:42, hashimoto wrote:
> > Run error_callback
> 
> Done.

[satorux1, 2012-08-03 05:26:07]

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.cc
File chromeos/dbus/mtpd_client.cc (right):

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:68: "org.chromium.MTPD",
Constants like this should be defined in service_constants.h. Oh wait, there is
a TODO() above. then fine by me for now. You might want to file a bug and add a
link to it.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:137: virtual void ReadDirectoryByPath(
// MTPDClient override.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:154: virtual void ReadDirectoryById(
// MTPDClient override.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:443: // Initialize |this| from |response| given by
the mtpd service.
Initializes

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:535: file_type_ = FILE_TYPE_FOLDER;
You might want to just fall through for known types

case FILE_TYPE_FOLDER:
case FILE_TYPE_JPEG:
case FILE_TYPE_JFIF:
...
  file_type_ = file_type;
default:
  file_type_ = FILE_TYPE_OTHER

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h
File chromeos/dbus/mtpd_client.h (right):

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:3: // found in the LICENSE file.
Could you write some brief description about MTPD, and what this D-Bus client is
used for?

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:57: std::string storage_name() const { return
storage_name_; }
const std::string&

ditto for elsewhere.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:133: std::string filename() const { return
filename_; }
file_name() to be consistent with file_type()?

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:136: uint64 filesize() const { return filesize_; }
file_size()?

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:166: typedef base::Callback<void(const
std::vector<std::string>&)
please add parameter name

typedef base::Callback<void(const std::vector<std::string>& device_paths)

as well as other places

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:217: virtual void CloseStorage(const std::string&
handle,
what is |handle|? Could you document?

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:229: // call succeeds, otherwise, |error_callback|
is called.
what's |file_id|? I'm guessing this is FileEntry::item_id. Please document about
the parameter.

[satorux1, 2012-08-03 05:37:10]

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h
File chromeos/dbus/mtpd_client.h (right):

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:187: typedef base::Callback<void(const
std::string&)> ReadFileCallback;
btw, this may be bad for large files, like a 2GB movie file. You might want to
pass a file descriptor instead.

[satorux1, 2012-08-03 05:41:57]

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h
File chromeos/dbus/mtpd_client.h (right):

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:187: typedef base::Callback<void(const
std::string&)> ReadFileCallback;
On 2012/08/03 05:37:10, satorux1 wrote:
> btw, this may be bad for large files, like a 2GB movie file. You might want to
> pass a file descriptor instead.

Oh you could just add a TODO here. :)

[Lei Zhang, 2012-08-03 06:29:30]

I addressed the stype comments in cros_disks_client.* too.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.cc
File chromeos/dbus/mtpd_client.cc (right):

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:68: "org.chromium.MTPD",
On 2012/08/03 05:26:07, satorux1 wrote:
> Constants like this should be defined in service_constants.h. Oh wait, there
is
> a TODO() above. then fine by me for now. You might want to file a bug and add
a
> link to it.

I should have it fixed tomorrow once I get the constants into the .h file.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:137: virtual void ReadDirectoryByPath(
On 2012/08/03 05:26:07, satorux1 wrote:
> // MTPDClient override.

Done.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:154: virtual void ReadDirectoryById(
On 2012/08/03 05:26:07, satorux1 wrote:
> // MTPDClient override.

Done.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:443: // Initialize |this| from |response| given by
the mtpd service.
On 2012/08/03 05:26:07, satorux1 wrote:
> Initializes

Done.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:535: file_type_ = FILE_TYPE_FOLDER;
On 2012/08/03 05:26:07, satorux1 wrote:
> You might want to just fall through for known types
> 
> case FILE_TYPE_FOLDER:
> case FILE_TYPE_JPEG:
> case FILE_TYPE_JFIF:
> ...
>   file_type_ = file_type;
> default:
>   file_type_ = FILE_TYPE_OTHER

Good idea, thanks.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h
File chromeos/dbus/mtpd_client.h (right):

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:3: // found in the LICENSE file.
On 2012/08/03 05:26:07, satorux1 wrote:
> Could you write some brief description about MTPD, and what this D-Bus client
is
> used for?

Done.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:57: std::string storage_name() const { return
storage_name_; }
On 2012/08/03 05:26:07, satorux1 wrote:
> const std::string&
> 
> ditto for elsewhere.

Done.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:133: std::string filename() const { return
filename_; }
On 2012/08/03 05:26:07, satorux1 wrote:
> file_name() to be consistent with file_type()?

Done.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:136: uint64 filesize() const { return filesize_; }
On 2012/08/03 05:26:07, satorux1 wrote:
> file_size()?

Done.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:166: typedef base::Callback<void(const
std::vector<std::string>&)
On 2012/08/03 05:26:07, satorux1 wrote:
> please add parameter name
> 
> typedef base::Callback<void(const std::vector<std::string>& device_paths)
> 
> as well as other places

Done.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:187: typedef base::Callback<void(const
std::string&)> ReadFileCallback;
On 2012/08/03 05:37:10, satorux1 wrote:
> btw, this may be bad for large files, like a 2GB movie file. You might want to
> pass a file descriptor instead.

Yes, we have definitely considered this, but since we are limiting ourselves to
just photos for now, this is probably ok. I added a TODO.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:217: virtual void CloseStorage(const std::string&
handle,
On 2012/08/03 05:26:07, satorux1 wrote:
> what is |handle|? Could you document?

Done.

http://codereview.chromium.org/10825170/diff/9002/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:229: // call succeeds, otherwise, |error_callback|
is called.
On 2012/08/03 05:26:07, satorux1 wrote:
> what's |file_id|? I'm guessing this is FileEntry::item_id. Please document
about
> the parameter.

Added documentation, it's a MTP-ism.

[Lei Zhang, 2012-08-03 07:09:09]

With mock client in patch set 4. Actual user code will be a separate CL once I
figure things out. Not there yet.

Of course, this also assumes the MTPD dbus protocol is ok with benchan. I'll
know for sure once the server side code is reviewed and checked in.

[satorux1, 2012-08-03 17:38:42]

http://codereview.chromium.org/10825170/diff/3003/chromeos/dbus/mtpd_client.cc
File chromeos/dbus/mtpd_client.cc (right):

http://codereview.chromium.org/10825170/diff/3003/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:482:
MaybePopUint64(properties["FreeSpaceInObjects"], &free_space_in_objects_);
Here's an idea. As you've already learned, exchanging structured data as D-Bus
message is painful and results in crappy code.

What about using protocol buffers instead? It'd be much cleaner. 
PowerManagerClient does this, and I think this is the way to go.

http://codereview.chromium.org/10825170/diff/3003/chromeos/dbus/mtpd_client.h
File chromeos/dbus/mtpd_client.h (right):

http://codereview.chromium.org/10825170/diff/3003/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:164: class CHROMEOS_EXPORT MTPDClient {
MTPDClient looks rather cryptic. What about renaming this to MtpDaemonClient or
MediaTransferProtocolDaemonClient?

[Lei Zhang, 2012-08-03 18:44:15]

http://codereview.chromium.org/10825170/diff/3003/chromeos/dbus/mtpd_client.cc
File chromeos/dbus/mtpd_client.cc (right):

http://codereview.chromium.org/10825170/diff/3003/chromeos/dbus/mtpd_client.c...
chromeos/dbus/mtpd_client.cc:482:
MaybePopUint64(properties["FreeSpaceInObjects"], &free_space_in_objects_);
On 2012/08/03 17:38:42, satorux1 wrote:
> Here's an idea. As you've already learned, exchanging structured data as D-Bus
> message is painful and results in crappy code.
> 
> What about using protocol buffers instead? It'd be much cleaner. 
> PowerManagerClient does this, and I think this is the way to go.

Maybe in a later CL? I added a TODO.

http://codereview.chromium.org/10825170/diff/3003/chromeos/dbus/mtpd_client.h
File chromeos/dbus/mtpd_client.h (right):

http://codereview.chromium.org/10825170/diff/3003/chromeos/dbus/mtpd_client.h...
chromeos/dbus/mtpd_client.h:164: class CHROMEOS_EXPORT MTPDClient {
On 2012/08/03 17:38:42, satorux1 wrote:
> MTPDClient looks rather cryptic. What about renaming this to MtpDaemonClient
or
> MediaTransferProtocolDaemonClient?

MediaTransferProtocolDaemonClient it is. (I've gotten a few "what's MTP?"
questions)

[satorux1, 2012-08-03 20:22:42]

LGMT. The new name looks good!

Please update the patch description

[satorux1, 2012-08-03 20:22:52]

LGTM...

[commit-bot: I haz the power, 2012-08-06 21:59:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/10825170/1007

[commit-bot: I haz the power, 2012-08-06 21:59:19]

Presubmit check for 10825170-1007 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chromeos/dbus/mock_media_transfer_protocol_daemon_client.cc, line 9, 92 chars
\
  chromeos/dbus/mock_media_transfer_protocol_daemon_client.cc, line 11, 93 chars

Presubmit checks took 1.3s to calculate.