GTK: Make the drawing of selected tabs use the throb value.

GTK: Make the drawing of selected tabs use the throb value.

Previously, we used a different inactive tab image, but that had the problem
that selected background tabs that were showing the throbber would show the
normal tab image within the throbber bounds. This makes us match how views
renders.

BUG=none
TEST=Have a non-active, selected tab show the throbber. The area behind the throbber should match the rest of the tab.


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

[Elliot Glaysher, 2011-10-31 18:34:30]

dpapad: review
estade: fyi

[dpapad, 2011-10-31 19:54:52]

On 2011/10/31 18:34:30, Elliot Glaysher wrote:
> dpapad: review
> estade: fyi

It seems to me very hard to distinguish between selected not active and not
selected. Should the |kSelectedTabOpacity| value be increased a bit?

[Elliot Glaysher, 2011-10-31 20:00:34]

On 2011/10/31 19:54:52, dpapad wrote:
> On 2011/10/31 18:34:30, Elliot Glaysher wrote:
> > dpapad: review
> > estade: fyi
> 
> It seems to me very hard to distinguish between selected not active and not
> selected. Should the |kSelectedTabOpacity| value be increased a bit?

I don't know. Those constants are copied directly from views and the modified
version of TabRendererGtk::GetThrobberValue() was done in the spirit of the
view's Tab::GetThrobberValue(), using the same min/scale system.

[dpapad, 2011-10-31 20:29:23]

On 2011/10/31 20:00:34, Elliot Glaysher wrote:
> On 2011/10/31 19:54:52, dpapad wrote:
> > On 2011/10/31 18:34:30, Elliot Glaysher wrote:
> > > dpapad: review
> > > estade: fyi
> > 
> > It seems to me very hard to distinguish between selected not active and not
> > selected. Should the |kSelectedTabOpacity| value be increased a bit?
> 
> I don't know. Those constants are copied directly from views and the modified
> version of TabRendererGtk::GetThrobberValue() was done in the spirit of the
> view's Tab::GetThrobberValue(), using the same min/scale system.

LGTM on the changes (but also wait for estade's LG). I will file a bug about the
issue I mentioned once this change lands.

[dpapad, 2011-10-31 20:29:34]

http://codereview.chromium.org/8429012/diff/1/chrome/browser/ui/gtk/tabs/tab_...
File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right):

http://codereview.chromium.org/8429012/diff/1/chrome/browser/ui/gtk/tabs/tab_...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:78: const double
kSelectedTabOpacity = .45;
nit: s/.45/0.45 for consistency within this file.

http://codereview.chromium.org/8429012/diff/1/chrome/browser/ui/gtk/tabs/tab_...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:81: const double
kSelectedTabThrobScale = .5;
nit: same here.

[Elliot Glaysher, 2011-10-31 20:34:53]

On 2011/10/31 20:29:23, dpapad wrote:
> LGTM on the changes (but also wait for estade's LG).

In that case ping estade (fyi moved to review).

[Evan Stade, 2011-10-31 20:48:17]

http://codereview.chromium.org/8429012/diff/2002/chrome/browser/ui/gtk/tabs/t...
File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right):

http://codereview.chromium.org/8429012/diff/2002/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:420: return true;
is this correct?

[Elliot Glaysher, 2011-10-31 20:50:02]

http://codereview.chromium.org/8429012/diff/2002/chrome/browser/ui/gtk/tabs/t...
File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right):

http://codereview.chromium.org/8429012/diff/2002/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:420: return true;
On 2011/10/31 20:48:17, Evan Stade wrote:
> is this correct?

Yes. This method is overriden with TabGtk::IsSelected() which calls the
appropriate delegate interface.

(I suspect that return false would be a better theoretical default, but TabGtk
is the only subclass.)

[Elliot Glaysher, 2011-10-31 20:54:01]

http://codereview.chromium.org/8429012/diff/2002/chrome/browser/ui/gtk/tabs/t...
File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right):

http://codereview.chromium.org/8429012/diff/2002/chrome/browser/ui/gtk/tabs/t...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:420: return true;
On 2011/10/31 20:50:03, Elliot Glaysher wrote:
> On 2011/10/31 20:48:17, Evan Stade wrote:
> > is this correct?
> 
> Yes. This method is overriden with TabGtk::IsSelected() which calls the
> appropriate delegate interface.
> 
> (I suspect that return false would be a better theoretical default, but TabGtk
> is the only subclass.)

I take that back. return true is correct and confusing.
 
DraggedViewGtk keeps raw TabRendererGtks around and all tabs that are being
dragged are selected.

[Evan Stade, 2011-10-31 21:39:21]

ok lgtm

[commit-bot: I haz the power, 2011-10-31 21:48:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/8429012/2002

[commit-bot: I haz the power, 2011-10-31 22:46:22]

Try job failure for 8429012-2002 (retry) on linux_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-10-31 22:53:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/8429012/2002

[commit-bot: I haz the power, 2011-10-31 22:56:11]

Try job failure for 8429012-2002 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2011-10-31 23:33:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/8429012/2002

[commit-bot: I haz the power, 2011-11-01 00:56:46]

Change committed as 108049