Allow cross dependencies between BCKSF and BSKSF

Allow cross dependencies between BCKSF and BSKSF

For incremental conversion of iOS code from BrowserContextKeyedServiceFactories
to BrowserStateKeyedServiceFactories, iOS uses cross-dependencies between both
types of factories.

This CL had two virtual methods to KeyedServiceBaseFactory to convert a context
to the correct type (content::BrowserContext or web::BrowserState) and back so
that we can have BrowserStateKeyedServiceFactories registered into a
BrowserContextDependencyManager (temporary).

Implement the method in all derived classes and refactor the implementation
of GetAssociatedPrefRegistry that no longer needs to be virtual.

BUG=459020, 478763
Committed: https://crrev.com/8c1791c63d6a05455eb6c1413e01844496a119f4
Cr-Commit-Position: refs/heads/master@{#326782}

[sdefresne, 2015-04-20 17:11:39]

sdefresne@chromium.org changed reviewers:
+ droger@chromium.org, erg@chromium.org

[sdefresne, 2015-04-20 17:11:40]

erg: this is required to get cross dependencies (as I designed in
https://docs.google.com/document/d/1HSHDxKiUui1io2odu5tkzddJZG21Fi7m0SGfnJj1s...)
to work as the conversion also needs to be done in the base classes in both
direction (to inform/query the DependencyManager and to deal with the
preferences).

Can you both (erg, droger) take a look?

[sdefresne, 2015-04-20 17:16:12]

Patchset #1 (id:1) has been deleted

[sdefresne, 2015-04-20 17:16:28]

Patchset #1 (id:1) has been deleted

[droger, 2015-04-21 14:19:18]

LGTM

This CL makes the code more complex, but we really need this on iOS. This is
only temporary though, so I think this is acceptable. If we don't do this
upstream we'll have to do it downstream in any case.

The upside is that it allows moving the preference registration to the core of
the component.

If we could make the comments and possibly the naming better, I think it would
be worth it. I realize that it's not very easy. I added some suggestions.

https://codereview.chromium.org/1090373003/diff/60001/components/keyed_servic...
File components/keyed_service/core/keyed_service_base_factory.h (right):

https://codereview.chromium.org/1090373003/diff/60001/components/keyed_servic...
components/keyed_service/core/keyed_service_base_factory.h:132: // TODO(ios):
remove this method and all usage of the methods once iOS code
Can we merge these comments and give a bit more details?
I suggest:

Return the underlying context and the original context corresponding to
|context|.
Used to allow cross dependencies between BrowserContextKeyedServiceFactories and
BrowserStateKeyedServiceFactories during the upstreaming of iOS.
The original context is the context used to compute the dependencies, and the
underlying context is the context that is exposed in the factories public APIs.
These methods do nothing by default (i.e. they return the context that was
passed in), but iOS provides
hooks to override this behavior temporarily while the upstreaming is in
progress.
TODO(ios): remove this method and all usage of the methods once iOS code
only use BrowserStateKeyedServiceFactory, http://crbug.com/478763

https://codereview.chromium.org/1090373003/diff/60001/components/keyed_servic...
components/keyed_service/core/keyed_service_base_factory.h:134:
base::SupportsUserData* GetOriginalContext(
I don't really like "Original" and "Underlying".
Also if I am not mistaken, the "Original" context is the one used by the
dependency manager, and since the dependency manager sits at the bottom of the
class hierarchy, I wonder if it would actually make more sense to swap the two
names.

Some other suggestions, maybe less confusing, but maybe not:
- "InternalContext" / "PublicContext"
- "DependencyContext" / "FactorySpecificContext"
- "BaseContext" / ???

https://codereview.chromium.org/1090373003/diff/60001/components/keyed_servic...
components/keyed_service/core/keyed_service_base_factory.h:148: // only use
BrowserStateKeyedServiceFactory, http://crbug.com/478763
Add something like:
Subclasses must implement these methods to convert between the context used for
dependencies and the context exposed by the concrete factories.
It is expected that all platforms expect iOS simply return the context that is
passed in.

[Elliot Glaysher, 2015-04-21 17:54:32]

https://codereview.chromium.org/1090373003/diff/60001/components/keyed_servic...
File components/keyed_service/core/keyed_service_factory.cc (right):

https://codereview.chromium.org/1090373003/diff/60001/components/keyed_servic...
components/keyed_service/core/keyed_service_factory.cc:25: context =
GetUnderlyingContext(context);
This seems like it could be very error prone. Is this one of the temporary
things?

[sdefresne, 2015-04-22 08:53:16]

https://codereview.chromium.org/1090373003/diff/60001/components/keyed_servic...
File components/keyed_service/core/keyed_service_factory.cc (right):

https://codereview.chromium.org/1090373003/diff/60001/components/keyed_servic...
components/keyed_service/core/keyed_service_factory.cc:25: context =
GetUnderlyingContext(context);
On 2015/04/21 17:54:32, Elliot Glaysher wrote:
> This seems like it could be very error prone. Is this one of the temporary
> things?

Yes, all calls to GetUnderlyingContext/GetOriginalContext are temporary.

The reason is that downstream iOS use a BrowserContextDependencyManager that
contains both BrowserContextKeyedServiceFactory and
BrowserStateKeyedServiceFactory.

When the service factory overrides ServiceIsNULLWhileTesting, the dependency
manager will call SetEmptyTestingFactory() with the context that is a
content::BrowserContext. But when the client code calls the service factory
GetForBrowserState() it passes a web::BrowserState to the service factory, and
the testing factory is not found (since they are different objects).

This cross-dependency is quite dangerous, but only happens on iOS downstream. To
mitigate the risk, I can change GetUnderlyingContext/GetOriginalContext to be
simple inline method doing nothing if not targetting iOS, something like:

  // Returns the underlying context to use for |context|. It is a simple
  // pass-through for all platform except iOS. On iOS is it used to allow
  // mixing dependencies between BrowserContextKeyedService and
  // BrowserStateKeyedService and returns the correctly typed context to
  // use (respectively content::BrowserContext and web::BrowserState) from
  // the one passed by DependencyManager or the super class. If the context
  // is already correctly typed, then it must be a pass-through.
#if !defined(OS_IOS)
  base::SupportsUserData* GetUnderlyingContext(
      base::SupportsUserData* context) const {
    return context;
  }
#else
  // Method comment ...
  base::SupportsUserData* GetUnderlyingContext(
      base::SupportsUserData* context) const;
  virtual base::SupportsUserData* GetUnderlyingContextInternal(
      base::SupportsUserData* context) const = 0;
#endif

[Elliot Glaysher, 2015-04-22 17:34:07]

https://codereview.chromium.org/1090373003/diff/60001/components/keyed_servic...
File components/keyed_service/core/keyed_service_factory.cc (right):

https://codereview.chromium.org/1090373003/diff/60001/components/keyed_servic...
components/keyed_service/core/keyed_service_factory.cc:25: context =
GetUnderlyingContext(context);
On 2015/04/22 08:53:15, sdefresne wrote:
> On 2015/04/21 17:54:32, Elliot Glaysher wrote:
> > This seems like it could be very error prone. Is this one of the temporary
> > things?
> 
> Yes, all calls to GetUnderlyingContext/GetOriginalContext are temporary.
> 
> The reason is that downstream iOS use a BrowserContextDependencyManager that
> contains both BrowserContextKeyedServiceFactory and
> BrowserStateKeyedServiceFactory.
> 
> When the service factory overrides ServiceIsNULLWhileTesting, the dependency
> manager will call SetEmptyTestingFactory() with the context that is a
> content::BrowserContext. But when the client code calls the service factory
> GetForBrowserState() it passes a web::BrowserState to the service factory, and
> the testing factory is not found (since they are different objects).
> 
> This cross-dependency is quite dangerous, but only happens on iOS downstream.
To
> mitigate the risk, I can change GetUnderlyingContext/GetOriginalContext to be
> simple inline method doing nothing if not targetting iOS, something like:
> 
>   // Returns the underlying context to use for |context|. It is a simple
>   // pass-through for all platform except iOS. On iOS is it used to allow
>   // mixing dependencies between BrowserContextKeyedService and
>   // BrowserStateKeyedService and returns the correctly typed context to
>   // use (respectively content::BrowserContext and web::BrowserState) from
>   // the one passed by DependencyManager or the super class. If the context
>   // is already correctly typed, then it must be a pass-through.
> #if !defined(OS_IOS)
>   base::SupportsUserData* GetUnderlyingContext(
>       base::SupportsUserData* context) const {
>     return context;
>   }
> #else
>   // Method comment ...
>   base::SupportsUserData* GetUnderlyingContext(
>       base::SupportsUserData* context) const;
>   virtual base::SupportsUserData* GetUnderlyingContextInternal(
>       base::SupportsUserData* context) const = 0;
> #endif

Thanks, that sounds like a good plan.

[sdefresne, 2015-04-23 14:30:10]

Patchset #4 (id:80001) has been deleted

[sdefresne, 2015-04-23 14:31:52]

The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-23 14:32:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1090373003/120001

[sdefresne, 2015-04-23 14:32:07]

Patchset #4 (id:100001) has been deleted

[sdefresne, 2015-04-23 14:34:07]

erg, droger: PTAL

[droger, 2015-04-23 15:33:41]

Still LGTM.
I like the new names better.

Now that only iOS implement this, we could optionally get rid of the "Internal"
version of the conversion functions by just duplicating the DCHECK in all
implementations (there are now only 2 implementations).

[commit-bot: I haz the power, 2015-04-23 15:42:48]

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

[commit-bot: I haz the power, 2015-04-23 15:42:49]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2015-04-23 17:14:45]

lgtm

[sdefresne, 2015-04-24 11:30:47]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-04-24 11:30:47]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
erg@chromium.org
Link to the patchset: https://codereview.chromium.org/1090373003/#ps140001
(title: "Remove wrapper methods")

[commit-bot: I haz the power, 2015-04-24 11:30:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1090373003/140001

[commit-bot: I haz the power, 2015-04-24 13:35:32]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-04-24 13:36:23]

Patchset 5 (id:??) landed as
https://crrev.com/8c1791c63d6a05455eb6c1413e01844496a119f4
Cr-Commit-Position: refs/heads/master@{#326782}