Changes tab restore service to handle restoring closed windows as a...

Changes tab restore service to handle restoring closed windows as a
single unit. Sadly I've written another ui test. Lets hope it isn't
flakey. Glen is going to change the NTP to deal with this appropriately.

BUG=4686
TEST=Try closing a window (with more than one window open), hitting
  control-shift-t, and make sure the window and all it's tabs comes
  back.

[jcampan, 2008-11-25 19:24:46]

Some nits.

http://codereview.chromium.org/11377/diff/1/10
File chrome/browser/tab_restore_service.cc (right):

http://codereview.chromium.org/11377/diff/1/10#newcode56
Line 56: entries_.push_front(new Tab());
Any reason why you are not creating the new tab, populating it and then adding
it to entries_ (instead of adding it first)?

http://codereview.chromium.org/11377/diff/1/10#newcode93
Line 93: closing_browsers_.erase(browser);
Should we DCHECK if it reurns closing_browsers_.end() (meaning BrowserClosed was
called with no BrowserClosing invoked)?

http://codereview.chromium.org/11377/diff/1/10#newcode120
Line 120: i = entries_.end();
Any reason for setting i, I don't see it used after this line?

http://codereview.chromium.org/11377/diff/1/10#newcode130
Line 130: } else {
OK, may be I am paranoid, but what about a DCHECK_EQ(WINDOW, entry->type)?

http://codereview.chromium.org/11377/diff/1/4
File chrome/browser/tab_restore_service.h (right):

http://codereview.chromium.org/11377/diff/1/4#newcode153
Line 153: // Set of tabs that we'vereceived a BrowserClosing method for but no
we'vereceived -> we've received

[sky, 2008-11-25 21:51:36]

New snapshot uploaded.

http://codereview.chromium.org/11377/diff/1/10
File chrome/browser/tab_restore_service.cc (right):

http://codereview.chromium.org/11377/diff/1/10#newcode56
Line 56: entries_.push_front(new Tab());
On 2008/11/25 19:24:46, jcampan wrote:
> Any reason why you are not creating the new tab, populating it and then adding
> it to entries_ (instead of adding it first)?

It comes from how I originally had the code when the vector was by value instead
of pointer. I've changed it to make more sense now that I'm using a pointer.

http://codereview.chromium.org/11377/diff/1/10#newcode93
Line 93: closing_browsers_.erase(browser);
On 2008/11/25 19:24:46, jcampan wrote:
> Should we DCHECK if it reurns closing_browsers_.end() (meaning BrowserClosed
was
> called with no BrowserClosing invoked)?

No harm will come if that happens. In fact closing_browsers_ won't have an entry
for Browser if it isn't a tabbed browser.

http://codereview.chromium.org/11377/diff/1/10#newcode120
Line 120: i = entries_.end();
On 2008/11/25 19:24:46, jcampan wrote:
> Any reason for setting i, I don't see it used after this line?

The call to erase on the previous line made it invalid. Just want to make sure
that if someone used it they would quickly see the problem rather than
potentially random behavior.

http://codereview.chromium.org/11377/diff/1/10#newcode130
Line 130: } else {
On 2008/11/25 19:24:46, jcampan wrote:
> OK, may be I am paranoid, but what about a DCHECK_EQ(WINDOW, entry->type)?

Done.

http://codereview.chromium.org/11377/diff/1/4
File chrome/browser/tab_restore_service.h (right):

http://codereview.chromium.org/11377/diff/1/4#newcode153
Line 153: // Set of tabs that we'vereceived a BrowserClosing method for but no
On 2008/11/25 19:24:46, jcampan wrote:
> we'vereceived -> we've received

Done.

[jcampan, 2008-11-25 22:00:39]

LGTM