Refactor APIResourceController to ProfileKeyedService.

Refactored away the abomination of poor object-oriented design that was APIResourceController.

- Removed APIResourceController from ExtensionService.
- Conformed "API" capitalization to match style guide.
- Introduced ApiResourceManager as the successor to APIResourceController. Each instance knows about exactly one ApiResource type, which allows us to avoid the horrible circular dependencies and static_casts<> of APIResourceController.
- Made each ApiResourceManager a ProfileKeyedService so that it's clearer that they're bound to a single Profile. I didn't see any benefit to moving the resource destructors to PKS's Shutdown method, so I didn't do that.

RECOMMENDED METHOD OF CONSUMPTION OF THIS CHANGELIST FOR MAXIMUM ENJOYMENT:

1. Understand what APIResourceController was trying to do.
2. See that APIResourceController is now gone.
3. Review the new ApiResourceManager template.
4. Confirm that all usage of APIRC is replaced with ApiRM<> usage.
5. Scan the remainder for API -> Api changes.

BUG=112902
TEST=updated

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

[miket_OOO, 2012-07-13 22:59:33]

Have fun.

[asargent_no_longer_on_chrome, 2012-07-16 18:08:25]

LGTM w/ a bunch of nits and questions

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
File chrome/browser/extensions/api/api_function.h (right):

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
chrome/browser/extensions/api/api_function.h:23: // specialized.
comment nit: when I first read this I had a hard time imagining what it would be
useful for, but then I saw that in practice you use it for common work in a
superclass. I guess that's what you mean by (paraphrasing) "not as specialized".
Consider explicitly menioning this, e.g. something like:

// Like Prepare(). A useful place to put common work in an
// ApiFunction superclass that multiple api functions want to
// share"

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
File chrome/browser/extensions/api/api_resource_manager.h (right):

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
chrome/browser/extensions/api/api_resource_manager.h:20: class
ApiResourceManager : public ProfileKeyedService,
nit: please add class overview comment

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
chrome/browser/extensions/api/api_resource_manager.h:76: // harder for
extensions to peek into each others' resources.
Another possibility would be to have one ApiResourceController instance per
extension - that might make it simpler to avoid forgetting to check the
extension id, remember to clean up all resources on extension disable/uninstall,
etc. It might create extra complexity elsewhere though so you might need to
start writing the code to see which way is better.

Either way, I think it's important that we try to prevent apps/extensions from
interfering with each other's resources by accidentally passing the wrong
resource id.

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:48: void
SocketExtensionFunction::Work() {
nit: why do we bother overriding the default AsyncApiFunction::Work() function
in this SocketExtensionFunction superclass? It looks like all of the subclasses
of SocketExtensionFunction override it anyway, but even if they didn't it's not
like we do anything different here.

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.h (right):

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.h:28: class
SocketExtensionFunction : public AsyncApiFunction {
would it make sense to name this "SocketAsyncApiFunction" for consistency with
the SerialAsyncApiFunction superclass you added for the serial API?

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_system.cc (right):

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
chrome/browser/extensions/extension_system.cc:287:
serial_connection_manager_->set_thread_id(BrowserThread::FILE);
Had you considered making the thread id either an argument to the constructor or
one of the template arguments? Then when creating these guys you'd do something
like: 


serial_connection_manager.reset(
    new ApiResourceManager<SerialConnection>(
                           BrowserThread::FILE))

or

serial_connection_manager.reset(
    new ApiResourceManager<SerialConnection,
                           BrowserThread::FILE>())

It seems like ApiResourceManager is designed with the assumption that the thread
id should not be changed once set, and this would help you to enforce that.

http://codereview.chromium.org/10777003/diff/5038/chrome/browser/extensions/a...
File chrome/browser/extensions/api/api_resource_manager.h (right):

http://codereview.chromium.org/10777003/diff/5038/chrome/browser/extensions/a...
chrome/browser/extensions/api/api_resource_manager.h:86: std::map<int,
linked_ptr<T> >* api_resource_map_;
Do we just leak the contents of these on shutdown? It would be nice to fix this.
(BTW, you'd probably get it for free if you free'd resources in a listener for
EXTENSION_DISABLED / EXTENSION_UNINSTALLED notifications)

[miket_OOO, 2012-07-16 19:46:02]

Thanks. Responses inline. If the tree ever goes green again I'll try to get this
committed as updated with your suggestions.

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
File chrome/browser/extensions/api/api_resource_manager.h (right):

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
chrome/browser/extensions/api/api_resource_manager.h:76: // harder for
extensions to peek into each others' resources.
On 2012/07/16 18:08:25, Antony Sargent wrote:
> Another possibility would be to have one ApiResourceController instance per
> extension - that might make it simpler to avoid forgetting to check the
> extension id, remember to clean up all resources on extension
disable/uninstall,
> etc. It might create extra complexity elsewhere though so you might need to
> start writing the code to see which way is better.
> 
> Either way, I think it's important that we try to prevent apps/extensions from
> interfering with each other's resources by accidentally passing the wrong
> resource id. 

I've entered crbug.com/137539 to track more loudly.

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:48: void
SocketExtensionFunction::Work() {
> nit: why do we bother overriding the default AsyncApiFunction::Work() function
> in this SocketExtensionFunction superclass? It looks like all of the
subclasses
> of SocketExtensionFunction override it anyway, but even if they didn't it's
not
> like we do anything different here.

Originally I believe it did do work, but we've since moved its functionality
elsewhere. I cleaned it up.

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.h (right):

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.h:28: class
SocketExtensionFunction : public AsyncApiFunction {
On 2012/07/16 18:08:25, Antony Sargent wrote:
> would it make sense to name this "SocketAsyncApiFunction" for consistency with
> the SerialAsyncApiFunction superclass you added for the serial API?

Done, and confirmed that usb devices are following the same scheme.

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_system.cc (right):

http://codereview.chromium.org/10777003/diff/17001/chrome/browser/extensions/...
chrome/browser/extensions/extension_system.cc:287:
serial_connection_manager_->set_thread_id(BrowserThread::FILE);
On 2012/07/16 18:08:25, Antony Sargent wrote:
> Had you considered making the thread id either an argument to the constructor
or
> one of the template arguments?

That's a good idea. I had something in my head about not everyone wanting to
specify the thread, but that's a YAGNI.

[miket_OOO, 2012-07-16 20:21:28]

http://codereview.chromium.org/10777003/diff/5038/chrome/browser/extensions/a...
File chrome/browser/extensions/api/api_resource_manager.h (right):

http://codereview.chromium.org/10777003/diff/5038/chrome/browser/extensions/a...
chrome/browser/extensions/api/api_resource_manager.h:86: std::map<int,
linked_ptr<T> >* api_resource_map_;
> Do we just leak the contents of these on shutdown? It would be nice to fix
this.
> (BTW, you'd probably get it for free if you free'd resources in a listener for
> EXTENSION_DISABLED / EXTENSION_UNINSTALLED notifications)

Oops, I missed this comment. I'll have a look.

[miket_OOO, 2012-07-19 18:49:07]

http://codereview.chromium.org/10777003/diff/5038/chrome/browser/extensions/a...
File chrome/browser/extensions/api/api_resource_manager.h (right):

http://codereview.chromium.org/10777003/diff/5038/chrome/browser/extensions/a...
chrome/browser/extensions/api/api_resource_manager.h:86: std::map<int,
linked_ptr<T> >* api_resource_map_;
On 2012/07/16 18:08:25, Antony Sargent wrote:
> Do we just leak the contents of these on shutdown? It would be nice to fix
this.
> (BTW, you'd probably get it for free if you free'd resources in a listener for
> EXTENSION_DISABLED / EXTENSION_UNINSTALLED notifications)

OK, I've had my look. I'm not understanding the comment. When the
ApiResourceManager is deleted, the BrowserThread::DeleteSoon in the destructor
gets called. And the ApiResourceManager itself is owned in a scoped_ptr by
ExtensionSystemImpl. Is there a missed handoff that I've overlooked?

[asargent_no_longer_on_chrome, 2012-07-19 20:52:21]

http://codereview.chromium.org/10777003/diff/5038/chrome/browser/extensions/a...
File chrome/browser/extensions/api/api_resource_manager.h (right):

http://codereview.chromium.org/10777003/diff/5038/chrome/browser/extensions/a...
chrome/browser/extensions/api/api_resource_manager.h:86: std::map<int,
linked_ptr<T> >* api_resource_map_;
On 2012/07/19 18:49:07, miket wrote:
> On 2012/07/16 18:08:25, Antony Sargent wrote:
> > Do we just leak the contents of these on shutdown? It would be nice to fix
> this.
> > (BTW, you'd probably get it for free if you free'd resources in a listener
for
> > EXTENSION_DISABLED / EXTENSION_UNINSTALLED notifications)
> 
> OK, I've had my look. I'm not understanding the comment. When the
> ApiResourceManager is deleted, the BrowserThread::DeleteSoon in the destructor
> gets called. And the ApiResourceManager itself is owned in a scoped_ptr by
> ExtensionSystemImpl. Is there a missed handoff that I've overlooked? 
Oops, I just hadn't noticed the DeleteSoon call in the destructor. My apologies
- mistakes were made.

[miket_OOO, 2012-07-19 20:55:38]

> Oops, I just hadn't noticed the DeleteSoon call in the destructor. My
apologies
> - mistakes were made.

Cool. It is a bit nonstandard because many of these I/O-or file-specific
resources expect special treatment thread-wise, so we can't just throw them into
our usual container classes and expect destruction to work.

[commit-bot: I haz the power, 2012-07-20 00:09:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/10777003/36003

[commit-bot: I haz the power, 2012-07-20 01:22:08]

Change committed as 147589