[Mac] Re-enable pinned tabs; add support for mini-tabs and phantom tabs.

[Mac] Re-enable pinned tabs; add support for mini-tabs and phantom tabs.

This CL rewires the old support for pinned tabs to support mini-tabs. This also
removes the kEnablePinnedTabs browser default now that all platforms support it.
Note that pinning is now only accessible through the context menu; drag-to-pin
has been removed.

BUG=36798, 32845
TEST=Right-click and pin two tabs. Test dragging on and off and around the tab strip.
TEST=Cmd+W a pinned tab and it should go phantom (renderer closes down) and the tab is made alpha.

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

[pink (ping after 24hrs), 2010-03-22 14:55:11]

I'm confused. What's the diff between pinned, mini, and phantom tabs? Are pinned
tabs gone? If so, why is there new code that checks the pinned state? If so, why
is the pinned state renamed to mini in some instances?

[sky, 2010-03-22 15:22:20]

From TabStripModel:

// Each tab may be any one of the following states:
// . Mini-tab. Mini tabs are locked to the left side of the tab strip and
//   rendered differently (small tabs with only a favicon). The model makes
//   sure all mini-tabs are at the beginning of the tab strip. For example,
//   if a non-mini tab is added it is forced to be with non-mini tabs. Requests
//   to move tabs outside the range of the tab type are ignored. For example,
//   a request to move a mini-tab after non-mini-tabs is ignored.
//   You'll notice there is no explcit api for making a tab a mini-tab, rather
//   there are two tab types that are implicitly mini-tabs:
//   . App. Corresponds to an extension that wants an app tab. App tabs are
//     identified by TabContents::is_app().
//   . Pinned. Any tab can be pinned. A pinned tab is made phantom when closed.
//     Non-app tabs whose pinned state is changed are moved to be with other
//     mini-tabs or non-mini tabs.
// . Phantom. Only pinned tabs may be made phantom. When a tab that can be made
//   phantom is closed the renderer is shutdown, a new
//   TabContents/NavigationController is created that has not yet loaded the
//   renderer and observers are notified via the TabReplacedAt method. When a
//   phantom tab is selected the renderer is loaded and the tab is no longer
//   phantom.
//   Phantom tabs do not prevent the tabstrip from closing, for example if the
//   tabstrip has one phantom and one non-phantom tab and the non-phantom tab is
//   closed, then the tabstrip/browser are closed.

If this isn't clear, let me know and I'll update the docs accordingly.

  -Scott

On Mon, Mar 22, 2010 at 7:55 AM,  <pinkerton@chromium.org> wrote:
> I'm confused. What's the diff between pinned, mini, and phantom tabs? Are
> pinned
> tabs gone? If so, why is there new code that checks the pinned state? If so,
> why
> is the pinned state renamed to mini in some instances?
>
> http://codereview.chromium.org/1119005
>

[pinkerton1, 2010-03-22 15:29:25]

What's the purpose of phantom tabs? What's a use case for them?

On Mon, Mar 22, 2010 at 11:22 AM, Scott Violet <sky@chromium.org> wrote:
> From TabStripModel:
>
> // Each tab may be any one of the following states:
> // . Mini-tab. Mini tabs are locked to the left side of the tab strip and
> //   rendered differently (small tabs with only a favicon). The model makes
> //   sure all mini-tabs are at the beginning of the tab strip. For example,
> //   if a non-mini tab is added it is forced to be with non-mini tabs.
Requests
> //   to move tabs outside the range of the tab type are ignored. For example,
> //   a request to move a mini-tab after non-mini-tabs is ignored.
> //   You'll notice there is no explcit api for making a tab a mini-tab, rather
> //   there are two tab types that are implicitly mini-tabs:
> //   . App. Corresponds to an extension that wants an app tab. App tabs are
> //     identified by TabContents::is_app().
> //   . Pinned. Any tab can be pinned. A pinned tab is made phantom when
closed.
> //     Non-app tabs whose pinned state is changed are moved to be with other
> //     mini-tabs or non-mini tabs.
> // . Phantom. Only pinned tabs may be made phantom. When a tab that can be
made
> //   phantom is closed the renderer is shutdown, a new
> //   TabContents/NavigationController is created that has not yet loaded the
> //   renderer and observers are notified via the TabReplacedAt method. When a
> //   phantom tab is selected the renderer is loaded and the tab is no longer
> //   phantom.
> //   Phantom tabs do not prevent the tabstrip from closing, for example if the
> //   tabstrip has one phantom and one non-phantom tab and the non-phantom tab
is
> //   closed, then the tabstrip/browser are closed.
>
> If this isn't clear, let me know and I'll update the docs accordingly.
>
>  -Scott
>
> On Mon, Mar 22, 2010 at 7:55 AM,  <pinkerton@chromium.org> wrote:
>> I'm confused. What's the diff between pinned, mini, and phantom tabs? Are
>> pinned
>> tabs gone? If so, why is there new code that checks the pinned state? If so,
>> why
>> is the pinned state renamed to mini in some instances?
>>
>> http://codereview.chromium.org/1119005
>>
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[pinkerton1, 2010-03-22 15:32:49]

Also, I'm not clear how the user marks a tab as phantom. Is it in the
context menu? Should "phantom" mean something to users?

-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[viettrungluu, 2010-03-22 16:27:23]

/me goes to check out Chrome on Windows before asking any more stupid
questions....

http://codereview.chromium.org/1119005/diff/3001/4001
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/1119005/diff/3001/4001#newcode1072
chrome/browser/cocoa/browser_window_controller.mm:1072: // pinned state so that
it can be maintained between the windows.
Is this really the desired behaviour?

Are pinned tabs still all at the left on the tab strip? (And what happens when
you drop a pinned tab to the right of an unpinned tab?)

[pink (ping after 24hrs), 2010-03-22 16:33:08]

LGTM code-wise. Would like to see the answer to trung's question as well.

[sky, 2010-03-22 16:43:37]

On Mon, Mar 22, 2010 at 9:27 AM,  <viettrungluu@chromium.org> wrote:
> /me goes to check out Chrome on Windows before asking any more stupid
> questions....
>
>
> http://codereview.chromium.org/1119005/diff/3001/4001
> File chrome/browser/cocoa/browser_window_controller.mm (right):
>
> http://codereview.chromium.org/1119005/diff/3001/4001#newcode1072
> chrome/browser/cocoa/browser_window_controller.mm:1072: // pinned state
> so that it can be maintained between the windows.
> Is this really the desired behaviour?
>
> Are pinned tabs still all at the left on the tab strip?

They should be.

> (And what
> happens when you drop a pinned tab to the right of an unpinned tab?)

On the windows side we visually let the user drag the tab any where
and on release snap it to the appropriate spot.

  -Scott

[viettrungluu, 2010-03-22 16:47:39]

@rsesek: Could you check that dragging a pinned tab to the right of an unpinned
tab works correctly?

Otherwise (and other than nits), LGTM on the code.

http://codereview.chromium.org/1119005/diff/3001/4003
File chrome/browser/cocoa/tab_controller.mm (right):

http://codereview.chromium.org/1119005/diff/3001/4003#newcode23
chrome/browser/cocoa/tab_controller.mm:23: namespace TabControllerInternal {
Hmmm, I wonder why this namespace has a name.

http://codereview.chromium.org/1119005/diff/3001/4004
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/1119005/diff/3001/4004#newcode142
chrome/browser/cocoa/tab_strip_controller.h:142: // stored by the |contents|,
but the |pinned| state is caller's responsibility.
s/caller's/the caller's/

http://codereview.chromium.org/1119005/diff/3001/4005
File chrome/browser/cocoa/tab_strip_controller.mm (left):

http://codereview.chromium.org/1119005/diff/3001/4005#oldcode1245
chrome/browser/cocoa/tab_strip_controller.mm:1245: // TODO(viettrungluu): I
don't think this is needed. Investigate. See also
Sweet. #TODO--.

http://codereview.chromium.org/1119005/diff/3001/4005
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/1119005/diff/3001/4005#newcode1323
chrome/browser/cocoa/tab_strip_controller.mm:1323: asPinnedTab:(BOOL)pinned {
Why do we call it "pinned" here instead of "mini"? I'm so confused.

http://codereview.chromium.org/1119005/diff/3001/4013
File chrome/browser/tab_menu_model.cc (right):

http://codereview.chromium.org/1119005/diff/3001/4013#newcode23
chrome/browser/tab_menu_model.cc:23: // the actual string based on the tab's
state via the delegate.
Does the pin/unpin code still work correctly?

[Avi (use Gerrit), 2010-03-22 16:57:48]

http://codereview.chromium.org/1119005/diff/3001/4003
File chrome/browser/cocoa/tab_controller.mm (right):

http://codereview.chromium.org/1119005/diff/3001/4003#newcode23
chrome/browser/cocoa/tab_controller.mm:23: namespace TabControllerInternal {
On 2010/03/22 16:47:40, viettrungluu wrote:
> Hmmm, I wonder why this namespace has a name.

So we can refer to its contents (in particular, the MenuDelegate) from the
header file.

[viettrungluu, 2010-03-22 17:00:48]

http://codereview.chromium.org/1119005/diff/3001/4003
File chrome/browser/cocoa/tab_controller.mm (right):

http://codereview.chromium.org/1119005/diff/3001/4003#newcode23
chrome/browser/cocoa/tab_controller.mm:23: namespace TabControllerInternal {
On 2010/03/22 16:57:48, Avi wrote:
> On 2010/03/22 16:47:40, viettrungluu wrote:
> > Hmmm, I wonder why this namespace has a name.
> 
> So we can refer to its contents (in particular, the MenuDelegate) from the
> header file.

Fair enough. In that case, the new constant |kPhantomTabAlpha| probably
shouldn't be in it, but in an anonymous namespace. It'd probably also be helpful
if instead of "// namespace" at the end, it were "// namespace
TabControllerInternal".

[Robert Sesek, 2010-03-22 17:59:52]

If a user closes a mini tab (either through "Close Tab" context menu or Cmd+W),
it becomes phantom. We render the entire TabView with alpha on the tabstrip. If
the user re-selects the phantom tab, it gets a new renderer and loads. If the
user closes the last non-phantom tab in a window, the window will close. I don't
know the purpose or use of mini tabs, but they're hooked up ;-).

Dragging a mini tab around in the tabstrip works as it does on windows: if the
user tries to drag it to the normal tab section, it slides back to the
left-hand-side where mini tabs live. This also happens if the user detaches the
mini tab into a new window but drops it into the normal tab section.

Mini tab state is also propagated to new windows when they're torn off, as this
is what Windows does. The only thing NOT done by this CL is make the favicon
"zoom" when the user hovers over a mini tab. It looks like crap on Windows (the
favicon is all pixely) and I didn't want to bring that over to the Mac.

http://codereview.chromium.org/1119005/diff/3001/4001
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/1119005/diff/3001/4001#newcode1072
chrome/browser/cocoa/browser_window_controller.mm:1072: // pinned state so that
it can be maintained between the windows.
On 2010/03/22 16:27:24, viettrungluu wrote:
> Is this really the desired behaviour?
> 
> Are pinned tabs still all at the left on the tab strip? (And what happens when
> you drop a pinned tab to the right of an unpinned tab?)

Yes, this is what Windows does. Also, dragging (or dropping) a mini tab into a
the non-mini section will cause it to slide over to the end of the mini tab
section (as it does on Windows).

http://codereview.chromium.org/1119005/diff/3001/4003
File chrome/browser/cocoa/tab_controller.mm (right):

http://codereview.chromium.org/1119005/diff/3001/4003#newcode23
chrome/browser/cocoa/tab_controller.mm:23: namespace TabControllerInternal {
On 2010/03/22 17:00:49, viettrungluu wrote:
> On 2010/03/22 16:57:48, Avi wrote:
> > On 2010/03/22 16:47:40, viettrungluu wrote:
> > > Hmmm, I wonder why this namespace has a name.
> > 
> > So we can refer to its contents (in particular, the MenuDelegate) from the
> > header file.
> 
> Fair enough. In that case, the new constant |kPhantomTabAlpha| probably
> shouldn't be in it, but in an anonymous namespace. It'd probably also be
helpful
> if instead of "// namespace" at the end, it were "// namespace
> TabControllerInternal".

Done.

http://codereview.chromium.org/1119005/diff/3001/4004
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/1119005/diff/3001/4004#newcode142
chrome/browser/cocoa/tab_strip_controller.h:142: // stored by the |contents|,
but the |pinned| state is caller's responsibility.
On 2010/03/22 16:47:40, viettrungluu wrote:
> s/caller's/the caller's/

Done.

http://codereview.chromium.org/1119005/diff/3001/4005
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/1119005/diff/3001/4005#newcode1323
chrome/browser/cocoa/tab_strip_controller.mm:1323: asPinnedTab:(BOOL)pinned {
On 2010/03/22 16:47:40, viettrungluu wrote:
> Why do we call it "pinned" here instead of "mini"? I'm so confused.

For the reason outlined in the .h: Mini tabs are mini because either the user
pinned them or the page is an "app" (whatever that means). The TabContents keeps
track of the app state, so we only need to propagate the pinned state. I
wrestled with calling this |mini| instead, but I figured that calling it
|pinned| is indicative of what actually happens. I've copied the comment from
the .h to make this more clear here.

http://codereview.chromium.org/1119005/diff/3001/4013
File chrome/browser/tab_menu_model.cc (right):

http://codereview.chromium.org/1119005/diff/3001/4013#newcode23
chrome/browser/tab_menu_model.cc:23: // the actual string based on the tab's
state via the delegate.
On 2010/03/22 16:47:40, viettrungluu wrote:
> Does the pin/unpin code still work correctly?

Yup. This just rewires and renames it.

[sky, 2010-03-22 18:24:30]

On Mon, Mar 22, 2010 at 10:59 AM,  <rsesek@chromium.org> wrote:
> If a user closes a mini tab (either through "Close Tab" context menu or
> Cmd+W),
> it becomes phantom.

It's only pinned tabs that this happens to. Mini-tabs include app
tabs, which are not by default pinned.

> Mini tab state is also propagated to new windows when they're torn off, as
> this
> is what Windows does. The only thing NOT done by this CL is make the favicon
> "zoom" when the user hovers over a mini tab. It looks like crap on Windows
> (the
> favicon is all pixely) and I didn't want to bring that over to the Mac.

It's been nuked on windows. The crap favicon zooming now happens when
the title of a mini-tab changes and it's not selected. Glen isn't too
happy with the current approach, so I would wait until we resolve it
until you change the behavior.

  -Scott

>
> http://codereview.chromium.org/1119005/diff/3001/4001
> File chrome/browser/cocoa/browser_window_controller.mm (right):
>
> http://codereview.chromium.org/1119005/diff/3001/4001#newcode1072
> chrome/browser/cocoa/browser_window_controller.mm:1072: // pinned state
> so that it can be maintained between the windows.
> On 2010/03/22 16:27:24, viettrungluu wrote:
>>
>> Is this really the desired behaviour?
>
>> Are pinned tabs still all at the left on the tab strip? (And what
>
> happens when
>>
>> you drop a pinned tab to the right of an unpinned tab?)
>
> Yes, this is what Windows does. Also, dragging (or dropping) a mini tab
> into a the non-mini section will cause it to slide over to the end of
> the mini tab section (as it does on Windows).
>
> http://codereview.chromium.org/1119005/diff/3001/4003
> File chrome/browser/cocoa/tab_controller.mm (right):
>
> http://codereview.chromium.org/1119005/diff/3001/4003#newcode23
> chrome/browser/cocoa/tab_controller.mm:23: namespace
> TabControllerInternal {
> On 2010/03/22 17:00:49, viettrungluu wrote:
>>
>> On 2010/03/22 16:57:48, Avi wrote:
>> > On 2010/03/22 16:47:40, viettrungluu wrote:
>> > > Hmmm, I wonder why this namespace has a name.
>> >
>> > So we can refer to its contents (in particular, the MenuDelegate)
>
> from the
>>
>> > header file.
>
>> Fair enough. In that case, the new constant |kPhantomTabAlpha|
>
> probably
>>
>> shouldn't be in it, but in an anonymous namespace. It'd probably also
>
> be helpful
>>
>> if instead of "// namespace" at the end, it were "// namespace
>> TabControllerInternal".
>
> Done.
>
> http://codereview.chromium.org/1119005/diff/3001/4004
> File chrome/browser/cocoa/tab_strip_controller.h (right):
>
> http://codereview.chromium.org/1119005/diff/3001/4004#newcode142
> chrome/browser/cocoa/tab_strip_controller.h:142: // stored by the
> |contents|, but the |pinned| state is caller's responsibility.
> On 2010/03/22 16:47:40, viettrungluu wrote:
>>
>> s/caller's/the caller's/
>
> Done.
>
> http://codereview.chromium.org/1119005/diff/3001/4005
> File chrome/browser/cocoa/tab_strip_controller.mm (right):
>
> http://codereview.chromium.org/1119005/diff/3001/4005#newcode1323
> chrome/browser/cocoa/tab_strip_controller.mm:1323:
> asPinnedTab:(BOOL)pinned {
> On 2010/03/22 16:47:40, viettrungluu wrote:
>>
>> Why do we call it "pinned" here instead of "mini"? I'm so confused.
>
> For the reason outlined in the .h: Mini tabs are mini because either the
> user pinned them or the page is an "app" (whatever that means). The
> TabContents keeps track of the app state, so we only need to propagate
> the pinned state. I wrestled with calling this |mini| instead, but I
> figured that calling it |pinned| is indicative of what actually happens.
> I've copied the comment from the .h to make this more clear here.
>
> http://codereview.chromium.org/1119005/diff/3001/4013
> File chrome/browser/tab_menu_model.cc (right):
>
> http://codereview.chromium.org/1119005/diff/3001/4013#newcode23
> chrome/browser/tab_menu_model.cc:23: // the actual string based on the
> tab's state via the delegate.
> On 2010/03/22 16:47:40, viettrungluu wrote:
>>
>> Does the pin/unpin code still work correctly?
>
> Yup. This just rewires and renames it.
>
> http://codereview.chromium.org/1119005
>

[Robert Sesek, 2010-03-23 14:43:22]

On Mon, Mar 22, 2010 at 2:24 PM, Scott Violet <sky@chromium.org> wrote:

> On Mon, Mar 22, 2010 at 10:59 AM,  <rsesek@chromium.org> wrote:
> > If a user closes a mini tab (either through "Close Tab" context menu or
> > Cmd+W),
> > it becomes phantom.
>
> It's only pinned tabs that this happens to. Mini-tabs include app
> tabs, which are not by default pinned.


Good to know, thanks. Do you have any examples of a site that is an app tab?
I'd like to test that functionality.

http://codereview.chromium.org/1119005

[sky, 2010-03-23 15:19:54]

On Tue, Mar 23, 2010 at 7:42 AM, Robert Sesek <rsesek@chromium.org> wrote:
> On Mon, Mar 22, 2010 at 2:24 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> On Mon, Mar 22, 2010 at 10:59 AM,  <rsesek@chromium.org> wrote:
>> > If a user closes a mini tab (either through "Close Tab" context menu or
>> > Cmd+W),
>> > it becomes phantom.
>>
>> It's only pinned tabs that this happens to. Mini-tabs include app
>> tabs, which are not by default pinned.
>
> Good to know, thanks. Do you have any examples of a site that is an app tab?
> I'd like to test that functionality.
> http://codereview.chromium.org/1119005

No, app support is still in the works. Aaron or Erik could likely
point you at an example.

  -Scott

[Robert Sesek, 2010-03-24 05:43:18]

Are there any more comments/questions, or am I free to land?

[viettrungluu, 2010-03-24 05:51:03]

On 2010/03/24 05:43:18, rsesek wrote:
> Are there any more comments/questions, or am I free to land?

I want my pinned tabs back. Ship it!

[sky, 2010-03-24 15:10:43]

LGTM