Remove GetExtensionService from Profile.

Remove GetExtensionService calls and replace with PKSF calls through the ExtensionSystem. Extension services were made into ProfileKeyedServices a while ago; this replaces the deprecated calls that ran directly through the Profile object.

This is about half of the total GetExtensionService calls in the codebase, but this CL is already ridiculously large.

BUG= http://code.google.com/p/chromium/issues/detail?id=104095


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

[Miranda Callahan, 2012-11-20 17:53:39]

This is a refactoring CL, so it's very large, but very nonthreatening, I hope.
There are many, many OWNERS, so hopefully this won't be very onerous for anyone.
Here is the division of labor:

MAIN REVIEWER: yoz

OWNERS:
sky: autocomplete, chrome_content_browser_client.cc,
chrome_render_view_host_observer.cc,  template_url_service.cc,
chrome/browser/ui/ash, browser.cc,  browser_browsertest.cc,
startup_browser_creator_browsertest.cc,  chrome/browser/ui/views 
phadjan.jr: automation, chrome/browser/download
rickcam: background
dmazzoni: chromeos/accessibility
yoz: chromeos/extensions, chrome/browser/extensions
rkc: chromeos/kiosk_mode 
davemoore: chromeos/login, chromeos/offline
yurys: chrome/browser/debugger 
bulach: chrome/browser/geolocation 
stevenjb: chrome/browser/notifications 
hans: chrome/browser/speech 
akalin: chrome/browser/sync 
avi: chrome/browser/tab_contents, chrome/browser/ui/cocoa 
yoshiki: chrome/browser/task_manager 
pkotwicz: chrome/browser/themes 
xiyuan: chrome/browser/ui/app_list 
erg: chrome/browser/ui/gtk 
groby: chrome/browser/ui/intents
arv: chrome/browser/ui/webui
thakis: chrome.gyp

[Elliot Glaysher, 2012-11-20 18:18:39]

gtk lgtm

[akalin, 2012-11-20 18:22:25]

sync lgtm

[bulach, 2012-11-20 18:22:57]

geolocation lgtm, thanks!

[hans, 2012-11-20 18:47:18]

speech lgtm

[dmazzoni, 2012-11-20 18:48:30]

accessibility lgtm

[stevenjb, 2012-11-20 18:48:53]

chrome/browser/notifications LGTM

[rkc, 2012-11-20 19:46:26]

On 2012/11/20 18:48:53, stevenjb (chromium) wrote:
> chrome/browser/notifications LGTM

kiosk mode lgtm

[sky, 2012-11-20 21:04:54]

https://chromiumcodereview.appspot.com/11365181/diff/15019/chrome/browser/chr...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/11365181/diff/15019/chrome/browser/chr...
chrome/browser/chrome_content_browser_client.cc:643: ExtensionService*
extension_service =
Does ::Get(profile) deal with NULL? If passed NULL results in always returning
NULL then I believe you can remove !profile from 645.

[Miranda Callahan, 2012-11-20 21:36:09]

Thanks, Scott -- yes, Get(Profile) does correctly return NULL for this case.
Fixed.

https://chromiumcodereview.appspot.com/11365181/diff/15019/chrome/browser/chr...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/11365181/diff/15019/chrome/browser/chr...
chrome/browser/chrome_content_browser_client.cc:643: ExtensionService*
extension_service =
On 2012/11/20 21:04:55, sky wrote:
> Does ::Get(profile) deal with NULL? If passed NULL results in always returning
> NULL then I believe you can remove !profile from 645.

Done. Also removed from 674 and 1808.

[sky, 2012-11-20 22:12:59]

LGTM

[Yoyo Zhou, 2012-11-21 00:27:12]

Nice progress so far.

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/browser/ext...
File chrome/browser/extensions/settings/settings_frontend_unittest.cc (right):

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/browser/ext...
chrome/browser/extensions/settings/settings_frontend_unittest.cc:97:
ExtensionServiceInterface* esi = static_cast<ExtensionServiceInterface*>(
I don't think you need this static_cast (here and below).

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/browser/ext...
File chrome/browser/extensions/settings/settings_sync_unittest.cc (right):

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/browser/ext...
chrome/browser/extensions/settings/settings_sync_unittest.cc:230:
ExtensionServiceInterface* esi = static_cast<ExtensionServiceInterface*>(
ditto

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/browser/ext...
File chrome/browser/extensions/settings/settings_test_util.cc (right):

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/browser/ext...
chrome/browser/extensions/settings/settings_test_util.cc:123:
static_cast<ExtensionServiceInterface*>(&extension_service_);
ditto

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/browser/ext...
File chrome/browser/extensions/settings/settings_test_util.h (right):

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/browser/ext...
chrome/browser/extensions/settings/settings_test_util.h:74: virtual
ExtensionService* extension_service() OVERRIDE;
This doesn't feel right, in the places that it's used. Maybe what we need is to
also move GetMockExtensionService to this class?

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/browser/ext...
chrome/browser/extensions/settings/settings_test_util.h:96: MockExtensionService
extension_service_;
If this isn't being used anymore, you can remove it.

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/browser/ui/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/browser/ui/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:459:
extensions::ExtensionPrefs::LAUNCH_DEFAULT);
nit: indent

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/browser/ui/...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/browser/ui/...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:512: // The
|extension_service| can be NULL in Incognito.
This comment is wrong. (However, I would not delete the check below it.)

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://chromiumcodereview.appspot.com/11365181/diff/22115/chrome/chrome.gyp#...
chrome/chrome.gyp:195: 'common/extensions/api/api.gyp:api',
I don't know what this is for.

[yurys, 2012-11-21 06:34:30]

debugger lgtm!

[yoshiki, 2012-11-26 16:29:30]

task_manager lgtm

[pkotwicz, 2012-11-26 16:35:35]

themes LGTM

[DaveMoore, 2012-11-26 17:13:17]

chromeos lgtm

[xiyuan, 2012-11-26 17:40:16]

app_list LGTM

[Paweł Hajdan Jr., 2012-11-26 18:21:46]

automation & download LGTM

nit: I think it'd be better to just ask chrome/-wide OWNERS if possible. I don't
mind at all, but it just might work better.

[Miranda Callahan, 2012-11-26 22:14:23]

Thanks, all -- and thanks especially for your comments, Yoz -- please take
another look.

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/extensions...
File chrome/browser/extensions/settings/settings_frontend_unittest.cc (right):

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/extensions...
chrome/browser/extensions/settings/settings_frontend_unittest.cc:97:
ExtensionServiceInterface* esi = static_cast<ExtensionServiceInterface*>(
On 2012/11/21 00:27:13, Yoyo Zhou wrote:
> I don't think you need this static_cast (here and below).

Done.

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/extensions...
File chrome/browser/extensions/settings/settings_sync_unittest.cc (right):

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/extensions...
chrome/browser/extensions/settings/settings_sync_unittest.cc:230:
ExtensionServiceInterface* esi = static_cast<ExtensionServiceInterface*>(
On 2012/11/21 00:27:13, Yoyo Zhou wrote:
> ditto

Done.

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/extensions...
File chrome/browser/extensions/settings/settings_test_util.cc (right):

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/extensions...
chrome/browser/extensions/settings/settings_test_util.cc:123:
static_cast<ExtensionServiceInterface*>(&extension_service_);
On 2012/11/21 00:27:13, Yoyo Zhou wrote:
> ditto

Unfortunately, all this casting is needed, as MockExtensionService ISA
TestExtensionService ISA ExtensionServiceInterface -- this is the common
ancestor of the Mock and the actual ExtensionService. Which is just icky, but
maybe refactoring this can be in a separate CL.

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/extensions...
File chrome/browser/extensions/settings/settings_test_util.h (right):

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/extensions...
chrome/browser/extensions/settings/settings_test_util.h:74: virtual
ExtensionService* extension_service() OVERRIDE;
On 2012/11/21 00:27:13, Yoyo Zhou wrote:
> This doesn't feel right, in the places that it's used. Maybe what we need is
to
> also move GetMockExtensionService to this class?

Hmm -- this would be nice, but unfortunately I don't think this will properly
exercise the tests. The issue is that when we use a MockExtensionSystem, we set
it as the Testing system through
ExtensionSystemFactory->SetTestingFactoryAndUse(MockExtensionSystem). This means
that every class that calls
extension::ExtensionSystem::Get(profile)->extension_service() will seamlessly
get the testing MockExtensionSystem, and call through to get the
MockExtensionService. At points in the test where we need the
MockExtensionService's ability to add an extension with a certain ID type, we
can downcast and trigger that capability. But it also means that code in the
general codebase that doesn't need that ability will Just Work -- for example,
the call on lines 174-176 of settings_frontend.cc.

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:459:
extensions::ExtensionPrefs::LAUNCH_DEFAULT);
On 2012/11/21 00:27:13, Yoyo Zhou wrote:
> nit: indent

Done.

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/ui/gtk/bro...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/ui/gtk/bro...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:512: // The
|extension_service| can be NULL in Incognito.
On 2012/11/21 00:27:13, Yoyo Zhou wrote:
> This comment is wrong. (However, I would not delete the check below it.)

Done.

https://codereview.chromium.org/11365181/diff/22115/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/11365181/diff/22115/chrome/chrome.gyp#newcode195
chrome/chrome.gyp:195: 'common/extensions/api/api.gyp:api',
On 2012/11/21 00:27:13, Yoyo Zhou wrote:
> I don't know what this is for.

When I added an #include extension_system.h to the debugger code, all of a
sudden I was getting an error that the usb.h file could not be found. Adding
this api.gyp dependency caused the missing .h file to be generated. Whether this
dependency should exist in the first place is another question; it's not an area
of the code I'm familiar with at all.

[Yoyo Zhou, 2012-11-26 22:58:03]

LGTM

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/extensions...
File chrome/browser/extensions/settings/settings_test_util.cc (right):

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/extensions...
chrome/browser/extensions/settings/settings_test_util.cc:123:
static_cast<ExtensionServiceInterface*>(&extension_service_);
On 2012/11/26 22:14:24, Miranda Callahan wrote:
> On 2012/11/21 00:27:13, Yoyo Zhou wrote:
> > ditto
> 
> Unfortunately, all this casting is needed, as MockExtensionService ISA
> TestExtensionService ISA ExtensionServiceInterface -- this is the common
> ancestor of the Mock and the actual ExtensionService. Which is just icky, but
> maybe refactoring this can be in a separate CL.

You don't need it for the upcasting on this line, I thought.

https://codereview.chromium.org/11365181/diff/22115/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/11365181/diff/22115/chrome/chrome.gyp#newcode195
chrome/chrome.gyp:195: 'common/extensions/api/api.gyp:api',
On 2012/11/26 22:14:24, Miranda Callahan wrote:
> On 2012/11/21 00:27:13, Yoyo Zhou wrote:
> > I don't know what this is for.
> 
> When I added an #include extension_system.h to the debugger code, all of a
> sudden I was getting an error that the usb.h file could not be found. Adding
> this api.gyp dependency caused the missing .h file to be generated. Whether
this
> dependency should exist in the first place is another question; it's not an
area
> of the code I'm familiar with at all.

Ah... yes, this is a sign of a bigger problem that's not in the scope of this
refactoring.

[Nico, 2012-11-26 23:31:28]

chrome.gyp lgtm

[Miranda Callahan, 2012-11-29 17:36:34]

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/extensions...
File chrome/browser/extensions/settings/settings_test_util.cc (right):

https://codereview.chromium.org/11365181/diff/22115/chrome/browser/extensions...
chrome/browser/extensions/settings/settings_test_util.cc:123:
static_cast<ExtensionServiceInterface*>(&extension_service_);
On 2012/11/26 22:58:04, Yoyo Zhou wrote:
> On 2012/11/26 22:14:24, Miranda Callahan wrote:
> > On 2012/11/21 00:27:13, Yoyo Zhou wrote:
> > > ditto
> > 
> > Unfortunately, all this casting is needed, as MockExtensionService ISA
> > TestExtensionService ISA ExtensionServiceInterface -- this is the common
> > ancestor of the Mock and the actual ExtensionService. Which is just icky,
but
> > maybe refactoring this can be in a separate CL.
> 
> You don't need it for the upcasting on this line, I thought.

This probably needs refactoring to get rid of the confusion it's cost both of
us. :-P  This is actually a downcast -- we cast the MockExtensionSVC up to the
ExtensionSVCInterface, and then back down to an ExtensionSVC. The real question
is "how does this even work?"

[commit-bot: I haz the power, 2012-11-30 17:41:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mirandac@chromium.org/11365181/11299

[commit-bot: I haz the power, 2012-11-30 18:47:16]

Change committed as 170497