Reduce dependency of TiclInvalidationService on Profile

sync/notifier/non_blocking_invalidator.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/notifier/non_blocking_invalidator.h"
 
 #include <cstddef>
 
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/thread.h"
 #include "jingle/notifier/listener/push_client.h"
+#include "sync/internal_api/public/util/weak_handle.h"
 #include "sync/notifier/gcm_network_channel_delegate.h"
 #include "sync/notifier/invalidation_notifier.h"
 #include "sync/notifier/object_id_invalidation_map.h"
 #include "sync/notifier/sync_system_resources.h"
 
 namespace syncer {
 
 struct NonBlockingInvalidator::InitializeOptions {
   InitializeOptions(
       NetworkChannelCreator network_channel_creator,
@@ skipping 152 matching lines @@
 
 void NonBlockingInvalidator::Core::RequestDetailedStatus(
     base::Callback<void(const base::DictionaryValue&)> callback) const {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
   invalidation_notifier_->RequestDetailedStatus(callback);
 }
 
 void NonBlockingInvalidator::Core::OnInvalidatorStateChange(
     InvalidatorState reason) {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
   delegate_observer_.Call(

[Nicolas Zea, 2014/04/15 16:30:42]

these calls here would no longer interact directly with the invalidation
handler's weak handle. Instead they'd be routed through the
NonBlockingInvalidator to deal with lifetime issues.

[bartfab (slow), 2014/04/15 16:48:44]

If it was only NonBlockingInvalidator::Core that accessed the
InvalidationStateTracker, what you said would be easy. However, the WeakHandle
is passed on to a lot of other classes that invoke methods on it:

- NonBlockingInvalidator::Core passes the invalidation_state_tracker to an
InvalidationNotifier.
- The InvalidationNotifier passes the invalidation_state_tracker to a
SyncInvalidationListener.
- The SyncInvalidationListener makes calls to the invalidation_state_tracker
directly.

You would need to route all these calls back up to NonBlockingInvalidator::Core
who could then dispatch them if the InvalidationStateTracker is still alive.

This is a large refactoring of your code. I do not know the code well enough to
want to make such refactorings. If you want to make such fundamental changes,
let's agree on a first step that will allow me to remove the Profile dependency
and I will hand off to you at that point.

[Nicolas Zea, 2014/04/15 17:50:55]

Anything beyond the core isn't a concern though. I still think this can be a
minimal change.

One way would be to have the NonBlockingInvalidator implement (or have a member
implement) the InvalidationHandler interface. It would then just delegate all
the methods to the real invalidation handler. You could then create a WeakHandle
to the NonBlockingInvalidator (or whichever member implements the interface),
and have that be what you pass around.

The main thing is that the NonBlockingInvalidator controls lifetime itself, so
there's no need to care about getting a weak pointer to anything outside of it.

       FROM_HERE, &InvalidationHandler::OnInvalidatorStateChange, reason);
 }
 
 void NonBlockingInvalidator::Core::OnIncomingInvalidation(
     const ObjectIdInvalidationMap& invalidation_map) {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
   delegate_observer_.Call(FROM_HERE,
                           &InvalidationHandler::OnIncomingInvalidation,
                           invalidation_map);
 }
 
 std::string NonBlockingInvalidator::Core::GetOwnerName() const {
   return "Sync";
 }
 
 NonBlockingInvalidator::NonBlockingInvalidator(
     NetworkChannelCreator network_channel_creator,
     const std::string& invalidator_client_id,
     const UnackedInvalidationsMap& saved_invalidations,
     const std::string& invalidation_bootstrap_data,
-    const WeakHandle<InvalidationStateTracker>&
-        invalidation_state_tracker,
+    InvalidationStateTracker* invalidation_state_tracker,
     const std::string& client_info,
     const scoped_refptr<net::URLRequestContextGetter>& request_context_getter)
     : parent_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       network_task_runner_(request_context_getter->GetNetworkTaskRunner()),
+      invalidation_state_tracker_weak_ptr_factory_(invalidation_state_tracker),
       weak_ptr_factory_(this) {
   core_ = new Core(MakeWeakHandle(weak_ptr_factory_.GetWeakPtr()));
 
   InitializeOptions initialize_options(
       network_channel_creator,
       invalidator_client_id,
       saved_invalidations,
       invalidation_bootstrap_data,
-      invalidation_state_tracker,
+      MakeWeakHandle(invalidation_state_tracker_weak_ptr_factory_.GetWeakPtr()),

[Nicolas Zea, 2014/04/15 16:30:42]

I don't think this is necessary. Instead just have any messages to the state
tracker go through the NonBlockingInvalidator (bound the response lifetime to a
weak ptr vended internally by the NonBlockingInvalidator).

Because the lifetime of the state tracker is guaranteed to outlive the
invalidator, there's no need for a weak pointer.

       client_info,
       request_context_getter);
 
   if (!network_task_runner_->PostTask(
           FROM_HERE,
           base::Bind(
               &NonBlockingInvalidator::Core::Initialize,
               core_.get(),
               initialize_options))) {
     NOTREACHED();
@@ skipping 86 matching lines @@
 
 NetworkChannelCreator NonBlockingInvalidator::MakeGCMNetworkChannelCreator(
     scoped_refptr<net::URLRequestContextGetter> request_context_getter,
     scoped_ptr<GCMNetworkChannelDelegate> delegate) {
   return base::Bind(&SyncNetworkChannel::CreateGCMNetworkChannel,
                     request_context_getter,
                     base::Passed(&delegate));
 }
 
 }  // namespace syncer