Changing SessionService to have a BaseSessionService, not being one.

chrome/browser/sessions/base_session_service.h

 // 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.
 
 #ifndef CHROME_BROWSER_SESSIONS_BASE_SESSION_SERVICE_H_
 #define CHROME_BROWSER_SESSIONS_BASE_SESSION_SERVICE_H_
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/files/file_path.h"
@@ skipping 21 matching lines @@
   // backend to determine the name of the files.
   enum SessionType {
     SESSION_RESTORE,
     TAB_RESTORE
   };
 
   typedef base::Callback<void(ScopedVector<SessionCommand>)>
       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.

     return pending_commands_;
   }
 
   // Whether the next save resets the file before writing to it.
   void set_pending_reset(bool value) { pending_reset_ = value; }
   bool pending_reset() const { return pending_reset_; }
 
   // Returns the number of commands sent down since the last reset.
   int commands_since_reset() const { return commands_since_reset_; }
 
   // Schedules a command. This adds |command| to pending_commands_ and
   // invokes StartSaveTimer to start a timer that invokes Save at a later
   // time.
   virtual void ScheduleCommand(scoped_ptr<SessionCommand> command);
 
   // 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);
+  // Uses the backend to load the last session commands from disc. |callback|
+  // gets called once the data has arrived.
+  base::CancelableTaskTracker::TaskId ScheduleGetLastSessionCommands(
+      const GetCommandsCallback& callback,
+      base::CancelableTaskTracker* tracker);
 
-  // Invokes SessionBackend::ReadLastSessionCommands with callback on the
-  // backend thread.
-  // If testing, SessionBackend::ReadLastSessionCommands is invoked directly.
-  base::CancelableTaskTracker::TaskId ScheduleGetLastSessionCommands(
-      const InternalGetCommandsCallback& callback,
-      base::CancelableTaskTracker* tracker);
+  // 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);
 
-  // Max number of navigation entries in each direction we'll persist.
-  static const int max_persist_navigation_count;
-
- private:
-  friend class BetterSessionRestoreCrashTest;
-
-  // The backend.
+  // The backend object which reads and saves commands.
   scoped_refptr<SessionBackend> backend_;
 
   // Commands we need to send over to the backend.
   ScopedVector<SessionCommand> pending_commands_;
 
   // Whether the backend file should be recreated the next time we send
   // over the commands.
   bool pending_reset_;
 
   // 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_;
 
   // Used to invoke Save.
   base::WeakPtrFactory<BaseSessionService> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(BaseSessionService);
 };
 
 #endif  // CHROME_BROWSER_SESSIONS_BASE_SESSION_SERVICE_H_