Cleanup many APIs to use ProfileKeyedAPI.

Cleanup many APIs to use ProfileKeyedAPI.

This allows extra boilerplate BrowserContextKeyedServiceFactory code to be deleted.

In this CL: ActivityLog, ExtensionSyncEventObserver, RuntimeAPI, DeveloperPrivateAPI, BluetoothAPI, AutotestPrivateAPI.

BUG=179951
TBR=erg@chromium.org

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

[Yoyo Zhou, 2014-02-18 20:37:20]

I think this is mostly uninteresting, but it's good to check that the old
factory.cc override functions and the new static const bools agree with each
other.

[James Cook, 2014-02-18 22:56:36]

I'm in bug-hell today, I won't get to this until tonight or tomorrow. Apologies.

[James Cook, 2014-02-19 02:37:42]

I'll look at this in more detail tomorrow.

I wonder if ProfileKeyedAPI can be made more slim somehow. I don't like how it
drags in profile.h, browser_context_keyed_service_factory.h and generates a
bunch of template code. This means, for example, that all 18 translation units
that include activity_log.h (for example) now pick up a bunch of stuff they
didn't need before.  It basically means that to use a service you now need to
have an implementation of its factory (and let the linker strip it out).

https://codereview.chromium.org/168253002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/activity_log.cc (left):

https://codereview.chromium.org/168253002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/activity_log.cc:352: return
ExtensionsBrowserClient::Get()->GetOriginalContext(context);
I'll bet this is the way it is because there's some code in app_shell that
instantiates an ActivityLog but doesn't use it. If you convert this to a
ProfileKeyedAPI that uses chrome::GetBrowserContextRedirectedInIncognito()
you'll break this behavior. Maybe ProfileKeyedAPI should be converted to use
ExtensionsBrowserClient for these lookups?

[James Cook, 2014-02-19 02:38:27]

Also, that doesn't seem to be the right bug number?  Although the linked bug is
pretty funny.

[Yoyo Zhou, 2014-02-19 02:49:05]

Hmm, I should also change ProfileKeyedAPI to use BrowserContext instead of
Profile, shouldn't I.

I fixed the bug number. Sorry about the off-by-one-character.

https://codereview.chromium.org/168253002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/activity_log.cc (left):

https://codereview.chromium.org/168253002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/activity_log.cc:352: return
ExtensionsBrowserClient::Get()->GetOriginalContext(context);
On 2014/02/19 02:37:43, James Cook wrote:
> I'll bet this is the way it is because there's some code in app_shell that
> instantiates an ActivityLog but doesn't use it. If you convert this to a
> ProfileKeyedAPI that uses chrome::GetBrowserContextRedirectedInIncognito()
> you'll break this behavior. Maybe ProfileKeyedAPI should be converted to use
> ExtensionsBrowserClient for these lookups?

Good call. But actually I think we don't use ActivityLog at all in app_shell.
I'll dig into why we changed this (I think it was to avoid references to
incognito?).

[James Cook, 2014-02-19 17:06:28]

OK. Let me know when you'd like me to look at this again.

[Yoyo Zhou, 2014-02-19 19:24:16]

On 2014/02/19 17:06:28, James Cook wrote:
> OK. Let me know when you'd like me to look at this again.

It looks like there are about 50 classes that use ProfileKeyedAPI that I'd need
to switch from Profile to BrowserContext. I think I'll have to change those
first before I come back to this CL.

[James Cook, 2014-02-19 19:32:46]

On 2014/02/19 19:24:16, Yoyo Zhou wrote:
> On 2014/02/19 17:06:28, James Cook wrote:
> > OK. Let me know when you'd like me to look at this again.
> 
> It looks like there are about 50 classes that use ProfileKeyedAPI that I'd
need
> to switch from Profile to BrowserContext. I think I'll have to change those
> first before I come back to this CL.

I would be OK with sprinkling the code with
static_cast<Profile*>(browser_context) if that made things easier.

[James Cook, 2014-02-26 23:30:09]

On 2014/02/19 19:32:46, James Cook wrote:
> On 2014/02/19 19:24:16, Yoyo Zhou wrote:
> > On 2014/02/19 17:06:28, James Cook wrote:
> > > OK. Let me know when you'd like me to look at this again.
> > 
> > It looks like there are about 50 classes that use ProfileKeyedAPI that I'd
> need
> > to switch from Profile to BrowserContext. I think I'll have to change those
> > first before I come back to this CL.
> 
> I would be OK with sprinkling the code with
> static_cast<Profile*>(browser_context) if that made things easier.

Did these changes land in your other CLs?  If so, can this codereview issue be
closed?

[Yoyo Zhou, 2014-02-26 23:32:42]

On 2014/02/26 23:30:09, James Cook wrote:
> On 2014/02/19 19:32:46, James Cook wrote:
> > On 2014/02/19 19:24:16, Yoyo Zhou wrote:
> > > On 2014/02/19 17:06:28, James Cook wrote:
> > > > OK. Let me know when you'd like me to look at this again.
> > > 
> > > It looks like there are about 50 classes that use ProfileKeyedAPI that I'd
> > need
> > > to switch from Profile to BrowserContext. I think I'll have to change
those
> > > first before I come back to this CL.
> > 
> > I would be OK with sprinkling the code with
> > static_cast<Profile*>(browser_context) if that made things easier.
> 
> Did these changes land in your other CLs?  If so, can this codereview issue be
> closed?

No, this is orthogonal to the other CLs. I was waiting for ProfileKeyedAPI to no
longer include profile.h. Now that that's so (pending it re-landing in the CQ)
you can re-review this.

[Yoyo Zhou, 2014-02-27 00:17:10]

On 2014/02/26 23:32:42, Yoyo Zhou wrote:
> On 2014/02/26 23:30:09, James Cook wrote:
> > On 2014/02/19 19:32:46, James Cook wrote:
> > > On 2014/02/19 19:24:16, Yoyo Zhou wrote:
> > > > On 2014/02/19 17:06:28, James Cook wrote:
> > > > > OK. Let me know when you'd like me to look at this again.
> > > > 
> > > > It looks like there are about 50 classes that use ProfileKeyedAPI that
I'd
> > > need
> > > > to switch from Profile to BrowserContext. I think I'll have to change
> those
> > > > first before I come back to this CL.
> > > 
> > > I would be OK with sprinkling the code with
> > > static_cast<Profile*>(browser_context) if that made things easier.
> > 
> > Did these changes land in your other CLs?  If so, can this codereview issue
be
> > closed?
> 
> No, this is orthogonal to the other CLs. I was waiting for ProfileKeyedAPI to
no
> longer include profile.h. Now that that's so (pending it re-landing in the CQ)
> you can re-review this.

Wait, I just reminded myself that I need to change these constructors to take
BrowserContext*.

[James Cook, 2014-02-27 00:37:33]

What you have LGTM.  Let me know if you want me to look again after the
constructor BrowserContext conversion.

https://codereview.chromium.org/168253002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/activity_log.h (right):

https://codereview.chromium.org/168253002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/activity_log.h:172: static const bool
kServiceRedirectedInIncognito = true;
Not for this CL, but... The fact that these constants have different default
values than the defaults in BrowserContextKeyedServiceFactory is very confusing
for me.  I frequently have to look at both the BCKS header and the
ProfileKeyedAPI header to figure out what is actually going on for a class.

At the very least, some comments in ProfileKeyedAPI about which defaults are
being overridden might help.

https://codereview.chromium.org/168253002/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/168253002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:239: #if
defined(ENABLE_EXTENSIONS)
Also not for this CL: We ought to pull this block of registration out into a
file in chrome/browser/extensions somewhere.

[Yoyo Zhou, 2014-02-27 01:03:33]

Uploaded a new patch, changing Profile to BrowserContext in constructors.

https://codereview.chromium.org/168253002/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/168253002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:239: #if
defined(ENABLE_EXTENSIONS)
On 2014/02/27 00:37:34, James Cook wrote:
> Also not for this CL: We ought to pull this block of registration out into a
> file in chrome/browser/extensions somewhere.

We considered this a while back and decided it wasn't an improvement to look in
two places where everything gets listed rather than one. But I'd be open to
reconsideration.

[Yoyo Zhou, 2014-02-27 01:05:21]

https://codereview.chromium.org/168253002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/activity_log.h (right):

https://codereview.chromium.org/168253002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/activity_log.h:172: static const bool
kServiceRedirectedInIncognito = true;
On 2014/02/27 00:37:34, James Cook wrote:
> Not for this CL, but... The fact that these constants have different default
> values than the defaults in BrowserContextKeyedServiceFactory is very
confusing
> for me.  I frequently have to look at both the BCKS header and the
> ProfileKeyedAPI header to figure out what is actually going on for a class.
> 
> At the very least, some comments in ProfileKeyedAPI about which defaults are
> being overridden might help.

Only one is different in ProfileKeyedAPI, but it's worth clarifying. It doesn't
help that BrowserContextKeyedBaseFactory was refactored so that e.g.
ServiceRedirectedInIncognito was replaced by GetBrowserContextToUse.

[James Cook, 2014-02-27 01:15:58]

Still LGTM.

https://codereview.chromium.org/168253002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/profile_keyed_api_factory.h (right):

https://codereview.chromium.org/168253002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/profile_keyed_api_factory.h:30: static const bool
kServiceIsNULLWhileTesting = false;
One tiny thing that might help is to make the order of these constants match the
order of the methods in BrowserContextKeyedBaseFactory, e.g. put
kServiceIsNULLWhileTesting last.

[Yoyo Zhou, 2014-02-27 04:54:41]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-02-27 04:55:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/168253002/160001

[commit-bot: I haz the power, 2014-02-27 04:55:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-27 04:55:40]

Failed to apply patch for
chrome/browser/extensions/api/profile_keyed_api_factory.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/api/profile_keyed_api_factory.h
  Hunk #1 FAILED at 99.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/api/profile_keyed_api_factory.h.rej

Patch:       chrome/browser/extensions/api/profile_keyed_api_factory.h
Index: chrome/browser/extensions/api/profile_keyed_api_factory.h
diff --git a/chrome/browser/extensions/api/profile_keyed_api_factory.h
b/chrome/browser/extensions/api/profile_keyed_api_factory.h
index
83f0d8c9a59590876e5441a0b55d6e2e3524a22f..b062ef218bfaa4bb0997f7f631036bc5b8628ccd
100644
--- a/chrome/browser/extensions/api/profile_keyed_api_factory.h
+++ b/chrome/browser/extensions/api/profile_keyed_api_factory.h
@@ -99,8 +99,8 @@ class ProfileKeyedAPIFactory : public
BrowserContextKeyedServiceFactory {
  private:
   // BrowserContextKeyedServiceFactory implementation.
   virtual BrowserContextKeyedService* BuildServiceInstanceFor(
-      content::BrowserContext* profile) const OVERRIDE {
-    return new T(static_cast<Profile*>(profile));
+      content::BrowserContext* context) const OVERRIDE {
+    return new T(static_cast<Profile*>(context));
   }
 
   // BrowserContextKeyedBaseFactory implementation.

[Yoyo Zhou, 2014-02-27 06:27:47]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-02-27 06:28:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/168253002/180001

[commit-bot: I haz the power, 2014-02-27 06:44:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-27 06:44:35]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Yoyo Zhou, 2014-02-27 15:54:52]

TBR=erg for uninteresting chrome_brower_main_extra_parts_profiles changes

[Yoyo Zhou, 2014-02-27 15:54:59]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-02-27 15:55:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/168253002/180001

[commit-bot: I haz the power, 2014-02-27 21:32:05]

Change committed as 253928