Changing SessionService to have a BaseSessionService, not being one.

chrome/browser/sessions/base_session_service.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 "chrome/browser/sessions/base_session_service.h"
 
 #include "base/bind.h"
 #include "base/threading/thread.h"
 #include "chrome/browser/sessions/base_session_service_delegate.h"
 #include "chrome/browser/sessions/session_backend.h"
 
 // BaseSessionService ---------------------------------------------------------
 
 namespace {
 
 // Helper used by ScheduleGetLastSessionCommands. It runs callback on TaskRunner
 // thread if it's not canceled.
 void RunIfNotCanceled(
     const base::CancelableTaskTracker::IsCanceledCallback& is_canceled,
-    const BaseSessionService::InternalGetCommandsCallback& callback,
+    const BaseSessionService::GetCommandsCallback& callback,
     ScopedVector<SessionCommand> commands) {
   if (is_canceled.Run())
     return;
   callback.Run(commands.Pass());
 }
 
 void PostOrRunInternalGetCommandsCallback(
     base::TaskRunner* task_runner,
-    const BaseSessionService::InternalGetCommandsCallback& callback,
+    const BaseSessionService::GetCommandsCallback& callback,
     ScopedVector<SessionCommand> commands) {
   if (task_runner->RunsTasksOnCurrentThread()) {
     callback.Run(commands.Pass());
   } else {
     task_runner->PostTask(FROM_HERE,
                           base::Bind(callback, base::Passed(&commands)));
   }
 }
 
 }  // namespace
 
 // Delay between when a command is received, and when we save it to the
 // backend.
 static const int kSaveDelayMS = 2500;
 
 // static
 const int BaseSessionService::max_persist_navigation_count = 6;
 
 BaseSessionService::BaseSessionService(
     SessionType type,
     const base::FilePath& path,
-    scoped_ptr<BaseSessionServiceDelegate> delegate)
+    BaseSessionServiceDelegate* delegate)
     : pending_reset_(false),
       commands_since_reset_(0),
-      delegate_(delegate.Pass()),
+      delegate_(delegate),
       sequence_token_(delegate_->GetBlockingPool()->GetSequenceToken()),
       weak_factory_(this) {
   backend_ = new SessionBackend(type, path);
   DCHECK(backend_.get());
 }
 
-BaseSessionService::~BaseSessionService() {
+BaseSessionService::~BaseSessionService() {}
+
+void BaseSessionService::MoveCurrentSessionToLastSession() {
+  SaveNow();
+  RunTaskOnBackendThread(
+      FROM_HERE, base::Bind(&SessionBackend::MoveCurrentSessionToLastSession,
+                            backend()));
 }
 
 void BaseSessionService::DeleteLastSession() {
   RunTaskOnBackendThread(
       FROM_HERE,
       base::Bind(&SessionBackend::DeleteLastSession, backend()));
 }
 
 void BaseSessionService::ScheduleCommand(scoped_ptr<SessionCommand> command) {
   DCHECK(command);
   commands_since_reset_++;
   pending_commands_.push_back(command.release());
   StartSaveTimer();
 }
 
 void BaseSessionService::StartSaveTimer() {
   // Don't start a timer when testing.
   if (delegate_->ShouldUseDelayedSave() && base::MessageLoop::current() &&
       !weak_factory_.HasWeakPtrs()) {
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
-        base::Bind(&BaseSessionService::Save, weak_factory_.GetWeakPtr()),
+        base::Bind(&BaseSessionService::SaveInternal,
+                   weak_factory_.GetWeakPtr()),
         base::TimeDelta::FromMilliseconds(kSaveDelayMS));
   }
 }
 
-void BaseSessionService::Save() {
-  DCHECK(backend());
-
-  if (pending_commands_.empty())
-    return;
-
-  // We create a new ScopedVector which will receive all elements from the
-  // current commands. This will also clear the current list.
-  RunTaskOnBackendThread(
-      FROM_HERE,
-      base::Bind(&SessionBackend::AppendCommands, backend(),
-                 new ScopedVector<SessionCommand>(pending_commands_.Pass()),
-                 pending_reset_));
-
-  if (pending_reset_) {
-    commands_since_reset_ = 0;
-    pending_reset_ = false;
-  }
-}
-
-bool BaseSessionService::ShouldTrackEntry(const GURL& url) {
-  return url.is_valid() && delegate_->ShouldTrackEntry(url);
+void BaseSessionService::SaveNow() {
+  if (backend())

[sky, 2014/10/31 22:36:16]

Why the check here? backend_ is basically never reset. In fact backend_ should
really be a value, but that can be done later.
Move the implementation back to this.

[Mr4D (OOO till 08-26), 2014/11/01 01:28:23]

Main reason for this change was to remove as many backend() uses as possible
from higher / other levels. In this case  ~PersistentTabRestoreService was
explicitly checking the backend() and did not call if it was NULL. Not quite
sure how the refptr could go to zero without being released by this ctor, but...

I agree that this is highly suspicious, but this explicit check made me wonder
if there were crashers upon shutdown happening. 

I think I know you well enough that you want me to remove it anyways - so it's
gone now. Done.

+    SaveInternal();
 }
 
 base::CancelableTaskTracker::TaskId
 BaseSessionService::ScheduleGetLastSessionCommands(
-    const InternalGetCommandsCallback& callback,
+    const GetCommandsCallback& callback,
     base::CancelableTaskTracker* tracker) {
   base::CancelableTaskTracker::IsCanceledCallback is_canceled;
   base::CancelableTaskTracker::TaskId id =
       tracker->NewTrackedTaskId(&is_canceled);
 
-  InternalGetCommandsCallback run_if_not_canceled =
+  GetCommandsCallback run_if_not_canceled =
       base::Bind(&RunIfNotCanceled, is_canceled, callback);
 
-  InternalGetCommandsCallback callback_runner =
+  GetCommandsCallback callback_runner =
       base::Bind(&PostOrRunInternalGetCommandsCallback,
                  base::MessageLoopProxy::current(), run_if_not_canceled);
 
   RunTaskOnBackendThread(
       FROM_HERE,
       base::Bind(&SessionBackend::ReadLastSessionCommands, backend(),
                  is_canceled, callback_runner));
   return id;
 }
 
+bool BaseSessionService::ProcessedAnyCommandsForTest() {
+  return backend()->inited() || !pending_commands().empty();

[sky, 2014/10/31 22:36:16]

This is racy as inited_ is set on another thread.

[Mr4D (OOO till 08-26), 2014/11/01 01:28:23]

Again - this was part of a test
(NoStartupWindowTest.DontInitSessionServiceForApps) and I moved it here to move
the use of the backend() out of higher levels.

[I was under the impression that this code did work - but if you are right and
this is racy - should I add a WaitUntilLoaded function to make sure that this is
there?]

[sky, 2014/11/03 15:53:22]

Tests can make sure to wait for the right events.

[Mr4D (OOO till 08-26), 2014/11/03 22:05:50]

Done.

+}
+
+void BaseSessionService::SaveInternal() {
+  DCHECK(backend());

[sky, 2014/10/31 22:36:16]

Again, why the dcheck?

[Mr4D (OOO till 08-26), 2014/11/01 01:28:23]

Same reasoning as above. But this is gone now for good and if crashers come they
will come.

[sky, 2014/11/03 15:53:22]

A DCHECK isn't going to stop crashers.

+
+  // Inform the delegate that we will save the commands now, giving it the
+  // opportunity to append more commands.
+  delegate_->OnWillSaveCommands();
+
+  if (pending_commands_.empty())
+    return;
+
+  // We create a new ScopedVector which will receive all elements from the
+  // current commands. This will also clear the current list.
+  RunTaskOnBackendThread(
+      FROM_HERE,
+      base::Bind(&SessionBackend::AppendCommands, backend(),
+                 new ScopedVector<SessionCommand>(pending_commands_.Pass()),
+                 pending_reset_));
+
+  if (pending_reset_) {
+    commands_since_reset_ = 0;
+    pending_reset_ = false;
+  }
+
+  delegate_->OnSavedCommands();
+}
+
 void BaseSessionService::RunTaskOnBackendThread(
     const tracked_objects::Location& from_here,
     const base::Closure& task) {
   base::SequencedWorkerPool* pool = delegate_->GetBlockingPool();
   if (!pool->IsShutdownInProgress()) {
     pool->PostSequencedWorkerTask(sequence_token_, from_here, task);
   } else {
     // Fall back to executing on the main thread if the sequence
     // worker pool has been requested to shutdown (around shutdown
     // time).
     task.Run();
   }
 }