Makes pinned tab restore on startup if you haven't enabled session

Makes pinned tab restore on startup if you haven't enabled session
restore.

BUG=23613
TEST=turn off session restore, pin a tab, restart chrome and make sure
you get back the pinned tab.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41958

[Paweł Hajdan Jr., 2010-03-17 18:10:25]

Drive-by with some test comments.

http://codereview.chromium.org/1026005/diff/2001/3003
File chrome/browser/browser_browsertest.cc (right):

http://codereview.chromium.org/1026005/diff/2001/3003#newcode468
chrome/browser/browser_browsertest.cc:468: ASSERT_TRUE(server);
nit: Could you move this ASSERT above the comment? That would make it more
clear.

http://codereview.chromium.org/1026005/diff/2001/3014
File chrome/browser/tabs/pinned_tab_codec_unittest.cc (right):

http://codereview.chromium.org/1026005/diff/2001/3014#newcode42
chrome/browser/tabs/pinned_tab_codec_unittest.cc:42: EXPECT_TRUE("" == result);
Could you use EXPECT_EQ so we have more data in case of failures?

http://codereview.chromium.org/1026005/diff/2001/3014#newcode60
chrome/browser/tabs/pinned_tab_codec_unittest.cc:60: EXPECT_TRUE(result ==
"http://www.google.com/::pinned:");
Same here.

[sky, 2010-03-17 18:47:27]

All addressed. I had tried EXPECT_EQ earlier and got a compile error. I must
have done something else wrong. None-the-less both use EXPECT_EQ now.

[Paweł Hajdan Jr., 2010-03-17 18:53:28]

The bits I commented about in the drive-by now LGTM.

[Erik does not do reviews, 2010-03-17 22:57:59]

http://codereview.chromium.org/1026005/diff/11001/12001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/1026005/diff/11001/12001#newcode660
chrome/browser/browser.cc:660: tab_restore_service->BrowserClosing(this);
it seems like this should just use your new notification below

http://codereview.chromium.org/1026005/diff/11001/12001#newcode728
chrome/browser/browser.cc:728: // By default, content believes it is not hidden.
 When adding contents
newline above comment

http://codereview.chromium.org/1026005/diff/11001/12003
File chrome/browser/browser_browsertest.cc (right):

http://codereview.chromium.org/1026005/diff/11001/12003#newcode464
chrome/browser/browser_browsertest.cc:464: IN_PROC_BROWSER_TEST_F(BrowserTest,
RestorePinnedTabs) {
it seems like you should mix in a non-pinned tab in the middle somewhere here as
well

http://codereview.chromium.org/1026005/diff/11001/12003#newcode518
chrome/browser/browser_browsertest.cc:518:
EXPECT_TRUE(new_model->IsTabPinned(0));
second index is wrong

http://codereview.chromium.org/1026005/diff/11001/12005
File chrome/browser/browser_init.h (right):

http://codereview.chromium.org/1026005/diff/11001/12005#newcode106
chrome/browser/browser_init.h:106: // Convenience for OpenTabsInBrowser that
converts |urls| into a set of
This feels pretty weird to me.  The entries aren't pinned tabs in this case.  I
think I understand how you got here, but it winds up being harder to understand
than necessary.  I don't have a great alternative suggestion.

http://codereview.chromium.org/1026005/diff/11001/12012
File chrome/browser/tabs/pinned_tab_codec.cc (right):

http://codereview.chromium.org/1026005/diff/11001/12012#newcode49
chrome/browser/tabs/pinned_tab_codec.cc:49:
UTF8ToUTF16(extension->app_launch_url().spec()));
I wonder if we should use the URL it's currently on like we do for other tabs. 
For example, if I had navigated to a different folder in my gmail.  Maybe we
should remember that.  I can see it both ways, but I'm just curious what you
think.

http://codereview.chromium.org/1026005/diff/11001/12012#newcode54
chrome/browser/tabs/pinned_tab_codec.cc:54: value->SetStringFromUTF16(kURL,
UTF8ToUTF16(active_entry->url().spec()));
these two blocks are pretty similar.  Can you pull out the URL first and then
use common code?

http://codereview.chromium.org/1026005/diff/11001/12012#newcode78
chrome/browser/tabs/pinned_tab_codec.cc:78: if (!value.GetStringAsUTF16(kURL,
&url_string))
Why go through these utf16 paths?  Value uses utf8 if you use std::string (in
fact, it converts everything to utf8 internally no matter what you pass it)

http://codereview.chromium.org/1026005/diff/11001/12014
File chrome/browser/tabs/pinned_tab_codec_unittest.cc (right):

http://codereview.chromium.org/1026005/diff/11001/12014#newcode51
chrome/browser/tabs/pinned_tab_codec_unittest.cc:51: // AddTab inserts at index
0, so order after this is url1, url2.
newline above comment

http://codereview.chromium.org/1026005/diff/11001/12015
File chrome/browser/tabs/pinned_tab_service.cc (right):

http://codereview.chromium.org/1026005/diff/11001/12015#newcode35
chrome/browser/tabs/pinned_tab_service.cc:35: if
(closing_browsers_.count(browser) == 0) {
A comment explaining this logic would help.  It looks like you only want to
write one browser at a time, but I'm not sure why you'd need to worry about this
if the bool in the notification is true.  In that case, you're exiting, right? 
So this code should only ever be called once.  Given this, maybe this should
just set got_exiting_ and then you don't need closing_browsers_ at all?

I'm probably missing something here.  So the comment would have helped me. :-)

http://codereview.chromium.org/1026005/diff/11001/12016
File chrome/browser/tabs/pinned_tab_service.h (right):

http://codereview.chromium.org/1026005/diff/11001/12016#newcode16
chrome/browser/tabs/pinned_tab_service.h:16: class PinnedTabService : public
NotificationObserver {
comment for class

http://codereview.chromium.org/1026005/diff/11001/12016#newcode34
chrome/browser/tabs/pinned_tab_service.h:34: ClosingBrowsers closing_browsers_;
comments for some of these instance variables

http://codereview.chromium.org/1026005/diff/11001/12019
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/1026005/diff/11001/12019#newcode199
chrome/common/notification_type.h:199: // sent more than once for a particular
browser.
under what conditions can it be sent more than once?

[sky, 2010-03-17 23:46:22]

All updated.

http://codereview.chromium.org/1026005/diff/11001/12001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/1026005/diff/11001/12001#newcode660
chrome/browser/browser.cc:660: tab_restore_service->BrowserClosing(this);
On 2010/03/17 22:58:00, Erik Kay wrote:
> it seems like this should just use your new notification below

I've added a TODO.

http://codereview.chromium.org/1026005/diff/11001/12003
File chrome/browser/browser_browsertest.cc (right):

http://codereview.chromium.org/1026005/diff/11001/12003#newcode464
chrome/browser/browser_browsertest.cc:464: IN_PROC_BROWSER_TEST_F(BrowserTest,
RestorePinnedTabs) {
On 2010/03/17 22:58:00, Erik Kay wrote:
> it seems like you should mix in a non-pinned tab in the middle somewhere here
as
> well

It would be forced to the end so that it wouldn't make much of a difference with
what's there now.

http://codereview.chromium.org/1026005/diff/11001/12005
File chrome/browser/browser_init.h (right):

http://codereview.chromium.org/1026005/diff/11001/12005#newcode106
chrome/browser/browser_init.h:106: // Convenience for OpenTabsInBrowser that
converts |urls| into a set of
On 2010/03/17 22:58:00, Erik Kay wrote:
> This feels pretty weird to me.  The entries aren't pinned tabs in this case. 
I
> think I understand how you got here, but it winds up being harder to
understand
> than necessary.  I don't have a great alternative suggestion.

I could move Entry out of PinnedTabCodec and into this class. What do you think?

http://codereview.chromium.org/1026005/diff/11001/12012
File chrome/browser/tabs/pinned_tab_codec.cc (right):

http://codereview.chromium.org/1026005/diff/11001/12012#newcode49
chrome/browser/tabs/pinned_tab_codec.cc:49:
UTF8ToUTF16(extension->app_launch_url().spec()));
On 2010/03/17 22:58:00, Erik Kay wrote:
> I wonder if we should use the URL it's currently on like we do for other tabs.

> For example, if I had navigated to a different folder in my gmail.  Maybe we
> should remember that.  I can see it both ways, but I'm just curious what you
> think.

I viewed this as starting over, or restarting an app, in which case the main
page seems best.

This behavior also matches what we do when closing a pinned tab then clicking on
it again.

http://codereview.chromium.org/1026005/diff/11001/12015
File chrome/browser/tabs/pinned_tab_service.cc (right):

http://codereview.chromium.org/1026005/diff/11001/12015#newcode35
chrome/browser/tabs/pinned_tab_service.cc:35: if
(closing_browsers_.count(browser) == 0) {
On 2010/03/17 22:58:00, Erik Kay wrote:
> A comment explaining this logic would help.  It looks like you only want to
> write one browser at a time, but I'm not sure why you'd need to worry about
this
> if the bool in the notification is true.  In that case, you're exiting, right?

> So this code should only ever be called once.  Given this, maybe this should
> just set got_exiting_ and then you don't need closing_browsers_ at all?
> 
> I'm probably missing something here.  So the comment would have helped me. :-)

No, you're right. This code kind of grew as I realized separate code paths and I
didn't try and refactor it. Sorry about that.

http://codereview.chromium.org/1026005/diff/11001/12019
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/1026005/diff/11001/12019#newcode199
chrome/common/notification_type.h:199: // sent more than once for a particular
browser.
On 2010/03/17 22:58:00, Erik Kay wrote:
> under what conditions can it be sent more than once?

I believe on unload handlers may trigger it to be invoked more than once.

[Erik does not do reviews, 2010-03-18 00:10:12]

http://codereview.chromium.org/1026005/diff/11001/12003
File chrome/browser/browser_browsertest.cc (right):

http://codereview.chromium.org/1026005/diff/11001/12003#newcode464
chrome/browser/browser_browsertest.cc:464: IN_PROC_BROWSER_TEST_F(BrowserTest,
RestorePinnedTabs) {
On 2010/03/17 23:46:23, sky wrote:
> On 2010/03/17 22:58:00, Erik Kay wrote:
> > it seems like you should mix in a non-pinned tab in the middle somewhere
here
> as
> > well
> 
> It would be forced to the end so that it wouldn't make much of a difference
with
> what's there now.

Yep.  I get that, but I figured since that's actual behavior that pinning
triggers, that it's worth testing.

http://codereview.chromium.org/1026005/diff/11001/12005
File chrome/browser/browser_init.h (right):

http://codereview.chromium.org/1026005/diff/11001/12005#newcode106
chrome/browser/browser_init.h:106: // Convenience for OpenTabsInBrowser that
converts |urls| into a set of
On 2010/03/17 23:46:23, sky wrote:
> On 2010/03/17 22:58:00, Erik Kay wrote:
> > This feels pretty weird to me.  The entries aren't pinned tabs in this case.

> I
> > think I understand how you got here, but it winds up being harder to
> understand
> > than necessary.  I don't have a great alternative suggestion.
> 
> I could move Entry out of PinnedTabCodec and into this class. What do you
think?

That feels better.  Either here, or maybe even TabContents? I can imagine it
being used by session restore as well.

http://codereview.chromium.org/1026005/diff/11001/12012
File chrome/browser/tabs/pinned_tab_codec.cc (right):

http://codereview.chromium.org/1026005/diff/11001/12012#newcode49
chrome/browser/tabs/pinned_tab_codec.cc:49:
UTF8ToUTF16(extension->app_launch_url().spec()));
On 2010/03/17 23:46:23, sky wrote:
> On 2010/03/17 22:58:00, Erik Kay wrote:
> > I wonder if we should use the URL it's currently on like we do for other
tabs.
> 
> > For example, if I had navigated to a different folder in my gmail.  Maybe we
> > should remember that.  I can see it both ways, but I'm just curious what you
> > think.
> 
> I viewed this as starting over, or restarting an app, in which case the main
> page seems best.
> 
> This behavior also matches what we do when closing a pinned tab then clicking
on
> it again.
> 

OK.  Add a comment about that?

http://codereview.chromium.org/1026005/diff/21001/22012#newcode47
chrome/browser/tabs/pinned_tab_codec.cc:47: value->SetStringFromUTF16(kAppID,
UTF8ToUTF16(extension->id()));
don't need these conversions either

http://codereview.chromium.org/1026005/diff/21001/22012#newcode56
chrome/browser/tabs/pinned_tab_codec.cc:56: } else if
(tab_contents->controller().entry_count()) {
or these

[sky, 2010-03-18 04:10:22]

All updated. I moved the struct into BrowserInit::LaunchWithProfile. If we find
we need it in other classes we can move it to a better location later.

Thanks,

  -Scott

[Erik does not do reviews, 2010-03-18 14:45:45]

LGTM