Changing SessionService to have a BaseSessionService, not being one.

chrome/browser/sessions/base_session_service.cc

Index: chrome/browser/sessions/base_session_service.cc
diff --git a/chrome/browser/sessions/base_session_service.cc b/chrome/browser/sessions/base_session_service.cc
index fa988d355e797892fb3ef2f9f42595020b703cf9..f93bc429137d78af09c1c8024eb77360d0447ab5 100644
--- a/chrome/browser/sessions/base_session_service.cc
+++ b/chrome/browser/sessions/base_session_service.cc
@@ -17,7 +17,7 @@ namespace {
 // 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;
@@ -26,7 +26,7 @@ void RunIfNotCanceled(
 
 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());
@@ -48,17 +48,23 @@ 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() {
@@ -80,47 +86,29 @@ void BaseSessionService::StartSaveTimer() {
       !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);
 
@@ -131,6 +119,36 @@ BaseSessionService::ScheduleGetLastSessionCommands(
   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) {