ChromeOS: Implement MediaTransferProtocolManager observer.

ChromeOS: Implement MediaTransferProtocolManager observer to receive media storage notifications.

Depends on https://chromiumcodereview.appspot.com/10854048/

BUG=144527
TEST=none

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

[Lei Zhang, 2012-08-29 23:11:14]

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
File chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc (right):

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc:19: namespace mtp{
nit: missing space

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc:21: using
base::SystemMonitor;
nit: You don't really need these. They're only used once.

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc:30:
MediaTransferProtocolManager* mtp_device_manager() {
nit: foo_function() usually means "this function is really cheap to call", and
FooFunction() means it is not cheap. So foo_function() calling BarFunction()
means it should be named FooFunction().

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc:48: std::string
unique_id;
If you move this and the label block into their own helper functions, then
GetDeviceInfo will be only 15 lines and even easier to understand.

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc:49: const std::string&
vendor_id = base::UintToString(storage_info.vendor_id());
UintToString return values should not be treated as references.

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc:92: :
get_storage_info_func_(&GetDeviceInfo) {
GetDeviceInfo -> GetStorageInfo
or
get_storage_info_func_ -> get_device_info_func_

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
File chrome/browser/chromeos/mtp/mtp_dev_manager_observer.h (right):

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.h:11: #include
"base/string16.h"
nit: alphabetical order

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.h:21: std::string* id,
nit: indentation

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.h:27: class
MtpDevManagerObserver
Can you be consistent with the name and call this
MediaTransferProtocolDeviceObserver?

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.h:28: : public
base::RefCountedThreadSafe<MtpDevManagerObserver>,
Why does this need to be RefCountedThreadSafe?

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
File chrome/browser/chromeos/mtp/mtp_dev_manager_observer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer_unittest.cc:43: if
(storage_name == kStorageName) {
if (storage_name != kStorageName) {
  NOTREACHED();
  return;
}
...

[kmadhusu, 2012-08-30 02:07:50]

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
File chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc (right):

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc:19: namespace mtp{
On 2012/08/29 23:11:15, Lei Zhang wrote:
> nit: missing space

Done.

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc:21: using
base::SystemMonitor;
On 2012/08/29 23:11:15, Lei Zhang wrote:
> nit: You don't really need these. They're only used once.

Done.

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc:30:
MediaTransferProtocolManager* mtp_device_manager() {
On 2012/08/29 23:11:15, Lei Zhang wrote:
> nit: foo_function() usually means "this function is really cheap to call", and
> FooFunction() means it is not cheap. So foo_function() calling BarFunction()
> means it should be named FooFunction().

Done.

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc:48: std::string
unique_id;
On 2012/08/29 23:11:15, Lei Zhang wrote:
> If you move this and the label block into their own helper functions, then
> GetDeviceInfo will be only 15 lines and even easier to understand.

Done.

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc:49: const std::string&
vendor_id = base::UintToString(storage_info.vendor_id());
On 2012/08/29 23:11:15, Lei Zhang wrote:
> UintToString return values should not be treated as references.

Done.

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.cc:92: :
get_storage_info_func_(&GetDeviceInfo) {
On 2012/08/29 23:11:15, Lei Zhang wrote:
> GetDeviceInfo -> GetStorageInfo
> or
> get_storage_info_func_ -> get_device_info_func_

GetDeviceInfo => GetStorageInfo

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
File chrome/browser/chromeos/mtp/mtp_dev_manager_observer.h (right):

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.h:11: #include
"base/string16.h"
On 2012/08/29 23:11:15, Lei Zhang wrote:
> nit: alphabetical order

Done.

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.h:21: std::string* id,
On 2012/08/29 23:11:15, Lei Zhang wrote:
> nit: indentation

Done.

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.h:27: class
MtpDevManagerObserver
On 2012/08/29 23:11:15, Lei Zhang wrote:
> Can you be consistent with the name and call this
> MediaTransferProtocolDeviceObserver?

Done.

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer.h:28: : public
base::RefCountedThreadSafe<MtpDevManagerObserver>,
On 2012/08/29 23:11:15, Lei Zhang wrote:
> Why does this need to be RefCountedThreadSafe?

RefCounting is not required here. Fixed.

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
File chrome/browser/chromeos/mtp/mtp_dev_manager_observer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10894045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/mtp/mtp_dev_manager_observer_unittest.cc:43: if
(storage_name == kStorageName) {
On 2012/08/29 23:11:15, Lei Zhang wrote:
> if (storage_name != kStorageName) {
>   NOTREACHED();
>   return;
> }
> ...

Done.

[Lei Zhang, 2012-08-30 02:30:19]

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
File chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer.cc
(right):

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer.cc:58: //
Helper functio to get device label from storage information.
nit: typo

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer.cc:62: if
(!vendor_name.empty())
You can get rid of this check. You'll do it again on line 67 anyway.

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer.cc:95:
*location = std::string(kRootPath) + storage_name;
You probably don't need to construct a string for |kRootPath| here? Ditto on
line 52.

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer.cc:139: if
(device_id.empty() || device_name.empty())
So we are assuming all mtp/ptp devices will always fill these in?

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer.cc:150:
return;
Should this ever happen?

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
File
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer_unittest.cc:12:
#include "base/memory/scoped_ptr.h"
nit: alphabetical order

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer_unittest.cc:54:
}  // namespace
You can put the entire test in the anonymous namespace too. There's no reason
for it to be outside.

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer_unittest.cc:122:
// Attach a mtp storage.
I would put this above the EXPECT_CALL, and get rid of the newline between
EXPECT_CALL and MtpStorageFoo(), so it's clear where the code for a particular
test starts and ends.

[kmadhusu, 2012-08-30 03:12:57]

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
File chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer.cc
(right):

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer.cc:58: //
Helper functio to get device label from storage information.
On 2012/08/30 02:30:20, Lei Zhang wrote:
> nit: typo

Done.

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer.cc:62: if
(!vendor_name.empty())
On 2012/08/30 02:30:20, Lei Zhang wrote:
> You can get rid of this check. You'll do it again on line 67 anyway.

Done.

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer.cc:95:
*location = std::string(kRootPath) + storage_name;
On 2012/08/30 02:30:20, Lei Zhang wrote:
> You probably don't need to construct a string for |kRootPath| here? Ditto on
> line 52.

Done.

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer.cc:139: if
(device_id.empty() || device_name.empty())
On 2012/08/30 02:30:20, Lei Zhang wrote:
> So we are assuming all mtp/ptp devices will always fill these in?

We are not assuming that. If they don't have valid id or name, we are going to
return from this function without sending any device attach notification
message.

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer.cc:150:
return;
On 2012/08/30 02:30:20, Lei Zhang wrote:
> Should this ever happen?

Very unlikely. Just to be on the safer side, I had the if block.

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
File
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer_unittest.cc:12:
#include "base/memory/scoped_ptr.h"
On 2012/08/30 02:30:20, Lei Zhang wrote:
> nit: alphabetical order

oops.. Fixed.

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer_unittest.cc:54:
}  // namespace
On 2012/08/30 02:30:20, Lei Zhang wrote:
> You can put the entire test in the anonymous namespace too. There's no reason
> for it to be outside.

Done.

https://chromiumcodereview.appspot.com/10894045/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/mtp/media_transfer_protocol_device_observer_unittest.cc:122:
// Attach a mtp storage.
On 2012/08/30 02:30:20, Lei Zhang wrote:
> I would put this above the EXPECT_CALL, and get rid of the newline between
> EXPECT_CALL and MtpStorageFoo(), so it's clear where the code for a particular
> test starts and ends.

Done.

[Lei Zhang, 2012-08-30 04:52:46]

lgtm

+zel for a second look from someone more familiar with CrOS. FYI, this depends
on:
https://chromiumcodereview.appspot.com/10868038/
and
https://chromiumcodereview.appspot.com/10854048/

[kmadhusu, 2012-08-31 20:14:31]

zel@: ping. PTAL.
thestig@: Rebased my changes and uploaded a new patch. PTAL.

Thanks.

[Lei Zhang, 2012-08-31 20:17:47]

still lgtm

[kmadhusu, 2012-09-08 03:15:14]

thestig@: 

(1) Moved media_transfer_protocol_device_observer code to
chrome/browser/system_monitor/chromeos location. 
(2) Modified GetDeviceIdFromStorageInfo to handle multiple storage paritions.

PTAL.
Thanks.

https://chromiumcodereview.appspot.com/10894045/diff/14010/chrome/browser/sys...
File
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer.cc
(right):

https://chromiumcodereview.appspot.com/10894045/diff/14010/chrome/browser/sys...
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer.cc:39:
std::string unique_id(chrome::kFSUniqueIdPrefix + storage_name);
This will help us to distinguish the storage partitions of any media device.

[Lei Zhang, 2012-09-10 18:18:01]

media_transfer_protocol_device_observer.h and friends can probably just go in to
chrome/browser/system_monitor.

https://chromiumcodereview.appspot.com/10894045/diff/24003/chrome/browser/sys...
File
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer.cc
(right):

https://chromiumcodereview.appspot.com/10894045/diff/24003/chrome/browser/sys...
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer.cc:36:
std::string GetDeviceIdFromStorageInfo(const MtpStorageInfo& storage_info,
|storage_info| is not used anymore. Function name is also out of date.

https://chromiumcodereview.appspot.com/10894045/diff/24003/chrome/browser/sys...
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer.cc:98:
: get_storage_info_func_(get_storage_info_func) {
It's weird how this doesn't call AddObserver(), whereas the other other ctor and
dtor does.

https://chromiumcodereview.appspot.com/10894045/diff/24003/chrome/browser/sys...
File
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer.h
(right):

https://chromiumcodereview.appspot.com/10894045/diff/24003/chrome/browser/sys...
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer.h:31:
// Only used in unit tests.
You can probably mark this and StorageChanged() as protected, and just write a
simple TestMediaTransferProtocolDeviceObserver in the unit test.

https://chromiumcodereview.appspot.com/10894045/diff/24003/chrome/browser/sys...
File
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10894045/diff/24003/chrome/browser/sys...
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer_unittest.cc:11:
#include "base/message_loop.h"
nit: alphabetical order

https://chromiumcodereview.appspot.com/10894045/diff/24003/chrome/chrome_brow...
File chrome/chrome_browser_chromeos.gypi (right):

https://chromiumcodereview.appspot.com/10894045/diff/24003/chrome/chrome_brow...
chrome/chrome_browser_chromeos.gypi:654:
'browser/system_monitor/chromeos/media_transfer_protocol_device_observer.cc',
Please add this to the right gypi file.

[kmadhusu, 2012-09-11 01:50:18]

Addressed review comments and moved media_transfer_* from
chrome/browser/system_monitor/chromeos to chrome/browser/system_monitor.

PTAL.
Thanks.

http://codereview.chromium.org/10894045/diff/24003/chrome/browser/system_moni...
File
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer.cc
(right):

http://codereview.chromium.org/10894045/diff/24003/chrome/browser/system_moni...
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer.cc:36:
std::string GetDeviceIdFromStorageInfo(const MtpStorageInfo& storage_info,
On 2012/09/10 18:18:01, Lei Zhang wrote:
> |storage_info| is not used anymore. Function name is also out of date.

oops. Fixed. Thanks for catching this.

http://codereview.chromium.org/10894045/diff/24003/chrome/browser/system_moni...
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer.cc:98:
: get_storage_info_func_(get_storage_info_func) {
On 2012/09/10 18:18:01, Lei Zhang wrote:
> It's weird how this doesn't call AddObserver(), whereas the other other ctor
and
> dtor does.

Added DCHECK and a comment in the function body.

http://codereview.chromium.org/10894045/diff/24003/chrome/browser/system_moni...
File
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer.h
(right):

http://codereview.chromium.org/10894045/diff/24003/chrome/browser/system_moni...
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer.h:31:
// Only used in unit tests.
On 2012/09/10 18:18:01, Lei Zhang wrote:
> You can probably mark this and StorageChanged() as protected, and just write a
> simple TestMediaTransferProtocolDeviceObserver in the unit test.

Done.

http://codereview.chromium.org/10894045/diff/24003/chrome/browser/system_moni...
File
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer_unittest.cc
(right):

http://codereview.chromium.org/10894045/diff/24003/chrome/browser/system_moni...
chrome/browser/system_monitor/chromeos/media_transfer_protocol_device_observer_unittest.cc:11:
#include "base/message_loop.h"
On 2012/09/10 18:18:01, Lei Zhang wrote:
> nit: alphabetical order

Done.

http://codereview.chromium.org/10894045/diff/24003/chrome/chrome_browser_chro...
File chrome/chrome_browser_chromeos.gypi (right):

http://codereview.chromium.org/10894045/diff/24003/chrome/chrome_browser_chro...
chrome/chrome_browser_chromeos.gypi:654:
'browser/system_monitor/chromeos/media_transfer_protocol_device_observer.cc',
On 2012/09/10 18:18:01, Lei Zhang wrote:
> Please add this to the right gypi file.

Added to chrome/chrome_browser.gypi

http://codereview.chromium.org/10894045/diff/21003/chrome/browser/system_moni...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

http://codereview.chromium.org/10894045/diff/21003/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:132:
UMA_HISTOGRAM_BOOLEAN("MediaDeviceNotification.mtp_device_name_available",
UMA names will not have underscores.

[Lei Zhang, 2012-09-11 02:00:08]

http://codereview.chromium.org/10894045/diff/7018/chrome/browser/system_monit...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

http://codereview.chromium.org/10894045/diff/7018/chrome/browser/system_monit...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:102:
// This constructor is only used by unit tests.
nit: put this above and explain a bit more what the DCHECK means.

I'm not sure if all compilers will be happy with the dcheck as is. It might
complain |mtp_dev_mgr| is unused?

[kmadhusu, 2012-09-11 03:20:40]

http://codereview.chromium.org/10894045/diff/7018/chrome/browser/system_monit...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

http://codereview.chromium.org/10894045/diff/7018/chrome/browser/system_monit...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:102:
// This constructor is only used by unit tests.
On 2012/09/11 02:00:08, Lei Zhang wrote:
> nit: put this above and explain a bit more what the DCHECK means.
> 
Done
> I'm not sure if all compilers will be happy with the dcheck as is. It might
> complain |mtp_dev_mgr| is unused?

Added DCHECK(GetMedia...());

[Lei Zhang, 2012-09-11 03:31:23]

lgtm

[zel, 2012-09-11 21:17:34]

delegating CR to tbarzic@

[tbarzic, 2012-09-11 22:03:05]

https://chromiumcodereview.appspot.com/10894045/diff/7019/chrome/browser/syst...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

https://chromiumcodereview.appspot.com/10894045/diff/7019/chrome/browser/syst...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:31:
MediaTransferProtocolManager::GetInstance();
I don't see the point of this function..
why not just use MediaTransferProtovolManager::GetInstance()?

https://chromiumcodereview.appspot.com/10894045/diff/7019/chrome/browser/syst...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:64:
MediaTransferProtocolManager* mtp_dev_mgr = GetMediaTransferProtocolManager();
it's not obvious what |mtp_dev_mgr| means (especially dev part).
maybe mtp_manager ?

https://chromiumcodereview.appspot.com/10894045/diff/7019/chrome/browser/syst...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h
(right):

https://chromiumcodereview.appspot.com/10894045/diff/7019/chrome/browser/syst...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:27:
: public MediaTransferProtocolManager::Observer {
nit:
indent 6->4

https://chromiumcodereview.appspot.com/10894045/diff/7019/chrome/browser/syst...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:44:
typedef std::map<std::string, std::string> StorageNameAndIdMap;
nit:
how about |StorageNameToIdMap|

[kmadhusu, 2012-09-11 23:30:53]

tbarzic@: Addressed review comments. PTAL.

Thanks.

http://codereview.chromium.org/10894045/diff/7019/chrome/browser/system_monit...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

http://codereview.chromium.org/10894045/diff/7019/chrome/browser/system_monit...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:31:
MediaTransferProtocolManager::GetInstance();
On 2012/09/11 22:03:05, tbarzic wrote:
> I don't see the point of this function..
> why not just use MediaTransferProtovolManager::GetInstance()?

Removed the helper function and called GetInstance directly.

http://codereview.chromium.org/10894045/diff/7019/chrome/browser/system_monit...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:64:
MediaTransferProtocolManager* mtp_dev_mgr = GetMediaTransferProtocolManager();
On 2012/09/11 22:03:05, tbarzic wrote:
> it's not obvious what |mtp_dev_mgr| means (especially dev part).
> maybe mtp_manager ?

Done.

http://codereview.chromium.org/10894045/diff/7019/chrome/browser/system_monit...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h
(right):

http://codereview.chromium.org/10894045/diff/7019/chrome/browser/system_monit...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:27:
: public MediaTransferProtocolManager::Observer {
On 2012/09/11 22:03:05, tbarzic wrote:
> nit:
> indent 6->4

Done.

http://codereview.chromium.org/10894045/diff/7019/chrome/browser/system_monit...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:44:
typedef std::map<std::string, std::string> StorageNameAndIdMap;
On 2012/09/11 22:03:05, tbarzic wrote:
> nit:
> how about |StorageNameToIdMap|

Done.

[tbarzic, 2012-09-11 23:36:15]

On 2012/09/11 23:30:53, kmadhusu wrote:
> tbarzic@: Addressed review comments. PTAL.
> 
> Thanks.
> 
>
http://codereview.chromium.org/10894045/diff/7019/chrome/browser/system_monit...
> File
>
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
> (right):
> 
>
http://codereview.chromium.org/10894045/diff/7019/chrome/browser/system_monit...
>
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:31:
> MediaTransferProtocolManager::GetInstance();
> On 2012/09/11 22:03:05, tbarzic wrote:
> > I don't see the point of this function..
> > why not just use MediaTransferProtovolManager::GetInstance()?
> 
> Removed the helper function and called GetInstance directly.
> 
>
http://codereview.chromium.org/10894045/diff/7019/chrome/browser/system_monit...
>
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:64:
> MediaTransferProtocolManager* mtp_dev_mgr = GetMediaTransferProtocolManager();
> On 2012/09/11 22:03:05, tbarzic wrote:
> > it's not obvious what |mtp_dev_mgr| means (especially dev part).
> > maybe mtp_manager ?
> 
> Done.
> 
>
http://codereview.chromium.org/10894045/diff/7019/chrome/browser/system_monit...
> File
>
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h
> (right):
> 
>
http://codereview.chromium.org/10894045/diff/7019/chrome/browser/system_monit...
>
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:27:
> : public MediaTransferProtocolManager::Observer {
> On 2012/09/11 22:03:05, tbarzic wrote:
> > nit:
> > indent 6->4
> 
> Done.
> 
>
http://codereview.chromium.org/10894045/diff/7019/chrome/browser/system_monit...
>
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:44:
> typedef std::map<std::string, std::string> StorageNameAndIdMap;
> On 2012/09/11 22:03:05, tbarzic wrote:
> > nit:
> > how about |StorageNameToIdMap|
> 
> Done.

lgtm

[commit-bot: I haz the power, 2012-09-12 02:29:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10894045/25008

[commit-bot: I haz the power, 2012-09-13 03:08:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10894045/20012

[commit-bot: I haz the power, 2012-09-13 05:32:07]

Change committed as 156498