Decouple TabRestoreService from Browser

Decouple TabRestoreService from Browser

Introduce a delegate interface to decouple TabRestoreService from
Browser/BrowserList. Also clean up dependency on TabStripModel in the
process.

BUG=
TEST=

[Ben Goodger (Google), 2011-03-10 18:28:35]

A few comments to help you along the way :-)

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
File chrome/browser/sessions/tab_restore_service_delegate.h (right):

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:22: virtual Profile*
profile() const = 0;
Google style: virtual methods should never use unix_hacker since they are not
cheap (i.e. not inline-able)

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:28: const
NavigationController* controller) const = 0;
Code other than tab-restore related code calls this method I think. Because of
this, I don't think Browser should "source" it's having this method from this
interface, because potentially the methods could be overridden in Browser as
protected.

It's probably worth a deeper understanding of what TabRestoreService is actually
doing and changing a lot of these methods to be more TabRestoreService focused.

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:30: virtual TabContents*
GetSelectedTabContents() const = 0;
Same comment as above with most of these methods.

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:40:
SessionStorageNamespace* storage_namespace) = 0;
This seems like a valid method to define on such an interface.

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:46:
SessionStorageNamespace* session_storage_namespace) = 0;
So does this.

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:47: virtual void
CloseTab() = 0;
This... not as much.

http://codereview.chromium.org/6660028/diff/1/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/6660028/diff/1/chrome/browser/ui/browser.h#new...
chrome/browser/ui/browser.h:388: virtual TabContents* AddRestoredTab(
In Chrome code we tend to put all the overrides from a particular base together.
We also remove all the comments, since the method is documented in the interface
or base class.

e.g.

// Overridden from SomeInterface:
virtual void Foo() OVERRIDE;
virtual int GetBar() const OVERRIDE;
virtual void StartBaz(int goat) OVERRIDE;

[sky, 2011-03-10 18:43:24]

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
File chrome/browser/sessions/tab_restore_service_delegate.h (right):

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:21: virtual bool
IsNormalBrowser() const = 0;
Remove this and instead push the code into the call to BrowserClosing.

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:22: virtual Profile*
profile() const = 0;
This shouldn't be necessary as TabRestoreService (actually
SessionRestoreService) has a profile.

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:23: virtual
BrowserWindow* window() const = 0;
If we're trying to minimize the surface area TabRestoreService sees, you should
add a concrete method for what is needed (Show) in this case.

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:27: virtual int
GetIndexOfController(
This shouldn't be needed. I believe the index can be passed in where it is
needed.

[Ben Goodger (Google), 2011-03-10 18:58:45]

My other suggestion would be that given my comment about how I didn't want
Browser to obtain its profile getter from this interface, and it'd be very ugly
to have TabRestoreDelegate specific method names like

GetProfileForTabRestoreDelegate

it might be a better idea to have a helper object that Browser owns that
implements this interface.

e.g.

class BrowserTabRestoreDelegate : public TabRestoreDelegate {
public:
  BrowserTabRestoreDelegate(Browser* browser)

...

This desktop-chrome specific object could be constructed with a Browser as
described above. It'd then be able to call methods on Browser to fulfill its
TabRestoreDelegate duties.

-Ben

[torne_google.com, 2011-03-11 11:51:49]

On 2011/03/10 18:58:45, Ben Goodger wrote:
> My other suggestion would be that given my comment about how I didn't want
> Browser to obtain its profile getter from this interface, and it'd be very
ugly
> to have TabRestoreDelegate specific method names like
> 
> GetProfileForTabRestoreDelegate
> 
> it might be a better idea to have a helper object that Browser owns that
> implements this interface.

This does seem like a much better way to do it, and will also solve most of the
issues in the comments; I'll do that. :)

T.

[torne_google.com, 2011-03-11 15:07:12]

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
File chrome/browser/sessions/tab_restore_service_delegate.h (right):

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:21: virtual bool
IsNormalBrowser() const = 0;
On 2011/03/10 18:43:24, sky wrote:
> Remove this and instead push the code into the call to BrowserClosing.

Done.

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:22: virtual Profile*
profile() const = 0;
On 2011/03/10 18:28:35, Ben Goodger wrote:
> Google style: virtual methods should never use unix_hacker since they are not
> cheap (i.e. not inline-able)

Done.

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:23: virtual
BrowserWindow* window() const = 0;
On 2011/03/10 18:43:24, sky wrote:
> If we're trying to minimize the surface area TabRestoreService sees, you
should
> add a concrete method for what is needed (Show) in this case.

Done.

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:27: virtual int
GetIndexOfController(
On 2011/03/10 18:43:24, sky wrote:
> This shouldn't be needed. I believe the index can be passed in where it is
> needed.

Done.

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:30: virtual TabContents*
GetSelectedTabContents() const = 0;
On 2011/03/10 18:28:35, Ben Goodger wrote:
> Same comment as above with most of these methods.

tab_count, GetTabContentsAt are used to iterate over all the tabs in a closing
browser.

GetSelectedTabContents is used to see if there's a current tab that needs
closing in the case for CloseTab I described below.

Not sure if there's anything to be done about those. I've eliminated
GetIndexOfController as it's more efficient to pass it in in the case where a
whole window is being iterated over and it's easy to pass it in manually for
single tabs.

http://codereview.chromium.org/6660028/diff/1/chrome/browser/sessions/tab_res...
chrome/browser/sessions/tab_restore_service_delegate.h:47: virtual void
CloseTab() = 0;
On 2011/03/10 18:28:35, Ben Goodger wrote:
> This... not as much.

This is used in the case where the current tab is being replaced with an entire
window's worth of tabs: it adds all the new tabs then closes the original one
(unlike when replacing a single tab, where the tab's NavigationController just
gets its state overwritten). I think we have to keep this.

[Ben Goodger (Google), 2011-03-11 15:21:32]

Much better!

I'll leave it to sky to review the substance since he is more familiar with this
stuff, my comments are related to integration/style. LGTM in general from that
perspective, just two nits.

http://codereview.chromium.org/6660028/diff/10001/chrome/browser/sessions/tab...
File chrome/browser/sessions/tab_restore_service_delegate.h (right):

http://codereview.chromium.org/6660028/diff/10001/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service_delegate.h:24: virtual void
ShowBrowserWindow() = 0;
This interface should document its methods or refer to where canonical
documentation lives.

http://codereview.chromium.org/6660028/diff/10001/chrome/browser/ui/browser_t...
File chrome/browser/ui/browser_tab_restore_service_delegate.h (right):

http://codereview.chromium.org/6660028/diff/10001/chrome/browser/ui/browser_t...
chrome/browser/ui/browser_tab_restore_service_delegate.h:53: };
DISALLOW_COPY_AND_ASSIGN

[torne_google.com, 2011-03-11 15:31:23]

Also fixed lint issues.

http://codereview.chromium.org/6660028/diff/10001/chrome/browser/sessions/tab...
File chrome/browser/sessions/tab_restore_service_delegate.h (right):

http://codereview.chromium.org/6660028/diff/10001/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service_delegate.h:24: virtual void
ShowBrowserWindow() = 0;
On 2011/03/11 15:21:32, Ben Goodger wrote:
> This interface should document its methods or refer to where canonical
> documentation lives.

Done.

http://codereview.chromium.org/6660028/diff/10001/chrome/browser/ui/browser_t...
File chrome/browser/ui/browser_tab_restore_service_delegate.h (right):

http://codereview.chromium.org/6660028/diff/10001/chrome/browser/ui/browser_t...
chrome/browser/ui/browser_tab_restore_service_delegate.h:53: };
On 2011/03/11 15:21:32, Ben Goodger wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[sky, 2011-03-11 17:40:37]

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
File chrome/browser/sessions/tab_restore_service.cc (right):

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service.cc:15: #include
"chrome/browser/browser_window.h"
Is this include still needed?

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service.cc:219: TabRestoreServiceDelegate*
browser =
browser -> delegate

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service.cc:340: TabRestoreServiceDelegate*
current_browser = browser;
current_delegate

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service.cc:879: TabRestoreServiceDelegate*
TabRestoreService::RestoreTab(const Tab& tab,
Put tab on the next line

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
File chrome/browser/sessions/tab_restore_service.h (right):

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service.h:135: void
BrowserClosing(TabRestoreServiceDelegate* browser);
rename argument to delegate (comment applies to all methods)

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service.h:324: // Set of tabs that we've
received a BrowserClosing method for but no
tabs-> delegates. Field should be renamed to delegates_closing_.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
File chrome/browser/sessions/tab_restore_service_delegate.h (right):

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service_delegate.h:58: 
Nuke one of these lines.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service_delegate.h:59: virtual
~TabRestoreServiceDelegate() {}
destructor should be protected.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service_delegate.h:62: static
TabRestoreServiceDelegate* CreateBrowser(Profile* profile);
CreateBrowser -> Create

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service_delegate.h:66: const
NavigationController* controller, int* index);
wrap int* onto its own line. Also, name this FindDelegateForController.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service_delegate.h:69: static
TabRestoreServiceDelegate* FindBrowserWithID(
FindDelegateWithID

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:31: #include
"chrome/browser/ui/browser_tab_restore_service_delegate.h"
Forward declare and use a scoped_ptr.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/ui/browser_t...
File chrome/browser/ui/browser_tab_restore_service_delegate.h (right):

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/ui/browser_t...
chrome/browser/ui/browser_tab_restore_service_delegate.h:30: virtual void
ShowBrowserWindow();
Use OVERRIDE on all these.

[torne_google.com, 2011-03-14 15:07:24]

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
File chrome/browser/sessions/tab_restore_service.cc (right):

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service.cc:15: #include
"chrome/browser/browser_window.h"
On 2011/03/11 17:40:37, sky wrote:
> Is this include still needed?

Done.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service.cc:219: TabRestoreServiceDelegate*
browser =
On 2011/03/11 17:40:37, sky wrote:
> browser -> delegate

Done.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service.cc:340: TabRestoreServiceDelegate*
current_browser = browser;
On 2011/03/11 17:40:37, sky wrote:
> current_delegate

Done.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service.cc:879: TabRestoreServiceDelegate*
TabRestoreService::RestoreTab(const Tab& tab,
On 2011/03/11 17:40:37, sky wrote:
> Put tab on the next line

Done.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
File chrome/browser/sessions/tab_restore_service.h (right):

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service.h:135: void
BrowserClosing(TabRestoreServiceDelegate* browser);
On 2011/03/11 17:40:37, sky wrote:
> rename argument to delegate (comment applies to all methods)

Done.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service.h:324: // Set of tabs that we've
received a BrowserClosing method for but no
On 2011/03/11 17:40:37, sky wrote:
> tabs-> delegates. Field should be renamed to delegates_closing_.

Done.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
File chrome/browser/sessions/tab_restore_service_delegate.h (right):

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service_delegate.h:58: 
On 2011/03/11 17:40:37, sky wrote:
> Nuke one of these lines.

Done.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service_delegate.h:59: virtual
~TabRestoreServiceDelegate() {}
On 2011/03/11 17:40:37, sky wrote:
> destructor should be protected.

Why? Browser's scoped_ptr can't destroy the object if it's not public...

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service_delegate.h:62: static
TabRestoreServiceDelegate* CreateBrowser(Profile* profile);
On 2011/03/11 17:40:37, sky wrote:
> CreateBrowser -> Create

Done.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service_delegate.h:66: const
NavigationController* controller, int* index);
On 2011/03/11 17:40:37, sky wrote:
> wrap int* onto its own line. Also, name this FindDelegateForController.

Done.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service_delegate.h:69: static
TabRestoreServiceDelegate* FindBrowserWithID(
On 2011/03/11 17:40:37, sky wrote:
> FindDelegateWithID

Done.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:31: #include
"chrome/browser/ui/browser_tab_restore_service_delegate.h"
On 2011/03/11 17:40:37, sky wrote:
> Forward declare and use a scoped_ptr.

Done.

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/ui/browser_t...
File chrome/browser/ui/browser_tab_restore_service_delegate.h (right):

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/ui/browser_t...
chrome/browser/ui/browser_tab_restore_service_delegate.h:30: virtual void
ShowBrowserWindow();
On 2011/03/11 17:40:37, sky wrote:
> Use OVERRIDE on all these.

Done.

[sky, 2011-03-14 15:26:21]

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
File chrome/browser/sessions/tab_restore_service_delegate.h (right):

http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
chrome/browser/sessions/tab_restore_service_delegate.h:59: virtual
~TabRestoreServiceDelegate() {}
On 2011/03/14 15:07:25, Torne (Richard Coles) wrote:
> On 2011/03/11 17:40:37, sky wrote:
> > destructor should be protected.
> 
> Why? Browser's scoped_ptr can't destroy the object if it's not public...

To make it clear TabRestoreService doesn't own the object.
BrowserTabRestoreServiceDelegates destructor should be public and that is what
Browser should own.

[torne_google.com, 2011-03-14 16:30:49]

On 2011/03/14 15:26:21, sky wrote:
>
http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
> File chrome/browser/sessions/tab_restore_service_delegate.h (right):
> 
>
http://codereview.chromium.org/6660028/diff/13005/chrome/browser/sessions/tab...
> chrome/browser/sessions/tab_restore_service_delegate.h:59: virtual
> ~TabRestoreServiceDelegate() {}
> On 2011/03/14 15:07:25, Torne (Richard Coles) wrote:
> > On 2011/03/11 17:40:37, sky wrote:
> > > destructor should be protected.
> > 
> > Why? Browser's scoped_ptr can't destroy the object if it's not public...
> 
> To make it clear TabRestoreService doesn't own the object.
> BrowserTabRestoreServiceDelegates destructor should be public and that is what
> Browser should own.

OK, that's done and some lint comments tidied also.

[sky, 2011-03-14 16:37:17]

LGTM

[torne_google.com, 2011-03-14 18:24:16]

On 2011/03/14 16:37:17, sky wrote:
> LGTM

Okay, one more small change in patchset 6: I changed the new tab UI to also use
TabRestoreServiceDelegate to get the delegate argument for restoring a tab
instead of using Browser for it. Is this sane?

[sky, 2011-03-14 18:31:24]

SLGTM

http://codereview.chromium.org/6660028/diff/13019/chrome/browser/ui/webui/new...
File chrome/browser/ui/webui/new_tab_ui.cc (right):

http://codereview.chromium.org/6660028/diff/13019/chrome/browser/ui/webui/new...
chrome/browser/ui/webui/new_tab_ui.cc:29: #include
"chrome/browser/ui/browser_tab_restore_service_delegate.h"
You shouldn't need this include, right?

[torne_google.com, 2011-03-15 15:53:10]

OK, Marcus has committed this from http://codereview.chromium.org/6677042/ 

Thanks ;)