GTK Themes: Set the text color of bookmark buttons to theme color.

GTK Themes: Set the text color of bookmark buttons to theme color.

http://crbug.com/13967

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

[Evan Stade, 2009-07-06 21:57:35]

http://codereview.chromium.org/155108/diff/1002/31
File chrome/browser/gtk/bookmark_utils_gtk.cc (right):

http://codereview.chromium.org/155108/diff/1002/31#newcode37
Line 37: // Recursively search for label among the children of |widget|.
can we please not reintroduce this function? Why can't we call
ConfigureButtonForNode() on each of the buttons when the theme changes?

[Elliot Glaysher, 2009-07-06 22:07:20]

On 2009/07/06 21:57:35, Evan Stade wrote:
> http://codereview.chromium.org/155108/diff/1002/31
> File chrome/browser/gtk/bookmark_utils_gtk.cc (right):
> 
> http://codereview.chromium.org/155108/diff/1002/31#newcode37
> Line 37: // Recursively search for label among the children of |widget|.
> can we please not reintroduce this function? Why can't we call
> ConfigureButtonForNode() on each of the buttons when the theme changes?

Can I replace it with a function that makes assumptions about the layout of the
button as set up in ConfigureButtonForNode? Some of the plumbing is a bit
awkward otherwise. Also, do we really want to do all the rest of the work for
setting up a button when we're just changing the label color?

[Evan Stade, 2009-07-06 22:20:33]

On 2009/07/06 22:07:20, Elliot Glaysher wrote:
> On 2009/07/06 21:57:35, Evan Stade wrote:
> > http://codereview.chromium.org/155108/diff/1002/31
> > File chrome/browser/gtk/bookmark_utils_gtk.cc (right):
> > 
> > http://codereview.chromium.org/155108/diff/1002/31#newcode37
> > Line 37: // Recursively search for label among the children of |widget|.
> > can we please not reintroduce this function? Why can't we call
> > ConfigureButtonForNode() on each of the buttons when the theme changes?
> 
> Can I replace it with a function that makes assumptions about the layout of
the
> button as set up in ConfigureButtonForNode? Some of the plumbing is a bit
> awkward otherwise.

that sounds fine.

> Also, do we really want to do all the rest of the work for
> setting up a button when we're just changing the label color?

imo a few ms in the changing themes performance isn't super important.

[Elliot Glaysher, 2009-07-06 22:38:43]

On 2009/07/06 22:20:33, Evan Stade wrote:
> On 2009/07/06 22:07:20, Elliot Glaysher wrote:
> > On 2009/07/06 21:57:35, Evan Stade wrote:
> > > http://codereview.chromium.org/155108/diff/1002/31
> > > File chrome/browser/gtk/bookmark_utils_gtk.cc (right):
> > > 
> > > http://codereview.chromium.org/155108/diff/1002/31#newcode37
> > > Line 37: // Recursively search for label among the children of |widget|.
> > > can we please not reintroduce this function? Why can't we call
> > > ConfigureButtonForNode() on each of the buttons when the theme changes?
> > 
> > Can I replace it with a function that makes assumptions about the layout of
> the
> > button as set up in ConfigureButtonForNode? Some of the plumbing is a bit
> > awkward otherwise.
> 
> that sounds fine.
> 
> > Also, do we really want to do all the rest of the work for
> > setting up a button when we're just changing the label color?
> 
> imo a few ms in the changing themes performance isn't super important.

On second thought, you might be right. Updated to just pull it out.

[Evan Stade, 2009-07-06 22:45:38]

lgtm