Support painting panels with chromium themes on GTK.

Support painting panels with chromium themes on GTK.

In this patch, I moved more panel-specific logic out of the base classes:
* Title decoration to draw the title with pango markup
* SendEnterNotifyToCloseButtonIfUnderMouse

I overrode UsingCustomPopupFrame in PanelBrowserWindowGtk to always return false. That way, panels are always painted with custom frame. Now no matter which theme the user picks and which state (drawing attention or not) the panel is at, we paint the frame in the same code path.

I also fixed the extra line shown in title-only panel by adding IsMinimized check to BrowserWindowGtk::ShouldDrawContentDropShadow.

BUG=117205
TEST=Manual test by verifying panels being painted with correct theme

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=134635

[jianli, 2012-04-27 00:15:00]

Evan, could you please review all changes related to BrowserTitlebar and
BrowserWindowGtk?

Jenn, could you please review all panel related changes?

[dcheng, 2012-04-27 01:18:39]

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:33: const SkColor
kInactiveTitleTextDefaultColor = 0x80888888;
Consider using SkColorSetARGB. It makes it clear what each of the components is.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:32: const SkColor
kActiveBackgroundDefaultColorStart = 0xfff0f8fa;
Consider using SkColorSetARGB.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:62: return new
gfx::Image(new SkBitmap(canvas.ExtractBitmap()));
I think you can just do gfx::Image(canvas.ExtractBitmap())

[jennb, 2012-04-27 18:36:13]

Evan - I don't have graphics experience. Could you also take a close look at the
drawing code?

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar.cc:642: break;
NOTREACHED()

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_window_gtk.cc (left):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_window_gtk.cc:2455: (browser()->is_type_popup() ||
browser()->is_type_panel());
FYI - (not for this patch) There are still other uses of is_type_panel() in this
base class. Now that we have a subclass, is it possible to move the remaining
panel-specific logic to the subclass?

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_window_gtk.cc:913: return !IsMaximized() &&
!IsMinimized() && UseCustomFrame();
Suggest overriding ShouldDrawContentDropShadow() in PanelBrowserWindowGtk
instead. That also avoids the confusion of adding IsMinimized to
PanelBrowserWindowGtk and avoids altering base class behavior in a patch that
should only affect Panels.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_window_gtk.h (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_window_gtk.h:382: virtual int
GetThemeFrameResource();
Accidental change? I don't see this overridden in the subclass.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:62: if
(browser_window_->IsDrawingAttention())
browser_window_->panel()->IsDrawingAttention()

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:79:
button->SetBackground(GetTitleColor(), NULL, NULL);
The button's background is the same as the title text color?

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_titlebar_gtk.h (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.h:59: SkColor
GetTitleColor() const;
GetTextColor

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:31: // Colors used to draw
active titlebar under default theme.
active titlebar and frame

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:35: // Colors used to draw
inactive titlebar under default theme.
titlebar and frame

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:39: // Colors used to draw
titlebar for drawing attention under default theme.
titlebar and frame

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:52: const int gradient_size
= 32;
How did you decide on this number?

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:231: bool
PanelBrowserWindowGtk::IsMinimized() const {
So confusing... Usually, Panel delegates to native panel. Not the other way
around.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:454: UserChangedTheme();
Seems like overkill. We just want to repaint the titlebar. UserChangedTheme does
a whole lot more.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:488: gfx::Size
PanelBrowserWindowGtk::IconOnlySize() const {
go ahead and delete this obsoleted method in this patch.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_window_gtk.h (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.h:44: // NativePanel overrides
Please don't separate the NativePanel overrides.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.h:129:
PanelBrowserTitlebarGtk* GetTitlebar() const;
This gets confusing with base class having a titlebar() method. How about
GetPanelTitlebar() to better distinguish it?

[jianli, 2012-04-27 20:22:34]

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar.cc:642: break;
On 2012/04/27 18:36:13, jennb wrote:
> NOTREACHED()

Done.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_window_gtk.cc:913: return !IsMaximized() &&
!IsMinimized() && UseCustomFrame();
On 2012/04/27 18:36:13, jennb wrote:
> Suggest overriding ShouldDrawContentDropShadow() in PanelBrowserWindowGtk
> instead. That also avoids the confusion of adding IsMinimized to
> PanelBrowserWindowGtk and avoids altering base class behavior in a patch that
> should only affect Panels.

Done.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_window_gtk.h (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_window_gtk.h:382: virtual int
GetThemeFrameResource();
On 2012/04/27 18:36:13, jennb wrote:
> Accidental change? I don't see this overridden in the subclass.

Done.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:33: const SkColor
kInactiveTitleTextDefaultColor = 0x80888888;
On 2012/04/27 01:18:39, dcheng wrote:
> Consider using SkColorSetARGB. It makes it clear what each of the components
is.

Done.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:62: if
(browser_window_->IsDrawingAttention())
On 2012/04/27 18:36:13, jennb wrote:
> browser_window_->panel()->IsDrawingAttention()

Changed to query about paint state, per discussion.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:79:
button->SetBackground(GetTitleColor(), NULL, NULL);
On 2012/04/27 18:36:13, jennb wrote:
> The button's background is the same as the title text color? 

Changed per discussion.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_titlebar_gtk.h (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.h:59: SkColor
GetTitleColor() const;
On 2012/04/27 18:36:13, jennb wrote:
> GetTextColor

Done.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:31: // Colors used to draw
active titlebar under default theme.
On 2012/04/27 18:36:13, jennb wrote:
> active titlebar and frame

Done.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:32: const SkColor
kActiveBackgroundDefaultColorStart = 0xfff0f8fa;
On 2012/04/27 01:18:39, dcheng wrote:
> Consider using SkColorSetARGB.

Done.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:35: // Colors used to draw
inactive titlebar under default theme.
On 2012/04/27 18:36:13, jennb wrote:
> titlebar and frame

Done.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:39: // Colors used to draw
titlebar for drawing attention under default theme.
On 2012/04/27 18:36:13, jennb wrote:
> titlebar and frame

Done.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:52: const int gradient_size
= 32;
On 2012/04/27 18:36:13, jennb wrote:
> How did you decide on this number?

Added comment to explain this.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:62: return new
gfx::Image(new SkBitmap(canvas.ExtractBitmap()));
On 2012/04/27 01:18:39, dcheng wrote:
> I think you can just do gfx::Image(canvas.ExtractBitmap())

Done.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:231: bool
PanelBrowserWindowGtk::IsMinimized() const {
On 2012/04/27 18:36:13, jennb wrote:
> So confusing... Usually, Panel delegates to native panel. Not the other way
> around.

Removed.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:454: UserChangedTheme();
On 2012/04/27 18:36:13, jennb wrote:
> Seems like overkill. We just want to repaint the titlebar. UserChangedTheme
does
> a whole lot more.

Done.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:488: gfx::Size
PanelBrowserWindowGtk::IconOnlySize() const {
On 2012/04/27 18:36:13, jennb wrote:
> go ahead and delete this obsoleted method in this patch.

Done.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_window_gtk.h (right):

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.h:44: // NativePanel overrides
On 2012/04/27 18:36:13, jennb wrote:
> Please don't separate the NativePanel overrides.

Moved it back since we're not relying on getting paint state, per discussion.

http://codereview.chromium.org/10180011/diff/13/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.h:129:
PanelBrowserTitlebarGtk* GetTitlebar() const;
On 2012/04/27 18:36:13, jennb wrote:
> This gets confusing with base class having a titlebar() method. How about
> GetPanelTitlebar() to better distinguish it?

Done.

[jennb, 2012-04-27 20:37:24]

LGTM if Evan and some graphics-skilled person agrees.

Appreciate you improving the look of GTK panels!

http://codereview.chromium.org/10180011/diff/16001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/16001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:56: // the experiment and
should work for most of cases.
s/the experiment/experimentation

http://codereview.chromium.org/10180011/diff/16001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:145: return
panel_->IsMinimized();
!IsMinimized ?

http://codereview.chromium.org/10180011/diff/16001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_window_gtk.h (right):

http://codereview.chromium.org/10180011/diff/16001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.h:30: PAINT_AS_ACTIVE,
nit: alpha order?

[jianli, 2012-04-27 20:44:24]

http://codereview.chromium.org/10180011/diff/16001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/16001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:56: // the experiment and
should work for most of cases.
On 2012/04/27 20:37:24, jennb wrote:
> s/the experiment/experimentation

Done.

http://codereview.chromium.org/10180011/diff/16001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:145: return
panel_->IsMinimized();
On 2012/04/27 20:37:24, jennb wrote:
> !IsMinimized ?

Done.

http://codereview.chromium.org/10180011/diff/16001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_window_gtk.h (right):

http://codereview.chromium.org/10180011/diff/16001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.h:30: PAINT_AS_ACTIVE,
On 2012/04/27 20:37:24, jennb wrote:
> nit: alpha order?

Done.

[Evan Stade, 2012-04-27 21:27:33]

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:55: // pre-determined, we
use a reasonablly bigger value that is obtained from
spelling

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:56: // the experimentation
and should work for most of cases.
s/ of//

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:92:
kAttentionBackgroundDefaultColorEnd);
indent

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:293: if
(theme_provider->UsingDefaultTheme()) {
I'm confused, the CL description talks about GTK theme mode, but this checks
UsingDefaultTheme (default = classic).

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:294: return (paint_state ==
PAINT_AS_ACTIVE) ?
no parens

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:295:
GetActiveBackgroundDefaultImage(): GetInactiveBackgroundDefaultImage();
space before :

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_window_gtk.h (right):

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.h:136: const gfx::Image*
GetFrameTheme() const;
docs. I don't think this is a good name for this function.

[jianli, 2012-04-27 22:21:06]

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:55: // pre-determined, we
use a reasonablly bigger value that is obtained from
On 2012/04/27 21:27:33, Evan Stade wrote:
> spelling

Done.

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:56: // the experimentation
and should work for most of cases.
On 2012/04/27 21:27:33, Evan Stade wrote:
> s/ of//

Done.

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:92:
kAttentionBackgroundDefaultColorEnd);
On 2012/04/27 21:27:33, Evan Stade wrote:
> indent

Done.

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:293: if
(theme_provider->UsingDefaultTheme()) {
On 2012/04/27 21:27:33, Evan Stade wrote:
> I'm confused, the CL description talks about GTK theme mode, but this checks
> UsingDefaultTheme (default = classic).

This is because the active color (white) and inactive color (some sort of blue)
defined in the default (classic) theme could cause confusion to the user. These
colors work fine for the tabs due to the default blue background. However, when
they're used towards the panel windows that do not have any underlying
background, the user might easily think that the panel with blue titlebar and
frame is active and the panel with white titlebar and frame is inactive. Per the
feedback from our UI expert, Ken, we decided to use the custom colors for
default theme. Added comment here.

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:294: return (paint_state ==
PAINT_AS_ACTIVE) ?
On 2012/04/27 21:27:33, Evan Stade wrote:
> no parens

Done.

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:295:
GetActiveBackgroundDefaultImage(): GetInactiveBackgroundDefaultImage();
On 2012/04/27 21:27:33, Evan Stade wrote:
> space before :

Done.

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_window_gtk.h (right):

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.h:136: const gfx::Image*
GetFrameTheme() const;
On 2012/04/27 21:27:33, Evan Stade wrote:
> docs. I don't think this is a good name for this function.

Renamed to GetThemeFrameImage and added comment.

[Evan Stade, 2012-04-27 22:29:23]

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:293: if
(theme_provider->UsingDefaultTheme()) {
On 2012/04/27 22:21:06, jianli wrote:
> On 2012/04/27 21:27:33, Evan Stade wrote:
> > I'm confused, the CL description talks about GTK theme mode, but this checks
> > UsingDefaultTheme (default = classic).
> 
> This is because the active color (white) and inactive color (some sort of
blue)
> defined in the default (classic) theme could cause confusion to the user.
These
> colors work fine for the tabs due to the default blue background. However,
when
> they're used towards the panel windows that do not have any underlying
> background, the user might easily think that the panel with blue titlebar and
> frame is active and the panel with white titlebar and frame is inactive. Per
the
> feedback from our UI expert, Ken, we decided to use the custom colors for
> default theme. Added comment here.

sounds like a problem that all themes would be prone to, not just the default
one. If we have to rely on a crutch like this it's a sign our theme system is
deficient and it's hard or impossible to create a good theme. I do not know of
any other places in chrome where we manually sidestep the theming system just
for the default theme. That's not to say such places don't exist, but in many
cases such as the NTP, we've bent over backwards to avoid this.

[jianli, 2012-04-27 22:40:11]

On 2012/04/27 22:29:23, Evan Stade wrote:
>
http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
> File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):
> 
>
http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
> chrome/browser/ui/panels/panel_browser_window_gtk.cc:293: if
> (theme_provider->UsingDefaultTheme()) {
> On 2012/04/27 22:21:06, jianli wrote:
> > On 2012/04/27 21:27:33, Evan Stade wrote:
> > > I'm confused, the CL description talks about GTK theme mode, but this
checks
> > > UsingDefaultTheme (default = classic).
> > 
> > This is because the active color (white) and inactive color (some sort of
> blue)
> > defined in the default (classic) theme could cause confusion to the user.
> These
> > colors work fine for the tabs due to the default blue background. However,
> when
> > they're used towards the panel windows that do not have any underlying
> > background, the user might easily think that the panel with blue titlebar
and
> > frame is active and the panel with white titlebar and frame is inactive. Per
> the
> > feedback from our UI expert, Ken, we decided to use the custom colors for
> > default theme. Added comment here.
> 
> sounds like a problem that all themes would be prone to, not just the default
> one. If we have to rely on a crutch like this it's a sign our theme system is
> deficient and it's hard or impossible to create a good theme. I do not know of
> any other places in chrome where we manually sidestep the theming system just
> for the default theme. That's not to say such places don't exist, but in many
> cases such as the NTP, we've bent over backwards to avoid this.

I agree that what we do for panels is a ugly hack to workaround the default
theme problem. However, panels are special type of windows that already get all
the specific painting and handling, like custom buttons and top-most property.
IMHO, using specific colors for default theme painting adds one more special
thing to what we have now.

[Evan Stade, 2012-04-27 23:22:37]

On 2012/04/27 22:40:11, jianli wrote:
> On 2012/04/27 22:29:23, Evan Stade wrote:
> >
>
http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
> > File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):
> > 
> >
>
http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
> > chrome/browser/ui/panels/panel_browser_window_gtk.cc:293: if
> > (theme_provider->UsingDefaultTheme()) {
> > On 2012/04/27 22:21:06, jianli wrote:
> > > On 2012/04/27 21:27:33, Evan Stade wrote:
> > > > I'm confused, the CL description talks about GTK theme mode, but this
> checks
> > > > UsingDefaultTheme (default = classic).
> > > 
> > > This is because the active color (white) and inactive color (some sort of
> > blue)
> > > defined in the default (classic) theme could cause confusion to the user.
> > These
> > > colors work fine for the tabs due to the default blue background. However,
> > when
> > > they're used towards the panel windows that do not have any underlying
> > > background, the user might easily think that the panel with blue titlebar
> and
> > > frame is active and the panel with white titlebar and frame is inactive.
Per
> > the
> > > feedback from our UI expert, Ken, we decided to use the custom colors for
> > > default theme. Added comment here.
> > 
> > sounds like a problem that all themes would be prone to, not just the
default
> > one. If we have to rely on a crutch like this it's a sign our theme system
is
> > deficient and it's hard or impossible to create a good theme. I do not know
of
> > any other places in chrome where we manually sidestep the theming system
just
> > for the default theme. That's not to say such places don't exist, but in
many
> > cases such as the NTP, we've bent over backwards to avoid this.
> 
> I agree that what we do for panels is a ugly hack to workaround the default
> theme problem. However, panels are special type of windows that already get
all
> the specific painting and handling, like custom buttons and top-most property.
> IMHO, using specific colors for default theme painting adds one more special
> thing to what we have now.

I don't see these things as equal. Those are just special in the sense of
"different from default browser windows". This breaks the entire theme system
because if panel windows are unusable for themed browsers, no one will use
themes. The correct solution is to generate colors (for the gradient stops) from
those we do have on hand and which are guaranteed to work together (for example,
toolbar text color is guaranteed to contrast toolbar background color).

This is a problem which popup windows also face, however the solution there
appears to be to make the window unthemeable. That would be better than the
possibility of really crappy (illegible) color combinations.

[jianli, 2012-04-28 00:01:42]

On 2012/04/27 23:22:37, Evan Stade wrote:
> On 2012/04/27 22:40:11, jianli wrote:
> > On 2012/04/27 22:29:23, Evan Stade wrote:
> > >
> >
>
http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
> > > File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10180011/diff/18003/chrome/browser/ui/panels/p...
> > > chrome/browser/ui/panels/panel_browser_window_gtk.cc:293: if
> > > (theme_provider->UsingDefaultTheme()) {
> > > On 2012/04/27 22:21:06, jianli wrote:
> > > > On 2012/04/27 21:27:33, Evan Stade wrote:
> > > > > I'm confused, the CL description talks about GTK theme mode, but this
> > checks
> > > > > UsingDefaultTheme (default = classic).
> > > > 
> > > > This is because the active color (white) and inactive color (some sort
of
> > > blue)
> > > > defined in the default (classic) theme could cause confusion to the
user.
> > > These
> > > > colors work fine for the tabs due to the default blue background.
However,
> > > when
> > > > they're used towards the panel windows that do not have any underlying
> > > > background, the user might easily think that the panel with blue
titlebar
> > and
> > > > frame is active and the panel with white titlebar and frame is inactive.
> Per
> > > the
> > > > feedback from our UI expert, Ken, we decided to use the custom colors
for
> > > > default theme. Added comment here.
> > > 
> > > sounds like a problem that all themes would be prone to, not just the
> default
> > > one. If we have to rely on a crutch like this it's a sign our theme system
> is
> > > deficient and it's hard or impossible to create a good theme. I do not
know
> of
> > > any other places in chrome where we manually sidestep the theming system
> just
> > > for the default theme. That's not to say such places don't exist, but in
> many
> > > cases such as the NTP, we've bent over backwards to avoid this.
> > 
> > I agree that what we do for panels is a ugly hack to workaround the default
> > theme problem. However, panels are special type of windows that already get
> all
> > the specific painting and handling, like custom buttons and top-most
property.
> > IMHO, using specific colors for default theme painting adds one more special
> > thing to what we have now.
> 
> I don't see these things as equal. Those are just special in the sense of
> "different from default browser windows". This breaks the entire theme system
> because if panel windows are unusable for themed browsers, no one will use
> themes. The correct solution is to generate colors (for the gradient stops)
from
> those we do have on hand and which are guaranteed to work together (for
example,
> toolbar text color is guaranteed to contrast toolbar background color).
> 
> This is a problem which popup windows also face, however the solution there
> appears to be to make the window unthemeable. That would be better than the
> possibility of really crappy (illegible) color combinations.

Per suggestion from Jenn, I changed how we handled under default theme. Now we
use IDR_THEME_FRAME and IDR_THEME_FRAME_INACTIVE to paint panels under default
theme. How does it sound to you?

[Evan Stade, 2012-04-30 20:41:55]

can you try this out and post screenshots for a variety of dark and light
themes?

[Evan Stade, 2012-04-30 23:07:21]

lgtm

http://codereview.chromium.org/10180011/diff/17023/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10180011/diff/17023/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:260: return
GetAttentionBackgroundDefaultImage();
this function appears to be used regardless of whether we're on the default
theme, so the function name is misleading

http://codereview.chromium.org/10180011/diff/17023/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:268: return
theme_provider->GetImageNamed((paint_state == PAINT_AS_ACTIVE) ?
get rid of unnecessary parens

http://codereview.chromium.org/10180011/diff/17023/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:272: return
theme_provider->GetImageNamed((paint_state == PAINT_AS_ACTIVE) ?
ditto