Implement App Tabs for GTK

Implement App Tabs for GTK

BUG=45540
TEST=none

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

[rafaelw, 2010-07-01 01:27:40]

erg, please review.

sky, fyi -- in case you want to skim and gut check that i didn't miss anything
subtle that i may not have noticed.

[sky, 2010-07-01 16:13:22]

I thought we were getting rid of nano tabs?

[rafaelw1, 2010-07-01 16:27:00]

+aa

I don't think so, Aaron?

On Thu, Jul 1, 2010 at 9:13 AM,  <sky@chromium.org> wrote:
> I thought we were getting rid of nano tabs?
>
> http://codereview.chromium.org/2824044/show
>

[sky, 2010-07-01 16:33:06]

I thought Glen mentioned this the other day, but perhaps it's not a
final decision? Glen?

  -Scott

On Thu, Jul 1, 2010 at 9:26 AM, Rafael Weinstein <rafaelw@google.com> wrote:
> +aa
>
> I don't think so, Aaron?
>
> On Thu, Jul 1, 2010 at 9:13 AM,  <sky@chromium.org> wrote:
>> I thought we were getting rid of nano tabs?
>>
>> http://codereview.chromium.org/2824044/show
>>
>

[Elliot Glaysher, 2010-07-01 16:48:55]

The code LGTM, but you might want to wait to find out if this should
be committed.

On Wed, Jun 30, 2010 at 6:27 PM,  <rafaelw@chromium.org> wrote:
> Reviewers: Elliot Glaysher,
>
> Message:
> erg, please review.
>
> sky, fyi -- in case you want to skim and gut check that i didn't miss
> anything
> subtle that i may not have noticed.
>
> Description:
> Implement App Tabs for GTK
>
> BUG=45540
> TEST=none
>
> Please review this at http://codereview.chromium.org/2824044/show
>
> SVN Base: http://src.chromium.org/git/chromium.git
>
> Affected files:
>  M chrome/browser/gtk/tabs/dragged_tab_gtk.cc
>  M chrome/browser/gtk/tabs/tab_gtk.h
>  M chrome/browser/gtk/tabs/tab_gtk.cc
>  M chrome/browser/gtk/tabs/tab_renderer_gtk.h
>  M chrome/browser/gtk/tabs/tab_renderer_gtk.cc
>  M chrome/browser/gtk/tabs/tab_strip_gtk.cc
>
>
>

[sky, 2010-07-01 16:59:03]

http://codereview.chromium.org/2824044/diff/2001/3004
File chrome/browser/gtk/tabs/tab_renderer_gtk.cc (right):

http://codereview.chromium.org/2824044/diff/2001/3004#newcode863
chrome/browser/gtk/tabs/tab_renderer_gtk.cc:863: int favIconHeightOffset =
data_.app ? -2 : 0;
Can't we also get a bigger icon if the tab isn't an app tab but there is an app
whose extent contains the url?

http://codereview.chromium.org/2824044/diff/2001/3006
File chrome/browser/gtk/tabs/tab_strip_gtk.cc (right):

http://codereview.chromium.org/2824044/diff/2001/3006#newcode682
chrome/browser/gtk/tabs/tab_strip_gtk.cc:682: const int
TabStripGtk::mini_to_non_mini_gap_ = 13;
You should only add the extra 10 for nano tabs. See how tab_strip in views deals
with extra_gap_for_nano_ .

[glen, 2010-07-01 17:01:40]

It's not a final decision - there's still plenty of arguing. The
current state is:

- Don't like having pinned tabs AND nano tabs.
- Pinned tabs useful
- Considering making pinned a user-chosen property of apps
  with some intelligent default
- Majority hatred of phantom tabs
- Split dislike of visual style of pinned and nano tabs
- More arguing.


On Thu, Jul 1, 2010 at 9:48 AM, Elliot Glaysher (Chromium)
<erg@chromium.org> wrote:
> The code LGTM, but you might want to wait to find out if this should
> be committed.
>
> On Wed, Jun 30, 2010 at 6:27 PM,  <rafaelw@chromium.org> wrote:
>> Reviewers: Elliot Glaysher,
>>
>> Message:
>> erg, please review.
>>
>> sky, fyi -- in case you want to skim and gut check that i didn't miss
>> anything
>> subtle that i may not have noticed.
>>
>> Description:
>> Implement App Tabs for GTK
>>
>> BUG=45540
>> TEST=none
>>
>> Please review this at http://codereview.chromium.org/2824044/show
>>
>> SVN Base: http://src.chromium.org/git/chromium.git
>>
>> Affected files:
>>  M chrome/browser/gtk/tabs/dragged_tab_gtk.cc
>>  M chrome/browser/gtk/tabs/tab_gtk.h
>>  M chrome/browser/gtk/tabs/tab_gtk.cc
>>  M chrome/browser/gtk/tabs/tab_renderer_gtk.h
>>  M chrome/browser/gtk/tabs/tab_renderer_gtk.cc
>>  M chrome/browser/gtk/tabs/tab_strip_gtk.cc
>>
>>
>>
>

[rafaelw, 2010-07-01 18:54:41]

http://codereview.chromium.org/2824044/diff/2001/3004
File chrome/browser/gtk/tabs/tab_renderer_gtk.cc (right):

http://codereview.chromium.org/2824044/diff/2001/3004#newcode863
chrome/browser/gtk/tabs/tab_renderer_gtk.cc:863: int favIconHeightOffset =
data_.app ? -2 : 0;
On 2010/07/01 16:59:03, sky wrote:
> Can't we also get a bigger icon if the tab isn't an app tab but there is an
app
> whose extent contains the url?

Per offline chat, we don't show the app icons in normal tabs. I've removed the
misleading sentence in TabContents::GetExtensionAppIcon() header comment that
suggests we do.

http://codereview.chromium.org/2824044/diff/2001/3006
File chrome/browser/gtk/tabs/tab_strip_gtk.cc (right):

http://codereview.chromium.org/2824044/diff/2001/3006#newcode682
chrome/browser/gtk/tabs/tab_strip_gtk.cc:682: const int
TabStripGtk::mini_to_non_mini_gap_ = 13;
On 2010/07/01 16:59:03, sky wrote:
> You should only add the extra 10 for nano tabs. See how tab_strip in views
deals
> with extra_gap_for_nano_ .

Done. Thanks, this was exactly the kind of subtlety I was worried I missed =-).

[sky, 2010-07-01 19:25:50]

http://codereview.chromium.org/2824044/diff/13001/10007
File chrome/browser/gtk/tabs/tab_strip_gtk.cc (right):

http://codereview.chromium.org/2824044/diff/13001/10007#newcode1239
chrome/browser/gtk/tabs/tab_strip_gtk.cc:1239: else if
(!tab_data_[i].tab->app())
app tabs are always considered mini, so you shouldn't need this.

http://codereview.chromium.org/2824044/diff/13001/10007#newcode1250
chrome/browser/gtk/tabs/tab_strip_gtk.cc:1250: else if
(!tab_data_[i].tab->mini())
Can you add this to the view's tab_strip too.

http://codereview.chromium.org/2824044/diff/13001/10007#newcode1462
chrome/browser/gtk/tabs/tab_strip_gtk.cc:1462: int offset =
mini_to_non_mini_gap_ + kTabHOffset;
Why does having a nano tab present make it so we get the bigger gap? Consider
the following, you have a mini-tab and a normal tab. Why should adding a nano
tab (possibly to the front of the tab strip) result in a different size gap?

I think both types should have the same gap.

[rafaelw, 2010-07-01 22:37:52]

Ok, PTAL. Sorry for all the snapshots.

http://codereview.chromium.org/2824044/diff/13001/10007
File chrome/browser/gtk/tabs/tab_strip_gtk.cc (right):

http://codereview.chromium.org/2824044/diff/13001/10007#newcode1239
chrome/browser/gtk/tabs/tab_strip_gtk.cc:1239: else if
(!tab_data_[i].tab->app())
On 2010/07/01 19:25:51, sky wrote:
> app tabs are always considered mini, so you shouldn't need this.

Done.

http://codereview.chromium.org/2824044/diff/13001/10007#newcode1250
chrome/browser/gtk/tabs/tab_strip_gtk.cc:1250: else if
(!tab_data_[i].tab->mini())
On 2010/07/01 19:25:51, sky wrote:
> Can you add this to the view's tab_strip too.

Done.

http://codereview.chromium.org/2824044/diff/13001/10007#newcode1462
chrome/browser/gtk/tabs/tab_strip_gtk.cc:1462: int offset =
mini_to_non_mini_gap_ + kTabHOffset;
On 2010/07/01 19:25:51, sky wrote:
> Why does having a nano tab present make it so we get the bigger gap? Consider
> the following, you have a mini-tab and a normal tab. Why should adding a nano
> tab (possibly to the front of the tab strip) result in a different size gap?
> 
> I think both types should have the same gap.

Done. Extra gap for nano tabs removed from views & gtk

[sky, 2010-07-01 22:40:47]

LGTM

http://codereview.chromium.org/2824044/diff/24001/25006
File chrome/browser/gtk/tabs/tab_strip_gtk.cc (right):

http://codereview.chromium.org/2824044/diff/24001/25006#newcode964
chrome/browser/gtk/tabs/tab_strip_gtk.cc:964: model_->IsAppTab(index), false);
nit: align with ( on previous line