Ensure that inactive tab images line up correctly with frame image when theme...

Ensure that inactive tab images line up correctly with frame image when themed.  Also fix HasCustomImage so that it only returns true when the image is in the extension's original set, not just a generated theme for the cache.

BUG= http://crbug.com/21134
TEST= install a theme with a detailed frame background.  Open many tabs.  Make sure the inactive tab image lines up with the frame image.


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

[Elliot Glaysher, 2009-09-04 23:35:58]

LGTM

On Fri, Sep 4, 2009 at 4:33 PM, <mirandac@chromium.org> wrote:
> Reviewers: glen,
>
> Description:
> Ensure that inactive tab images line up correctly with frame image when
> themed.
>
> BUG=3D http://crbug.com/21134
> TEST=3D install a theme with a detailed frame background. =A0Open many ta=
bs.
> =A0Make sure the inactive tab image lines up with the frame image.
>
>
> Please review this at http://codereview.chromium.org/193030
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
> =A0M =A0 =A0 chrome/browser/views/tabs/tab_renderer.cc
>
>
> Index: chrome/browser/views/tabs/tab_renderer.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/tabs/tab_renderer.cc =A0 (revision 25458)
> +++ chrome/browser/views/tabs/tab_renderer.cc =A0 (working copy)
> @@ -41,6 +41,10 @@
> =A0static const int kFaviconSize =3D 16;
> =A0static const int kSelectedTitleColor =3D SK_ColorBLACK;
>
> +// How far up to push the tab background inactive image so it lines up w=
ith
> +// the frame image.
> +static const int kTabFrameOffset =3D 16;
> +
> =A0// How long the hover state takes.
> =A0static const int kHoverDurationMs =3D 90;
>
> @@ -612,7 +616,8 @@
> =A0 // should be at the top of the tab. Otherwise, we assume that the
> background
> =A0 // image is a composited foreground + frame image.
> =A0 int bg_offset_y =3D GetThemeProvider()->HasCustomImage(tab_id) ?
> - =A0 =A0 =A00 : background_offset_.y();
> + =A0 =A0 =A0(GetWidget()->GetWindow()->IsMaximized() ? 0 : kTabFrameOffs=
et) :
> + =A0 =A0 =A0background_offset_.y();
>
> =A0 // Draw left edge. =A0Don't draw over the toolbar, as we're not the
> foreground
> =A0 // tab.
>
>
>

[Miranda Callahan, 2009-09-04 23:53:39]

Whoops, this is a bit trickier than I had thought; I need to change BTP a little
bit to make this work properly.  Will upload new change soon.

On 2009/09/04 23:35:58, Elliot Glaysher wrote:
> LGTM
> 
> On Fri, Sep 4, 2009 at 4:33 PM, <mailto:mirandac@chromium.org> wrote:
> > Reviewers: glen,
> >
> > Description:
> > Ensure that inactive tab images line up correctly with frame image when
> > themed.
> >
> > BUG=3D http://crbug.com/21134
> > TEST=3D install a theme with a detailed frame background. =A0Open many ta=
> bs.
> > =A0Make sure the inactive tab image lines up with the frame image.
> >
> >
> > Please review this at http://codereview.chromium.org/193030
> >
> > SVN Base: svn://chrome-svn/chrome/trunk/src/
> >
> > Affected files:
> > =A0M =A0 =A0 chrome/browser/views/tabs/tab_renderer.cc
> >
> >
> > Index: chrome/browser/views/tabs/tab_renderer.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/tabs/tab_renderer.cc =A0 (revision 25458)
> > +++ chrome/browser/views/tabs/tab_renderer.cc =A0 (working copy)
> > @@ -41,6 +41,10 @@
> > =A0static const int kFaviconSize =3D 16;
> > =A0static const int kSelectedTitleColor =3D SK_ColorBLACK;
> >
> > +// How far up to push the tab background inactive image so it lines up w=
> ith
> > +// the frame image.
> > +static const int kTabFrameOffset =3D 16;
> > +
> > =A0// How long the hover state takes.
> > =A0static const int kHoverDurationMs =3D 90;
> >
> > @@ -612,7 +616,8 @@
> > =A0 // should be at the top of the tab. Otherwise, we assume that the
> > background
> > =A0 // image is a composited foreground + frame image.
> > =A0 int bg_offset_y =3D GetThemeProvider()->HasCustomImage(tab_id) ?
> > - =A0 =A0 =A00 : background_offset_.y();
> > + =A0 =A0 =A0(GetWidget()->GetWindow()->IsMaximized() ? 0 : kTabFrameOffs=
> et) :
> > + =A0 =A0 =A0background_offset_.y();
> >
> > =A0 // Draw left edge. =A0Don't draw over the toolbar, as we're not the
> > foreground
> > =A0 // tab.
> >
> >
> >

[Miranda Callahan, 2009-09-08 23:38:01]

This isn't nice; there is no path from the BrowserThemeProvider to the data
owned by the OpaqueBrowserFrameView that is pleasant or structurally
intelligent.  The data it needs to get from the OBFV is hardcoded -- so I simply
added the hardcoded data to the BTP itself. :-(  

Other than that -- this makes all the frames look nice, and should fix Linux as
well (though the hardcoded value may need to be changed ... ?)

[Miranda Callahan, 2009-09-09 22:45:32]

ping!

[Miranda Callahan, 2009-09-09 23:08:23]

Glen, Elliot reviewed this before, but I think you should take a look at the
revised code, since it involves a more in-depth change to BTP.  

On 2009/09/09 22:45:32, Miranda Callahan wrote:
> ping!

[Glen Murphy, 2009-09-09 23:10:11]

LG, with comments.

http://codereview.chromium.org/193030/diff/2004/1005
File chrome/browser/browser_theme_provider.cc (right):

http://codereview.chromium.org/193030/diff/2004/1005#newcode632
Line 632: if (images_.find(id) != images_.end())
Can you add a comment describing this mechanism for differentiating original and
generated images, and how HasCustomImage deals with this.