Banish boilerplate. Profile-keyed API factory for extension API classes.

Banish boilerplate. Profile-keyed API factory for extension API classes.

This converts 2 factories to use it; more to follow.

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

[Yoyo Zhou, 2012-12-21 00:34:08]

jyasskin: please review.
erg: please review profiles.
awong, mpcomplete: FYI. This seems more promising than the previous attempt.

[Jeffrey Yasskin, 2012-12-21 01:43:49]

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/extensions/...
File chrome/browser/extensions/api/profile_keyed_api_factory.h (right):

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/profile_keyed_api_factory.h:35: template <typename
T>
Document what T is.

Can you make the trait fields into static members of T instead of making people
specialize ProfileKeyedAPIFactoryTraits?

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/profile_keyed_api_factory.h:53: // specializations
can override this.
Give some example code for how they'd override it.

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/profile_keyed_api_factory.h:64: virtual
~ProfileKeyedAPIFactory<T>() {
s/<T>//. C++ expands the plain name of the current template to the current
specialization.

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/profile_keyed_api_factory.h:90: static
base::LazyInstance<scoped_ptr<ProfileKeyedAPIFactory<T> > >
Why LazyInstance<scoped_ptr>? Do you want LazyInstance<ProfileKeyedAPIFactory<T>
>::Leaky?

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/profile_keyed_api_factory.h:98:
ProfileKeyedAPIFactory<T>::instance_ = LAZY_INSTANCE_INITIALIZER;
This is going to be sad if it's #included from multiple DLLs. You'll get
multiple instances.

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_dependency_manager.cc:265:
extensions::ProfileKeyedAPIFactory<extensions::TabsWindowsAPI>::GetInstance();
Sort these?

[Yoyo Zhou, 2012-12-21 22:41:33]

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/extensions/...
File chrome/browser/extensions/api/profile_keyed_api_factory.h (right):

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/profile_keyed_api_factory.h:35: template <typename
T>
On 2012/12/21 01:43:50, Jeffrey Yasskin wrote:
> Document what T is.
> 
> Can you make the trait fields into static members of T instead of making
people
> specialize ProfileKeyedAPIFactoryTraits?

Expanded the comment.

If I make them static members of T, then in order to have default properties, I
need to define a ProfileKeyedService base here that the Ts inherit from.

I'll upload 2 patchsets:

3: leaving this as is
4: making the traits static members of ProfileKeyedAPI (inherits from
ProfileKeyedService)

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/profile_keyed_api_factory.h:53: // specializations
can override this.
On 2012/12/21 01:43:50, Jeffrey Yasskin wrote:
> Give some example code for how they'd override it.

Done.

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/profile_keyed_api_factory.h:64: virtual
~ProfileKeyedAPIFactory<T>() {
On 2012/12/21 01:43:50, Jeffrey Yasskin wrote:
> s/<T>//. C++ expands the plain name of the current template to the current
> specialization.

Done throughout.

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/profile_keyed_api_factory.h:90: static
base::LazyInstance<scoped_ptr<ProfileKeyedAPIFactory<T> > >
On 2012/12/21 01:43:50, Jeffrey Yasskin wrote:
> Why LazyInstance<scoped_ptr>? Do you want
LazyInstance<ProfileKeyedAPIFactory<T>
> >::Leaky?

Can't be done here:

invalid application of 'sizeof' to incomplete type
'extensions::ProfileKeyedAPIFactory<extensions::ProcessesAPI

The alternative to this scoped_ptr shenanigan is declaring the lazy instance in
each instantiator. I went and did that. I'm not certain that leakiness matters
for my use case.

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/profile_keyed_api_factory.h:98:
ProfileKeyedAPIFactory<T>::instance_ = LAZY_INSTANCE_INITIALIZER;
On 2012/12/21 01:43:50, Jeffrey Yasskin wrote:
> This is going to be sad if it's #included from multiple DLLs. You'll get
> multiple instances.

Moved the lazy instances to each .cc file.

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://codereview.chromium.org/11649053/diff/2002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_dependency_manager.cc:265:
extensions::ProfileKeyedAPIFactory<extensions::TabsWindowsAPI>::GetInstance();
On 2012/12/21 01:43:50, Jeffrey Yasskin wrote:
> Sort these?

Done.

[Jeffrey Yasskin, 2012-12-22 00:16:04]

I vote for patch #4. :)

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/processes/processes_api.cc:535:
ProfileKeyedAPIFactory<ProcessesAPI>*
You should declare this specialization in processes_api.h (just after the
definition of ProcessesAPI). Since there's no default template definition, you
won't get undefined behavior by failing to declare it, but it's a good habit to
get into for when there is a default definition.

Document the code you use in the comment for
ProfileKeyedApiFactory::GetInstance().

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
File chrome/browser/extensions/api/profile_keyed_api_factory.h (right):

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/profile_keyed_api_factory.h:15: // Instantiations
of ProfileKeyedAPIFactory should use this base class
You should mention that the fields need to be accessible to
ProfileKeyedAPIFactory<TheService>. I don't think it would hurt to make them
public, but I don't have a strong preference between that and friend'ing the
factory.

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/profile_keyed_api_factory.h:16: // and also
declare a static const char* service_name() function (used in the
s/declare/define/

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/profile_keyed_api_factory.h:17: //
ProfileKeyedBase constructor).
"ProfileKeyedBaseFactory"?

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/profile_keyed_api_factory.h:39: // included in
more than one translation unit (and thus cannot be initialized
Please include the code users will need to write in their headers and .cc files.
(See processes_api.cc for my other comment on this.)

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/profile_keyed_api_factory.h:48: //
ProfileKeyedAPIFactory<PushMessagingAPI>::DeclareFactoryDependencies() {
And please remind them to declare the specialization in a header file just after
the definition of PushMessageAPI so that the compiler knows to call the
specialization instead of inlining the default. (The definition of the explicit
specialization will have to be 'inline' or in the .cc file since it's just an
ordinary function.)

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
File chrome/browser/extensions/api/tabs/tabs_windows_api.h (right):

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/tabs/tabs_windows_api.h:39: static const char*
service_name() {
Possibly comment this as "ProfileKeyedAPI implementation" to parallel what we do
for virtual functions, since these are basically the template version of virtual
functions.

[Yoyo Zhou, 2012-12-26 17:46:33]

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/processes/processes_api.cc:535:
ProfileKeyedAPIFactory<ProcessesAPI>*
On 2012/12/22 00:16:04, Jeffrey Yasskin wrote:
> You should declare this specialization in processes_api.h (just after the
> definition of ProcessesAPI). Since there's no default template definition, you
> won't get undefined behavior by failing to declare it, but it's a good habit
to
> get into for when there is a default definition.
> 
> Document the code you use in the comment for
> ProfileKeyedApiFactory::GetInstance().

Honestly, I was surprised to find that not declaring it in the header worked in
the first place.

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
File chrome/browser/extensions/api/profile_keyed_api_factory.h (right):

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/profile_keyed_api_factory.h:15: // Instantiations
of ProfileKeyedAPIFactory should use this base class
On 2012/12/22 00:16:04, Jeffrey Yasskin wrote:
> You should mention that the fields need to be accessible to
> ProfileKeyedAPIFactory<TheService>. I don't think it would hurt to make them
> public, but I don't have a strong preference between that and friend'ing the
> factory.

Mentioned.
I think it would be okay too if they were public, but I'd prefer for the
visibility to be made explicit.

By the way, I couldn't figure out a way to do this without using protected
visibility.

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/profile_keyed_api_factory.h:16: // and also
declare a static const char* service_name() function (used in the
On 2012/12/22 00:16:04, Jeffrey Yasskin wrote:
> s/declare/define/

Done.

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/profile_keyed_api_factory.h:17: //
ProfileKeyedBase constructor).
On 2012/12/22 00:16:04, Jeffrey Yasskin wrote:
> "ProfileKeyedBaseFactory"?

Done.

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/profile_keyed_api_factory.h:39: // included in
more than one translation unit (and thus cannot be initialized
On 2012/12/22 00:16:04, Jeffrey Yasskin wrote:
> Please include the code users will need to write in their headers and .cc
files.
> (See processes_api.cc for my other comment on this.)

Done.

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/profile_keyed_api_factory.h:48: //
ProfileKeyedAPIFactory<PushMessagingAPI>::DeclareFactoryDependencies() {
On 2012/12/22 00:16:04, Jeffrey Yasskin wrote:
> And please remind them to declare the specialization in a header file just
after
> the definition of PushMessageAPI so that the compiler knows to call the
> specialization instead of inlining the default. (The definition of the
explicit
> specialization will have to be 'inline' or in the .cc file since it's just an
> ordinary function.)

Done.

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
File chrome/browser/extensions/api/tabs/tabs_windows_api.h (right):

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/tabs/tabs_windows_api.h:39: static const char*
service_name() {
On 2012/12/22 00:16:04, Jeffrey Yasskin wrote:
> Possibly comment this as "ProfileKeyedAPI implementation" to parallel what we
do
> for virtual functions, since these are basically the template version of
virtual
> functions.

Done.

[Jeffrey Yasskin, 2012-12-26 19:21:55]

lgtm

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/processes/processes_api.cc:535:
ProfileKeyedAPIFactory<ProcessesAPI>*
On 2012/12/26 17:46:33, Yoyo Zhou wrote:
> On 2012/12/22 00:16:04, Jeffrey Yasskin wrote:
> > You should declare this specialization in processes_api.h (just after the
> > definition of ProcessesAPI). Since there's no default template definition,
you
> > won't get undefined behavior by failing to declare it, but it's a good habit
> to
> > get into for when there is a default definition.
> > 
> > Document the code you use in the comment for
> > ProfileKeyedApiFactory::GetInstance().
> 
> Honestly, I was surprised to find that not declaring it in the header worked
in
> the first place.

Yeah, the biggest problem with missing declarations for explicit specializations
is that forgetting them will often work for the person checking in the code, and
then break for someone else, later.

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
File chrome/browser/extensions/api/profile_keyed_api_factory.h (right):

https://codereview.chromium.org/11649053/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/profile_keyed_api_factory.h:15: // Instantiations
of ProfileKeyedAPIFactory should use this base class
On 2012/12/26 17:46:33, Yoyo Zhou wrote:
> On 2012/12/22 00:16:04, Jeffrey Yasskin wrote:
> > You should mention that the fields need to be accessible to
> > ProfileKeyedAPIFactory<TheService>. I don't think it would hurt to make them
> > public, but I don't have a strong preference between that and friend'ing the
> > factory.
> 
> Mentioned.
> I think it would be okay too if they were public, but I'd prefer for the
> visibility to be made explicit.
> 
> By the way, I couldn't figure out a way to do this without using protected
> visibility.

Yeah, ProfileKeyedBaseFactory isn't a friend of ProfileKeyedAPI, so it can't see
declarations in here. Making them protected lets the subclass see them, and
thence expose them using that friend declaration. I think.

[memyy, 2012-12-26 19:38:10]

metanata4@gmail.com

[commit-bot: I haz the power, 2012-12-26 19:58:13]

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

[commit-bot: I haz the power, 2012-12-26 19:58:19]

Presubmit check for 11649053-14016 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/profiles

Presubmit checks took 1.3s to calculate.

[Yoyo Zhou, 2012-12-26 20:03:28]

+sail: please review profiles
+thakis: please review gypi

[sail, 2012-12-26 20:15:13]

profiles/* LGTM

[Nico, 2012-12-26 20:31:18]

gypi lgtm

Feel free to tbr minor gypi changes like this to me (but please email me the
review anyway if you do that, so that I can take a look post-commit)

[commit-bot: I haz the power, 2012-12-26 20:37:56]

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

[commit-bot: I haz the power, 2012-12-26 23:22:53]

Change committed as 174645