Make ChromeGeolocationPermissionContextFactory a ProfileKeyedServiceFactory

Make ChromeGeolocationPermissionContextFactory a ProfileKeyedServiceFactory

content::GeolocationPermissionContext derives RefCountedThreadSafe, so
we can't mix a RefcountedProfileKeyedService into
ChromeGeolocationPermissionContextFactory. Instead, create a separate
ProfileKeyedService to hold the
scoped_refptr<GeolocationPermissionContext>.

Ground work for https://codereview.chromium.org/11590002/

TEST=Existing
BUG=155029
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175521

[rpetterson, 2012-12-18 21:22:17]

https://codereview.chromium.org/11587003/diff/10001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11587003/diff/10001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:859:
ProfileImpl::GetGeolocationPermissionContext() {
Do you have any plans to eventually remove this function as well? (usually done
in a separate CL if there are a lot of instances in the codebase)

https://codereview.chromium.org/11587003/diff/10001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/11587003/diff/10001/chrome/test/base/testing_...
chrome/test/base/testing_profile.cc:671: return
ChromeGeolocationPermissionContextFactory::GetForProfile(this);
Normally GetForProfile will return NULL by default in a TestingProfile. It
doesn't look like you want this to be the case, so you should override
ServiceHasOwnInstanceInIncognito in the factory.

[John Knottenbelt, 2012-12-19 16:01:09]

Thanks a lot for your help, please see my comments inline.

https://codereview.chromium.org/11587003/diff/10001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11587003/diff/10001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:859:
ProfileImpl::GetGeolocationPermissionContext() {
Not sure yet. One problem is that RenderProcessHostImpl::CreateMessageFilters in
the content/ layer refers to BrowserContext::GetGeolocationPermissionContext, to
get the ChromeGeolocationPermissionContext. So we couldn't directly call
ChromeGeolocationPermissionContextFactory::GetForProfile because this would be a
layering violation.

Have you seen any similar issues / solutions with other profile keyed services?

On 2012/12/18 21:22:17, rpetterson wrote:
> Do you have any plans to eventually remove this function as well? (usually
done
> in a separate CL if there are a lot of instances in the codebase)

https://codereview.chromium.org/11587003/diff/10001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/11587003/diff/10001/chrome/test/base/testing_...
chrome/test/base/testing_profile.cc:671: return
ChromeGeolocationPermissionContextFactory::GetForProfile(this);
I am not sure about the relationship between the TestingProfile and Incognito.
It seems that a TestingProfile can be made to be incognito through the
set_incognito API. And it seems that ProfileKeyedBaseFactory::GetProfileToUse is
the place where this information takes effect. 

The way we have it right now, we want the GeolocationPermissionContext to be
shared between regular and off the record profiles, so intuitively I would have
thought that I need to implement ServiceRedirectedInIncognito, rather than
ServiceHasOwnInstanceInIncognito?

On 2012/12/18 21:22:17, rpetterson wrote:
> Normally GetForProfile will return NULL by default in a TestingProfile. It
> doesn't look like you want this to be the case, so you should override
> ServiceHasOwnInstanceInIncognito in the factory.

[rpetterson, 2012-12-19 22:22:44]

https://codereview.chromium.org/11587003/diff/10001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11587003/diff/10001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:859:
ProfileImpl::GetGeolocationPermissionContext() {
On 2012/12/19 16:01:09, John Knottenbelt wrote:
> Not sure yet. One problem is that RenderProcessHostImpl::CreateMessageFilters
in
> the content/ layer refers to BrowserContext::GetGeolocationPermissionContext,
to
> get the ChromeGeolocationPermissionContext. So we couldn't directly call
> ChromeGeolocationPermissionContextFactory::GetForProfile because this would be
a
> layering violation.
> 
> Have you seen any similar issues / solutions with other profile keyed
services?
> 
> On 2012/12/18 21:22:17, rpetterson wrote:
> > Do you have any plans to eventually remove this function as well? (usually
> done
> > in a separate CL if there are a lot of instances in the codebase)
> 

There are still a couple services that are in that situation and the "solution"
is to leave the getter as you have hear. The important part is that the Profile
no longer owns the object which you've taken care of with this CL :)

https://codereview.chromium.org/11587003/diff/10001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/11587003/diff/10001/chrome/test/base/testing_...
chrome/test/base/testing_profile.cc:671: return
ChromeGeolocationPermissionContextFactory::GetForProfile(this);
On 2012/12/19 16:01:09, John Knottenbelt wrote:
> I am not sure about the relationship between the TestingProfile and Incognito.
> It seems that a TestingProfile can be made to be incognito through the
> set_incognito API. And it seems that ProfileKeyedBaseFactory::GetProfileToUse
is
> the place where this information takes effect. 
> 
> The way we have it right now, we want the GeolocationPermissionContext to be
> shared between regular and off the record profiles, so intuitively I would
have
> thought that I need to implement ServiceRedirectedInIncognito, rather than
> ServiceHasOwnInstanceInIncognito?
> 
> On 2012/12/18 21:22:17, rpetterson wrote:
> > Normally GetForProfile will return NULL by default in a TestingProfile. It
> > doesn't look like you want this to be the case, so you should override
> > ServiceHasOwnInstanceInIncognito in the factory.
> 

Apologies -- I was confusing TestingProfile and Incognito in my comment.
Although it did raise a good point about incognito. If you want the Incognito
profile to refer to the same GeolocationPermissionContext as the original
profile, then yes, you'll need to override ServiceRedirectedInIncognito to
return true.

[John Knottenbelt, 2012-12-20 13:28:00]

Hi Joth,

Marcus is on holiday until the New Year. If you have a moment, can you take a
look at this patch?

Cheers

John

[rpetterson, 2012-12-20 19:06:55]

LGTM

From a profile standpoint it's fine right now given that you need to implement
BrowserContext::GetGeolocationPermissionContext. If you decide to switch it up
with ServiceHasOwnInstanceInIncognito, I'm happy to take another look.

[joth, 2012-12-20 19:17:11]

Hi John
Will try but not sure I'll get chance... but note that as you're an OWNER
for geolocaiton yourself, you can ask any competent chromium reviewer to
look at it (and esp a profile-keyed service expert for this patch).  no
need to get another geolocation OWNER involved.



On 20 December 2012 05:28, <jknotten@chromium.org> wrote:

> Hi Joth,
>
> Marcus is on holiday until the New Year. If you have a moment, can you
> take a
> look at this patch?
>
> Cheers
>
> John
>
>
https://chromiumcodereview.**appspot.com/11587003/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2013-01-02 11:43:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jknotten@chromium.org/11587003/25001

[commit-bot: I haz the power, 2013-01-02 11:43:31]

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

Patch:      
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h
Index:
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h
diff --git
a/chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h
b/chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h
index
8e598b272e3627f59007562ac3c229abaae3ea62..764496a8c931206a10e39c23d13e1ec2a347b28a
100644
--- a/chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h
+++ b/chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h
@@ -5,20 +5,33 @@
 #ifndef
CHROME_BROWSER_GEOLOCATION_CHROME_GEOLOCATION_PERMISSION_CONTEXT_FACTORY_H_
 #define
CHROME_BROWSER_GEOLOCATION_CHROME_GEOLOCATION_PERMISSION_CONTEXT_FACTORY_H_
 
+#include "base/memory/singleton.h"
 #include "base/values.h"
 #include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/profiles/profile_keyed_service_factory.h"
 
 class ChromeGeolocationPermissionContext;
 class Profile;
 
-class ChromeGeolocationPermissionContextFactory {
+class ChromeGeolocationPermissionContextFactory
+    : public ProfileKeyedServiceFactory {
  public:
-  ChromeGeolocationPermissionContextFactory() {}
-  ~ChromeGeolocationPermissionContextFactory() {}
-  static ChromeGeolocationPermissionContext* Create(Profile* profile);
-  static void RegisterUserPrefs(PrefService* user_prefs);
+  static ChromeGeolocationPermissionContext* GetForProfile(Profile* profile);
+
+  static ChromeGeolocationPermissionContextFactory* GetInstance();
 
  private:
+  friend struct
+  DefaultSingletonTraits<ChromeGeolocationPermissionContextFactory>;
+
+  ChromeGeolocationPermissionContextFactory();
+  virtual ~ChromeGeolocationPermissionContextFactory();
+
+  // |ProfileKeyedBaseFactory| methods:
+  virtual ProfileKeyedService*
+  BuildServiceInstanceFor(Profile* profile) const OVERRIDE;
+  virtual void RegisterUserPrefs(PrefService* user_prefs) OVERRIDE;
+  virtual bool ServiceRedirectedInIncognito() const OVERRIDE;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeGeolocationPermissionContextFactory);
 };

[John Knottenbelt, 2013-01-02 14:32:51]

Please could you do an owners review:

sky: chrome/test/
bauerb: chrome/browser/prefs/

Thanks!

[Bernhard Bauer, 2013-01-02 14:59:53]

chrome/browser/prefs LGTM, but I found some nits elsewhere:

https://codereview.chromium.org/11587003/diff/36002/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context_factory.cc
(right):

https://codereview.chromium.org/11587003/diff/36002/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.cc:23:
context_ =  new ChromeGeolocationPermissionContext(profile);
Nit: only one space

https://codereview.chromium.org/11587003/diff/36002/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.cc:32:
scoped_refptr<content::GeolocationPermissionContext> context_;
If you make this a ChromeGeolocationPermissionContext, I think you could get rid
of the static_cast above?

https://codereview.chromium.org/11587003/diff/36002/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.cc:52:
: ProfileKeyedServiceFactory(
Nit: Indent four spaces (in total)

https://codereview.chromium.org/11587003/diff/36002/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.cc:53:
"ChromeGeolocationPermissionContext",
...and then this line four spaces w/r/t to the ProfileKeyedService in the
previous line.

[John Knottenbelt, 2013-01-02 15:49:02]

Thanks, Bernhard. Nit's addressed.

https://codereview.chromium.org/11587003/diff/36002/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context_factory.cc
(right):

https://codereview.chromium.org/11587003/diff/36002/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.cc:23:
context_ =  new ChromeGeolocationPermissionContext(profile);
On 2013/01/02 14:59:53, Bernhard Bauer wrote:
> Nit: only one space

Done.

https://codereview.chromium.org/11587003/diff/36002/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.cc:32:
scoped_refptr<content::GeolocationPermissionContext> context_;
Yes. Thanks, that's much neater!

On 2013/01/02 14:59:53, Bernhard Bauer wrote:
> If you make this a ChromeGeolocationPermissionContext, I think you could get
rid
> of the static_cast above?

Done.

https://codereview.chromium.org/11587003/diff/36002/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.cc:52:
: ProfileKeyedServiceFactory(
On 2013/01/02 14:59:53, Bernhard Bauer wrote:
> Nit: Indent four spaces (in total)

Done.

https://codereview.chromium.org/11587003/diff/36002/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.cc:53:
"ChromeGeolocationPermissionContext",
On 2013/01/02 14:59:53, Bernhard Bauer wrote:
> ...and then this line four spaces w/r/t to the ProfileKeyedService in the
> previous line.

Done.

[commit-bot: I haz the power, 2013-01-04 15:23:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jknotten@chromium.org/11587003/42001

[commit-bot: I haz the power, 2013-01-04 15:23:49]

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

Patch:       chrome/browser/profiles/profile_impl.h
Index: chrome/browser/profiles/profile_impl.h
diff --git a/chrome/browser/profiles/profile_impl.h
b/chrome/browser/profiles/profile_impl.h
index
e461908c7ec75cd36cccdd5b44e68726c36bce5c..85f4f2669bdfee8c8e8367a3d0b17bd8dddbffb3
100644
--- a/chrome/browser/profiles/profile_impl.h
+++ b/chrome/browser/profiles/profile_impl.h
@@ -231,8 +231,6 @@ class ProfileImpl : public Profile,
   scoped_refptr<PromoResourceService> promo_resource_service_;
   scoped_ptr<SSLConfigServiceManager> ssl_config_service_manager_;
   scoped_refptr<HostContentSettingsMap> host_content_settings_map_;
-  scoped_refptr<content::GeolocationPermissionContext>
-      geolocation_permission_context_;
   scoped_ptr<GAIAInfoUpdateService> gaia_info_update_service_;
   scoped_refptr<history::ShortcutsBackend> shortcuts_backend_;

[commit-bot: I haz the power, 2013-01-04 15:48:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jknotten@chromium.org/11587003/47001

[commit-bot: I haz the power, 2013-01-04 15:48:59]

Presubmit check for 11587003-47001 failed and returned exit status 1.


Running presubmit commit checks ...

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

Presubmit checks took 1.6s to calculate.

[sky, 2013-01-07 14:51:44]

LGTM

https://codereview.chromium.org/11587003/diff/47001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context_factory.cc
(right):

https://codereview.chromium.org/11587003/diff/47001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.cc:33:
};
DISALLOW_...

https://codereview.chromium.org/11587003/diff/47001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h
(right):

https://codereview.chromium.org/11587003/diff/47001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h:25:
DefaultSingletonTraits<ChromeGeolocationPermissionContextFactory>;
indent 4 more.

https://codereview.chromium.org/11587003/diff/47001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h:32:
BuildServiceInstanceFor(Profile* profile) const OVERRIDE;
indent 4 more

[John Knottenbelt, 2013-01-08 10:24:27]

https://codereview.chromium.org/11587003/diff/47001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h
(right):

https://codereview.chromium.org/11587003/diff/47001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h:25:
DefaultSingletonTraits<ChromeGeolocationPermissionContextFactory>;
On 2013/01/07 14:51:44, sky wrote:
> indent 4 more.

Done.

https://codereview.chromium.org/11587003/diff/47001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h:32:
BuildServiceInstanceFor(Profile* profile) const OVERRIDE;
On 2013/01/07 14:51:44, sky wrote:
> indent 4 more

Done.

[commit-bot: I haz the power, 2013-01-08 10:24:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jknotten@chromium.org/11587003/56001

[commit-bot: I haz the power, 2013-01-08 13:41:01]

Change committed as 175521