Cleanup extension permissions module.

Cleanup extension permissions module.

- renames ExtensionPermissionsManager to extensions::PermissionsUpdater
- PermissionsUpdaters are now created on demand (vs one per ExtensionService)
- move some includes into the *.cc files
- moves a couple common methods to extension_permissions_api_helpers.cc 

BUG=none
TEST=existing

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

[jstritar, 2011-11-07 20:47:42]

http://codereview.chromium.org/8493017/diff/1/chrome/browser/download/chrome_...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8493017/diff/1/chrome/browser/download/chrome_...
chrome/browser/download/chrome_download_manager_delegate.cc:31: #include
"chrome/common/chrome_notification_types.h"
I guess these were getting included from extension_service.h that was including
extension_permissions_api.h

[Aaron Boodman, 2011-11-14 19:26:05]

Whenever you find yourself having a dependency on ExtensionService, you should
stop and think if it's really necessary or if it is just playing the role of a
god object (http://en.wikipedia.org/wiki/God_object).

In the future, it would be cool to eliminate the ES dependency from
ExtensionPermissionUpdater, but it looks like it is too non-trivial to do in
this CL.

http://codereview.chromium.org/8493017/diff/1024/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_permissions_api.cc (right):

http://codereview.chromium.org/8493017/diff/1024/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.cc:395:
perms_updater.AddPermissions(GetExtension(), requested_permissions_.get());
I can't remember - if an extension is unloaded, does GetExtension() start
returning NULL? I think it might. In that case, you'll need to NULL-check this.

http://codereview.chromium.org/8493017/diff/1024/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_permissions_api.h (right):

http://codereview.chromium.org/8493017/diff/1024/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.h:22: class
ExtensionPermissionsUpdater {
In general, it's not great to have more than one top-level class in a .h file
because it makes it harder to find definitions when reading sources.

Can you move this out into its own file?

[jstritar, 2011-12-12 22:28:03]

I took care of your comments and got rid of the ExtensionService dependency on
the PermissionUpdater. It now depends on a Profile, but this is really just so
we can source the Notification correctly.

http://codereview.chromium.org/8493017/diff/1024/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_permissions_api.cc (right):

http://codereview.chromium.org/8493017/diff/1024/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.cc:395:
perms_updater.AddPermissions(GetExtension(), requested_permissions_.get());
On 2011/11/14 19:26:05, Aaron Boodman wrote:
> I can't remember - if an extension is unloaded, does GetExtension() start
> returning NULL? I think it might. In that case, you'll need to NULL-check
this.

The ExtensionFunction has a scoped_refptr to the Extension, so I think it should
stay around.

http://codereview.chromium.org/8493017/diff/1024/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_permissions_api.h (right):

http://codereview.chromium.org/8493017/diff/1024/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.h:22: class
ExtensionPermissionsUpdater {
On 2011/11/14 19:26:05, Aaron Boodman wrote:
> In general, it's not great to have more than one top-level class in a .h file
> because it makes it harder to find definitions when reading sources.
> 
> Can you move this out into its own file?

Done.

[Aaron Boodman, 2011-12-14 01:00:14]

Sorry for lagging on this for long. Looks cool.

http://codereview.chromium.org/8493017/diff/12003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_permissions_api_helpers.h (right):

http://codereview.chromium.org/8493017/diff/12003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_permissions_api_helpers.h:18: namespace
extension_permissions_api {
I'd prefer extensions::permissions_api::Pack... for consistency.

http://codereview.chromium.org/8493017/diff/12003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_permissions_api_helpers.h:25: bool
UnpackPermissionsFromValue(base::DictionaryValue* value,
These functions seem like the kind of thing that would be easy to unit test.

http://codereview.chromium.org/8493017/diff/12003/chrome/browser/extensions/p...
File chrome/browser/extensions/permissions_updater.h (right):

http://codereview.chromium.org/8493017/diff/12003/chrome/browser/extensions/p...
chrome/browser/extensions/permissions_updater.h:23: class PermissionsUpdater {
Unit test for this class? Also a class-level comment would be good.

[jstritar, 2012-01-04 21:53:32]

Sorry for lagging on updating this as well. Thanks for reviewing!

http://codereview.chromium.org/8493017/diff/12003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_permissions_api_helpers.h (right):

http://codereview.chromium.org/8493017/diff/12003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_permissions_api_helpers.h:18: namespace
extension_permissions_api {
On 2011/12/14 01:00:14, Aaron Boodman wrote:
> I'd prefer extensions::permissions_api::Pack... for consistency.

Done.

http://codereview.chromium.org/8493017/diff/12003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_permissions_api_helpers.h:25: bool
UnpackPermissionsFromValue(base::DictionaryValue* value,
On 2011/12/14 01:00:14, Aaron Boodman wrote:
> These functions seem like the kind of thing that would be easy to unit test.

Done.

http://codereview.chromium.org/8493017/diff/12003/chrome/browser/extensions/p...
File chrome/browser/extensions/permissions_updater.h (right):

http://codereview.chromium.org/8493017/diff/12003/chrome/browser/extensions/p...
chrome/browser/extensions/permissions_updater.h:23: class PermissionsUpdater {
On 2011/12/14 01:00:14, Aaron Boodman wrote:
> Unit test for this class? Also a class-level comment would be good.

Done.

[Aaron Boodman, 2012-01-06 02:51:09]

lgtm

[commit-bot: I haz the power, 2012-01-06 15:06:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/8493017/32002

[commit-bot: I haz the power, 2012-01-06 15:06:35]

Can't process patch for file
chrome/browser/tab_contents/render_view_host_delegate_helper.cc.
File's status is None, patchset upload is incomplete.

[jstritar, 2012-01-06 15:48:14]

+sky for chrome/browser/ui OWNERS check

[sky, 2012-01-06 16:41:20]

chrome/browser/ui LGTM