Changing SessionService to have a BaseSessionService, not being one.

chrome/browser/sessions/base_session_service.h

Index: chrome/browser/sessions/base_session_service.h
diff --git a/chrome/browser/sessions/base_session_service.h b/chrome/browser/sessions/base_session_service.h
index 1a78ddf7e159a71096d8ecae3c4eee6535f36aef..e5dc2448cf46c0edb9133859fbe2fb3a32cb6ec8 100644
--- a/chrome/browser/sessions/base_session_service.h
+++ b/chrome/browser/sessions/base_session_service.h
@@ -39,24 +39,20 @@ class BaseSessionService {
       GetCommandsCallback;
 
   // Creates a new BaseSessionService. After creation you need to invoke
-  // Init.
+  // Init. |delegate| will remain owned by the creator and it is guaranteed
+  // that its lifetime surpasses this class.
   // |type| gives the type of session service, |path| the path to save files to.
   BaseSessionService(SessionType type,
                      const base::FilePath& path,
-                     scoped_ptr<BaseSessionServiceDelegate> delegate);
+                     BaseSessionServiceDelegate* delegate);
+  virtual ~BaseSessionService();

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

No one subclasses this anymore, right? Can this be non-virtual? You'll need to
remove virtual from all functions, but that should be possible now anyway,
right?

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

Done.

+
+  // Moves the current session to the last session.
+  void MoveCurrentSessionToLastSession();
 
   // Deletes the last session.
   void DeleteLastSession();
 
-  typedef base::Callback<void(ScopedVector<SessionCommand>)>
-      InternalGetCommandsCallback;
-
- protected:
-  virtual ~BaseSessionService();
-
-  // Returns the backend.
-  SessionBackend* backend() const { return backend_.get(); }
-
   // Returns the set of commands which were scheduled to be written. Once
   // committed to the backend, the commands are removed from here.
   ScopedVector<SessionCommand>& pending_commands() {

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

I don't like exposing pending_commands_ like this, and the style guide says you
should use * for this anyway.
I think the signature should be:
AddCommands(ScopedVe...);

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

That is not enough. The class ReplacePendingCommands is removing obsolete
commands from the stream (before they get written).

Since ReplacePendingCommands has however intimate knowledge of the pending
commands themselves (commando id's) that function has to reside in a higher
function and cannot move into BaseSessionService or below.

However - I changed the '&' to a '*'. 

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

I don't like exposing the detail of the commands. I think it better to expose
the commands as const and have a specific function to remove an entry. I believe
that is all you need.

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

More was needed - Done.

@@ -78,32 +74,39 @@ class BaseSessionService {
   // Starts the timer that invokes Save (if timer isn't already running).
   void StartSaveTimer();
 
-  // Saves pending commands to the backend. This is invoked from the timer
-  // scheduled by StartSaveTimer.
-  virtual void Save();
+  // Passes all pending commands to the backend for saving. Does nothing if
+  // no backend exists
+  virtual void SaveNow();
 
-  // Returns true if the entry at specified |url| should be written to disk.
-  bool ShouldTrackEntry(const GURL& url);
-
-  // Invokes SessionBackend::ReadLastSessionCommands with callback on the
-  // backend thread.
-  // If testing, SessionBackend::ReadLastSessionCommands is invoked directly.
+  // Uses the backend to load the last session commands from disc. |callback|
+  // gets called once the data has arrived.
   base::CancelableTaskTracker::TaskId ScheduleGetLastSessionCommands(
-      const InternalGetCommandsCallback& callback,
+      const GetCommandsCallback& callback,
       base::CancelableTaskTracker* tracker);
 
-  // This posts the task to the SequencedWorkerPool, or run immediately
-  // if the SequencedWorkerPool has been shutdown.
-  void RunTaskOnBackendThread(const tracked_objects::Location& from_here,
-                              const base::Closure& task);
+  // Returns true if any commands got processed yet (used by unit tests).
+  bool ProcessedAnyCommandsForTest();

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

Don't make this public. Access it through SessionServiceTestHelper which is a
friend.

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

Done.

 
   // Max number of navigation entries in each direction we'll persist.
   static const int max_persist_navigation_count;
 
  private:
   friend class BetterSessionRestoreCrashTest;
+  friend class SessionServiceTestHelper;
+
+  // Saves pending commands to the backend immediately. This gets invoked by the
+  // timer scheduled by StartSaveTimer - or via SaveNow().
+  void SaveInternal();
+
+  // Returns the backend.
+  SessionBackend* backend() const { return backend_.get(); }

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

Any reason you can't use backend_ directly?

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

Yes, there was still one more reference from a unit tests. I added now instead
the referenced function and called through. So done.

+
+  // This posts the task to the SequencedWorkerPool, or run immediately
+  // if the SequencedWorkerPool has been shutdown.
+  void RunTaskOnBackendThread(const tracked_objects::Location& from_here,
+                              const base::Closure& task);
 
-  // The backend.
+  // The backend object which reads and saves commands.
   scoped_refptr<SessionBackend> backend_;
 
   // Commands we need to send over to the backend.
@@ -116,7 +119,7 @@ class BaseSessionService {
   // The number of commands sent to the backend before doing a reset.
   int commands_since_reset_;
 
-  scoped_ptr<BaseSessionServiceDelegate> delegate_;
+  BaseSessionServiceDelegate* delegate_;
 
   // A token to make sure that all tasks will be serialized.
   base::SequencedWorkerPool::SequenceToken sequence_token_;