Theme support for the extension shelf

ExtensionShelf now uses the BookmarkExtensionBackground, just like the BookmarkBarView.

Changed the WebKit API to add an optional |id| parameter to the insertStyleText, which is needed to be able to replace style sheets that have been previously added.
Added an interface that both BookmarkBarView and ExtensionShelf implement.
This new interface tells us whether we are located at the top or at the bottom and whether we are detached from the frame or not.
Factored out some of the duplicate painting-related code to a namespace of its own. Not happy with the name (welcome suggestions).
Moved the check for whether extensions are on top to new class and now cache the value for the lifetime of the process.
Toolstrip text color values are no longer hard-coded but use the color specified in the theme.
Decreased the timeouts for showing and hiding the toolstrip handle.
Replaced the pressed background image and the hover background image for the toolstrip to match what the bookmark bar uses.

Known issues:

Some themes expose the fact that:
- The background for the extension shelf when in detached mode (and located on the bottom) does not seamlessly blend in with background of new tab page. Still works surprisingly well when it breaks, though.
- Didn't spend much time theming the shelf handle (just used the solid color from the theme).

BUG=18452, 21272, 21273
TEST=Install a theme for Chrome and make sure everything looks correct and is updated on a theme change. Also make sure painting problems in bugs 21272 and 21273 are fixed.

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

[Erik does not do reviews, 2009-09-11 20:44:16]

http://codereview.chromium.org/203034/diff/2005/2012
File chrome/browser/resources/extensions_toolstrip.css (right):

http://codereview.chromium.org/203034/diff/2005/2012#newcode89
Line 89:
-webkit-border-image:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAA4AAAAaCAYAAACHD21cAAAAAXNSR0IArs4c6QAAAAZiS0dEAPYA+QD9Ry7YfAAAAAlwSFlzAAALEwAACxMBAJqcGAAAAAd0SU1FB9kJChUuJy+ouI4AAAFXSURBVDjL7dSxTsJQFIDh/95SmqpEYjASAgPGGI26wGv4BMbBVUZewBQfRBxAZ0cXG2d8AANoFEJgAGOAkqK31wEGJ23cTPiTs53vjEcAYj5RYAmwgAjQYlYG+AR8wAOmgAaQwLLjlHL1RrM4HI0rKtA1z1c1z1c1FejacDSu1BvNouOUcsAyIA3AcpzS7mmhcJxKpQ4Nw0yoQM9OAirQSCOSiMfjB3t7+6ux2ErHdd2BANbqjeZJNps9UsFs8adeX56rO9tbZQlYyWQyH4RAAIn1jTxgScCwbXsz0L8jANu2NwEjArSnn9okZEIIE2hL/tgCLuB/guloRHyEBVrrDyAtATWZTJ7CwvmukoDf7XYfpAgHe73uA+BLwKtWKu7bYHD/Gxr0+/fXV1UX8AwgcN2791hspZPJZIZm1Iqappn4DjzPe2y3W7eX5Yubc+fsEfDFXx/yF0HjkTNryckZAAAAAElFTkSuQmCC)
6 round round;
when I looked at this with you last time, the image was a bit sketchy.  Did you
update this?

http://codereview.chromium.org/203034/diff/2005/2007
File chrome/browser/views/detachable_toolbar_view.cc (right):

http://codereview.chromium.org/203034/diff/2005/2007#newcode22
Line 22: // Draw the background to match the new tab page.
nit: swap comment with previous line or add a newline above the comment.

http://codereview.chromium.org/203034/diff/2005/2007#newcode253
Line 253: }
nit: add // namespace DetachableToolbar

http://codereview.chromium.org/203034/diff/2005/2006
File chrome/browser/views/detachable_toolbar_view.h (right):

http://codereview.chromium.org/203034/diff/2005/2006#newcode21
Line 21: virtual bool IsDetachedStyle() const = 0;
"Style" on the end seems weird.  From the comment, I'd just call this
IsDetached() (or from the methods below, you could use IsDetachedMode(), but I
prefer the shorter one)

http://codereview.chromium.org/203034/diff/2005/2006#newcode27
Line 27: virtual double GetSizeAnimationValue() const = 0;
Is this really a "size" animation?  Maybe GetDetachAnimationValue()?

http://codereview.chromium.org/203034/diff/2005/2006#newcode33
Line 33: namespace DetachableToolbar {
Why not make these methods on DetachableToolbarView? (some static obviously) It
seems odd to have a separate namespace.

http://codereview.chromium.org/203034/diff/2005/2008
File chrome/browser/views/extensions/extension_shelf.cc (right):

http://codereview.chromium.org/203034/diff/2005/2008#newcode512
Line 512: title_->SetColor(
do we need to listen for theme changed and update this as well?

http://codereview.chromium.org/203034/diff/2005/2009
File chrome/browser/views/extensions/extension_shelf.h (right):

http://codereview.chromium.org/203034/diff/2005/2009#newcode102
Line 102: void InitBackground(gfx::Canvas* canvas, const SkRect& subset, bool
detached);
why do you need the bool here?  can't you just use IsDetached?

http://codereview.chromium.org/203034/diff/2005/2019
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/203034/diff/2005/2019#newcode718
Line 718: WebCore::AtomicString id("__DynamicallyInsertedStylesheet__");
Since this is a generic method, I think we should pass the id in as an argument.
 Then in theory we could support multiple independent style sheets being
inserted.

[Finnur, 2009-09-14 17:35:02]

Updated. Please take another look.

http://codereview.chromium.org/203034/diff/2005/2012
File chrome/browser/resources/extensions_toolstrip.css (right):

http://codereview.chromium.org/203034/diff/2005/2012#newcode89
Line 89:
-webkit-border-image:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAA4AAAAaCAYAAACHD21cAAAAAXNSR0IArs4c6QAAAAZiS0dEAPYA+QD9Ry7YfAAAAAlwSFlzAAALEwAACxMBAJqcGAAAAAd0SU1FB9kJChUuJy+ouI4AAAFXSURBVDjL7dSxTsJQFIDh/95SmqpEYjASAgPGGI26wGv4BMbBVUZewBQfRBxAZ0cXG2d8AANoFEJgAGOAkqK31wEGJ23cTPiTs53vjEcAYj5RYAmwgAjQYlYG+AR8wAOmgAaQwLLjlHL1RrM4HI0rKtA1z1c1z1c1FejacDSu1BvNouOUcsAyIA3AcpzS7mmhcJxKpQ4Nw0yoQM9OAirQSCOSiMfjB3t7+6ux2ErHdd2BANbqjeZJNps9UsFs8adeX56rO9tbZQlYyWQyH4RAAIn1jTxgScCwbXsz0L8jANu2NwEjArSnn9okZEIIE2hL/tgCLuB/guloRHyEBVrrDyAtATWZTJ7CwvmukoDf7XYfpAgHe73uA+BLwKtWKu7bYHD/Gxr0+/fXV1UX8AwgcN2791hspZPJZIZm1Iqappn4DjzPe2y3W7eX5Yubc+fsEfDFXx/yF0HjkTNryckZAAAAAElFTkSuQmCC)
6 round round;
Yes. I constructed this from the bitmaps for the bookmarks bar, so now they
should look the same.

On 2009/09/11 20:44:17, Erik Kay wrote:
> when I looked at this with you last time, the image was a bit sketchy.  Did
you
> update this?

http://codereview.chromium.org/203034/diff/2005/2007
File chrome/browser/views/detachable_toolbar_view.cc (right):

http://codereview.chromium.org/203034/diff/2005/2007#newcode22
Line 22: // Draw the background to match the new tab page.
On 2009/09/11 20:44:17, Erik Kay wrote:
> nit: swap comment with previous line or add a newline above the comment.

Done.

http://codereview.chromium.org/203034/diff/2005/2007#newcode253
Line 253: }
Removed namespace, as suggested.

On 2009/09/11 20:44:17, Erik Kay wrote:
> nit: add // namespace DetachableToolbar

http://codereview.chromium.org/203034/diff/2005/2006
File chrome/browser/views/detachable_toolbar_view.h (right):

http://codereview.chromium.org/203034/diff/2005/2006#newcode21
Line 21: virtual bool IsDetachedStyle() const = 0;
Agreed. Done.


On 2009/09/11 20:44:17, Erik Kay wrote:
> "Style" on the end seems weird.  From the comment, I'd just call this
> IsDetached() (or from the methods below, you could use IsDetachedMode(), but I
> prefer the shorter one)

http://codereview.chromium.org/203034/diff/2005/2006#newcode27
Line 27: virtual double GetSizeAnimationValue() const = 0;
It is for attaching/detaching and appearing/disappearing, depending on what
state you are in and what page you are on (ntp or not). I have shortened it to
just GetAnimationValue.


On 2009/09/11 20:44:17, Erik Kay wrote:
> Is this really a "size" animation?  Maybe GetDetachAnimationValue()?

http://codereview.chromium.org/203034/diff/2005/2006#newcode33
Line 33: namespace DetachableToolbar {
Yeah, you are right. Done.


On 2009/09/11 20:44:17, Erik Kay wrote:
> Why not make these methods on DetachableToolbarView? (some static obviously)
It
> seems odd to have a separate namespace.

http://codereview.chromium.org/203034/diff/2005/2008
File chrome/browser/views/extensions/extension_shelf.cc (right):

http://codereview.chromium.org/203034/diff/2005/2008#newcode512
Line 512: title_->SetColor(
No, this is read from the theme provider every time we are about to show the
menu, so this value always gives you the current theme color for the text.


On 2009/09/11 20:44:17, Erik Kay wrote:
> do we need to listen for theme changed and update this as well?

http://codereview.chromium.org/203034/diff/2005/2009
File chrome/browser/views/extensions/extension_shelf.h (right):

http://codereview.chromium.org/203034/diff/2005/2009#newcode102
Line 102: void InitBackground(gfx::Canvas* canvas, const SkRect& subset, bool
detached);
Sure can.

On 2009/09/11 20:44:17, Erik Kay wrote:
> why do you need the bool here?  can't you just use IsDetached?

http://codereview.chromium.org/203034/diff/2005/2019
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/203034/diff/2005/2019#newcode718
Line 718: WebCore::AtomicString id("__DynamicallyInsertedStylesheet__");
Man, that small change introduces a lot of file creep to this changelist. :) It
is a good change, though. I was planning to do that as a follow-up CL so that I
could check in and not be blocked on the corresponding WebKit API change that
would be needed for that, but I really should just bite the bullet and do it
now.


On 2009/09/11 20:44:17, Erik Kay wrote:
> Since this is a generic method, I think we should pass the id in as an
argument.
>  Then in theory we could support multiple independent style sheets being
> inserted.

[Erik does not do reviews, 2009-09-14 17:52:22]

LGTM - Thanks for making the id change.  Definitely an improvement.

[darin (slow to review), 2009-09-14 17:52:41]

http://codereview.chromium.org/203034/diff/30/4015
File webkit/api/public/WebFrame.h (right):

http://codereview.chromium.org/203034/diff/30/4015#newcode221
Line 221: virtual bool insertStyleText(const WebString& css,
css -> styleText (to match the function name)
id -> elementId (to match the name used within WebKit for this)

http://codereview.chromium.org/203034/diff/30/4014
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/203034/diff/30/4014#newcode714
Line 714: if (!id.isEmpty() && !id.isNull()) {
the isNull check is redundant

http://codereview.chromium.org/203034/diff/30/4014#newcode715
Line 715: WebCore::Element* old_element = document->getElementById(id.data());
calling id.data() here is a bad idea.  id is a WebString, which can be cheaply
converted to a String, which can be cheaply converted to an AtomicString.  you
should do that instead.  just use glue_util::WebStringToString here.

http://codereview.chromium.org/203034/diff/30/4014#newcode726
Line 726: if (!id.isEmpty() && !id.isNull())
ditto

http://codereview.chromium.org/203034/diff/30/4016
File webkit/glue/webframe_impl.h (right):

http://codereview.chromium.org/203034/diff/30/4016#newcode110
Line 110: virtual bool insertStyleText(const WebKit::WebString& style_text,
nit: please use the same line wrapping style used elsewhere in this list of
WebFrame methods.

[Finnur, 2009-09-14 18:09:27]

Thanks for quick turnarounds!

Updated CL. I think I have addressed all comments by Darin.

http://codereview.chromium.org/203034/diff/30/4014
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/203034/diff/30/4014#newcode714
Line 714: if (!id.isEmpty() && !id.isNull()) {
Ah, I wasn't sure. I added it after seeing this done elsewhere.

On 2009/09/14 17:52:41, darin wrote:
> the isNull check is redundant

http://codereview.chromium.org/203034/diff/30/4014#newcode715
Line 715: WebCore::Element* old_element = document->getElementById(id.data());
Excellent point. Thanks!

On 2009/09/14 17:52:41, darin wrote:
> calling id.data() here is a bad idea.  id is a WebString, which can be cheaply
> converted to a String, which can be cheaply converted to an AtomicString.  you
> should do that instead.  just use glue_util::WebStringToString here.

[darin (slow to review), 2009-09-14 23:31:39]

webkit/* changes LGTM

[Aaron Boodman, 2009-09-17 19:42:38]

ExtensionVeiw changes lgtm

http://codereview.chromium.org/203034/diff/7001/7004
File chrome/browser/views/extensions/extension_shelf.cc (right):

http://codereview.chromium.org/203034/diff/7001/7004#newcode880
Line 880: if (!background_needs_repaint_ && background_for_detached_ ==
detached)
Seems like you could consolidate these two cases by listening for the detach
command and resetting the background_needs_repaint_ flag there.

[Finnur, 2009-09-17 22:23:07]

http://codereview.chromium.org/203034/diff/7001/7004
File chrome/browser/views/extensions/extension_shelf.cc (right):

http://codereview.chromium.org/203034/diff/7001/7004#newcode880
Line 880: if (!background_needs_repaint_ && background_for_detached_ ==
detached)
Yeah, in my upcoming changelist this flag (background_for_detached_) is no
longer there.

On 2009/09/17 19:42:38, Aaron Boodman wrote:
> Seems like you could consolidate these two cases by listening for the detach
> command and resetting the background_needs_repaint_ flag there.