CrOS: Convert MediaTransferProtocolDaemonClient to use protobufs.

CrOS: Convert MediaTransferProtocolDaemonClient to use protobufs.

BUG=chromium-os:29557
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156468

[Lei Zhang, 2012-09-01 00:46:34]

https://chromiumcodereview.appspot.com/10913048/diff/1/chromeos/cryptohome/as...
File chromeos/cryptohome/async_method_caller.h (right):

https://chromiumcodereview.appspot.com/10913048/diff/1/chromeos/cryptohome/as...
chromeos/cryptohome/async_method_caller.h:10: #include "base/basictypes.h"
Otherwise the header fails to compile due to the uint32 in service_constants.h.
Alternatively, include basictypes.h. in service_constants.h or #define the
uint32 value.

https://chromiumcodereview.appspot.com/10913048/diff/1/chromeos/dbus/media_tr...
File chromeos/dbus/media_transfer_protocol_daemon_client.h (left):

https://chromiumcodereview.appspot.com/10913048/diff/1/chromeos/dbus/media_tr...
chromeos/dbus/media_transfer_protocol_daemon_client.h:35: enum FileType {
I hate to have this enum to match the one in the protobuf, to match the one in
libmtp. However, if I get rid of this, then the chromeos_test_support target
will depend on the mtp_file_entry_proto target.

[Lei Zhang, 2012-09-01 00:47:38]

mtpd changes are here: https://gerrit.chromium.org/gerrit/#/c/32059/

[satorux1, 2012-09-01 01:03:50]

http://codereview.chromium.org/10913048/diff/1/DEPS
File DEPS (right):

http://codereview.chromium.org/10913048/diff/1/DEPS#newcode519
DEPS:519: "@5795e9bd3edb8a979318a08a79abd1bb1329c864",
It's safer to land this part in a separate patch. please create a separate patch

http://codereview.chromium.org/10913048/diff/1/chromeos/dbus/media_transfer_p...
File chromeos/dbus/media_transfer_protocol_daemon_client.cc (right):

http://codereview.chromium.org/10913048/diff/1/chromeos/dbus/media_transfer_p...
chromeos/dbus/media_transfer_protocol_daemon_client.cc:465:
volume_identifier_(storage_info.volume_identifier()) {
do you need to copy the proto to the class? Wouldn't it be simpler to use the
proto directly?

http://codereview.chromium.org/10913048/diff/1/chromeos/dbus/media_transfer_p...
chromeos/dbus/media_transfer_protocol_daemon_client.cc:487:
file_type_(entry.file_type()) {
ditto. Do you need FileEntry class? Can you use MtpFileEntry directly instead?

http://codereview.chromium.org/10913048/diff/1/chromeos/dbus/media_transfer_p...
chromeos/dbus/media_transfer_protocol_daemon_client.cc:510: }  // namespace
chromeos
139 lines removed. nice!

http://codereview.chromium.org/10913048/diff/1/chromeos/dbus/media_transfer_p...
File chromeos/dbus/media_transfer_protocol_daemon_client.h (left):

http://codereview.chromium.org/10913048/diff/1/chromeos/dbus/media_transfer_p...
chromeos/dbus/media_transfer_protocol_daemon_client.h:35: enum FileType {
On 2012/09/01 00:46:34, Lei Zhang wrote:
> I hate to have this enum to match the one in the protobuf, to match the one in
> libmtp. However, if I get rid of this, then the chromeos_test_support target
> will depend on the mtp_file_entry_proto target.

that sounds ok to me. mtp_file_entry_proto target is small.

[Lei Zhang, 2012-09-04 20:09:38]

http://codereview.chromium.org/10913048/diff/1/DEPS
File DEPS (right):

http://codereview.chromium.org/10913048/diff/1/DEPS#newcode519
DEPS:519: "@5795e9bd3edb8a979318a08a79abd1bb1329c864",
On 2012/09/01 01:03:50, satorux1 wrote:
> It's safer to land this part in a separate patch. please create a separate
patch

Sure, can do.

http://codereview.chromium.org/10913048/diff/1/chromeos/dbus/media_transfer_p...
File chromeos/dbus/media_transfer_protocol_daemon_client.cc (right):

http://codereview.chromium.org/10913048/diff/1/chromeos/dbus/media_transfer_p...
chromeos/dbus/media_transfer_protocol_daemon_client.cc:465:
volume_identifier_(storage_info.volume_identifier()) {
On 2012/09/01 01:03:50, satorux1 wrote:
> do you need to copy the proto to the class? Wouldn't it be simpler to use the
> proto directly?

Ya, we can. I just need to add the "storage name" to the protobuf.

http://codereview.chromium.org/10913048/diff/1/chromeos/dbus/media_transfer_p...
chromeos/dbus/media_transfer_protocol_daemon_client.cc:487:
file_type_(entry.file_type()) {
On 2012/09/01 01:03:50, satorux1 wrote:
> ditto. Do you need FileEntry class? Can you use MtpFileEntry directly instead?

Done.

[satorux1, 2012-09-06 17:20:05]

LGTM. Sorry for the belated response. I'm back to work.

http://codereview.chromium.org/10913048/diff/11001/chromeos/dbus/media_transf...
File chromeos/dbus/media_transfer_protocol_daemon_client.cc (right):

http://codereview.chromium.org/10913048/diff/11001/chromeos/dbus/media_transf...
chromeos/dbus/media_transfer_protocol_daemon_client.cc:453: }  // namespace
chromeos
almost 200 lines gone!

http://codereview.chromium.org/10913048/diff/11001/chromeos/dbus/media_transf...
File chromeos/dbus/media_transfer_protocol_daemon_client.h (right):

http://codereview.chromium.org/10913048/diff/11001/chromeos/dbus/media_transf...
chromeos/dbus/media_transfer_protocol_daemon_client.h:176: #endif  //
CHROMEOS_DBUS_MEDIA_TRANSFER_PROTOCOL_DAEMON_CLIENT_H_
129 lines reduced!

[commit-bot: I haz the power, 2012-09-10 21:51:53]

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

[commit-bot: I haz the power, 2012-09-10 22:24:51]

Try job failure for 10913048-7008 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-09-10 23:19:53]

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

[commit-bot: I haz the power, 2012-09-10 23:52:35]

Try job failure for 10913048-7008 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-09-10 23:59:35]

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

[commit-bot: I haz the power, 2012-09-11 00:28:47]

Try job failure for 10913048-7008 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Lei Zhang, 2012-09-11 05:23:45]

+akalin since he might be interested in the protoc + gyp change.
+rsleevi,evan FYI

The try jobs would have worked if the try server understood. svn:executable.
(gah!) Works for me locally with the linux_chromeos bot's settings.

[Lei Zhang, 2012-09-11 07:12:06]

(per discussion with rsleevi)

We could just modify
third_party/protobuf/src/google/protobuf/compiler/cpp/cpp_generator.cc instead,
but I'm worried that will take too long to upstream. rsleevi also mentioned
other projects 'normally' #define the declspec, so changes to cpp_generator.cc
may get rejected.

[Evan Martin, 2012-09-11 16:07:31]

If it helps any, sometimes you can land the .py file first (and mark
it +x), then do a follow-up change that makes use of the file (and
that will work on the trybot).

On Mon, Sep 10, 2012 at 10:23 PM,  <thestig@chromium.org> wrote:
> +akalin since he might be interested in the protoc + gyp change.
> +rsleevi,evan FYI
>
> The try jobs would have worked if the try server understood. svn:executable.
> (gah!) Works for me locally with the linux_chromeos bot's settings.
>
> https://chromiumcodereview.appspot.com/10913048/

[Evan Martin, 2012-09-11 16:11:09]

just because i opened the review already

https://chromiumcodereview.appspot.com/10913048/diff/7015/tools/protoc_wrappe...
File tools/protoc_wrapper/protoc_wrapper.py (right):

https://chromiumcodereview.appspot.com/10913048/diff/7015/tools/protoc_wrappe...
tools/protoc_wrapper/protoc_wrapper.py:27: for i in xrange(0,
len(header_contents)):
Constructing the list of lines as you iterate through the source file eliminates
the above read/split and the array indexing found in this loop.

contents = []
with open(header_file) as f:
  for line in f:
    if line == PROTOC_INCLUDE_POINT:
      contents.append(...)
      include_point_found = True
    else:
      contents.append(line)

[Lei Zhang, 2012-09-11 21:03:41]

Thanks for the feedback, my python-fu has gotten rusty.

https://chromiumcodereview.appspot.com/10913048/diff/7015/tools/protoc_wrappe...
File tools/protoc_wrapper/protoc_wrapper.py (right):

https://chromiumcodereview.appspot.com/10913048/diff/7015/tools/protoc_wrappe...
tools/protoc_wrapper/protoc_wrapper.py:27: for i in xrange(0,
len(header_contents)):
On 2012/09/11 16:11:09, Evan Martin wrote:
> Constructing the list of lines as you iterate through the source file
eliminates
> the above read/split and the array indexing found in this loop.
> 
> contents = []
> with open(header_file) as f:
>   for line in f:
>     if line == PROTOC_INCLUDE_POINT:
>       contents.append(...)
>       include_point_found = True
>     else:
>       contents.append(line)

Done.

[Evan Martin, 2012-09-11 21:12:26]

well, now that I'm reviewing, I may as well add some more comments...

https://chromiumcodereview.appspot.com/10913048/diff/13020/tools/protoc_wrapp...
File tools/protoc_wrapper/protoc_wrapper.py (right):

https://chromiumcodereview.appspot.com/10913048/diff/13020/tools/protoc_wrapp...
tools/protoc_wrapper/protoc_wrapper.py:13: def modifyHeader(header_file,
extra_header):
ModifyHeader

https://chromiumcodereview.appspot.com/10913048/diff/13020/tools/protoc_wrapp...
tools/protoc_wrapper/protoc_wrapper.py:29: f.close()
the "with..." bit already does this

https://chromiumcodereview.appspot.com/10913048/diff/13020/tools/protoc_wrapp...
tools/protoc_wrapper/protoc_wrapper.py:30: except:
this catch will catch more than you expect, like for example typos in variable
names.  probably better to just not catch exceptions at all and let python exit
with its own error code on failure

https://chromiumcodereview.appspot.com/10913048/diff/13020/tools/protoc_wrapp...
tools/protoc_wrapper/protoc_wrapper.py:39: f.close()
close not needed here too

[Lei Zhang, 2012-09-11 21:51:25]

I also fixed some newline issues so the modified protobuf header doesn't come
out fugly.

https://chromiumcodereview.appspot.com/10913048/diff/13020/tools/protoc_wrapp...
File tools/protoc_wrapper/protoc_wrapper.py (right):

https://chromiumcodereview.appspot.com/10913048/diff/13020/tools/protoc_wrapp...
tools/protoc_wrapper/protoc_wrapper.py:13: def modifyHeader(header_file,
extra_header):
On 2012/09/11 21:12:26, Evan Martin wrote:
> ModifyHeader

Done.

https://chromiumcodereview.appspot.com/10913048/diff/13020/tools/protoc_wrapp...
tools/protoc_wrapper/protoc_wrapper.py:29: f.close()
On 2012/09/11 21:12:26, Evan Martin wrote:
> the "with..." bit already does this

Good to know, removed.

https://chromiumcodereview.appspot.com/10913048/diff/13020/tools/protoc_wrapp...
tools/protoc_wrapper/protoc_wrapper.py:30: except:
On 2012/09/11 21:12:26, Evan Martin wrote:
> this catch will catch more than you expect, like for example typos in variable
> names.  probably better to just not catch exceptions at all and let python
exit
> with its own error code on failure

Done.

https://chromiumcodereview.appspot.com/10913048/diff/13020/tools/protoc_wrapp...
tools/protoc_wrapper/protoc_wrapper.py:39: f.close()
On 2012/09/11 21:12:26, Evan Martin wrote:
> close not needed here too

Done.

[Ryan Sleevi, 2012-09-12 19:47:31]

So my biggest concern is the use of ':' to concatenate the arguments. I think it
would be better if the wrapper script used 'real' argument processing (eg:
[opt|arg]parse + a callback to handle trailing args) so that it's not dependent
on a 'magic' value being preserved across file systems and build tools.

http://codereview.chromium.org/10913048/diff/6029/build/protoc.gypi
File build/protoc.gypi (right):

http://codereview.chromium.org/10913048/diff/6029/build/protoc.gypi#newcode46
build/protoc.gypi:46: 'cc_extra%': '',
Please add documentation about cc_extra and cc_include to the above comment.

I'd also suggest renaming cc_extra to be more indicative of its purpose (unless
this is what protoc also calls it?)

http://codereview.chromium.org/10913048/diff/6029/build/protoc.gypi#newcode66
build/protoc.gypi:66: '<(cc_include)' ':' '<(cc_dir)/<(RULE_INPUT_ROOT).pb.h',
If no 'cc_include' is specified, this expands to an argument of :foo/bar.pb.h

Unfortunately, because this is a GYPI that is included by multiple targets, it's
not "cleanly" possible to use nested variables to create some 'cc_include_flag'
meta-variable.

Further, Nico's been working on supporting ':' in filenames in Ninja, so I'm not
sure using ':' as a delimiter is wise.

Finally, concatenating strings like GYP in this way is not really a 'supported'
or 'good' way, it just happens to work as a side-effect of how Python's parsing
things. The correct form would be:
'<(cc_include):<(cc_dir)/<(RULE_INPUT_ROOT).pb.h' (although, like I said, using
':' isn't that good here. This string concatenation note also applies to line
74.

My recommendation, for those reasons, is to make the cc_include and the pb.h
flagged arguments to protoc_wrapper. eg:

--include_file (path_or_null)
--protobuf_file (path)
-- (used by the Python script to say 'ignore all that follows')
(protoc command to execute and remaining args)

[Lei Zhang, 2012-09-12 21:30:50]

rsleevi: PTAL at patch set 8.

[Ryan Sleevi, 2012-09-12 22:21:25]

LGTM with nits and one BUG? mentioned below

http://codereview.chromium.org/10913048/diff/8009/build/protoc.gypi
File build/protoc.gypi (right):

http://codereview.chromium.org/10913048/diff/8009/build/protoc.gypi#newcode46
build/protoc.gypi:46: # Declspec to add to the generated protobuf c++ headers.
nit: declspec is Windows only, but presumably the dllexport_decl can be used
with Mac/Linux (eg: __attribute__((visibility, "default")) or whatever)

cc_generator_options seems to match what protoc calls it

http://codereview.chromium.org/10913048/diff/8009/build/protoc.gypi#newcode50
build/protoc.gypi:50: # e.g. 'base/base_export.h'
nit: For consistency with the proto_in_dir/proto_out_dir documentation, seems
like this should be at line 28

http://codereview.chromium.org/10913048/diff/8009/build/protoc.gypi#newcode96
build/protoc.gypi:96: '<(DEPTH)',
BUG? Seems like this should also be propagated in direct_dependent_settings
(line 101), since anyone who includes the .pb.h will also need to include
whatever cc_declspec_include requires

http://codereview.chromium.org/10913048/diff/8009/chrome/browser/chromeos/mtp...
File chrome/browser/chromeos/mtp/media_transfer_protocol_manager.h (right):

http://codereview.chromium.org/10913048/diff/8009/chrome/browser/chromeos/mtp...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.h:33: typedef
base::Callback<void(const std::vector<MtpFileEntry>& file_entries,
IWYU: Looks like you should be forward declaring these types ~ line 15 (you are
relying on _daemon_client.h to do so)

[Lei Zhang, 2012-09-12 23:33:59]

http://codereview.chromium.org/10913048/diff/8009/build/protoc.gypi
File build/protoc.gypi (right):

http://codereview.chromium.org/10913048/diff/8009/build/protoc.gypi#newcode46
build/protoc.gypi:46: # Declspec to add to the generated protobuf c++ headers.
On 2012/09/12 22:21:25, Ryan Sleevi wrote:
> nit: declspec is Windows only, but presumably the dllexport_decl can be used
> with Mac/Linux (eg: __attribute__((visibility, "default")) or whatever)
> 
> cc_generator_options seems to match what protoc calls it

Done.

http://codereview.chromium.org/10913048/diff/8009/build/protoc.gypi#newcode50
build/protoc.gypi:50: # e.g. 'base/base_export.h'
On 2012/09/12 22:21:25, Ryan Sleevi wrote:
> nit: For consistency with the proto_in_dir/proto_out_dir documentation, seems
> like this should be at line 28

Done.

http://codereview.chromium.org/10913048/diff/8009/build/protoc.gypi#newcode96
build/protoc.gypi:96: '<(DEPTH)',
On 2012/09/12 22:21:25, Ryan Sleevi wrote:
> BUG? Seems like this should also be propagated in direct_dependent_settings
> (line 101), since anyone who includes the .pb.h will also need to include
> whatever cc_declspec_include requires

Done.

http://codereview.chromium.org/10913048/diff/8009/chrome/browser/chromeos/mtp...
File chrome/browser/chromeos/mtp/media_transfer_protocol_manager.h (right):

http://codereview.chromium.org/10913048/diff/8009/chrome/browser/chromeos/mtp...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.h:33: typedef
base::Callback<void(const std::vector<MtpFileEntry>& file_entries,
On 2012/09/12 22:21:25, Ryan Sleevi wrote:
> IWYU: Looks like you should be forward declaring these types ~ line 15 (you
are
> relying on _daemon_client.h to do so)

Done.

[commit-bot: I haz the power, 2012-09-12 23:34:37]

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

[commit-bot: I haz the power, 2012-09-13 02:27:56]

Change committed as 156468