Allow themes to change the background of the new tab page....

Allow themes to change the background of the new tab page. Adds support for display properties to themes (stored internally as ints/enums, but parsed from text).

BUG=12768
TEST=Install a theme with an new tab page background and verify that the background appears on the new tab page.


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

[arv (Not doing code reviews), 2009-06-02 07:01:12]

http://codereview.chromium.org/115910/diff/1123/1128
File chrome/browser/browser_theme_provider.cc (right):

http://codereview.chromium.org/115910/diff/1123/1128#newcode246
Line 246: display_properties_.end())
Curly braces for multiline ifs?

http://codereview.chromium.org/115910/diff/1123/1127
File chrome/browser/browser_theme_provider.h (right):

http://codereview.chromium.org/115910/diff/1123/1127#newcode87
Line 87: static std::string AlignmentToString(int alignment);
Shouldn't the alignment type be the enum?

AlignmentToString(AlignmentProperty alignment);

same for the inverse method.

http://codereview.chromium.org/115910/diff/1123/1136
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/115910/diff/1123/1136#newcode1249
Line 1249: NotificationService::AllSources());
fix indentation

http://codereview.chromium.org/115910/diff/1123/1125
File chrome/browser/resources/new_tab.html (right):

http://codereview.chromium.org/115910/diff/1123/1125#newcode938
Line 938: document.getElementsByTagName('html')[0].
document.documentElement

or

document.querySelector('html')

http://codereview.chromium.org/115910/diff/1123/1126
File chrome/browser/resources/new_tab_theme.css (right):

http://codereview.chromium.org/115910/diff/1123/1126#newcode6
Line 6: overflow:hidden;
-webkit-transition: background-position .12s;

:-)

http://codereview.chromium.org/115910/diff/1123/1126#newcode20
Line 20: background-color: $7;
These $n placeholders are getting a bit hard to maintain. We should probably use
named placeholders instead (in the future of course).

http://codereview.chromium.org/115910/diff/1123/1130
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/115910/diff/1123/1130#newcode570
Line 570: if (alignment == BrowserThemeProvider::ALIGN_TOP ||
Maybe using a bit mask for the alignment would simplify things?

http://codereview.chromium.org/115910/diff/1123/1130#newcode577
Line 577: else if (alignment == BrowserThemeProvider::ALIGN_TOP_RIGHT)
curly?

http://codereview.chromium.org/115910/diff/1123/1130#newcode584
Line 584: }
I have some sad news. What if the window is small enough to require us to paint
the background even if it is bottom aligned?

[Glen Murphy, 2009-06-02 18:16:39]

Updated, also comments:

> http://codereview.chromium.org/115910/diff/1123/1126
> File chrome/browser/resources/new_tab_theme.css (right):
> 
> http://codereview.chromium.org/115910/diff/1123/1126#newcode6
> Line 6: overflow:hidden;
> -webkit-transition: background-position .12s;

This doesn't look any better (we'd need to animate the background downwards in
the bubble as it closes to match, and then we'd have to show something above
it).

> http://codereview.chromium.org/115910/diff/1123/1130#newcode584
> Line 584: }
> I have some sad news. What if the window is small enough to require us to
paint
> the background even if it is bottom aligned?

I'm going to worry about this later - I haven't decided whether I want to cover
it by adding a small gradient to the top of the NTP, or to actually paint it in
the bookmark bar.

[arv (Not doing code reviews), 2009-06-02 18:35:10]

http://codereview.chromium.org/115910/diff/73/1145
File chrome/browser/browser_theme_provider.h (right):

http://codereview.chromium.org/115910/diff/73/1145#newcode87
Line 87: static AlignmentProperty StringToAlignment(const std::string
&alignment);
We either need to use a the enum type or the a bitfield mask. We should not do
both since that is pretty confusing.

[arv (Not doing code reviews), 2009-06-02 19:11:54]

LGTM

http://codereview.chromium.org/115910/diff/1177/1179
File chrome/browser/resources/new_tab.html (right):

http://codereview.chromium.org/115910/diff/1177/1179#newcode936
Line 936: }
new line between these