First few API implementation of AppsDebuggerPrivate.

First few API implementation of AppsDebuggerPrivate.

This CL adds the skeleton for AppsDebuggerPrivate APIs with 
Implementation of getItemInfo, Inspect and AutoUpdate.

Will be adding apitest to the CL.

BUG=149036
TBR=estrade@chromium.org
TBR=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173612

[Gaurav, 2012-11-30 20:31:55]

@miket Working on the apitest which I will add to this CL. Will publish for
review again. But, can you take an initial pass.

Thanks.

[asargent_no_longer_on_chrome, 2012-11-30 22:56:02]

Overall naming question: is this API only going to be used for debugging apps?
Or will it also be for extensions?

https://codereview.chromium.org/11428116/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_registry.cc (right):

https://codereview.chromium.org/11428116/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_registry.cc:11: #include
"chrome/browser/extensions/api/apps_debugger_private/apps_debugger_private_api.h"
Do you still need this here?

https://codereview.chromium.org/11428116/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/11428116/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/_permission_features.json:26: "extension_types":
["packaged_app"]
Should this be named appsDebuggerPrivate? Also, do you also want 

  "location": "component"

here?

https://codereview.chromium.org/11428116/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/apps_debugger_private.idl (right):

https://codereview.chromium.org/11428116/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/apps_debugger_private.idl:52: boolean incognito;
Are extensionId and incognito really needed here? Without looking at the code, I
would have expected that the combination of renderProcessId and renderViewId
would be sufficient to lookup the right information.

[Gaurav, 2012-11-30 23:32:42]

@Antony, I think the API has a broader future scope. Any name suggestions? How
about appsDeveloperPrivate ?

Thanks

https://codereview.chromium.org/11428116/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_registry.cc (right):

https://codereview.chromium.org/11428116/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_registry.cc:11: #include
"chrome/browser/extensions/api/apps_debugger_private/apps_debugger_private_api.h"
On 2012/11/30 22:56:02, Antony Sargent wrote:
> Do you still need this here?

Done.

https://codereview.chromium.org/11428116/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/11428116/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/_permission_features.json:26: "extension_types":
["packaged_app"]
On 2012/11/30 22:56:02, Antony Sargent wrote:
> Should this be named appsDebuggerPrivate? Also, do you also want 
> 
>   "location": "component"
> 
> here?

Done.

https://codereview.chromium.org/11428116/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/apps_debugger_private.idl (right):

https://codereview.chromium.org/11428116/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/apps_debugger_private.idl:52: boolean incognito;
We use extensionId and incognito for lazy background pages for which
render_process_id = -1

On 2012/11/30 22:56:02, Antony Sargent wrote:
> Are extensionId and incognito really needed here? Without looking at the code,
I
> would have expected that the combination of renderProcessId and renderViewId
> would be sufficient to lookup the right information.

[asargent_no_longer_on_chrome, 2012-12-03 23:10:07]

On 2012/11/30 23:32:42, Gaurav wrote:
> @Antony, I think the API has a broader future scope. Any name suggestions? How
> about appsDeveloperPrivate ?

If it will apply to both apps and extensions, then something neutral may be best
(for instance, we've moved some functions that used to live in chrome.extension
into the chrome.runtime namespace).

So maybe chrome.developerPrivate or chrome.developerUIPrivate?

[Gaurav, 2012-12-05 00:03:02]

chrome.developerPrivate sounds good (As they may not be UI specific).

Will rename and upadate.

On 2012/12/03 23:10:07, Antony Sargent wrote:
> On 2012/11/30 23:32:42, Gaurav wrote:
> > @Antony, I think the API has a broader future scope. Any name suggestions?
How
> > about appsDeveloperPrivate ?
> 
> If it will apply to both apps and extensions, then something neutral may be
best
> (for instance, we've moved some functions that used to live in
chrome.extension
> into the chrome.runtime namespace).
> 
> So maybe chrome.developerPrivate or chrome.developerUIPrivate?

[miket_OOO, 2012-12-06 19:31:15]

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:36: }
// namespace
always two spaces before //

(please fix all instances; I'll stop repeating myself)

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:75: }
Consider some defensive checks here that your catchall else cases are correct.
If someone adds a new type in the future, they might not know about this code,
and suddenly (and silently) this API will break. I'm thinking something like
this:

CHECK(item.is_packaged_app()) in the first else case
CHECK(item.is_extension()) in the second case.

Or else do this

if (foo) {
} else if (bar) {
} else if (baz) {
} else {
NOTREACHED();
}

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:102:
item.update_url().spec()));
is this wrapping necessary?

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:232:
std::string extension_id;
These declarations could be closer to where they're used.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:240:
incognito = options.incognito;
indentation

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:242: if
(render_process_id == -1) {
Not a big deal, but why convert render_view_id to an int if you already know
that you won't need it?

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:248: if
(incognito)
Is this logic out of order? Are you asking the non-incognito service for the
extension in the incognito case?

I asked asargent about this, and he wasn't sure whether it was even necessary to
get the incognito service separately. Can you please confirm that this is
correct, and if it is, put some kind of comment in here to let people know why
it's weird?

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:259: //
This can happen if the host has gone away since the page was dispalyed.
spelling

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:267: }
// namespace api
vertical whitespace (please fix all)

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:15:
namespace extensions {
Vertical whitespace inside each namespace open/close.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:20: }
These normally get commented with // namespace name_of_namespace_that_is_closed,
but use your judgment if the opening brace is nearby.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:56: 
Be consistent about vertical whitespace after access specifier (see public:
above)

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:82:
content::RenderViewHost* deleting_rvh_;
Avoid abbreviations if possible/practical.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:92:
virtual ~DeveloperPrivateAutoUpdateFunction() {}
See http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts. Don't
inline virtual destructors!

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:103:
virtual ~DeveloperPrivateGetItemsInfoFunction() {}
Same as above.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:114:
virtual ~DeveloperPrivateInspectFunction() {}
etc.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:120: }
// namespace api
vertical whitespace

always exactly two spaces before //

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
File
chrome/browser/extensions/api/developer_private/developer_private_api_factory.h
(right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api_factory.h:15:
class DeveloperPrivateAPIFactory : public ProfileKeyedServiceFactory {
comment on class purpose, please

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
File
chrome/browser/extensions/api/developer_private/developer_private_apitest.cc
(right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_apitest.cc:21:
FilePath basedir = test_data_dir_.AppendASCII("developer");
base_dir to be consistent

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_apitest.cc:27:
const std::string& name) {
wrapping probably not necessary here

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_apitest.cc:43:
std::map<std::string, std::string> extension_ids_;
If you name this something like "extension_names_to_ids_" then you won't need
the comment.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/common/exte...
File chrome/common/extensions/api/developer_private.idl (right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/common/exte...
chrome/common/extensions/api/developer_private.idl:7: namespace developerPrivate
{
There could be much better documentation in this file. Remember that the IDL is
the authoritative, single source for API definitions, so it's the right place to
have the best documentation.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/common/exte...
chrome/common/extensions/api/developer_private.idl:11: packaged_app,
why the different naming style?

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/common/exte...
chrome/common/extensions/api/developer_private.idl:67: // Runs autoUpdate
immediately.
no internal capitalization

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/common/exte...
chrome/common/extensions/api/developer_private.idl:68: // |callback| : Called
with the boolean result.
this comment adds no information.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/test/data/e...
File chrome/test/data/extensions/api_test/developer/packaged_app/manifest.json
(right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/test/data/e...
chrome/test/data/extensions/api_test/developer/packaged_app/manifest.json:10:
"offline_enabled": true
You don't need this key anymore; it's true by default.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/test/data/e...
File chrome/test/data/extensions/api_test/developer/test/basics.html (right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/test/data/e...
chrome/test/data/extensions/api_test/developer/test/basics.html:2: * Copyright
(c) 2012222222222222222222222 The Chromium Authors. All rights reserved.  Use of
this
This is too far in the future.

[Gaurav, 2012-12-12 02:23:38]

Thanks Mike. Inline response to the comments.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:75: }
On 2012/12/06 19:31:15, miket wrote:
> Consider some defensive checks here that your catchall else cases are correct.
> If someone adds a new type in the future, they might not know about this code,
> and suddenly (and silently) this API will break. I'm thinking something like
> this:
> 
> CHECK(item.is_packaged_app()) in the first else case
> CHECK(item.is_extension()) in the second case.
> 
> Or else do this
> 
> if (foo) {
> } else if (bar) {
> } else if (baz) {
> } else {
> NOTREACHED();
> }

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:102:
item.update_url().spec()));
update_url is optional parameter and is a string pointer.

On 2012/12/06 19:31:15, miket wrote:
> is this wrapping necessary?

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:232:
std::string extension_id;
On 2012/12/06 19:31:15, miket wrote:
> These declarations could be closer to where they're used.

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:240:
incognito = options.incognito;
On 2012/12/06 19:31:15, miket wrote:
> indentation

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:242: if
(render_process_id == -1) {
On 2012/12/06 19:31:15, miket wrote:
> Not a big deal, but why convert render_view_id to an int if you already know
> that you won't need it?

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:248: if
(incognito)
The lazy background pages may return -1 as render_process_id. In that case, we
identify the correct service normal/incognito and wake up the page.
On 2012/12/06 19:31:15, miket wrote:
> Is this logic out of order? Are you asking the non-incognito service for the
> extension in the incognito case?
> 
> I asked asargent about this, and he wasn't sure whether it was even necessary
to
> get the incognito service separately. Can you please confirm that this is
> correct, and if it is, put some kind of comment in here to let people know why
> it's weird?

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:259: //
This can happen if the host has gone away since the page was dispalyed.
On 2012/12/06 19:31:15, miket wrote:
> spelling

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:267: }
// namespace api
On 2012/12/06 19:31:15, miket wrote:
> vertical whitespace (please fix all)

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:15:
namespace extensions {
On 2012/12/06 19:31:15, miket wrote:
> Vertical whitespace inside each namespace open/close.

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:20: }
On 2012/12/06 19:31:15, miket wrote:
> These normally get commented with // namespace
name_of_namespace_that_is_closed,
> but use your judgment if the opening brace is nearby.

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:56: 
On 2012/12/06 19:31:15, miket wrote:
> Be consistent about vertical whitespace after access specifier (see public:
> above)

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:82:
content::RenderViewHost* deleting_rvh_;
On 2012/12/06 19:31:15, miket wrote:
> Avoid abbreviations if possible/practical.

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:92:
virtual ~DeveloperPrivateAutoUpdateFunction() {}
On 2012/12/06 19:31:15, miket wrote:
> See http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts. Don't
> inline virtual destructors!

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:103:
virtual ~DeveloperPrivateGetItemsInfoFunction() {}
On 2012/12/06 19:31:15, miket wrote:
> Same as above.

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:114:
virtual ~DeveloperPrivateInspectFunction() {}
On 2012/12/06 19:31:15, miket wrote:
> etc.

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:120: }
// namespace api
On 2012/12/06 19:31:15, miket wrote:
> vertical whitespace
> 
> always exactly two spaces before //

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
File
chrome/browser/extensions/api/developer_private/developer_private_api_factory.h
(right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api_factory.h:15:
class DeveloperPrivateAPIFactory : public ProfileKeyedServiceFactory {
On 2012/12/06 19:31:15, miket wrote:
> comment on class purpose, please

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
File
chrome/browser/extensions/api/developer_private/developer_private_apitest.cc
(right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_apitest.cc:21:
FilePath basedir = test_data_dir_.AppendASCII("developer");
On 2012/12/06 19:31:15, miket wrote:
> base_dir to be consistent

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_apitest.cc:27:
const std::string& name) {
I will be loading more test extensions here.
On 2012/12/06 19:31:15, miket wrote:
> wrapping probably not necessary here

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_apitest.cc:43:
std::map<std::string, std::string> extension_ids_;
On 2012/12/06 19:31:15, miket wrote:
> If you name this something like "extension_names_to_ids_" then you won't need
> the comment.

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/common/exte...
File chrome/common/extensions/api/developer_private.idl (right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/common/exte...
chrome/common/extensions/api/developer_private.idl:11: packaged_app,
the code generator for API's generates code as NAMESPACE_TYPE_NAME. thus
"hosted_app" is generated as DEVELOPER_PRIVATE_TYPE_HOSTED_APP while hostedApp
is generated as DEVELOPER_PRIVATE_TYPE_HOSTEDAPP.

Changing all the variables in the file to the underscore case.


On 2012/12/06 19:31:15, miket wrote:
> why the different naming style?

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/common/exte...
chrome/common/extensions/api/developer_private.idl:67: // Runs autoUpdate
immediately.
On 2012/12/06 19:31:15, miket wrote:
> no internal capitalization

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/common/exte...
chrome/common/extensions/api/developer_private.idl:68: // |callback| : Called
with the boolean result.
On 2012/12/06 19:31:15, miket wrote:
> this comment adds no information.

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/test/data/e...
File chrome/test/data/extensions/api_test/developer/packaged_app/manifest.json
(right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/test/data/e...
chrome/test/data/extensions/api_test/developer/packaged_app/manifest.json:10:
"offline_enabled": true
On 2012/12/06 19:31:15, miket wrote:
> You don't need this key anymore; it's true by default.

Done.

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/test/data/e...
File chrome/test/data/extensions/api_test/developer/test/basics.html (right):

https://chromiumcodereview.appspot.com/11428116/diff/15001/chrome/test/data/e...
chrome/test/data/extensions/api_test/developer/test/basics.html:2: * Copyright
(c) 2012222222222222222222222 The Chromium Authors. All rights reserved.  Use of
this
On 2012/12/06 19:31:15, miket wrote:
> This is too far in the future.

Done.

[miket_OOO, 2012-12-14 19:07:25]

lgtm

https://codereview.chromium.org/11428116/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/11428116/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/developer_private/developer_private_api.h:20: 
Hmmmm. I take back part of my earlier comment, as this looks weird. I think it
should be like this:

namespace x {

namespace foo {

namespace bar {
class Baz;
struct Qux;
}

}  // namespace foo

}  // namespace x

If the inner namespace is homogeneous (i.e., you can run it all together without
vertical whitespace), then the opening/closing braces can also be run together,
and because of that you don't need the closing comment on the closing brace.

https://codereview.chromium.org/11428116/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/developer_private/developer_private_api.h:28:
class ExtensionSystem;
You probably want to put this as close as possible to the *start* of this
namespace (i.e., up top), rather than at the end.

https://codereview.chromium.org/11428116/diff/24001/chrome/browser/extensions...
File
chrome/browser/extensions/api/developer_private/developer_private_api_factory.h
(right):

https://codereview.chromium.org/11428116/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/developer_private/developer_private_api_factory.h:16:
// DeveloperPrivateAPI class .
fix

https://codereview.chromium.org/11428116/diff/24001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/developer/packaged_app/main.html
(right):

https://codereview.chromium.org/11428116/diff/24001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/developer/packaged_app/main.html:2: *
Copyright (c) 2011 The Chromium Authors. All rights reserved.  Use of this
This is too far in the past.

https://codereview.chromium.org/11428116/diff/24001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/developer/packaged_app/main.js
(right):

https://codereview.chromium.org/11428116/diff/24001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/developer/packaged_app/main.js:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
Same.

https://codereview.chromium.org/11428116/diff/24001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/developer/simple_extension/pages/options.html
(right):

https://codereview.chromium.org/11428116/diff/24001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/developer/simple_extension/pages/options.html:2:
* Copyright (c) 2011 The Chromium Authors. All rights reserved.  Use of this
Two things: you need to upload with similarity=80 (or something higher than 50),
and the year is wrong.

[Gaurav, 2012-12-15 01:36:51]

Thanks Mike.

On 2012/12/14 19:07:25, miket wrote:
> lgtm
> 
>
https://codereview.chromium.org/11428116/diff/24001/chrome/browser/extensions...
> File chrome/browser/extensions/api/developer_private/developer_private_api.h
> (right):
> 
>
https://codereview.chromium.org/11428116/diff/24001/chrome/browser/extensions...
> chrome/browser/extensions/api/developer_private/developer_private_api.h:20: 
> Hmmmm. I take back part of my earlier comment, as this looks weird. I think it
> should be like this:
> 
> namespace x {
> 
> namespace foo {
> 
> namespace bar {
> class Baz;
> struct Qux;
> }
> 
> }  // namespace foo
> 
> }  // namespace x
> 
> If the inner namespace is homogeneous (i.e., you can run it all together
without
> vertical whitespace), then the opening/closing braces can also be run
together,
> and because of that you don't need the closing comment on the closing brace.
> 
>
https://codereview.chromium.org/11428116/diff/24001/chrome/browser/extensions...
> chrome/browser/extensions/api/developer_private/developer_private_api.h:28:
> class ExtensionSystem;
> You probably want to put this as close as possible to the *start* of this
> namespace (i.e., up top), rather than at the end.
> 
>
https://codereview.chromium.org/11428116/diff/24001/chrome/browser/extensions...
> File
>
chrome/browser/extensions/api/developer_private/developer_private_api_factory.h
> (right):
> 
>
https://codereview.chromium.org/11428116/diff/24001/chrome/browser/extensions...
>
chrome/browser/extensions/api/developer_private/developer_private_api_factory.h:16:
> // DeveloperPrivateAPI class .
> fix
> 
>
https://codereview.chromium.org/11428116/diff/24001/chrome/test/data/extensio...
> File chrome/test/data/extensions/api_test/developer/packaged_app/main.html
> (right):
> 
>
https://codereview.chromium.org/11428116/diff/24001/chrome/test/data/extensio...
> chrome/test/data/extensions/api_test/developer/packaged_app/main.html:2: *
> Copyright (c) 2011 The Chromium Authors. All rights reserved.  Use of this
> This is too far in the past.
> 
>
https://codereview.chromium.org/11428116/diff/24001/chrome/test/data/extensio...
> File chrome/test/data/extensions/api_test/developer/packaged_app/main.js
> (right):
> 
>
https://codereview.chromium.org/11428116/diff/24001/chrome/test/data/extensio...
> chrome/test/data/extensions/api_test/developer/packaged_app/main.js:1: //
> Copyright (c) 2011 The Chromium Authors. All rights reserved.
> Same.
> 
>
https://codereview.chromium.org/11428116/diff/24001/chrome/test/data/extensio...
> File
>
chrome/test/data/extensions/api_test/developer/simple_extension/pages/options.html
> (right):
> 
>
https://codereview.chromium.org/11428116/diff/24001/chrome/test/data/extensio...
>
chrome/test/data/extensions/api_test/developer/simple_extension/pages/options.html:2:
> * Copyright (c) 2011 The Chromium Authors. All rights reserved.  Use of this
> Two things: you need to upload with similarity=80 (or something higher than
50),
> and the year is wrong.

[Gaurav, 2012-12-15 01:37:01]

https://chromiumcodereview.appspot.com/11428116/diff/24001/chrome/browser/ext...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://chromiumcodereview.appspot.com/11428116/diff/24001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:20: 
On 2012/12/14 19:07:25, miket wrote:
> Hmmmm. I take back part of my earlier comment, as this looks weird. I think it
> should be like this:
> 
> namespace x {
> 
> namespace foo {
> 
> namespace bar {
> class Baz;
> struct Qux;
> }
> 
> }  // namespace foo
> 
> }  // namespace x
> 
> If the inner namespace is homogeneous (i.e., you can run it all together
without
> vertical whitespace), then the opening/closing braces can also be run
together,
> and because of that you don't need the closing comment on the closing brace.

Done.

https://chromiumcodereview.appspot.com/11428116/diff/24001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.h:28:
class ExtensionSystem;
On 2012/12/14 19:07:25, miket wrote:
> You probably want to put this as close as possible to the *start* of this
> namespace (i.e., up top), rather than at the end.

Done.

[commit-bot: I haz the power, 2012-12-15 01:38:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/11428116/36001

[commit-bot: I haz the power, 2012-12-15 01:38:56]

Can't process patch for file
chrome/test/data/extensions/api_test/developer/simple_extension/icon_128.png.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[commit-bot: I haz the power, 2012-12-15 01:58:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/11428116/33032

[commit-bot: I haz the power, 2012-12-15 02:57:11]

Retried try job too often on linux_chromeos for step(s) unit_tests

[commit-bot: I haz the power, 2012-12-17 21:28:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/11428116/43001

[commit-bot: I haz the power, 2012-12-17 22:54:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-17 22:54:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/11428116/54001

[commit-bot: I haz the power, 2012-12-18 00:59:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/11428116/54001

[commit-bot: I haz the power, 2012-12-18 01:34:38]

Change committed as 173612

[Gaurav, 2012-12-18 04:39:59]

New CL fixing the problem here https://codereview.chromium.org/11620011/

https://chromiumcodereview.appspot.com/11428116/diff/54001/chrome/browser/ext...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://chromiumcodereview.appspot.com/11428116/diff/54001/chrome/browser/ext...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:190:
source_profile = content::Source<Profile>(source).ptr();
This caused the clang build to break, as source_profile is not yet used.
Updating in a new CL 11620011