Changing SessionService to have a BaseSessionService, not being one.

Changing SessionService to have a BaseSessionService, not being one.

BUG=424679
TEST=covered by existing unit tests

Committed: https://crrev.com/d1e250bc34f3e17256bcbe9a5a394a641a9eb804
Cr-Commit-Position: refs/heads/master@{#302803}

[Mr4D (OOO till 08-26), 2014-10-31 19:20:28]

skuhne@chromium.org changed reviewers:
+ sky@chromium.org

[Mr4D (OOO till 08-26), 2014-10-31 19:21:54]

Please have a look!

[Mr4D (OOO till 08-26), 2014-10-31 19:24:01]

Added one comment.

https://codereview.chromium.org/694813003/diff/1/chrome/browser/sessions/base...
File chrome/browser/sessions/base_session_service.h (right):

https://codereview.chromium.org/694813003/diff/1/chrome/browser/sessions/base...
chrome/browser/sessions/base_session_service.h:90: // Max number of navigation
entries in each direction we'll persist.
I do not exactly like to have this constant here - neither as function nor as
constant. It could get moved into the browser dependent part - but where?
session_command_utils? Not really. SessionService AND
PersistentTabRestoreService? Not much better. Any good ideas?

[sky, 2014-10-31 22:36:17]

https://codereview.chromium.org/694813003/diff/1/chrome/browser/sessions/base...
File chrome/browser/sessions/base_session_service.h (right):

https://codereview.chromium.org/694813003/diff/1/chrome/browser/sessions/base...
chrome/browser/sessions/base_session_service.h:90: // Max number of navigation
entries in each direction we'll persist.
On 2014/10/31 19:24:01, Mr4D wrote:
> I do not exactly like to have this constant here - neither as function nor as
> constant. It could get moved into the browser dependent part - but where?
> session_command_utils? Not really. SessionService AND
> PersistentTabRestoreService? Not much better. Any good ideas?

I agree with you that this shouldn't be here. Not sure where to put it though.
What classes do you plan on promoting?

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/base_session_service.cc (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.cc:96: if (backend())
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.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.cc:123: return backend()->inited()
|| !pending_commands().empty();
This is racy as inited_ is set on another thread.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.cc:127: DCHECK(backend());
Again, why the dcheck?

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/base_session_service.h (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:48: virtual
~BaseSessionService();
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?

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:58: ScopedVector<SessionCommand>&
pending_commands() {
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...);

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:88: bool
ProcessedAnyCommandsForTest();
Don't make this public. Access it through SessionServiceTestHelper which is a
friend.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:102: SessionBackend* backend()
const { return backend_.get(); }
Any reason you can't use backend_ directly?

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/persistent_tab_restore_service.cc:230: int
entries_written_;
Isn't this the same as base_session_service_->commands_since_reset()?

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/session_service.cc:834:
base_session_service_->set_pending_reset(true);
I think you should remove this and the next line and after
BuildCommandsFromBrowsers have a:
base_session_service_->ScheduleReset(); (which sets pending_resets_ and clears
pending_commands_).
base_session_service_->AddCommands(xxx);

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/session_service.cc:847:
base_session_service_->StartSaveTimer();
StartSaveTimer shouldn't be necessary with the changes I've outlined.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/session_service.h (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/session_service.h:217: BaseSessionService*
GetBaseSessionServiceForTest();
Same comment about preferring friend here.

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

Addressed - for the most part. See comments!

https://codereview.chromium.org/694813003/diff/1/chrome/browser/sessions/base...
File chrome/browser/sessions/base_session_service.h (right):

https://codereview.chromium.org/694813003/diff/1/chrome/browser/sessions/base...
chrome/browser/sessions/base_session_service.h:90: // Max number of navigation
entries in each direction we'll persist.
Well I think the main thing is that this constant is an arbitrary limiter which
might make sense for Chrome - but not for example for athena.

As such this might be simply added to session_service_utils as a global constant
which can be used by SessionService and PersistentTabRestoreService. Would that
make sense?

Note: I want to make the "component cut" at the |BaseSessionSerrvice| level (+
SessionCommand creation functions).

Moved it there - if you know a better spot - let me know!

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/base_session_service.cc (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.cc:96: if (backend())
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.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.cc:123: return backend()->inited()
|| !pending_commands().empty();
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?]

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.cc:127: DCHECK(backend());
Same reasoning as above. But this is gone now for good and if crashers come they
will come.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/base_session_service.h (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:48: virtual
~BaseSessionService();
On 2014/10/31 22:36:16, sky wrote:
> 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?

Done.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:58: ScopedVector<SessionCommand>&
pending_commands() {
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 '*'.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:88: bool
ProcessedAnyCommandsForTest();
On 2014/10/31 22:36:16, sky wrote:
> Don't make this public. Access it through SessionServiceTestHelper which is a
> friend.

Done.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:102: SessionBackend* backend()
const { return backend_.get(); }
Yes, there was still one more reference from a unit tests. I added now instead
the referenced function and called through. So done.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/persistent_tab_restore_service.cc:230: int
entries_written_;
No. It's not. entries_to_write_ specifies the amount of windows / tabs being
written. To write such a structure multiple commands are required. Since
base_session_service_->commands_since_reset() counts the number of single
commands, it will be much higher.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/session_service.cc:834:
base_session_service_->set_pending_reset(true);
I see what you are getting at, but this isn't as easy as you think it is:
BuildCommandsForBrowsers will add all commands to a vector in your suggestion
and then it will also call ScheduleCommand which will then add commands to the
default stream and even calling ReplacePendingCommand on them.

So if we want to do this more restructuring and careful thought needs to be
applied. I would suggest to postpone that.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/session_service.cc:847:
base_session_service_->StartSaveTimer();
Ditto.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/session_service.h (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/session_service.h:217: BaseSessionService*
GetBaseSessionServiceForTest();
On 2014/10/31 22:36:17, sky wrote:
> Same comment about preferring friend here.

Done.

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

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/base_session_service.cc (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.cc:123: return backend()->inited()
|| !pending_commands().empty();
On 2014/11/01 01:28:23, Mr4D wrote:
> 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?]

Tests can make sure to wait for the right events.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.cc:127: DCHECK(backend());
On 2014/11/01 01:28:23, Mr4D wrote:
> Same reasoning as above. But this is gone now for good and if crashers come
they
> will come.

A DCHECK isn't going to stop crashers.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/base_session_service.h (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:58: ScopedVector<SessionCommand>&
pending_commands() {
On 2014/11/01 01:28:23, Mr4D wrote:
> 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 '*'. 

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]

Please have another look!

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/base_session_service.cc (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.cc:123: return backend()->inited()
|| !pending_commands().empty();
On 2014/11/03 15:53:22, sky wrote:
> On 2014/11/01 01:28:23, Mr4D wrote:
> > 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?]
> 
> Tests can make sure to wait for the right events.

Done.

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
File chrome/browser/sessions/base_session_service.h (right):

https://codereview.chromium.org/694813003/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:58: ScopedVector<SessionCommand>&
pending_commands() {
More was needed - Done.

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser_browsertest.cc:2212: bool
WaitForBackendInitialized(BaseSessionService* base_session_service) {
I think that a busy waiting for the thread initialization should suffice at this
point for this one test...

[sky, 2014-11-03 22:48:03]

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/sessions/...
File chrome/browser/sessions/base_session_service.h (right):

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:80: void
EraseCommand(scoped_ptr<SessionCommand> old_command);
It doesn't make sense for this to take a scoped_ptr since it's not really owned
by the scopedptr, rather its owned by pending_commands_.

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:85: void
SwapCommand(scoped_ptr<SessionCommand> old_command,
old_command shouldn't be scoped here either, new_command, yes.

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/sessions/...
File chrome/browser/sessions/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/sessions/...
chrome/browser/sessions/persistent_tab_restore_service.cc:230: int
entries_written_;
Isn't this the same as base_session_service_->commands_since_reset()?

[Mr4D (OOO till 08-26), 2014-11-03 23:04:53]

Please have another look!

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/sessions/...
File chrome/browser/sessions/base_session_service.h (right):

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:80: void
EraseCommand(scoped_ptr<SessionCommand> old_command);
I had it this way first but then thought that since the command (see below)
needed to be deleted explicitly I pass the ownership. But I am fine this way as
well since I wasn't exactly happy about it either. Done.

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service.h:85: void
SwapCommand(scoped_ptr<SessionCommand> old_command,
Done.

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/sessions/...
File chrome/browser/sessions/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/sessions/...
chrome/browser/sessions/persistent_tab_restore_service.cc:230: int
entries_written_;
As commented already in your earlier question (see patchset 2):

"No. It's not. entries_to_write_ specifies the amount of windows / tabs being
written. To write such a structure multiple commands are required. Since
base_session_service_->commands_since_reset() counts the number of single
commands, it will be much higher."

[sky, 2014-11-04 00:02:40]

LGTM

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/sessions/...
File chrome/browser/sessions/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/694813003/diff/60001/chrome/browser/sessions/...
chrome/browser/sessions/persistent_tab_restore_service.cc:230: int
entries_written_;
On 2014/11/03 23:04:53, Mr4D wrote:
> As commented already in your earlier question (see patchset 2):
> 
> "No. It's not. entries_to_write_ specifies the amount of windows / tabs being
> written. To write such a structure multiple commands are required. Since
> base_session_service_->commands_since_reset() counts the number of single
> commands, it will be much higher."

Sorry, I missed that.

[Mr4D (OOO till 08-26), 2014-11-04 00:05:11]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-11-04 00:06:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/694813003/80002

[Mr4D (OOO till 08-26), 2014-11-04 00:06:15]

Can happen! Thanks for reviewing!

[commit-bot: I haz the power, 2014-11-04 14:48:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-04 14:48:28]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Mr4D (OOO till 08-26), 2014-11-04 15:05:43]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-11-04 15:06:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/694813003/110001

[commit-bot: I haz the power, 2014-11-04 16:07:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-04 16:07:32]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mr4D (OOO till 08-26), 2014-11-04 18:11:31]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-11-04 18:11:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/694813003/130001

[commit-bot: I haz the power, 2014-11-04 19:31:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-04 19:31:56]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mr4D (OOO till 08-26), 2014-11-04 21:51:51]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-11-04 21:52:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/694813003/130001

[commit-bot: I haz the power, 2014-11-04 23:13:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-04 23:13:56]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mr4D (OOO till 08-26), 2014-11-05 00:47:22]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-11-05 00:49:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/694813003/130001

[commit-bot: I haz the power, 2014-11-05 02:01:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-05 02:01:34]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mr4D (OOO till 08-26), 2014-11-05 14:57:35]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-11-05 14:58:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/694813003/130001

[commit-bot: I haz the power, 2014-11-05 15:27:03]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2014-11-05 15:27:49]

Patchset 8 (id:??) landed as
https://crrev.com/d1e250bc34f3e17256bcbe9a5a394a641a9eb804
Cr-Commit-Position: refs/heads/master@{#302803}