Decoupled the SchemaRegistryService from SchemaRegistry.

Decoupled the SchemaRegistryService from SchemaRegistry.

The service is not a SchemaRegistry anymore but owns one instead. This
enables the service to own subclasses of SchemaRegistry.

Also added a ForwardingSchemaRegisy, which wraps another
SchemaRegistry. This will be used in a future CL.

R=bartfab@chromium.org
TBR=kalman@chromium.org
BUG=224596
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278735

[Joao da Silva, 2014-06-20 14:24:40]

PTAL

[bartfab (slow), 2014-06-20 14:48:01]

lgtm

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
File chrome/browser/policy/schema_registry_service.cc (right):

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
chrome/browser/policy/schema_registry_service.cc:7: #include "base/logging.h"
Nit: No longer used.

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
File chrome/browser/policy/schema_registry_service.h (right):

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
chrome/browser/policy/schema_registry_service.h:9: #include
"base/compiler_specific.h"
Nit: No longer needed.

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
chrome/browser/policy/schema_registry_service.h:11: #include
"components/policy/core/common/schema_registry.h"
Nit: A forward-declaration is sufficient now.

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
chrome/browser/policy/schema_registry_service.h:31: scoped_ptr<SchemaRegistry>
registry_;
Nit: #include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
File chrome/browser/policy/schema_registry_service_factory.cc (right):

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
chrome/browser/policy/schema_registry_service_factory.cc:64:
SchemaRegistryService* service = new SchemaRegistryService(
Nit: Since you are updating this part of the code anyway: The
SchemaRegistryService should be captured in a scoped_ptr from the moment it is
created, not just at the end of the method.

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
File components/policy/core/common/schema_registry.cc (right):

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
components/policy/core/common/schema_registry.cc:213: // else ignore.
Nit: Make this more like a sentence, e.g. "Ignore otherwise."

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
components/policy/core/common/schema_registry.cc:219: // else ignore.
Nit: Make this more like a sentence, e.g. "Ignore otherwise."

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
File components/policy/core/common/schema_registry.h (right):

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
components/policy/core/common/schema_registry.h:46: // This observer is only
meant to be used by subclasses.
Why make it public then?

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
components/policy/core/common/schema_registry.h:82: void
AddInternalObserver(InternalObserver* observer);
Why are these public if they are meant for subclasses only?

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
File components/policy/core/common/schema_registry_unittest.cc (right):

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
components/policy/core/common/schema_registry_unittest.cc:134:
scoped_ptr<SchemaRegistry> registry1(new SchemaRegistry);
Nit: #include "base/memory/scoped_ptr.h"

[bartfab (slow), 2014-06-20 14:51:02]

Still lgtm.

[Joao da Silva, 2014-06-20 15:30:14]

kalman@ to TBR for the mechanical changes to the extensions code.

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
File chrome/browser/policy/schema_registry_service.cc (right):

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
chrome/browser/policy/schema_registry_service.cc:7: #include "base/logging.h"
On 2014/06/20 14:48:01, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
File chrome/browser/policy/schema_registry_service.h (right):

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
chrome/browser/policy/schema_registry_service.h:9: #include
"base/compiler_specific.h"
On 2014/06/20 14:48:01, bartfab wrote:
> Nit: No longer needed.

Done.

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
chrome/browser/policy/schema_registry_service.h:11: #include
"components/policy/core/common/schema_registry.h"
On 2014/06/20 14:48:01, bartfab wrote:
> Nit: A forward-declaration is sufficient now.

Done.

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
chrome/browser/policy/schema_registry_service.h:31: scoped_ptr<SchemaRegistry>
registry_;
On 2014/06/20 14:48:01, bartfab wrote:
> Nit: #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
File chrome/browser/policy/schema_registry_service_factory.cc (right):

https://codereview.chromium.org/349643002/diff/1/chrome/browser/policy/schema...
chrome/browser/policy/schema_registry_service_factory.cc:64:
SchemaRegistryService* service = new SchemaRegistryService(
On 2014/06/20 14:48:01, bartfab wrote:
> Nit: Since you are updating this part of the code anyway: The
> SchemaRegistryService should be captured in a scoped_ptr from the moment it is
> created, not just at the end of the method.

Done.

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
File components/policy/core/common/schema_registry.cc (right):

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
components/policy/core/common/schema_registry.cc:213: // else ignore.
On 2014/06/20 14:48:01, bartfab wrote:
> Nit: Make this more like a sentence, e.g. "Ignore otherwise."

Done.

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
components/policy/core/common/schema_registry.cc:219: // else ignore.
On 2014/06/20 14:48:01, bartfab wrote:
> Nit: Make this more like a sentence, e.g. "Ignore otherwise."

Done.

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
File components/policy/core/common/schema_registry.h (right):

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
components/policy/core/common/schema_registry.h:46: // This observer is only
meant to be used by subclasses.
On 2014/06/20 14:48:01, bartfab wrote:
> Why make it public then?

Ha :-) It was protected initially, but it doesn't work as you'd expect.

Basically ForwardingSchemaRegistry can't inherit from this if this is protected,
even if it inherits from SchemaRegistry.

Same for the AddInternalObserver() method: you can call that method on |this|
but you can't call it on other objects of the superclass (you can call it on
objects of the same class though).

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
components/policy/core/common/schema_registry.h:82: void
AddInternalObserver(InternalObserver* observer);
On 2014/06/20 14:48:01, bartfab wrote:
> Why are these public if they are meant for subclasses only?

See above

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
File components/policy/core/common/schema_registry_unittest.cc (right):

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
components/policy/core/common/schema_registry_unittest.cc:134:
scoped_ptr<SchemaRegistry> registry1(new SchemaRegistry);
On 2014/06/20 14:48:01, bartfab wrote:
> Nit: #include "base/memory/scoped_ptr.h"

Done.

[bartfab (slow), 2014-06-20 15:44:18]

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
File components/policy/core/common/schema_registry.h (right):

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
components/policy/core/common/schema_registry.h:46: // This observer is only
meant to be used by subclasses.
On 2014/06/20 15:30:14, Joao da Silva wrote:
> On 2014/06/20 14:48:01, bartfab wrote:
> > Why make it public then?
> 
> Ha :-) It was protected initially, but it doesn't work as you'd expect.
> 
> Basically ForwardingSchemaRegistry can't inherit from this if this is
protected,
> even if it inherits from SchemaRegistry.
> 
> Same for the AddInternalObserver() method: you can call that method on |this|
> but you can't call it on other objects of the superclass (you can call it on
> objects of the same class though).

Makes sense, when you take the meaning of "protected" absolutely literally (as I
guess compilers do). LGTM as-is then. I wonder though whether a cryptic name
like InternalObserver is really necessary/useful here. It could be called
something like LifetimeObserver. It is mostly useful for subclasses but if some
other class ever decides it wants to track this class' lifetime, why not let it?

[Joao da Silva, 2014-06-20 15:47:43]

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
File components/policy/core/common/schema_registry.h (right):

https://codereview.chromium.org/349643002/diff/1/components/policy/core/commo...
components/policy/core/common/schema_registry.h:46: // This observer is only
meant to be used by subclasses.
On 2014/06/20 15:44:18, bartfab wrote:
> On 2014/06/20 15:30:14, Joao da Silva wrote:
> > On 2014/06/20 14:48:01, bartfab wrote:
> > > Why make it public then?
> > 
> > Ha :-) It was protected initially, but it doesn't work as you'd expect.
> > 
> > Basically ForwardingSchemaRegistry can't inherit from this if this is
> protected,
> > even if it inherits from SchemaRegistry.
> > 
> > Same for the AddInternalObserver() method: you can call that method on
|this|
> > but you can't call it on other objects of the superclass (you can call it on
> > objects of the same class though).
> 
> Makes sense, when you take the meaning of "protected" absolutely literally (as
I
> guess compilers do). LGTM as-is then. I wonder though whether a cryptic name
> like InternalObserver is really necessary/useful here. It could be called
> something like LifetimeObserver. It is mostly useful for subclasses but if
some
> other class ever decides it wants to track this class' lifetime, why not let
it?

Sounds reasonable. The current name hints at its current usage; let's rename it
when we get an external observer then.

[Joao da Silva, 2014-06-20 17:14:21]

Committed patchset #4 manually as r278735 (presubmit successful).