ChromeOS: Implement MediaTransferProtocolManager.

ChromeOS: Implement MediaTransferProtocolManager.

BUG=chromium-os:29557
TEST=none

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

[Lei Zhang, 2012-08-08 21:53:29]

I can't tell if this really works until we try and hook it up. Leaving this in
an unreviewed state so we can play with it.

[Lei Zhang, 2012-08-29 05:55:44]

Very similiar to cros_disk_manager.

[tbarzic, 2012-08-29 19:52:27]

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
File chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc (right):

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc:61:
std::vector<std::string> ret;
what do you think about "result" instead of "ret"

it took me few seconds to get what ret means :)

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc:72: StorageInfo*
info) const OVERRIDE {
Personally, I'd prefer to return Storage info const pointer instead of bool (and
return NULL in case of error). At least assuming |info| will be used as const.

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc:90:
open_storage_callbacks_.push(callback);
I guess we can assume that open storage callbacks are received in order
OpenStorage requests are received.. maybe you can document that here (or where
queues are declared).

is there a reason we don't just pass callback as parameter to
OnOpenStorage/OnOpenStorageError (other than reducing number of copies)

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc:283: // should be
for new storages, or storage detachments.
I don't see this being called on storage detachments (@ the end of the comment)

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc:368: // Map to
keep track of attached storages.
"Map to keep track of attached storaged by name."

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
File chrome/browser/chromeos/mtp/media_transfer_protocol_manager.h (right):

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.h:60: virtual
~MediaTransferProtocolManager() {}
should ctor/dtor be protected (afaik they should only be used in
MediaTransferProtocolManager)

[Lei Zhang, 2012-08-30 01:32:00]

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
File chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc (right):

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc:61:
std::vector<std::string> ret;
On 2012/08/29 19:52:27, tbarzic wrote:
> what do you think about "result" instead of "ret"
> 
> it took me few seconds to get what ret means :)

s/ret/storages/

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc:72: StorageInfo*
info) const OVERRIDE {
On 2012/08/29 19:52:27, tbarzic wrote:
> Personally, I'd prefer to return Storage info const pointer instead of bool
(and
> return NULL in case of error). At least assuming |info| will be used as const.

Done.

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc:90:
open_storage_callbacks_.push(callback);
On 2012/08/29 19:52:27, tbarzic wrote:
> I guess we can assume that open storage callbacks are received in order
> OpenStorage requests are received.. maybe you can document that here (or where
> queues are declared).

Done. Where the queue's are typedef'd.

> is there a reason we don't just pass callback as parameter to
> OnOpenStorage/OnOpenStorageError (other than reducing number of copies) 

It would mean the next level down
(chromeos/dbus/media_transfer_protocol_daemon_client.h) has to have callbacks
that have another callback as a parameter. I'd like to avoid going deeper if
possible.

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc:283: // should be
for new storages, or storage detachments.
On 2012/08/29 19:52:27, tbarzic wrote:
> I don't see this being called on storage detachments (@ the end of the
comment)

There's no need to call OnGetStorageInfo() during detachment. I'm not sure what
your concern is exactly, so I tried to write more comments here to clarify why
this condition is a NOTREACHED().

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc:368: // Map to
keep track of attached storages.
On 2012/08/29 19:52:27, tbarzic wrote:
> "Map to keep track of attached storaged by name."

Done.

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
File chrome/browser/chromeos/mtp/media_transfer_protocol_manager.h (right):

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.h:60: virtual
~MediaTransferProtocolManager() {}
On 2012/08/29 19:52:27, tbarzic wrote:
> should ctor/dtor be protected (afaik they should only be used in
> MediaTransferProtocolManager)

There are lots of pure classes that have a public ctor. e.g. RenderViewHost and
WebContents.

[toni.barzic, 2012-08-30 01:46:34]

lgtm

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
File chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc (right):

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc:90:
open_storage_callbacks_.push(callback);
On 2012/08/30 01:32:00, Lei Zhang wrote:
> On 2012/08/29 19:52:27, tbarzic wrote:
> > I guess we can assume that open storage callbacks are received in order
> > OpenStorage requests are received.. maybe you can document that here (or
where
> > queues are declared).
> 
> Done. Where the queue's are typedef'd.
> 
> > is there a reason we don't just pass callback as parameter to
> > OnOpenStorage/OnOpenStorageError (other than reducing number of copies) 
> 
> It would mean the next level down
> (chromeos/dbus/media_transfer_protocol_daemon_client.h) has to have callbacks
> that have another callback as a parameter. I'd like to avoid going deeper if
> possible.

yeah, makes sense..

http://codereview.chromium.org/10854048/diff/16001/chrome/browser/chromeos/mt...
chrome/browser/chromeos/mtp/media_transfer_protocol_manager.cc:283: // should be
for new storages, or storage detachments.
On 2012/08/30 01:32:00, Lei Zhang wrote:
> On 2012/08/29 19:52:27, tbarzic wrote:
> > I don't see this being called on storage detachments (@ the end of the
> comment)
> 
> There's no need to call OnGetStorageInfo() during detachment. I'm not sure
what
> your concern is exactly, so I tried to write more comments here to clarify why
> this condition is a NOTREACHED().

My comment was about not necessary needing "or storage detachments" in the
comment since OnGetStorageInfo is never called when storage is detached.. sorry
for not being clear.. 
I probably partially missed the core point of that part of the comment...

[tbarzic, 2012-08-30 01:47:41]

lgtm.. from the right acc

[commit-bot: I haz the power, 2012-08-30 22:38:05]

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

[commit-bot: I haz the power, 2012-08-31 03:27:53]

Change committed as 154365