Profile shouldn't own DesktopNotificationService.

Profile shouldn't own DesktopNotificationService.

DesktopNotificationService is now owned by
DesktopNotificationServiceFactory, using Profile as a key. This uses the
ProfileKeyedService infrastructure originally created for
ThemeServiceFactory.

BUG=77155
TEST=existing tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81399

[Torne, 2011-04-06 14:08:08]

This service is much less problematic than CloudPrintProxyService :)

[Torne, 2011-04-06 14:53:50]

On 2011/04/06 14:08:08, Torne wrote:
> This service is much less problematic than CloudPrintProxyService :)

Okay, I said that but there is a problem after all: TestingProfile used to
override GetDesktopNotificationService to create it with different arguments
during testing. There's no obvious mechanism to do this now that won't end up
putting test case code into the main binary... thoughts? :)

[Elliot Glaysher, 2011-04-06 17:32:00]

On 2011/04/06 14:53:50, Torne wrote:
> On 2011/04/06 14:08:08, Torne wrote:
> > This service is much less problematic than CloudPrintProxyService :)
> 
> Okay, I said that but there is a problem after all: TestingProfile used to
> override GetDesktopNotificationService to create it with different arguments
> during testing. There's no obvious mechanism to do this now that won't end up
> putting test case code into the main binary... thoughts? :)

Some equivalent of ThemeServiceFactory::ForceAssociationBetween() called in
TestingProfile's constructor? (I don't like that a lazily initialized thing is
being turned into a constructor initialized thing, but thems the breaks)

[torne_google.com, 2011-04-06 18:01:17]

I tried this and it doesn't work because the constructor/destructor
for TestingProfile is not always called on the UI thread - I even
tried having it only construct it in the constructor if it's on the UI
thread and it still crashes some of the unit tests because the
*destructor* is being called from a different thread.

It might well be argued that this represents bugs in the tests in question ;)

On 6 April 2011 18:32,  <erg@chromium.org> wrote:
> On 2011/04/06 14:53:50, Torne wrote:
>>
>> On 2011/04/06 14:08:08, Torne wrote:
>> > This service is much less problematic than CloudPrintProxyService :)
>
>> Okay, I said that but there is a problem after all: TestingProfile used to
>> override GetDesktopNotificationService to create it with different
>> arguments
>> during testing. There's no obvious mechanism to do this now that won't end
>> up
>> putting test case code into the main binary... thoughts? :)
>
> Some equivalent of ThemeServiceFactory::ForceAssociationBetween() called in
> TestingProfile's constructor? (I don't like that a lazily initialized thing
> is
> being turned into a constructor initialized thing, but thems the breaks)
>
> http://codereview.chromium.org/6803012/
>



-- 
Torne (Richard Coles)
torne@google.com

[Miranda Callahan, 2011-04-06 19:08:02]

On 2011/04/06 18:01:17, Torne (Richard Coles) wrote:
> I tried this and it doesn't work because the constructor/destructor
> for TestingProfile is not always called on the UI thread - I even
> tried having it only construct it in the constructor if it's on the UI
> thread and it still crashes some of the unit tests because the
> *destructor* is being called from a different thread.
> 
> It might well be argued that this represents bugs in the tests in question ;)
> 
> On 6 April 2011 18:32,  <mailto:erg@chromium.org> wrote:
> > On 2011/04/06 14:53:50, Torne wrote:
> >>
> >> On 2011/04/06 14:08:08, Torne wrote:
> >> > This service is much less problematic than CloudPrintProxyService :)
> >
> >> Okay, I said that but there is a problem after all: TestingProfile used to
> >> override GetDesktopNotificationService to create it with different
> >> arguments
> >> during testing. There's no obvious mechanism to do this now that won't end
> >> up
> >> putting test case code into the main binary... thoughts? :)
> >
> > Some equivalent of ThemeServiceFactory::ForceAssociationBetween() called in
> > TestingProfile's constructor? (I don't like that a lazily initialized thing
> > is
> > being turned into a constructor initialized thing, but thems the breaks)
> >
> > http://codereview.chromium.org/6803012/
> >
> 
> 
> 
> -- 
> Torne (Richard Coles)
> mailto:torne@google.com

Is it perhaps failing only on those unit tests that don't artificially construct
a UI BrowserThread using the two-parameter constructor in browser_thread.h?

[Torne, 2011-04-08 11:36:34]

OK, this new approach seems to be working fine: have a unit-test only API which
lets factories' creation functions be overridden. This is done in the speech
bubble system already, from which I have largely lifted this :)

So, TestingProfile can override anything it likes this way, without having to
copypaste the implementation of things it wants to leave being created normally.

[Miranda Callahan, 2011-04-08 15:29:05]

On 2011/04/08 11:36:34, Torne wrote:
> OK, this new approach seems to be working fine: have a unit-test only API
which
> lets factories' creation functions be overridden. This is done in the speech
> bubble system already, from which I have largely lifted this :)
> 
> So, TestingProfile can override anything it likes this way, without having to
> copypaste the implementation of things it wants to leave being created
normally.

LGTM, but definitely wait for Elliot. :-)

[Elliot Glaysher, 2011-04-08 17:43:32]

http://codereview.chromium.org/6803012/diff/12005/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_keyed_service_factory.h (right):

http://codereview.chromium.org/6803012/diff/12005/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_keyed_service_factory.h:29: void
set_factory_function(FactoryFunction factory) { factory_ = factory; }
This is fine, but I'm worried that there isn't an association between a Profile
and the factory function.

Could this be modified to be something like set_factory_function_for(Profile* p,
FactoryFunction factory);? I know there are places in our unit tests where we
override the construction of one of the Profile::GetBlah() methods with a mock
only for one or two tests. Since ProfileKeyedServiceFactory is also a Singleton
in most cases, I'm also worried about one test setting state that will bleed
into another.

After doing that, could you please update the unit tests in theme_util_test.cc
to use your new system instead of the previous one I wrote?

[Torne, 2011-04-11 11:52:51]

http://codereview.chromium.org/6803012/diff/12005/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_keyed_service_factory.h (right):

http://codereview.chromium.org/6803012/diff/12005/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_keyed_service_factory.h:29: void
set_factory_function(FactoryFunction factory) { factory_ = factory; }
On 2011/04/08 17:43:32, Elliot Glaysher wrote:
> This is fine, but I'm worried that there isn't an association between a
Profile
> and the factory function.
> 
> Could this be modified to be something like set_factory_function_for(Profile*
p,
> FactoryFunction factory);? I know there are places in our unit tests where we
> override the construction of one of the Profile::GetBlah() methods with a mock
> only for one or two tests. Since ProfileKeyedServiceFactory is also a
Singleton
> in most cases, I'm also worried about one test setting state that will bleed
> into another.
> 
> After doing that, could you please update the unit tests in theme_util_test.cc
> to use your new system instead of the previous one I wrote?

I was assuming that test code does not create normal profiles, and thus having
TestingProfile's changes be global was reasonable (and is certainly easier...).
I don't actually know if this is true; I considered having it only apply to
things which were TestingProfiles... 

I wasn't intending that this should replace the mechanism you used for the theme
service. I don't see any benefit to the added complexity of installing a factory
function rather than just a constructed instance of the service in the case
where the test is doing it as a one-off thing; the reason to have a factory
function for stuff that was being replaced in TestingProfile is because of the
issues we previously discussed in this review (eager creation being a problem
for tests that use setups where that's not a legitimate thing to do).

I'll have a think about this a bit more...

[Torne, 2011-04-11 14:27:41]

OK, I've tried a new approach here:

1) I've moved ForceAssociationWith to the parent class so services don't have to
implement it individually (it's guarded by #ifdef UNIT_TESTS so it can't be
accidentally used by normal code)
2) The test factory function is still global per-factory but it only applies if
ForceAssociationWith has been called to explicitly set the profile's service to
NULL.

This means that normal non-test profiles won't be affected by the test factory
and will be created normally.

This should work unless two different profile implementations want to override
the same service in different ways at the same time...

Elliot, what do you think?

[Elliot Glaysher, 2011-04-11 18:07:58]

On Mon, Apr 11, 2011 at 7:27 AM,  <torne@chromium.org> wrote:
> This should work unless two different profile implementations want to
> override
> the same service in different ways at the same time...
>
> Elliot, what do you think?

That sounds unlikely enough that we shouldn't worry about it. LGTM.

-- Elliot

[Torne, 2011-04-12 10:47:35]

Elliot,

I had to change the way the ProfileDependencyManager unit test worked because
creating a TestingProfile and then changing its dependency manager causes any
services associated in the TestingProfile constructor to leak.

There's no actual need for a profile to exist in this test, as far as I can see,
so I changed it to just explicitly call the manager with a NULL profile and it
works fine that way.

I also removed the SetProfileDependencyManager method from TestingProfile as
it's not needed any more.

Does this seem sane? Hopefully the trybots will be green this time ;)

[Avi (use Gerrit), 2011-04-12 13:57:15]

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service_factory.cc
(right):

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_factory.cc:29: {}
Opening brace on previous line.

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_factory.cc:31:
DesktopNotificationServiceFactory::~DesktopNotificationServiceFactory() {}
Closing brace on following line.

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service_factory.h
(right):

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_factory.h:32: virtual
ProfileKeyedService* BuildServiceInstanceFor(Profile* profile) const;
OVERRIDE

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_factory.h:34: virtual
bool ServiceHasOwnInstanceInIncognito();
OVERRIDE

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_keyed_service_factory.h (right):

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_keyed_service_factory.h:24: typedef
ProfileKeyedService* (*FactoryFunction)(Profile* profile);
Can you pull this into the ifdef?

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_keyed_service_factory.h:104: // A mock factory
function to use to create the service, used by tests.
If this is only used by tests, can you ifdef this too?

[Torne, 2011-04-12 15:22:12]

New CL uploaded with the cosmetic changes; open to suggestions for a nicer way
to handle the test factory thing but I don't think I can do better than using
one pointer..

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service_factory.cc
(right):

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_factory.cc:29: {}
On 2011/04/12 13:57:15, Avi wrote:
> Opening brace on previous line.

Done.

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_factory.cc:31:
DesktopNotificationServiceFactory::~DesktopNotificationServiceFactory() {}
On 2011/04/12 13:57:15, Avi wrote:
> Closing brace on following line.

Done.

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service_factory.h
(right):

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_factory.h:32: virtual
ProfileKeyedService* BuildServiceInstanceFor(Profile* profile) const;
On 2011/04/12 13:57:15, Avi wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_factory.h:34: virtual
bool ServiceHasOwnInstanceInIncognito();
On 2011/04/12 13:57:15, Avi wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_keyed_service_factory.h (right):

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_keyed_service_factory.h:24: typedef
ProfileKeyedService* (*FactoryFunction)(Profile* profile);
On 2011/04/12 13:57:15, Avi wrote:
> Can you pull this into the ifdef?

No, it's needed to declare factory_, which is accessed by the implementation of
GetServiceForProfile.

http://codereview.chromium.org/6803012/diff/14013/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_keyed_service_factory.h:104: // A mock factory
function to use to create the service, used by tests.
On 2011/04/12 13:57:15, Avi wrote:
> If this is only used by tests, can you ifdef this too?

No, the implementation of the class is compiled into libbrowser.a so nothing
significant can be conditional on UNIT_TEST, only inline functions.

[Avi (use Gerrit), 2011-04-12 16:27:35]

Oh, right. LGTM

[Elliot Glaysher, 2011-04-12 17:31:00]

On 2011/04/12 16:27:35, Avi wrote:
> Oh, right. LGTM

LGTM

[commit-bot: I haz the power, 2011-04-13 10:50:03]

Commit queue failed due to updated description.