Add systeminfo.storage API implementation skeleton

Add systemInfo.storage API implementation skeleton

BUG=141229, 136519
TEST=browser_tests --gtest_filter=SystemInfoStorageApiTest.*


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

[Hongbo Min, 2012-08-09 14:33:11]

This is the patch for storage bit implementation skeleton. Next I will implement
the StorageInfoProvider for different platforms. Pls have a review. Thanks.

[James Hawkins, 2012-08-09 15:47:34]

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h
(right):

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:11:
using api::experimental_systeminfo_storage::StorageInfo;
nit: 'using' is not allowed in headers.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:14:
const char kStorageTypeUnknown[] = "unknown";
Any reason this can't be an enum?

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:19:
class StorageInfoProvider {
nit: Document class.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:23:
static StorageInfoProvider* Create();
Optional nit: Separate these disparate pieces with blank lines for readability.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:31:
class MockStorageInfoProvider : public StorageInfoProvider {
What is this used for?

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:38:
bool MockStorageInfoProvider::GetStorageInfo(StorageInfo* info) {
Implementations need to move to the source file.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:55: }
// namespace extensions
nit: Two spaces before comments.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc
(right):

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc:28:
base::Bind(&SysteminfoStorageGetFunction::WorkOnFileThread, this));
nit: Start of parameter rows must align on the same column.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc:37:
if (provider.get() && provider->GetStorageInfo(&info)) {
Why would provider.get() ever be non-NULL?

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc:42:
success = false;
nit: This line is redundant with line 34.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc:45:
// Response on UI thread
nit: Comments must end with a period.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc:48:
base::Bind(&SysteminfoStorageGetFunction::RespondOnUIThread,
nit: Alignment is off.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.h
(right):

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.h:11:
class SysteminfoStorageGetFunction : public AsyncExtensionFunction {
nit: Document class.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.h:16:
protected:
nit: Blank line between access sections.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.h:16:
protected:
Why do these need to be protected as opposed to private?

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.h:17:
virtual bool RunImpl() OVERRIDE;
nit: // AsyncExtensionFunction implementation.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.h:18:
void WorkOnFileThread();
nit: Document these methods.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.h:22: }
// namespace extensions
nit: Two spaces before comment.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_apitest.cc
(right):

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_apitest.cc:14:
virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
nit: // ExtensionApiTest implementation.

http://codereview.chromium.org/10824232/diff/3001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_systeminfo_storage.idl (right):

http://codereview.chromium.org/10824232/diff/3001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_systeminfo_storage.idl:19: // The
unique id
nit: Comments must end with a period, here and elsewhere.

[Hongbo Min, 2012-08-10 02:43:29]

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h
(right):

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:14:
const char kStorageTypeUnknown[] = "unknown";
It aligns with idl. The enum defined in idl is convert to string type.

Using string directly here to avoid write enum->string converter.

Update: after consideration again, I will add add enum for storage type.

On 2012/08/09 15:47:34, James Hawkins wrote:
> Any reason this can't be an enum?

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc
(right):

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc:37:
if (provider.get() && provider->GetStorageInfo(&info)) {
In case of out of memory, the allocation for provider fails.
On 2012/08/09 15:47:34, James Hawkins wrote:
> Why would provider.get() ever be non-NULL?

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.h
(right):

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.h:16:
protected:
Actually, there is no criteria for it.

Just going through the source code of extension api implementation, take RunImpl
for example, some of them are declared as public, and some are protected, and
some are private.

On 2012/08/09 15:47:34, James Hawkins wrote:
> Why do these need to be protected as opposed to private?

[Hongbo Min, 2012-08-10 15:33:16]

The StorageInfoProvider impl for Windows platform is uploaded. Pls have a
review. Thanks.

[Mihai Parparita -not on Chrome, 2012-08-10 20:46:47]

http://codereview.chromium.org/10824232/diff/3003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc
(right):

http://codereview.chromium.org/10824232/diff/3003/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc:61:
const char kStorageTypeUnknownStr[] = "unknown";
BTW, filed http://crbug.com/141940, which, if fixed, would allow you to reuse
the enum values from your .idl file.

[James Hawkins, 2012-08-11 16:37:54]

Please reply to each review comment (in the diff viewer) by pressing the 'Done'
button to let me know which review comments you have fixed.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc
(right):

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc:37:
if (provider.get() && provider->GetStorageInfo(&info)) {
On 2012/08/10 02:43:29, Hongbo wrote:
> In case of out of memory, the allocation for provider fails.
> On 2012/08/09 15:47:34, James Hawkins wrote:
> > Why would provider.get() ever be non-NULL?
> 

Chrome code does not check for OOM situations, so the check should be removed.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.h
(right):

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.h:16:
protected:
Review process nit: Please bottom-post review replies.  I'll top-post on this
one so it remains semi-readable.

Please use the most limited access possible, which could mean private here.

On 2012/08/10 02:43:29, Hongbo wrote:
> Actually, there is no criteria for it.
> 
> Just going through the source code of extension api implementation, take
RunImpl
> for example, some of them are declared as public, and some are protected, and
> some are private.
> 
> On 2012/08/09 15:47:34, James Hawkins wrote:
> > Why do these need to be protected as opposed to private?
>

[Hongbo Min, 2012-08-12 03:31:58]

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h
(right):

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:11:
using api::experimental_systeminfo_storage::StorageInfo;
On 2012/08/09 15:47:34, James Hawkins wrote:
> nit: 'using' is not allowed in headers.

Done.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:14:
const char kStorageTypeUnknown[] = "unknown";
On 2012/08/10 02:43:29, Hongbo wrote:
> It aligns with idl. The enum defined in idl is convert to string type.
> 
> Using string directly here to avoid write enum->string converter.
> 
> Update: after consideration again, I will add add enum for storage type.
> 
> On 2012/08/09 15:47:34, James Hawkins wrote:
> > Any reason this can't be an enum?
> 

Depends on http://crbug.com/141940

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:19:
class StorageInfoProvider {
On 2012/08/09 15:47:34, James Hawkins wrote:
> nit: Document class.

Done.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:23:
static StorageInfoProvider* Create();
On 2012/08/09 15:47:34, James Hawkins wrote:
> Optional nit: Separate these disparate pieces with blank lines for
readability.

Done.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:31:
class MockStorageInfoProvider : public StorageInfoProvider {
On 2012/08/09 15:47:34, James Hawkins wrote:
> What is this used for?

Done.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:38:
bool MockStorageInfoProvider::GetStorageInfo(StorageInfo* info) {
On 2012/08/09 15:47:34, James Hawkins wrote:
> Implementations need to move to the source file.

Done.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/storage_info_provider.h:55: }
// namespace extensions
On 2012/08/09 15:47:34, James Hawkins wrote:
> nit: Two spaces before comments.

Done.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc
(right):

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc:28:
base::Bind(&SysteminfoStorageGetFunction::WorkOnFileThread, this));
On 2012/08/09 15:47:34, James Hawkins wrote:
> nit: Start of parameter rows must align on the same column.

Done.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc:42:
success = false;
On 2012/08/09 15:47:34, James Hawkins wrote:
> nit: This line is redundant with line 34.

Done.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc:45:
// Response on UI thread
On 2012/08/09 15:47:34, James Hawkins wrote:
> nit: Comments must end with a period.

Done.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_api.cc:48:
base::Bind(&SysteminfoStorageGetFunction::RespondOnUIThread,
On 2012/08/09 15:47:34, James Hawkins wrote:
> nit: Alignment is off.

Done.

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_apitest.cc
(right):

http://codereview.chromium.org/10824232/diff/3001/chrome/browser/extensions/a...
chrome/browser/extensions/api/systeminfo_storage/systeminfo_storage_apitest.cc:14:
virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
On 2012/08/09 15:47:34, James Hawkins wrote:
> nit: // ExtensionApiTest implementation.

Done.

http://codereview.chromium.org/10824232/diff/3001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_systeminfo_storage.idl (right):

http://codereview.chromium.org/10824232/diff/3001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_systeminfo_storage.idl:19: // The
unique id
On 2012/08/09 15:47:34, James Hawkins wrote:
> nit: Comments must end with a period, here and elsewhere.

Done.

[Mihai Parparita -not on Chrome, 2012-08-14 00:07:31]

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:5:
#include
"chrome/browser/extensions/api/system_info_storage/storage_info_provider.h"
For the sake of brevity, you may want to start with all of the system_info APIs
being in the same directory ("system_info"), and only move them out into
separate ones if it gets overwhelming.

The directories are meant to be per-API, and I see all of SystemInfo as being
one API.

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h
(right):

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:12: //
doesn't support enum type yet. Once http://crbug.com/141940 is fixed, we could
> 80 chars

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc
(right):

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:15:
// ExtensionApiTest implementation
Remove this comment, it doesn't add anything.

http://codereview.chromium.org/10824232/diff/6003/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_system_info_storage.idl (right):

http://codereview.chromium.org/10824232/diff/6003/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_system_info_storage.idl:48: interface
Events {
Remove unused parts of the IDL.

http://codereview.chromium.org/10824232/diff/6003/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/systeminfo/storage/manifest.json
(right):

http://codereview.chromium.org/10824232/diff/6003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/systeminfo/storage/manifest.json:5:
"description": "Test systeminfo.cpu API",
This should say systeminfo.storage.

http://codereview.chromium.org/10824232/diff/6003/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/systeminfo/storage/test_storage_api.js
(right):

http://codereview.chromium.org/10824232/diff/6003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/systeminfo/storage/test_storage_api.js:12:
// TODO: Need to check the value from Mock implementation
Put a username for this TODO.

[Hongbo Min, 2012-08-14 01:37:23]

Update the patch. Thanks for your comments.

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:5:
#include
"chrome/browser/extensions/api/system_info_storage/storage_info_provider.h"
On 2012/08/14 00:07:31, Mihai Parparita wrote:
> For the sake of brevity, you may want to start with all of the system_info
APIs
> being in the same directory ("system_info"), and only move them out into
> separate ones if it gets overwhelming.
> 
> The directories are meant to be per-API, and I see all of SystemInfo as being
> one API.

If the namespace specified in IDL is systemInfo.X, the generated_api.h generated
from IDL parser assumes the X APIs are in system_info_x directory.

So each SystemInfo api category has to be in a separate directory, not in the
same dir ("system_info").

If the IDL allows to add nested namespaces specified IDL file, it could get
fixed, like
namespace experimental.systemInfo {
  namespace cpu {
  }
  namespace storage {
  }
}

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h
(right):

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:12: //
doesn't support enum type yet. Once http://crbug.com/141940 is fixed, we could
On 2012/08/14 00:07:31, Mihai Parparita wrote:
> > 80 chars

Done.

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc
(right):

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:15:
// ExtensionApiTest implementation
On 2012/08/14 00:07:31, Mihai Parparita wrote:
> Remove this comment, it doesn't add anything.

Done.

http://codereview.chromium.org/10824232/diff/6003/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_system_info_storage.idl (right):

http://codereview.chromium.org/10824232/diff/6003/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_system_info_storage.idl:48: interface
Events {
On 2012/08/14 00:07:31, Mihai Parparita wrote:
> Remove unused parts of the IDL.

Done.

http://codereview.chromium.org/10824232/diff/6003/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/systeminfo/storage/test_storage_api.js
(right):

http://codereview.chromium.org/10824232/diff/6003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/systeminfo/storage/test_storage_api.js:12:
// TODO: Need to check the value from Mock implementation
On 2012/08/14 00:07:31, Mihai Parparita wrote:
> Put a username for this TODO.

Done.

[Hongbo Min, 2012-08-14 04:25:57]

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:5:
#include
"chrome/browser/extensions/api/system_info_storage/storage_info_provider.h"
On 2012/08/14 00:07:31, Mihai Parparita wrote:
> For the sake of brevity, you may want to start with all of the system_info
APIs
> being in the same directory ("system_info"), and only move them out into
> separate ones if it gets overwhelming.
> 
> The directories are meant to be per-API, and I see all of SystemInfo as being
> one API.

If using JSON format, all systemInfo API could be in the same one directory. But
currently the IDL compiler can't support such usage model.

Is it necessary to use JSON format instead? or fire a bug to IDL compiler?

[Mihai Parparita -not on Chrome, 2012-08-15 00:14:04]

LGTM

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:5:
#include
"chrome/browser/extensions/api/system_info_storage/storage_info_provider.h"
On 2012/08/14 04:25:57, Hongbo wrote:
> On 2012/08/14 00:07:31, Mihai Parparita wrote:
> > For the sake of brevity, you may want to start with all of the system_info
> APIs
> > being in the same directory ("system_info"), and only move them out into
> > separate ones if it gets overwhelming.
> > 
> > The directories are meant to be per-API, and I see all of SystemInfo as
being
> > one API.
> 
> If using JSON format, all systemInfo API could be in the same one directory.
But
> currently the IDL compiler can't support such usage model.
> 
> Is it necessary to use JSON format instead? or fire a bug to IDL compiler?

It's not worth switching to the JSON format just for this. You can file a bug.

[commit-bot: I haz the power, 2012-08-15 10:39:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10824232/13002

[commit-bot: I haz the power, 2012-08-15 10:39:49]

Presubmit check for 10824232-13002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.2s to calculate.

[Hongbo Min, 2012-08-15 10:43:50]

Nico, I need your LGTM for chrome/*.gypi. Thanks.

[Hongbo Min, 2012-08-15 13:43:07]

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

http://codereview.chromium.org/10824232/diff/6003/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:5:
#include
"chrome/browser/extensions/api/system_info_storage/storage_info_provider.h"
On 2012/08/15 00:14:05, Mihai Parparita wrote:
> On 2012/08/14 04:25:57, Hongbo wrote:
> > On 2012/08/14 00:07:31, Mihai Parparita wrote:
> > > For the sake of brevity, you may want to start with all of the system_info
> > APIs
> > > being in the same directory ("system_info"), and only move them out into
> > > separate ones if it gets overwhelming.
> > > 
> > > The directories are meant to be per-API, and I see all of SystemInfo as
> being
> > > one API.
> > 
> > If using JSON format, all systemInfo API could be in the same one directory.
> But
> > currently the IDL compiler can't support such usage model.
> > 
> > Is it necessary to use JSON format instead? or fire a bug to IDL compiler?
> 
> It's not worth switching to the JSON format just for this. You can file a bug.

The bug is filed at http://crbug.com/142841

[James Hawkins, 2012-08-15 15:43:37]

Mostly readability nits.

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:8:
namespace systeminfo {
nit: Blank line after this.

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:12: }
 // namespace systeminfo
nit: Blank line before this.

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h
(right):

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:10:
namespace systeminfo {
nit: Blank line after this.

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:11: //
TODO(hmin): here the storage type is declared as string type due to the IDL
nit: s/here/Here/

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:32: //
Return true if succeed to get storage information, other return fasle.
nit: Remove the ", other return false".

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:37: } 
// namespace extensions
nit: Blank line before this.

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
File
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.cc
(right):

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.cc:11:
namespace extensions {
nit: Blank line after this.

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.cc:19:
// StorageInfoProvider implementation on Windows platform
nit: Period at end of sentence.

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.cc:24:
virtual bool GetStorageInfo(StorageInfo* info) OVERRIDE;
nit: Add a blank line above.

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.cc:24:
virtual bool GetStorageInfo(StorageInfo* info) OVERRIDE;
nit: // StorageInfoProvider implementation.

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.cc:75:
linked_ptr<StorageUnitInfo> unit(new StorageUnitInfo());
Why use a linked_ptr here?

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.cc:87:
StorageInfoProvider* StorageInfoProvider::Create() {
nit: The ordering of the methods in the implementation file need to match the
ordering in the header, so this needs to be moved to the top.

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
File
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.cc
(right):

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.cc:16:
namespace extensions {
nit: Blank line after this.

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.cc:46:
// Response the result on UI thread.
Hmm, response isn't the word you're looking for.  'Return' might be it.

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
File chrome/browser/extensions/api/system_info_storage/system_info_storage_api.h
(right):

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.h:11:
// Implement systeminfo.storage.get API. It is an asynchronous call relative to
nit: s/Implement/Implementation of the/

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.h:22:
virtual bool RunImpl() OVERRIDE;
nit: // AsyncExtensionFunction implementation.

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.h:27:
// Respond the result back to UI thread
nit: s/Respond/Responds/

https://chromiumcodereview.appspot.com/10824232/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.h:29:
};  // SystemInfoStorageGetFunction
nit: We don't comment the close of classes.

[Nico, 2012-08-15 17:35:05]

chrome/chrome_tests.gypi change lgtm

[Hongbo Min, 2012-08-16 09:42:47]

Thanks James' comments.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:8:
namespace systeminfo {
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: Blank line after this.

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:12: }
 // namespace systeminfo
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: Blank line before this.

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h
(right):

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:10:
namespace systeminfo {
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: Blank line after this.

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:11: //
TODO(hmin): here the storage type is declared as string type due to the IDL
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: s/here/Here/

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:32: //
Return true if succeed to get storage information, other return fasle.
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: Remove the ", other return false".

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:37: } 
// namespace extensions
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: Blank line before this.

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.cc
(right):

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.cc:19:
// StorageInfoProvider implementation on Windows platform
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: Period at end of sentence.

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.cc:24:
virtual bool GetStorageInfo(StorageInfo* info) OVERRIDE;
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: Add a blank line above.

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.cc:24:
virtual bool GetStorageInfo(StorageInfo* info) OVERRIDE;
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: Add a blank line above.

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.cc:75:
linked_ptr<StorageUnitInfo> unit(new StorageUnitInfo());
On 2012/08/15 15:43:37, James Hawkins wrote:
> Why use a linked_ptr here?
The StorageInfo structure generated by IDL parser is:
struct StorageInfo {
public:
  std::vector<linked_ptr<StorageUnitInfo> > units;
...
};
So here use linked_ptr for units.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.cc:87:
StorageInfoProvider* StorageInfoProvider::Create() {
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: The ordering of the methods in the implementation file need to match the
> ordering in the header, so this needs to be moved to the top.

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.cc
(right):

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.cc:16:
namespace extensions {
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: Blank line after this.

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.cc:46:
// Response the result on UI thread.
On 2012/08/15 15:43:37, James Hawkins wrote:
> Hmm, response isn't the word you're looking for.  'Return' might be it.

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_storage/system_info_storage_api.h
(right):

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.h:11:
// Implement systeminfo.storage.get API. It is an asynchronous call relative to
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: s/Implement/Implementation of the/

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.h:22:
virtual bool RunImpl() OVERRIDE;
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: // AsyncExtensionFunction implementation.

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.h:27:
// Respond the result back to UI thread
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: s/Respond/Responds/

Done.

http://codereview.chromium.org/10824232/diff/13002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_api.h:29:
};  // SystemInfoStorageGetFunction
On 2012/08/15 15:43:37, James Hawkins wrote:
> nit: We don't comment the close of classes.

Done.

[commit-bot: I haz the power, 2012-08-16 23:33:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10824232/2007

[commit-bot: I haz the power, 2012-08-17 00:09:18]

Try job failure for 10824232-2007 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-17 01:23:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10824232/12003

[commit-bot: I haz the power, 2012-08-17 04:39:15]

Change committed as 152046