Use JSON schema compiler in chrome.management code

Use JSON schema compiler in chrome.management code

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

[Aaron Boodman, 2012-08-09 13:17:19]

Nice cleanup

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/management/management_api.cc (right):

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:58: namespace Get =
extensions::api::management::Get;
I think it would be more clear to just do:

using extensions::Management.

Then we don't have to list these all individually, and it will also be more
obvious where things like GetAll come from.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:77: typedef
std::vector<linked_ptr<ExtensionInfo> > ExtensionInfoList;
ScopedVector<T> is now preferred over std::vector<linked_ptr<T> >. It does
essentially the same thing.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:88: void
CreateWarningsList(const Extension* extension,
You can just return std::vector<std::string>. It's easier to read, the copy
doesn't matter here, and is usually optimized away by the compiler anyway.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:107: static
scoped_ptr<ExtensionInfo> CreateExtensionInfo(
Can you move this into the anonymous namespace? We prefer that over static
global functions.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:118:
info->options_url = extension.options_url().possibly_invalid_spec();
I think just spec() will do? Same thing below.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:121:
info->may_disable = ExtensionSystem::Get(service->profile())->
Pass ExtensionSystem* instead into this function. You can get ExtensionService*
from it.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:125: if
(!info->enabled) {
Flip this check to put the positive branch first.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:155: &extension,
Put multiple arguments on one line, so long as the first arg of each line
aligns.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:208: static void
AddExtensionInfo(const ExtensionSet& extensions,
Same comment as above about anon namespace.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:627: std::string json
= "";
No need to initialize non-primitives.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:642: //
OnInstalled::ToJson, OnEnabled::ToJson, and OnDisabled::ToJson are all
This is brittle - if one of these changed to be non-equal, the person writing
the change would not know about this dependency.

I prefer the extra code and just having another branch (or you can get fancy and
use a function pointer to call the right version of ToJson).

[mitchellwrosen, 2012-08-09 18:15:41]

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/management/management_api.cc (right):

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:58: namespace Get =
extensions::api::management::Get;
On 2012/08/09 13:17:19, Aaron Boodman wrote:
> I think it would be more clear to just do:
> 
> using extensions::Management.
> 
> Then we don't have to list these all individually, and it will also be more
> obvious where things like GetAll come from.

Do you mean using namespace extensions::api::management?

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:77: typedef
std::vector<linked_ptr<ExtensionInfo> > ExtensionInfoList;
On 2012/08/09 13:17:19, Aaron Boodman wrote:
> ScopedVector<T> is now preferred over std::vector<linked_ptr<T> >. It does
> essentially the same thing.

Hm. For now, the JSC uses std::vector<std::linked_ptr>>. Should that be changed?

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:88: void
CreateWarningsList(const Extension* extension,
On 2012/08/09 13:17:19, Aaron Boodman wrote:
> You can just return std::vector<std::string>. It's easier to read, the copy
> doesn't matter here, and is usually optimized away by the compiler anyway.

Done.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:107: static
scoped_ptr<ExtensionInfo> CreateExtensionInfo(
On 2012/08/09 13:17:19, Aaron Boodman wrote:
> Can you move this into the anonymous namespace? We prefer that over static
> global functions.

Done.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:118:
info->options_url = extension.options_url().possibly_invalid_spec();
On 2012/08/09 13:17:19, Aaron Boodman wrote:
> I think just spec() will do? Same thing below.

Done.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:121:
info->may_disable = ExtensionSystem::Get(service->profile())->
On 2012/08/09 13:17:19, Aaron Boodman wrote:
> Pass ExtensionSystem* instead into this function. You can get
ExtensionService*
> from it.

Done.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:125: if
(!info->enabled) {
On 2012/08/09 13:17:19, Aaron Boodman wrote:
> Flip this check to put the positive branch first.

Done.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:155: &extension,
On 2012/08/09 13:17:19, Aaron Boodman wrote:
> Put multiple arguments on one line, so long as the first arg of each line
> aligns.

Agh, that's how I did it, but was told by another reviewer that it's either all
on one line or one per line.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:208: static void
AddExtensionInfo(const ExtensionSet& extensions,
On 2012/08/09 13:17:19, Aaron Boodman wrote:
> Same comment as above about anon namespace.

Done.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:627: std::string json
= "";
On 2012/08/09 13:17:19, Aaron Boodman wrote:
> No need to initialize non-primitives.

Done.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:642: //
OnInstalled::ToJson, OnEnabled::ToJson, and OnDisabled::ToJson are all
On 2012/08/09 13:17:19, Aaron Boodman wrote:
> This is brittle - if one of these changed to be non-equal, the person writing
> the change would not know about this dependency.
> 
> I prefer the extra code and just having another branch (or you can get fancy
and
> use a function pointer to call the right version of ToJson).

Good point. Done.

[Aaron Boodman, 2012-08-10 06:12:36]

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/management/management_api.cc (right):

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:58: namespace Get =
extensions::api::management::Get;
On 2012/08/09 18:15:41, mitchellwrosen wrote:
> On 2012/08/09 13:17:19, Aaron Boodman wrote:
> > I think it would be more clear to just do:
> > 
> > using extensions::Management.
> > 
> > Then we don't have to list these all individually, and it will also be more
> > obvious where things like GetAll come from.
> 
> Do you mean using namespace extensions::api::management?

I meant:

using extensions::management.

So you'd have to write, e.g., management::GetAll::<whatever>.

I know this is different than what we've done in other places, but it has the
advantages:

- one line.
- easy to see where symbols come from.
- namespaces in Chrome are typically lower case.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:77: typedef
std::vector<linked_ptr<ExtensionInfo> > ExtensionInfoList;
On 2012/08/09 18:15:41, mitchellwrosen wrote:
> On 2012/08/09 13:17:19, Aaron Boodman wrote:
> > ScopedVector<T> is now preferred over std::vector<linked_ptr<T> >. It does
> > essentially the same thing.
> 
> Hm. For now, the JSC uses std::vector<std::linked_ptr>>. Should that be
changed?

Ah, makes sense. Nevermind.

http://codereview.chromium.org/10834160/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/management/management_api.cc:155: &extension,
On 2012/08/09 18:15:41, mitchellwrosen wrote:
> On 2012/08/09 13:17:19, Aaron Boodman wrote:
> > Put multiple arguments on one line, so long as the first arg of each line
> > aligns.
> 
> Agh, that's how I did it, but was told by another reviewer that it's either
all
> on one line or one per line.

Don't think so:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

[mitchellwrosen, 2012-08-10 18:10:13]

Meh, DispatchEventToRenderer's signature changed, it no longer takes the event
args as json.

https://chromiumcodereview.appspot.com/10834160/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://chromiumcodereview.appspot.com/10834160/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/management/management_api.cc:58: namespace Get =
extensions::api::management::Get;
On 2012/08/10 06:12:36, Aaron Boodman wrote:
> On 2012/08/09 18:15:41, mitchellwrosen wrote:
> > On 2012/08/09 13:17:19, Aaron Boodman wrote:
> > > I think it would be more clear to just do:
> > > 
> > > using extensions::Management.
> > > 
> > > Then we don't have to list these all individually, and it will also be
more
> > > obvious where things like GetAll come from.
> > 
> > Do you mean using namespace extensions::api::management?
> 
> I meant:
> 
> using extensions::management.
> 
> So you'd have to write, e.g., management::GetAll::<whatever>.
> 
> I know this is different than what we've done in other places, but it has the
> advantages:
> 
> - one line.
> - easy to see where symbols come from.
> - namespaces in Chrome are typically lower case.

Oh, I gotcha. namespace management = extensions::api::management, just like
brettw wanted in bookmarks code. I agree that it's much better looking than the
old style

[Aaron Boodman, 2012-08-10 18:51:34]

lgtm

[commit-bot: I haz the power, 2012-08-10 18:51:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10834160/9001

[commit-bot: I haz the power, 2012-08-10 20:17:40]

Change committed as 151113