[SystemInfo API] Rewrite storage info provider using storage monitor impl

[SystemInfo API] Rewrite storage info provider using storage monitor impl

o Make use of StorageMonitor implementation to reuse the common code as possible.
o Expose more storage metadata information, such as human readable name and so on,
  via systemInfo.storage API

BUG=177605
TEST=browser_test --gtest_filter=SystemInfoStorageApiTest.*
TEST=unit_tests --gtest_filter=StorageInfoProviderTest.*

[Hongbo Min, 2013-05-26 07:18:24]

vandebo@, thestig@, please have a review on this big change of
StorageInfoProvider. Thanks.

[Lei Zhang, 2013-05-28 08:08:07]

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_api.cc (right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_api.cc:238: // Called on
UI thread since the observer is added from UI thread.
Turn this comment into a DCHECK.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_api.cc:260:
StorageInfoProvider::Get(), mount_path),
nit: indentation

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:43:
SystemInfoProvider() : is_waiting_for_completion_(false) {
(Slightly off-topic) Why are all the implementations in the .h file?

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h
(right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h:7:
#include "chrome/common/extensions/api/experimental_system_info_storage.h"
probably all you need is <string>

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h:14:
virtual ~StorageFreeSpaceObserver() {}
the dtor can be protected.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:81:
StorageMonitor* storage_monitor = StorageMonitor::GetInstance();
just do:
return StorageMonitor::GetInstance()->GetAllAvailableStorages();

If you ever crash on this line with a NULL dereference, it should be pretty
obvious what happened.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:123:
int64 StorageInfoProvider::GetStorageFreeSpace(
anonymous function instead?

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h
(right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:30:
void BuildStorageUnitInfo(const chrome::StorageInfo& info,
Just return a StorageUnitInfo instead?

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:45: //
Overridens from SystemInfoProvider:
typo

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:62:
virtual bool GetStoragePathFromId(const std::string& id,
Return an FilePath instead, with empty FilePath as failure?

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor.h:113: virtual
std::vector<StorageInfo> GetAllAvailableStorages() const;
How is this different from GetAttachedStorage() below? Did you want this to
return all storages, removable or fixed? If so, you should check all the
implementations. The CrOS StorageMonitor, for instance, currently only handles
removable devices.

https://codereview.chromium.org/15896007/diff/30001/chrome/chrome_tests_unit....
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/15896007/diff/30001/chrome/chrome_tests_unit....
chrome/chrome_tests_unit.gypi:764:
'browser/extensions/api/system_info_storage/test_storage_info_provider.cc',
When multiple tests use the same files, put it in the test_support_common
target.

https://codereview.chromium.org/15896007/diff/30001/chrome/common/extensions/...
File chrome/common/extensions/api/experimental_system_info_storage.idl (right):

https://codereview.chromium.org/15896007/diff/30001/chrome/common/extensions/...
chrome/common/extensions/api/experimental_system_info_storage.idl:17: // The
unique storage id. It is persistent between storage attachments.
Have you talked to anyone on the extensions team about changing the API?

[Hongbo Min, 2013-05-28 14:24:01]

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_api.cc (right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_api.cc:238: // Called on
UI thread since the observer is added from UI thread.
On 2013/05/28 08:08:07, Lei Zhang wrote:
> Turn this comment into a DCHECK.

Done.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_api.cc:260:
StorageInfoProvider::Get(), mount_path),
On 2013/05/28 08:08:07, Lei Zhang wrote:
> nit: indentation

Done.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:43:
SystemInfoProvider() : is_waiting_for_completion_(false) {
On 2013/05/28 08:08:07, Lei Zhang wrote:
> (Slightly off-topic) Why are all the implementations in the .h file?

Because the SystemInfoProvider class is just a template class. We might extract
some code into .cc file by introducing a new class like SystemInfoProviderBase.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h
(right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h:7:
#include "chrome/common/extensions/api/experimental_system_info_storage.h"
On 2013/05/28 08:08:07, Lei Zhang wrote:
> probably all you need is <string>

Done.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h:14:
virtual ~StorageFreeSpaceObserver() {}
On 2013/05/28 08:08:07, Lei Zhang wrote:
> the dtor can be protected.

Done.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:81:
StorageMonitor* storage_monitor = StorageMonitor::GetInstance();
On 2013/05/28 08:08:07, Lei Zhang wrote:
> just do:
> return StorageMonitor::GetInstance()->GetAllAvailableStorages();
> 
> If you ever crash on this line with a NULL dereference, it should be pretty
> obvious what happened.

Done.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:123:
int64 StorageInfoProvider::GetStorageFreeSpace(
On 2013/05/28 08:08:07, Lei Zhang wrote:
> anonymous function instead?

No, it should be overriden by TestStorageInfoProvider to provide a test-specific
implemenation.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h
(right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:30:
void BuildStorageUnitInfo(const chrome::StorageInfo& info,
On 2013/05/28 08:08:07, Lei Zhang wrote:
> Just return a StorageUnitInfo instead?

My original thought on this is, with passing a pointer to StorageUnitInfo, we
can avoid unnecessary ctor calling for temporary usage, e.g. for return value.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:45: //
Overridens from SystemInfoProvider:
On 2013/05/28 08:08:07, Lei Zhang wrote:
> typo

Done.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:62:
virtual bool GetStoragePathFromId(const std::string& id,
On 2013/05/28 08:08:07, Lei Zhang wrote:
> Return an FilePath instead, with empty FilePath as failure?

The same reason as BuildStorageUnitInfo. I am wondering what is the extra
benefit by returning an FilePath directly instead of by passing a FilePath
pointer?

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor.h:113: virtual
std::vector<StorageInfo> GetAllAvailableStorages() const;
On 2013/05/28 08:08:07, Lei Zhang wrote:
> How is this different from GetAttachedStorage() below? Did you want this to
> return all storages, removable or fixed? If so, you should check all the
> implementations. The CrOS StorageMonitor, for instance, currently only handles
> removable devices.

Yes, GetAllAvailableStorages is designed to return the fixed and removabled
storages on the system, while GetAttachedStorage only returns the removable
storages.

https://codereview.chromium.org/15896007/diff/30001/chrome/chrome_tests_unit....
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/15896007/diff/30001/chrome/chrome_tests_unit....
chrome/chrome_tests_unit.gypi:764:
'browser/extensions/api/system_info_storage/test_storage_info_provider.cc',
On 2013/05/28 08:08:07, Lei Zhang wrote:
> When multiple tests use the same files, put it in the test_support_common
> target.

Done.

https://codereview.chromium.org/15896007/diff/30001/chrome/common/extensions/...
File chrome/common/extensions/api/experimental_system_info_storage.idl (right):

https://codereview.chromium.org/15896007/diff/30001/chrome/common/extensions/...
chrome/common/extensions/api/experimental_system_info_storage.idl:17: // The
unique storage id. It is persistent between storage attachments.
On 2013/05/28 08:08:07, Lei Zhang wrote:
> Have you talked to anyone on the extensions team about changing the API?

Not yet. Do you know who is the owner of extensions team?

[vandebo (ex-Chrome), 2013-05-28 20:00:29]

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_api.cc (right):

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_api.cc:92: //
StorageFreeSpaceObserver implementation.
What was the conclusion of the discussion about having a freespace change event?

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_api.cc:132:
DCHECK(chrome::StorageMonitor::GetInstance());
When is SystemInfoEventRouter created? We are changing the consumers of
StorageMonitor to only initialize it when they actively need it instead of
initializing it at startup.

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:31:
unit->id = info.device_id();
you need to get the transient id.  StorageInfo::device_id() may contain personal
information and can't be exposed to javascript.

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:37:
// TODO(hmin): Might need to take MTP device into consideration.
If needed, we could an IsMassStorage predicate.

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor.h:79: void
RegisterInitCompletedCallback(base::Closure callback);
This is the same as Initialize - remove.

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor.h:110: // Returns the meta
information for all available storages on the system,
This change should be done as a separate CL.

https://codereview.chromium.org/15896007/diff/41001/chrome/common/extensions/...
File chrome/common/extensions/api/experimental_system_info_storage.idl (right):

https://codereview.chromium.org/15896007/diff/41001/chrome/common/extensions/...
chrome/common/extensions/api/experimental_system_info_storage.idl:17: // The
unique storage id. It is persistent between storage attachments.
This will take more work.

[Lei Zhang, 2013-05-28 21:16:44]

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:43:
SystemInfoProvider() : is_waiting_for_completion_(false) {
On 2013/05/28 14:24:01, Hongbo Min wrote:
> On 2013/05/28 08:08:07, Lei Zhang wrote:
> > (Slightly off-topic) Why are all the implementations in the .h file?
> 
> Because the SystemInfoProvider class is just a template class. We might
extract
> some code into .cc file by introducing a new class like
SystemInfoProviderBase.

It doesn't have to be in the .h file. Maybe after this CL, I can help you move
things into a .cc file.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h
(right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:30:
void BuildStorageUnitInfo(const chrome::StorageInfo& info,
On 2013/05/28 14:24:01, Hongbo Min wrote:
> On 2013/05/28 08:08:07, Lei Zhang wrote:
> > Just return a StorageUnitInfo instead?
> 
> My original thought on this is, with passing a pointer to StorageUnitInfo, we
> can avoid unnecessary ctor calling for temporary usage, e.g. for return value.

Modern C++ compilers have RVO so you can write your code to be easier for humans
to understand, but internally the code is compiled to avoid the temporary
object.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:62:
virtual bool GetStoragePathFromId(const std::string& id,
On 2013/05/28 14:24:01, Hongbo Min wrote:
> On 2013/05/28 08:08:07, Lei Zhang wrote:
> > Return an FilePath instead, with empty FilePath as failure?
> 
> The same reason as BuildStorageUnitInfo. I am wondering what is the extra
> benefit by returning an FilePath directly instead of by passing a FilePath
> pointer?

Simpler to deal with one return value rather than 2.

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/15896007/diff/30001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor.h:113: virtual
std::vector<StorageInfo> GetAllAvailableStorages() const;
On 2013/05/28 14:24:01, Hongbo Min wrote:
> On 2013/05/28 08:08:07, Lei Zhang wrote:
> > How is this different from GetAttachedStorage() below? Did you want this to
> > return all storages, removable or fixed? If so, you should check all the
> > implementations. The CrOS StorageMonitor, for instance, currently only
handles
> > removable devices.
> 
> Yes, GetAllAvailableStorages is designed to return the fixed and removabled
> storages on the system, while GetAttachedStorage only returns the removable
> storages.

I would check and make sure all the per-platform implementations are actually
returning all the storages and not just the removable ones.

https://codereview.chromium.org/15896007/diff/30001/chrome/common/extensions/...
File chrome/common/extensions/api/experimental_system_info_storage.idl (right):

https://codereview.chromium.org/15896007/diff/30001/chrome/common/extensions/...
chrome/common/extensions/api/experimental_system_info_storage.idl:17: // The
unique storage id. It is persistent between storage attachments.
On 2013/05/28 14:24:01, Hongbo Min wrote:
> On 2013/05/28 08:08:07, Lei Zhang wrote:
> > Have you talked to anyone on the extensions team about changing the API?
> 
> Not yet. Do you know who is the owner of extensions team?
> 

I'd talk to mpcomplete@ or yoz@.

[Hongbo Min, 2013-05-29 01:05:38]

thanks for your reviewing. I will extract some change into separate CL to move
forward.

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_api.cc (right):

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_api.cc:92: //
StorageFreeSpaceObserver implementation.
On 2013/05/28 20:00:29, vandebo wrote:
> What was the conclusion of the discussion about having a freespace change
event?

It is still pending now.

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_api.cc:132:
DCHECK(chrome::StorageMonitor::GetInstance());
On 2013/05/28 20:00:29, vandebo wrote:
> When is SystemInfoEventRouter created? We are changing the consumers of
> StorageMonitor to only initialize it when they actively need it instead of
> initializing it at startup.

The SystemInfoEventRouter instance is declared as a LazyInstance, which means it
will be created only when it is needed. See |g_system_info_event_router| and
|SytemInfoEventRouter::GetInstance()|

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:37:
// TODO(hmin): Might need to take MTP device into consideration.
On 2013/05/28 20:00:29, vandebo wrote:
> If needed, we could an IsMassStorage predicate.

In that sense, MTP device can be categoried as removable storage, right?

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor.h:79: void
RegisterInitCompletedCallback(base::Closure callback);
On 2013/05/28 20:00:29, vandebo wrote:
> This is the same as Initialize - remove.

No, from the source code of Initialize, it will invoke Init at last, while
RegisterInitCompletedCallback won't invoke Init. The purpose is to allow the
consumer of StorageMonitor to register callback without invoking Init.

BTW, I don't find Initialize is called in some place, what is it used for?

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor.h:110: // Returns the meta
information for all available storages on the system,
On 2013/05/28 20:00:29, vandebo wrote:
> This change should be done as a separate CL.

OK. I will do that in a separate CL.

https://codereview.chromium.org/15896007/diff/41001/chrome/common/extensions/...
File chrome/common/extensions/api/experimental_system_info_storage.idl (right):

https://codereview.chromium.org/15896007/diff/41001/chrome/common/extensions/...
chrome/common/extensions/api/experimental_system_info_storage.idl:17: // The
unique storage id. It is persistent between storage attachments.
On 2013/05/28 20:00:29, vandebo wrote:
> This will take more work.

Do you mean using transient ID instead?

[Lei Zhang, 2013-05-29 01:35:14]

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:37:
// TODO(hmin): Might need to take MTP device into consideration.
On 2013/05/29 01:05:38, Hongbo Min wrote:
> On 2013/05/28 20:00:29, vandebo wrote:
> > If needed, we could an IsMassStorage predicate.
> 
> In that sense, MTP device can be categoried as removable storage, right?

Yes, MTP devices are certainly removable devices. I don't know if we have all
the code needed to get free disk space from a MTP device hooked up.

[Greg Billock, 2013-06-26 21:00:14]

This review is getting a bit unwieldy, with a lot going on. I think we all
understand the code better now, and know what we want to do with it, so it's
definitely been profitable. Do you want to start answering some of these issues
with TODOs so we don't lose the comments? Some things need fixed before we check
it in. Reviewers, let's start marking those with a (!). But many can be deferred
to other CLs -- it's still in experimental after all.

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:31:
unit->id = info.device_id();
On 2013/05/28 20:00:29, vandebo wrote:
> you need to get the transient id.  StorageInfo::device_id() may contain
personal
> information and can't be exposed to javascript.

Do this with chrome::TransientDeviceIds::GetTransientIdForDeviceId()

https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:37:
// TODO(hmin): Might need to take MTP device into consideration.
On 2013/05/29 01:35:14, Lei Zhang wrote:
> On 2013/05/29 01:05:38, Hongbo Min wrote:
> > On 2013/05/28 20:00:29, vandebo wrote:
> > > If needed, we could an IsMassStorage predicate.
> > 
> > In that sense, MTP device can be categoried as removable storage, right?
> 
> Yes, MTP devices are certainly removable devices. I don't know if we have all
> the code needed to get free disk space from a MTP device hooked up.

IsRemovableDevice should convey what you want here. Looks fine as is. I think
we're just planning to leave free disk space out for now.

[Greg Billock, 2013-06-26 21:21:04]

On 2013/06/26 21:00:14, Greg Billock wrote:
> This review is getting a bit unwieldy, with a lot going on. I think we all
> understand the code better now, and know what we want to do with it, so it's
> definitely been profitable. Do you want to start answering some of these
issues
> with TODOs so we don't lose the comments? Some things need fixed before we
check
> it in. Reviewers, let's start marking those with a (!). But many can be
deferred
> to other CLs -- it's still in experimental after all.
> 
>
https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
> File
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
> (right):
> 
>
https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
> chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:31:
> unit->id = info.device_id();
> On 2013/05/28 20:00:29, vandebo wrote:
> > you need to get the transient id.  StorageInfo::device_id() may contain
> personal
> > information and can't be exposed to javascript.
> 
> Do this with chrome::TransientDeviceIds::GetTransientIdForDeviceId()
> 
>
https://codereview.chromium.org/15896007/diff/41001/chrome/browser/extensions...
> chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:37:
> // TODO(hmin): Might need to take MTP device into consideration.
> On 2013/05/29 01:35:14, Lei Zhang wrote:
> > On 2013/05/29 01:05:38, Hongbo Min wrote:
> > > On 2013/05/28 20:00:29, vandebo wrote:
> > > > If needed, we could an IsMassStorage predicate.
> > > 
> > > In that sense, MTP device can be categoried as removable storage, right?
> > 
> > Yes, MTP devices are certainly removable devices. I don't know if we have
all
> > the code needed to get free disk space from a MTP device hooked up.
> 
> IsRemovableDevice should convey what you want here. Looks fine as is. I think
> we're just planning to leave free disk space out for now.

Realized that no wonder this looked unwieldy -- it's the old one. :-) Sorry
about that. Can you delete it in favor of
https://codereview.chromium.org/15896007/ ?

[Hongbo Min, 2013-06-27 01:21:39]

Closed and please see https://codereview.chromium.org/15896007/.