Refactoring of SessionService to get componentized.

Refactoring of SessionService into a component part 3.

This CL breaks out the command handling (command creation and session information restore) from |SessionService| into |SessionServiceCommands|, which does have no dependencies on Browser or Content.

Furthermore it breaks out the command handling from |BaseSessionService| into |BaseSessionServiceCommands|. This way the |PersistentTabRestoreService| and the |SessionService| can keep using the base command set which is used by both users.

BUG=424679
TEST=covered by existing tests

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

[Mr4D (OOO till 08-26), 2014-10-23 01:03:41]

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

[Mr4D (OOO till 08-26), 2014-10-23 01:03:42]

As discussed this morning: This is not ready yet.

The idea:

Pull the browser/content independent command creates and from load re-parse
functions out of SessionService since they are not dependent on Browser /
content / ...

So in its current setup the SessionServiceCommand creates its BaseSessionService
and also takes over some general functionalities which are making use of the
underlying modules.

However I am not really happy with it the way it is now, since 
SessionServiceCommands has public BaseSessionService and 
SessionService has public SessionServiceCommand.

There are many ways around it:
a. Have the SessionServiceCommand create explicitly a BaseSessionService so that
the SessionService does not get access to it.
   The required functions could be passed through then (like ScheduleCommand()).
   This way we would have a clear separation from SessionService towards the
levels below.

b. Remove the BaseSessionService entirely from SessionServiceCommand.
   Move then the commands defined in BaseSessionService into the
SessionServiceCommand
   (requires more refactoring of course).
   This way the SessionServiceCommand class has absolutely no dependencies on
anything else, but some portions would need
   to be re-created in Athena.

But when using b. several things would need to move back into SessionService and
therefore be re-created by Athena.

Again this is work in progress - any thoughts are welcome!

[and yes, I still have to puzzle out the WindowType <-> BrowserType problem, so
please ignore!]

[sky, 2014-10-23 13:20:53]

The session service code has the following distinct parts:
1. Queues up commands, pushing them to the backend when necessary.
2. Managing the backend.
3. Generating commands from appropriate actions.

BaseSessionService currently does 1,2 and parts of 3. I think BaseSessionService
should only do 1 & 2. 3 should be moved into its own class (SessionService) that
has a BaseSessionService. That is important, SessionService shouldn't be a BSS,
it should have a BSS. This should make the code more flexible and allow you to
easily promote BSS and the backend. Common command generation, as it doesn't
depend upon any state of BSS or SS could be moved into standalone functions and
promoted as well.
WDYT?

I added marja (another sessions owner) to the cc list in case she has any
thoughts.

[Mr4D (OOO till 08-26), 2014-10-23 16:07:30]

That was roughly what I have meant with option b. Okay - as I mentioned in my
chat with you: That was the "clean" option.

One problem here is that 4 commands - which are shared between
Persistent_tab_restore_service and Session_service have different ID's.
(e.g. SessionService::kCommandUpdateTabNavigation = 6,
PersistentTabRestoreService::kCommandUpdateTabNavigation = 1).

These 4 commands are only a tiny subset of commands (4 out of 20 for
SessionService and 4 out of 8 of for Persistent).
As such it does not justify code awkwardness. => I would suggest to duplicate
these lines unless you disagree. 

Alternatively I could pass a translation array for commands, create restore
commands for all 20 commands and expose all of them - but that seems like
overkill considering that most of them are only reading one variable anyways.

[sky, 2014-10-23 17:11:36]

On 2014/10/23 16:07:30, Mr4D wrote:
> That was roughly what I have meant with option b. Okay - as I mentioned in my
> chat with you: That was the "clean" option.
> 
> One problem here is that 4 commands - which are shared between
> Persistent_tab_restore_service and Session_service have different ID's.
> (e.g. SessionService::kCommandUpdateTabNavigation = 6,
> PersistentTabRestoreService::kCommandUpdateTabNavigation = 1).
> 
> These 4 commands are only a tiny subset of commands (4 out of 20 for
> SessionService and 4 out of 8 of for Persistent).
> As such it does not justify code awkwardness. => I would suggest to duplicate
> these lines unless you disagree. 

I'm not sure I see where the awkwardness comes in. What I was thinking is that
for any commands we want to share the encoding/decoding exists as standalone
functions (in components). Said functions could take the id, this way the same
command need not have the same id. The interpretation of the id is left up to
the consumer (and not BaseSessionService).

  -Scott
> 
> Alternatively I could pass a translation array for commands, create restore
> commands for all 20 commands and expose all of them - but that seems like
> overkill considering that most of them are only reading one variable anyways.

[Mr4D (OOO till 08-26), 2014-10-23 20:09:11]

I have found a way to make us both happy :).

The patch is now ready for review!

https://codereview.chromium.org/672083002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/672083002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/sessions/sessions_api.cc:288: case
SessionWindow::TYPE_TABBED:  // TODO(skuhne) Check Browser::TYPE_TABBED
I did go over all uses of WindowSession::type (which was properly handled as
BrowserType) and all commands which were storing WindowType.

Everything looks now in order and we should use for commands as well as for the
SessionWindow only the WindowType - thus decoupling from Browser. Remaining is
the Sync data which should also get changed towards WindowType.

https://codereview.chromium.org/672083002/diff/1/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_types.cc (left):

https://codereview.chromium.org/672083002/diff/1/chrome/browser/sessions/sess...
chrome/browser/sessions/session_types.cc:77: sync_data.set_browser_type(
This is still storing BrowserType and should get changed as well - but that can
happen later.

https://codereview.chromium.org/672083002/diff/1/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_types.h (left):

https://codereview.chromium.org/672083002/diff/1/chrome/browser/sessions/sess...
chrome/browser/sessions/session_types.h:134: // This would be Browser::Type, but
that would cause a circular dependency.
Converted this into WindowType - decoupling from Browser specific code and
opening it for other types (required by athena). At the same time we are getting
rid of the untyped variable which cost me some grief to fiddle out.

[sky, 2014-10-23 21:51:50]

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

https://codereview.chromium.org/672083002/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service_commands.h:20: class
BaseSessionServiceCommands {
None of these functions need any state. I think they are better served as
standalone functions.

https://codereview.chromium.org/672083002/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service_commands.h:26: SessionCommand*
CreateUpdateTabNavigationCommand(
These should all return scoped_ptr to make ownership clear.

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

https://codereview.chromium.org/672083002/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/session_service.h:58: public SessionServiceCommands,
The style guide says, "Composition is often more appropriate than inheritance.
When using inheritance, make it public." This is a case where you don't need
inheritance, composition works and is cleaner.

As I outlined last time around I think we should have the following classes:
. Remove all command creation from BaseSessionService (such as
RestoreSetWindowAppNameCommand and CreateSetTabExtensionAppIDCommand) and make
them standalone functions.
. SessionService is changed to *have* a BaseSessionService, not be a
SessionService. No doubt this will require some changes to BaseSessionService.

[Mr4D (OOO till 08-26), 2014-10-27 22:26:35]

Took me some time to address - but here you go!

Please have another look!

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

https://codereview.chromium.org/672083002/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service_commands.h:20: class
BaseSessionServiceCommands {
On 2014/10/23 21:51:49, sky wrote:
> None of these functions need any state. I think they are better served as
> standalone functions.

Done.

https://codereview.chromium.org/672083002/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service_commands.h:26: SessionCommand*
CreateUpdateTabNavigationCommand(
Wow. This was close to a day of work - but it's done.

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

https://codereview.chromium.org/672083002/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/session_service.h:58: public SessionServiceCommands,
|BaseSessionService| and |SessionServiceCommands| are now owned and not
inherited anymore. Done.

Extracting the commands had already be done in this CL as it is though.

[sky, 2014-10-27 23:49:35]

Ugh, this is too gargantuan to review. Can you split this out into multiple
patches. Start with one that does the moving of functions to create the commands
(and the scoped_ptrs).

https://codereview.chromium.org/672083002/diff/80001/chrome/browser/sessions/...
File chrome/browser/sessions/base_session_service_commands.h (right):

https://codereview.chromium.org/672083002/diff/80001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service_commands.h:21: namespace
BaseSessionServiceCommands {
no namespace here.

[Mr4D (OOO till 08-26), 2014-10-28 21:45:38]

As discussed - here is now a version #2 + addressing your basic concerns.

I will address the scoped_ptr & composition request in separate CL's.

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

https://codereview.chromium.org/672083002/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service_commands.h:20: class
BaseSessionServiceCommands {
Addressed also in #6.

https://codereview.chromium.org/672083002/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service_commands.h:26: SessionCommand*
CreateUpdateTabNavigationCommand(
Will create separate patch for this.

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

https://codereview.chromium.org/672083002/diff/20001/chrome/browser/sessions/...
chrome/browser/sessions/session_service.h:58: public SessionServiceCommands,
Will create separate patch for this.

https://codereview.chromium.org/672083002/diff/80001/chrome/browser/sessions/...
File chrome/browser/sessions/base_session_service_commands.h (right):

https://codereview.chromium.org/672083002/diff/80001/chrome/browser/sessions/...
chrome/browser/sessions/base_session_service_commands.h:21: namespace
BaseSessionServiceCommands {
On 2014/10/27 23:49:34, sky wrote:
> no namespace here.

Done.

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

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service.h:203: // SessionServiceCommands:
Note: This function is very unfortunate and will be a local function only in the
end (see patch set #5). In the meantime however it remains here.

[sky, 2014-10-28 22:52:38]

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
File chrome/browser/sessions/base_session_service_commands.h (right):

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/base_session_service_commands.h:20: 
Add a TODO to convert to returning scoped_ptr and document ownership for now.

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

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/persistent_tab_restore_service.cc:417:
ScheduleCommand(::CreateSetWindowAppNameCommand(kCommandSetWindowAppName,
Why do you need the :: in all of these?

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

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:116: default:
Remove the default so that we get a compile error if a new type is added.

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:128: default:
Same comment here about removing default.

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

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service.h:73: static SessionWindow::WindowType
WindowTypeForBrowserType(Browser::Type type);
Move these to session_types, or create a new file if you don't want them there.

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
File chrome/browser/sessions/session_service_commands.h (right):

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service_commands.h:32: enum AppType {
Move to session_types.

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service_commands.h:63: SessionCommand*
CreateSetTabWindowCommand(const SessionID& window_id,
None of these functions have any state. Move them into a standalone file (just
like you did for base_session_service_commands). Again, my rationale here is you
shouldn't need to subclass any of this, instead provide a delegate that lets you
customize what you need. That said, I think you pass in the set of WindowTypes
that are allowed and forgo a delegate entirely.

[Mr4D (OOO till 08-26), 2014-10-29 02:04:13]

Addressed! Please have another look!

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
File chrome/browser/sessions/base_session_service_commands.h (right):

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/base_session_service_commands.h:20: 
On 2014/10/28 22:52:38, sky wrote:
> Add a TODO to convert to returning scoped_ptr and document ownership for now.

Done.

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

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/persistent_tab_restore_service.cc:417:
ScheduleCommand(::CreateSetWindowAppNameCommand(kCommandSetWindowAppName,
Actually - I do not need them here. Removed.

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

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:116: default:
On 2014/10/28 22:52:38, sky wrote:
> Remove the default so that we get a compile error if a new type is added.

Done.

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:128: default:
Actually that's not want we want. This is a specific implementation for Browser.
Athena will add more WindowTypes - but the Browser will only use 2 of them.

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

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service.h:73: static SessionWindow::WindowType
WindowTypeForBrowserType(Browser::Type type);
We want to break the dependency to browser - so it cannot be in session_types.
Will create a separate file then for it.

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
File chrome/browser/sessions/session_service_commands.h (right):

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service_commands.h:32: enum AppType {
It is not stored by any structure in session_types. It is only used by
"ShouldRestoreWindowOfType" and that will move (see patchset above) into a
delegate. It should move therefore there. I added a comment accordingly.

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service_commands.h:63: SessionCommand*
CreateSetTabWindowCommand(const SessionID& window_id,
Understood. But please see the previous patch set. This command will require a
delegate which it needs to store. Can I leave this until I have landed that
patch?

[sky, 2014-10-29 15:44:21]

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

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:128: default:
On 2014/10/29 02:04:13, Mr4D wrote:
> Actually that's not want we want. This is a specific implementation for
Browser.
> Athena will add more WindowTypes - but the Browser will only use 2 of them.

That's fine, but you should still have explicit case statements so that we catch
newly added ones.

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
File chrome/browser/sessions/session_service_commands.h (right):

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service_commands.h:63: SessionCommand*
CreateSetTabWindowCommand(const SessionID& window_id,
On 2014/10/29 02:04:13, Mr4D wrote:
> Understood. But please see the previous patch set. This command will require a
> delegate which it needs to store. Can I leave this until I have landed that
> patch?

I don't understand this, can you elaborate?

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

https://codereview.chromium.org/672083002/diff/120001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:110: bool
SessionService::ShouldNewWindowStartSession() {
You're reordering of this file is making it hard for me to see what has actually
changed. I'm totally fine with reordering, but please do it separately. You can
do that first, in a separate patch, if you prefer.

[Mr4D (OOO till 08-26), 2014-10-29 19:35:30]

Done. Please have another look.

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

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:128: default:
On 2014/10/29 15:44:20, sky wrote:
> On 2014/10/29 02:04:13, Mr4D wrote:
> > Actually that's not want we want. This is a specific implementation for
> Browser.
> > Athena will add more WindowTypes - but the Browser will only use 2 of them.
> 
> That's fine, but you should still have explicit case statements so that we
catch
> newly added ones.

Done.

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
File chrome/browser/sessions/session_service_commands.h (right):

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service_commands.h:63: SessionCommand*
CreateSetTabWindowCommand(const SessionID& window_id,
As discussed offline. Changed.

It is good to see that this dependency is gone, but it got bought by additional
overhead and a namespace pollution which I admit to like less.

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

https://codereview.chromium.org/672083002/diff/120001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:110: bool
SessionService::ShouldNewWindowStartSession() {
On 2014/10/29 15:44:21, sky wrote:
> You're reordering of this file is making it hard for me to see what has
actually
> changed. I'm totally fine with reordering, but please do it separately. You
can
> do that first, in a separate patch, if you prefer.

Done.

[sky, 2014-10-29 20:49:15]

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
File chrome/browser/sessions/session_service_commands.h (right):

https://codereview.chromium.org/672083002/diff/100001/chrome/browser/sessions...
chrome/browser/sessions/session_service_commands.h:63: SessionCommand*
CreateSetTabWindowCommand(const SessionID& window_id,
On 2014/10/29 19:35:30, Mr4D wrote:
> As discussed offline. Changed.
> 
> It is good to see that this dependency is gone, but it got bought by
additional
> overhead and a namespace pollution which I admit to like less.

In theory these headers should only be included by implementations, right? Also,
I suspect some of these are going to end up in the sessions namespace.

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

https://codereview.chromium.org/672083002/diff/140001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:284: if
(!ShouldRestoreWindowOfType(window_type, app_type))
Where is ShouldRestoreWindowOfType implemented?

https://codereview.chromium.org/672083002/diff/140001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:948: std::string
name("SessionRestore.SavePeriod");
Why are you changing histogram names? This effects all existing histograms.
Please move to a separate patch.

https://codereview.chromium.org/672083002/diff/140001/chrome/browser/sessions...
File chrome/browser/sessions/session_service_test_helper.cc (right):

https://codereview.chromium.org/672083002/diff/140001/chrome/browser/sessions...
chrome/browser/sessions/session_service_test_helper.cc:65:
RemoveUnusedRestoreWindows(windows);
Where is this implemented?

https://codereview.chromium.org/672083002/diff/140001/chrome/browser/sessions...
File chrome/browser/sessions/session_types.h (right):

https://codereview.chromium.org/672083002/diff/140001/chrome/browser/sessions...
chrome/browser/sessions/session_types.h:112: // Possible window types which can
be stored here.
Document that these values are written to disk.

[Mr4D (OOO till 08-26), 2014-10-29 22:22:18]

Please have another look!

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

https://codereview.chromium.org/672083002/diff/140001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:284: if
(!ShouldRestoreWindowOfType(window_type, app_type))
In SessionServiceUtils.

Note: SessionServiceUtils = functions which are required for SessionService,
SessionRestore and associated unit/browser tests. But everything is in relation
to Chrome / Browser and will (after componentization) remain in Chrome.

https://codereview.chromium.org/672083002/diff/140001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:948: std::string
name("SessionRestore.SavePeriod");
I did not. If you look closely you will see that the two functions have swapped
place to match header file. Changed back and left for later.

https://codereview.chromium.org/672083002/diff/140001/chrome/browser/sessions...
File chrome/browser/sessions/session_service_test_helper.cc (right):

https://codereview.chromium.org/672083002/diff/140001/chrome/browser/sessions...
chrome/browser/sessions/session_service_test_helper.cc:65:
RemoveUnusedRestoreWindows(windows);
In SessionServiceUtils.

Note: SessionServiceUtils = functions which are required for SessionService,
SessionRestore and associated unit/browser tests. But everything is in relation
to Chrome / Browser and will (after componentization) remain in Chrome.

https://codereview.chromium.org/672083002/diff/140001/chrome/browser/sessions...
File chrome/browser/sessions/session_types.h (right):

https://codereview.chromium.org/672083002/diff/140001/chrome/browser/sessions...
chrome/browser/sessions/session_types.h:112: // Possible window types which can
be stored here.
Done. 

Note: I was under the assumption that that would not need to be commented since
this class has nothing (direct) with the session commands to do.

[sky, 2014-10-29 23:00:37]

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

https://codereview.chromium.org/672083002/diff/140001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:284: if
(!ShouldRestoreWindowOfType(window_type, app_type))
On 2014/10/29 22:22:17, Mr4D wrote:
> In SessionServiceUtils.
> 
> Note: SessionServiceUtils = functions which are required for SessionService,
> SessionRestore and associated unit/browser tests. But everything is in
relation
> to Chrome / Browser and will (after componentization) remain in Chrome.

Would that be session_service_utils that you forgot to git add? ;)

[Mr4D (OOO till 08-26), 2014-10-29 23:06:33]

Doh! Added.

[sky, 2014-10-29 23:14:51]

https://codereview.chromium.org/672083002/diff/180001/chrome/browser/sessions...
File chrome/browser/sessions/session_service_utils.cc (right):

https://codereview.chromium.org/672083002/diff/180001/chrome/browser/sessions...
chrome/browser/sessions/session_service_utils.cc:7: bool
ShouldRestoreWindowOfType(SessionWindow::WindowType window_type,
AFAICT the ShouldRestore/RemoveUnusedRestoreWindows is only used in
SessionService. Is there a reason it needs to be here? If it's needed for tests
how about private static functions in SessionService that are friended (I'm
pretty sure the test helper is a friend already).

https://codereview.chromium.org/672083002/diff/180001/chrome/browser/sessions...
chrome/browser/sessions/session_service_utils.cc:25: window_list->erase(i);
This leaks.

[Mr4D (OOO till 08-26), 2014-10-29 23:47:44]

Please have another look

https://codereview.chromium.org/672083002/diff/180001/chrome/browser/sessions...
File chrome/browser/sessions/session_service_utils.cc (right):

https://codereview.chromium.org/672083002/diff/180001/chrome/browser/sessions...
chrome/browser/sessions/session_service_utils.cc:7: bool
ShouldRestoreWindowOfType(SessionWindow::WindowType window_type,
On 2014/10/29 23:14:51, sky wrote:
> AFAICT the ShouldRestore/RemoveUnusedRestoreWindows is only used in
> SessionService. Is there a reason it needs to be here? If it's needed for
tests
> how about private static functions in SessionService that are friended (I'm
> pretty sure the test helper is a friend already).

Done.

https://codereview.chromium.org/672083002/diff/180001/chrome/browser/sessions...
chrome/browser/sessions/session_service_utils.cc:25: window_list->erase(i);
On 2014/10/29 23:14:51, sky wrote:
> This leaks.

Done.

[sky, 2014-10-29 23:58:03]

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

https://codereview.chromium.org/672083002/diff/200001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:465: while (i != window_list->end())
{
You never increment i if it should be restored.

https://codereview.chromium.org/672083002/diff/200001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:472: i = window_list->begin();
See line 1013 old for how to do this in an efficient manner.

[Mr4D (OOO till 08-26), 2014-10-30 00:04:25]

Please have another look

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

https://codereview.chromium.org/672083002/diff/200001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:465: while (i != window_list->end())
{
On 2014/10/29 23:58:03, sky wrote:
> You never increment i if it should be restored.

Done.

https://codereview.chromium.org/672083002/diff/200001/chrome/browser/sessions...
chrome/browser/sessions/session_service.cc:472: i = window_list->begin();
On 2014/10/29 23:58:03, sky wrote:
> See line 1013 old for how to do this in an efficient manner.

Done.

[sky, 2014-10-30 03:25:41]

LGTM

[Mr4D (OOO till 08-26), 2014-10-30 04:54:32]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-10-30 04:54:44]

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

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

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-10-30 05:14:20]

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

[commit-bot: I haz the power, 2014-10-30 06:02:54]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2014-10-30 06:03:32]

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