Ensure that popup windows are not themed....

Ensure that popup windows are not themed.

BUG= http://crbug.com/18093
TEST= While running Chrome with a theme installed, force a popup.  Note that the popup window is not themed.


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

[Glen Murphy, 2009-08-05 22:27:46]

http://codereview.chromium.org/159871/diff/1/4
File chrome/browser/views/frame/browser_frame_win.cc (right):

http://codereview.chromium.org/159871/diff/1/4#newcode53
Line 53: GetNonClientView()->ForceNativeTheme();
'Native' has doubled-up meanings - it looks like you're forcing Chrome to use
the native system frame, which has weird mental overlaps with
ShouldUseNativeFrame() - something that is only true when on Vista when Glass is
enabled and no theme is installed (the default opaque frame is not a native
frame). See BrowserThemeProvider::ShouldUseNativeFrame

Maybe some renaming - 'Native' renamed to 'Default' where it's referring to the
default frame, and 'Native' kept as 'Native' when referring to the system (Aero)
frame.

Now I've gone and confused myself.

[Miranda Callahan, 2009-08-06 20:52:46]

All right -- completely new approach based on our conversation yesterday.  It's
actually clearer, though; and it seems to work with vista, transparency enabled
and disabled, and nonvista.

[Glen Murphy, 2009-08-06 21:37:40]

Won't this mean that on Vista, you'll still get the Opaque/Default frame on
non-browser Windows? You should get AeroGlassFrame on Vista for those windows.

Behavior looks correct for XP, though.

[Miranda Callahan, 2009-08-06 22:11:09]

Ah, of course -- sorry, I misunderstood that part of our conversation yesterday;
fixed (I added back in the code that had fixed the Vista version yesterday, and
changed the name to something more sensible).

On 2009/08/06 21:37:40, Glen Murphy wrote:
> Won't this mean that on Vista, you'll still get the Opaque/Default frame on
> non-browser Windows? You should get AeroGlassFrame on Vista for those windows.
> 
> Behavior looks correct for XP, though.

[Glen Murphy, 2009-08-06 23:17:28]

nittery and incognitoery

http://codereview.chromium.org/159871/diff/3010/2007
File chrome/browser/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/159871/diff/3010/2007#newcode570
Line 570: if (frame_->GetWindow()->IsActive()) {
Dialogs can be in incognito mode (incognito > popuptest)

http://codereview.chromium.org/159871/diff/3010/2009
File views/window/non_client_view.h (right):

http://codereview.chromium.org/159871/diff/3010/2009#newcode203
Line 203: void ForceGlassFrame();
nit: AeroGlass (to match the aero_glass_frame class name).

http://codereview.chromium.org/159871/diff/3010/2009#newcode229
Line 229: bool force_glass_frame_;
aero_glass

[Miranda Callahan, 2009-08-07 01:11:16]

denitted and incognitified!

On 2009/08/06 23:17:28, Glen Murphy wrote:
> nittery and incognitoery
> 
> http://codereview.chromium.org/159871/diff/3010/2007
> File chrome/browser/views/frame/opaque_browser_frame_view.cc (right):
> 
> http://codereview.chromium.org/159871/diff/3010/2007#newcode570
> Line 570: if (frame_->GetWindow()->IsActive()) {
> Dialogs can be in incognito mode (incognito > popuptest)
> 
> http://codereview.chromium.org/159871/diff/3010/2009
> File views/window/non_client_view.h (right):
> 
> http://codereview.chromium.org/159871/diff/3010/2009#newcode203
> Line 203: void ForceGlassFrame();
> nit: AeroGlass (to match the aero_glass_frame class name).
> 
> http://codereview.chromium.org/159871/diff/3010/2009#newcode229
> Line 229: bool force_glass_frame_;
> aero_glass

[Miranda Callahan, 2009-08-07 01:11:33]

http://codereview.chromium.org/159871/diff/3010/2007
File chrome/browser/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/159871/diff/3010/2007#newcode570
Line 570: if (frame_->GetWindow()->IsActive()) {
On 2009/08/06 23:17:28, Glen Murphy wrote:
> Dialogs can be in incognito mode (incognito > popuptest)

Done.

http://codereview.chromium.org/159871/diff/3010/2009
File views/window/non_client_view.h (right):

http://codereview.chromium.org/159871/diff/3010/2009#newcode203
Line 203: void ForceGlassFrame();
On 2009/08/06 23:17:28, Glen Murphy wrote:
> nit: AeroGlass (to match the aero_glass_frame class name).

Done.

http://codereview.chromium.org/159871/diff/3010/2009#newcode229
Line 229: bool force_glass_frame_;
On 2009/08/06 23:17:28, Glen Murphy wrote:
> aero_glass

Done.

[Ben Goodger (Google), 2009-08-07 02:46:03]

Really? I don't think we should do this.

I've been using the app frames with themes and they look perfectly fine.

-Ben

On Tue, Aug 4, 2009 at 2:04 PM, <mirandac@chromium.org> wrote:
> Reviewers: Glen Murphy,
>
> Description:
> Ensure that popups and app windows are not themed.
>
> BUG=3D http://crbug.com/18093
> TEST=3D While running Chrome with a theme installed, force a popup, or
> create an application shortcut. =A0Note that these windows are not themed=
.
>
>
> Please review this at http://codereview.chromium.org/159871
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
> =A0M =A0 =A0 chrome/browser/views/frame/browser_frame_win.cc
> =A0M =A0 =A0 views/window/non_client_view.h
> =A0M =A0 =A0 views/window/non_client_view.cc
>
>
> Index: views/window/non_client_view.cc
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- views/window/non_client_view.cc =A0 =A0 (revision 22330)
> +++ views/window/non_client_view.cc =A0 =A0 (working copy)
> @@ -32,7 +32,8 @@
>
> =A0NonClientView::NonClientView(Window* frame)
> =A0 =A0 : frame_(frame),
> - =A0 =A0 =A0client_view_(NULL) {
> + =A0 =A0 =A0client_view_(NULL),
> + =A0 =A0 =A0use_native_theme_(false) {
> =A0}
>
> =A0NonClientView::~NonClientView() {
> @@ -68,6 +69,9 @@
> =A0}
>
> =A0bool NonClientView::UseNativeFrame() const {
> + =A0// Never use a custom theme if we are in an app or popup.
> + =A0if (use_native_theme_)
> + =A0 =A0return true;
> =A0 // The frame view may always require a custom frame, e.g. Constrained
> Windows.
> =A0 if (frame_view_.get() && frame_view_->AlwaysUseCustomFrame())
> =A0 =A0 return false;
> @@ -201,6 +205,10 @@
> =A0 accessible_name_ =3D name;
> =A0}
>
> +void NonClientView::ForceNativeTheme() {
> + =A0use_native_theme_ =3D true;
> +}
> +
> =A0//////////////////////////////////////////////////////////////////////=
//////////
> =A0// NonClientFrameView, View overrides:
>
> Index: views/window/non_client_view.h
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- views/window/non_client_view.h =A0 =A0 =A0(revision 22330)
> +++ views/window/non_client_view.h =A0 =A0 =A0(working copy)
> @@ -198,6 +198,10 @@
> =A0 virtual bool GetAccessibleName(std::wstring* name);
> =A0 virtual void SetAccessibleName(const std::wstring& name);
>
> + =A0// Set if the nonclientview is in an app or popup, to prevent themin=
g of
> + =A0// these windows.
> + =A0void ForceNativeTheme();
> +
> =A0protected:
> =A0 // NonClientView, View overrides:
> =A0 virtual void ViewHierarchyChanged(bool is_add, View* parent, View* ch=
ild);
> @@ -220,6 +224,10 @@
> =A0 // The accessible name of this view.
> =A0 std::wstring accessible_name_;
>
> + =A0// True when the NonClientView appears in an app or popup, which sho=
uld
> + =A0// never be themed.
> + =A0bool use_native_theme_;
> +
> =A0 DISALLOW_COPY_AND_ASSIGN(NonClientView);
> =A0};
>
> Index: chrome/browser/views/frame/browser_frame_win.cc
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- chrome/browser/views/frame/browser_frame_win.cc =A0 =A0 (revision 223=
30)
> +++ chrome/browser/views/frame/browser_frame_win.cc =A0 =A0 (working copy=
)
> @@ -49,6 +49,8 @@
> =A0 =A0 =A0 profile_(profile) {
> =A0 browser_view_->set_frame(this);
> =A0 GetNonClientView()->SetFrameView(CreateFrameViewForWindow());
> + =A0if (!browser_view_->IsBrowserTypeNormal())
> + =A0 =A0GetNonClientView()->ForceNativeTheme();
> =A0 // Don't focus anything on creation, selecting a tab will set the foc=
us.
> =A0 set_focus_on_creation(false);
> =A0}
> @@ -298,7 +300,8 @@
> =A0}
>
> =A0views::NonClientFrameView* BrowserFrameWin::CreateFrameViewForWindow()=
 {
> - =A0if (GetThemeProvider()->ShouldUseNativeFrame())
> + =A0if (!browser_view_->IsBrowserTypeNormal() ||
> + =A0 =A0 =A0GetThemeProvider()->ShouldUseNativeFrame())
> =A0 =A0 browser_frame_view_ =3D new GlassBrowserFrameView(this, browser_v=
iew_);
> =A0 else
> =A0 =A0 browser_frame_view_ =3D new OpaqueBrowserFrameView(this, browser_=
view_);
>
>
>

[Glen Murphy, 2009-08-07 03:36:42]

When we discussed this we said that the browser is the only thing that
should get themed; at the time it was because Themes aren't designed
to have text on top of their frame images, and app windows are
technically meant to be conceptually separate from the browser.


On Thu, Aug 6, 2009 at 7:45 PM, Ben Goodger (Google)<ben@chromium.org> wrot=
e:
> Really? I don't think we should do this.
>
> I've been using the app frames with themes and they look perfectly fine.
>
> -Ben
>
> On Tue, Aug 4, 2009 at 2:04 PM, <mirandac@chromium.org> wrote:
>> Reviewers: Glen Murphy,
>>
>> Description:
>> Ensure that popups and app windows are not themed.
>>
>> BUG=3D http://crbug.com/18093
>> TEST=3D While running Chrome with a theme installed, force a popup, or
>> create an application shortcut. =A0Note that these windows are not theme=
d.
>>
>>
>> Please review this at http://codereview.chromium.org/159871
>>
>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>
>> Affected files:
>> =A0M =A0 =A0 chrome/browser/views/frame/browser_frame_win.cc
>> =A0M =A0 =A0 views/window/non_client_view.h
>> =A0M =A0 =A0 views/window/non_client_view.cc
>>
>>
>> Index: views/window/non_client_view.cc
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> --- views/window/non_client_view.cc =A0 =A0 (revision 22330)
>> +++ views/window/non_client_view.cc =A0 =A0 (working copy)
>> @@ -32,7 +32,8 @@
>>
>> =A0NonClientView::NonClientView(Window* frame)
>> =A0 =A0 : frame_(frame),
>> - =A0 =A0 =A0client_view_(NULL) {
>> + =A0 =A0 =A0client_view_(NULL),
>> + =A0 =A0 =A0use_native_theme_(false) {
>> =A0}
>>
>> =A0NonClientView::~NonClientView() {
>> @@ -68,6 +69,9 @@
>> =A0}
>>
>> =A0bool NonClientView::UseNativeFrame() const {
>> + =A0// Never use a custom theme if we are in an app or popup.
>> + =A0if (use_native_theme_)
>> + =A0 =A0return true;
>> =A0 // The frame view may always require a custom frame, e.g. Constraine=
d
>> Windows.
>> =A0 if (frame_view_.get() && frame_view_->AlwaysUseCustomFrame())
>> =A0 =A0 return false;
>> @@ -201,6 +205,10 @@
>> =A0 accessible_name_ =3D name;
>> =A0}
>>
>> +void NonClientView::ForceNativeTheme() {
>> + =A0use_native_theme_ =3D true;
>> +}
>> +
>> =A0/////////////////////////////////////////////////////////////////////=
///////////
>> =A0// NonClientFrameView, View overrides:
>>
>> Index: views/window/non_client_view.h
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> --- views/window/non_client_view.h =A0 =A0 =A0(revision 22330)
>> +++ views/window/non_client_view.h =A0 =A0 =A0(working copy)
>> @@ -198,6 +198,10 @@
>> =A0 virtual bool GetAccessibleName(std::wstring* name);
>> =A0 virtual void SetAccessibleName(const std::wstring& name);
>>
>> + =A0// Set if the nonclientview is in an app or popup, to prevent themi=
ng of
>> + =A0// these windows.
>> + =A0void ForceNativeTheme();
>> +
>> =A0protected:
>> =A0 // NonClientView, View overrides:
>> =A0 virtual void ViewHierarchyChanged(bool is_add, View* parent, View* c=
hild);
>> @@ -220,6 +224,10 @@
>> =A0 // The accessible name of this view.
>> =A0 std::wstring accessible_name_;
>>
>> + =A0// True when the NonClientView appears in an app or popup, which sh=
ould
>> + =A0// never be themed.
>> + =A0bool use_native_theme_;
>> +
>> =A0 DISALLOW_COPY_AND_ASSIGN(NonClientView);
>> =A0};
>>
>> Index: chrome/browser/views/frame/browser_frame_win.cc
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> --- chrome/browser/views/frame/browser_frame_win.cc =A0 =A0 (revision 22=
330)
>> +++ chrome/browser/views/frame/browser_frame_win.cc =A0 =A0 (working cop=
y)
>> @@ -49,6 +49,8 @@
>> =A0 =A0 =A0 profile_(profile) {
>> =A0 browser_view_->set_frame(this);
>> =A0 GetNonClientView()->SetFrameView(CreateFrameViewForWindow());
>> + =A0if (!browser_view_->IsBrowserTypeNormal())
>> + =A0 =A0GetNonClientView()->ForceNativeTheme();
>> =A0 // Don't focus anything on creation, selecting a tab will set the fo=
cus.
>> =A0 set_focus_on_creation(false);
>> =A0}
>> @@ -298,7 +300,8 @@
>> =A0}
>>
>> =A0views::NonClientFrameView* BrowserFrameWin::CreateFrameViewForWindow(=
) {
>> - =A0if (GetThemeProvider()->ShouldUseNativeFrame())
>> + =A0if (!browser_view_->IsBrowserTypeNormal() ||
>> + =A0 =A0 =A0GetThemeProvider()->ShouldUseNativeFrame())
>> =A0 =A0 browser_frame_view_ =3D new GlassBrowserFrameView(this, browser_=
view_);
>> =A0 else
>> =A0 =A0 browser_frame_view_ =3D new OpaqueBrowserFrameView(this, browser=
_view_);
>>
>>
>>
>

[Miranda Callahan, 2009-08-07 15:40:27]

What about using the tab color and text color for the frame and text of a popup?
 Conceptually, a popup is really more like an escaped tab than a browser, and
this would ensure the title is readable.

On 2009/08/07 03:36:42, Glen Murphy wrote:
> When we discussed this we said that the browser is the only thing that
> should get themed; at the time it was because Themes aren't designed
> to have text on top of their frame images, and app windows are
> technically meant to be conceptually separate from the browser.
> 
> 
> On Thu, Aug 6, 2009 at 7:45 PM, Ben Goodger (Google)<ben@chromium.org> wrot=
> e:
> > Really? I don't think we should do this.
> >
> > I've been using the app frames with themes and they look perfectly fine.
> >
> > -Ben
> >
> > On Tue, Aug 4, 2009 at 2:04 PM, <mirandac@chromium.org> wrote:
> >> Reviewers: Glen Murphy,
> >>
> >> Description:
> >> Ensure that popups and app windows are not themed.
> >>
> >> BUG=3D http://crbug.com/18093
> >> TEST=3D While running Chrome with a theme installed, force a popup, or
> >> create an application shortcut. =A0Note that these windows are not theme=
> d.
> >>
> >>
> >> Please review this at http://codereview.chromium.org/159871
> >>
> >> SVN Base: svn://chrome-svn/chrome/trunk/src/
> >>
> >> Affected files:
> >> =A0M =A0 =A0 chrome/browser/views/frame/browser_frame_win.cc
> >> =A0M =A0 =A0 views/window/non_client_view.h
> >> =A0M =A0 =A0 views/window/non_client_view.cc
> >>
> >>
> >> Index: views/window/non_client_view.cc
> >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> >> --- views/window/non_client_view.cc =A0 =A0 (revision 22330)
> >> +++ views/window/non_client_view.cc =A0 =A0 (working copy)
> >> @@ -32,7 +32,8 @@
> >>
> >> =A0NonClientView::NonClientView(Window* frame)
> >> =A0 =A0 : frame_(frame),
> >> - =A0 =A0 =A0client_view_(NULL) {
> >> + =A0 =A0 =A0client_view_(NULL),
> >> + =A0 =A0 =A0use_native_theme_(false) {
> >> =A0}
> >>
> >> =A0NonClientView::~NonClientView() {
> >> @@ -68,6 +69,9 @@
> >> =A0}
> >>
> >> =A0bool NonClientView::UseNativeFrame() const {
> >> + =A0// Never use a custom theme if we are in an app or popup.
> >> + =A0if (use_native_theme_)
> >> + =A0 =A0return true;
> >> =A0 // The frame view may always require a custom frame, e.g. Constraine=
> d
> >> Windows.
> >> =A0 if (frame_view_.get() && frame_view_->AlwaysUseCustomFrame())
> >> =A0 =A0 return false;
> >> @@ -201,6 +205,10 @@
> >> =A0 accessible_name_ =3D name;
> >> =A0}
> >>
> >> +void NonClientView::ForceNativeTheme() {
> >> + =A0use_native_theme_ =3D true;
> >> +}
> >> +
> >> =A0/////////////////////////////////////////////////////////////////////=
> ///////////
> >> =A0// NonClientFrameView, View overrides:
> >>
> >> Index: views/window/non_client_view.h
> >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> >> --- views/window/non_client_view.h =A0 =A0 =A0(revision 22330)
> >> +++ views/window/non_client_view.h =A0 =A0 =A0(working copy)
> >> @@ -198,6 +198,10 @@
> >> =A0 virtual bool GetAccessibleName(std::wstring* name);
> >> =A0 virtual void SetAccessibleName(const std::wstring& name);
> >>
> >> + =A0// Set if the nonclientview is in an app or popup, to prevent themi=
> ng of
> >> + =A0// these windows.
> >> + =A0void ForceNativeTheme();
> >> +
> >> =A0protected:
> >> =A0 // NonClientView, View overrides:
> >> =A0 virtual void ViewHierarchyChanged(bool is_add, View* parent, View* c=
> hild);
> >> @@ -220,6 +224,10 @@
> >> =A0 // The accessible name of this view.
> >> =A0 std::wstring accessible_name_;
> >>
> >> + =A0// True when the NonClientView appears in an app or popup, which sh=
> ould
> >> + =A0// never be themed.
> >> + =A0bool use_native_theme_;
> >> +
> >> =A0 DISALLOW_COPY_AND_ASSIGN(NonClientView);
> >> =A0};
> >>
> >> Index: chrome/browser/views/frame/browser_frame_win.cc
> >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> >> --- chrome/browser/views/frame/browser_frame_win.cc =A0 =A0 (revision 22=
> 330)
> >> +++ chrome/browser/views/frame/browser_frame_win.cc =A0 =A0 (working cop=
> y)
> >> @@ -49,6 +49,8 @@
> >> =A0 =A0 =A0 profile_(profile) {
> >> =A0 browser_view_->set_frame(this);
> >> =A0 GetNonClientView()->SetFrameView(CreateFrameViewForWindow());
> >> + =A0if (!browser_view_->IsBrowserTypeNormal())
> >> + =A0 =A0GetNonClientView()->ForceNativeTheme();
> >> =A0 // Don't focus anything on creation, selecting a tab will set the fo=
> cus.
> >> =A0 set_focus_on_creation(false);
> >> =A0}
> >> @@ -298,7 +300,8 @@
> >> =A0}
> >>
> >> =A0views::NonClientFrameView* BrowserFrameWin::CreateFrameViewForWindow(=
> ) {
> >> - =A0if (GetThemeProvider()->ShouldUseNativeFrame())
> >> + =A0if (!browser_view_->IsBrowserTypeNormal() ||
> >> + =A0 =A0 =A0GetThemeProvider()->ShouldUseNativeFrame())
> >> =A0 =A0 browser_frame_view_ =3D new GlassBrowserFrameView(this, browser_=
> view_);
> >> =A0 else
> >> =A0 =A0 browser_frame_view_ =3D new OpaqueBrowserFrameView(this, browser=
> _view_);
> >>
> >>
> >>
> >

[Ben Goodger (Google), 2009-08-07 17:03:24]

It's not perfect, but not perfect in the same way the existing theme
set is not perfect wrt readability of tab titles. Honestly, in my
usage over the past few weeks with a variety of themes, it's not
struck me as more of a problem. It seems possible to address.

-Ben

On Thu, Aug 6, 2009 at 8:36 PM, Glen Murphy<glen@chromium.org> wrote:
> When we discussed this we said that the browser is the only thing that
> should get themed; at the time it was because Themes aren't designed
> to have text on top of their frame images, and app windows are
> technically meant to be conceptually separate from the browser.
>
>
> On Thu, Aug 6, 2009 at 7:45 PM, Ben Goodger (Google)<ben@chromium.org> wr=
ote:
>> Really? I don't think we should do this.
>>
>> I've been using the app frames with themes and they look perfectly fine.
>>
>> -Ben
>>
>> On Tue, Aug 4, 2009 at 2:04 PM, <mirandac@chromium.org> wrote:
>>> Reviewers: Glen Murphy,
>>>
>>> Description:
>>> Ensure that popups and app windows are not themed.
>>>
>>> BUG=3D http://crbug.com/18093
>>> TEST=3D While running Chrome with a theme installed, force a popup, or
>>> create an application shortcut. =A0Note that these windows are not them=
ed.
>>>
>>>
>>> Please review this at http://codereview.chromium.org/159871
>>>
>>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>>
>>> Affected files:
>>> =A0M =A0 =A0 chrome/browser/views/frame/browser_frame_win.cc
>>> =A0M =A0 =A0 views/window/non_client_view.h
>>> =A0M =A0 =A0 views/window/non_client_view.cc
>>>
>>>
>>> Index: views/window/non_client_view.cc
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>> --- views/window/non_client_view.cc =A0 =A0 (revision 22330)
>>> +++ views/window/non_client_view.cc =A0 =A0 (working copy)
>>> @@ -32,7 +32,8 @@
>>>
>>> =A0NonClientView::NonClientView(Window* frame)
>>> =A0 =A0 : frame_(frame),
>>> - =A0 =A0 =A0client_view_(NULL) {
>>> + =A0 =A0 =A0client_view_(NULL),
>>> + =A0 =A0 =A0use_native_theme_(false) {
>>> =A0}
>>>
>>> =A0NonClientView::~NonClientView() {
>>> @@ -68,6 +69,9 @@
>>> =A0}
>>>
>>> =A0bool NonClientView::UseNativeFrame() const {
>>> + =A0// Never use a custom theme if we are in an app or popup.
>>> + =A0if (use_native_theme_)
>>> + =A0 =A0return true;
>>> =A0 // The frame view may always require a custom frame, e.g. Constrain=
ed
>>> Windows.
>>> =A0 if (frame_view_.get() && frame_view_->AlwaysUseCustomFrame())
>>> =A0 =A0 return false;
>>> @@ -201,6 +205,10 @@
>>> =A0 accessible_name_ =3D name;
>>> =A0}
>>>
>>> +void NonClientView::ForceNativeTheme() {
>>> + =A0use_native_theme_ =3D true;
>>> +}
>>> +
>>> =A0////////////////////////////////////////////////////////////////////=
////////////
>>> =A0// NonClientFrameView, View overrides:
>>>
>>> Index: views/window/non_client_view.h
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>> --- views/window/non_client_view.h =A0 =A0 =A0(revision 22330)
>>> +++ views/window/non_client_view.h =A0 =A0 =A0(working copy)
>>> @@ -198,6 +198,10 @@
>>> =A0 virtual bool GetAccessibleName(std::wstring* name);
>>> =A0 virtual void SetAccessibleName(const std::wstring& name);
>>>
>>> + =A0// Set if the nonclientview is in an app or popup, to prevent them=
ing of
>>> + =A0// these windows.
>>> + =A0void ForceNativeTheme();
>>> +
>>> =A0protected:
>>> =A0 // NonClientView, View overrides:
>>> =A0 virtual void ViewHierarchyChanged(bool is_add, View* parent, View* =
child);
>>> @@ -220,6 +224,10 @@
>>> =A0 // The accessible name of this view.
>>> =A0 std::wstring accessible_name_;
>>>
>>> + =A0// True when the NonClientView appears in an app or popup, which s=
hould
>>> + =A0// never be themed.
>>> + =A0bool use_native_theme_;
>>> +
>>> =A0 DISALLOW_COPY_AND_ASSIGN(NonClientView);
>>> =A0};
>>>
>>> Index: chrome/browser/views/frame/browser_frame_win.cc
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>> --- chrome/browser/views/frame/browser_frame_win.cc =A0 =A0 (revision 2=
2330)
>>> +++ chrome/browser/views/frame/browser_frame_win.cc =A0 =A0 (working co=
py)
>>> @@ -49,6 +49,8 @@
>>> =A0 =A0 =A0 profile_(profile) {
>>> =A0 browser_view_->set_frame(this);
>>> =A0 GetNonClientView()->SetFrameView(CreateFrameViewForWindow());
>>> + =A0if (!browser_view_->IsBrowserTypeNormal())
>>> + =A0 =A0GetNonClientView()->ForceNativeTheme();
>>> =A0 // Don't focus anything on creation, selecting a tab will set the f=
ocus.
>>> =A0 set_focus_on_creation(false);
>>> =A0}
>>> @@ -298,7 +300,8 @@
>>> =A0}
>>>
>>> =A0views::NonClientFrameView* BrowserFrameWin::CreateFrameViewForWindow=
() {
>>> - =A0if (GetThemeProvider()->ShouldUseNativeFrame())
>>> + =A0if (!browser_view_->IsBrowserTypeNormal() ||
>>> + =A0 =A0 =A0GetThemeProvider()->ShouldUseNativeFrame())
>>> =A0 =A0 browser_frame_view_ =3D new GlassBrowserFrameView(this, browser=
_view_);
>>> =A0 else
>>> =A0 =A0 browser_frame_view_ =3D new OpaqueBrowserFrameView(this, browse=
r_view_);
>>>
>>>
>>>
>>
>

[Glen Murphy, 2009-08-07 19:39:49]

LGTM, with one nit.

http://codereview.chromium.org/159871/diff/3017/2014
File chrome/browser/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/159871/diff/3017/2014#newcode681
Line 681: theme_frame = rb.GetBitmapNamed(IDR_FRAME);
nit: no braces for single-lines

[tony, 2009-08-07 21:09:33]

Please keep me in the loop on this discussion.  FWIW, I agree with Ben and would
rather have hard to read text on app mode/popup windows until a better way to
theme it is thought up.