Allow the tiling of theme background images on the NTP....

Allow the tiling of theme background images on the NTP.

BUG= http://crbug/com/15796
TEST= Add a property for tiling to a theme manifest.json.  Observe that the theme image is tiled on the NTP.


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

[Miranda Callahan, 2009-07-16 04:01:37]

This tiles themes, but the code in bookmark_bar_view needs to be compressed;
will do this tomorrow when I'm less bleary-eyed.

I used the $$1 + double-run method in the css builder instead of the $0 method
we discussed because I was afraid of breaking pre-existing code that uses the
string replacement method with a lot of off-by-one errors (suddenly the first
item in the vector would replace $0 instead of $1).  I am probably overlooking a
much more elegant solution to this problem.  

Will alert you tomorrow when the refactored bookmark_bar_view is up.

[Miranda Callahan, 2009-07-16 23:09:04]

Ready for review.

[Glen Murphy, 2009-07-16 23:48:11]

A bunch of mostly quick comments, nothing major (I think).

http://codereview.chromium.org/149741/diff/1043/1047
File chrome/browser/browser_theme_provider.cc (right):

http://codereview.chromium.org/149741/diff/1043/1047#newcode68
Line 68: "ntp_background_repetition";
Can you make this ntp_background_repeat (technically alignment should also be
'align', but it's a little late for that).

http://codereview.chromium.org/149741/diff/1043/1047#newcode279
Line 279: display_properties_.end()) {
indenting

http://codereview.chromium.org/149741/diff/1043/1047#newcode612
Line 612: if (tiling & BrowserThemeProvider::REPEAT_X)
Unlike alignment, tiling can't contain multiple values, so '==' instead of '&'
would be clearer.

http://codereview.chromium.org/149741/diff/1043/1046
File chrome/browser/browser_theme_provider.h (right):

http://codereview.chromium.org/149741/diff/1043/1046#newcode138
Line 138: // A bitfield mask for background tiling.
Unlike alignment, tiling is exclusive (ie you can't have repeat-x and repeat-y),
so this shouldn't be called 'Masks' and can just be a standard enum (using
standard base-10 ints would make this clearer, too).

http://codereview.chromium.org/149741/diff/1043/1048
File chrome/browser/dom_ui/dom_ui_theme_source.cc (right):

http://codereview.chromium.org/149741/diff/1043/1048#newcode107
Line 107: std::vector<string16> subst2;
Can you file a bug and reference it here so that we know to fix this as soon as
we can.

http://codereview.chromium.org/149741/diff/1043/1048#newcode134
Line 134: const std::string css_string = 
UTF16ToASCII(ReplaceStringPlaceholders(
instead of '_a' and '' css_string suffixes, can you use '' and '2' to match
subst and subst2.

http://codereview.chromium.org/149741/diff/1043/1045
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/149741/diff/1043/1045#newcode565
Line 565: // Default tiling is no repeat.
Can you add a BrowserThemeProvider::HasCustomImage(IDR_THEME_NTP_BACKGROUND)
check and exit early if false.

http://codereview.chromium.org/149741/diff/1043/1045#newcode574
Line 574: // ALIGN = TOP
This method is a bit large, it might be easier to break these into three
differnet methods. (PaintThemeBackgroundTop, PaintThemeBackgroundCenter).

http://codereview.chromium.org/149741/diff/1043/1045#newcode579
Line 579: if (tiling & BrowserThemeProvider::REPEAT ||
'==' and '&' (and below)

http://codereview.chromium.org/149741/diff/1043/1045#newcode582
Line 582: canvas->DrawBitmapInt(*ntp_background, x_pos, y_pos);
Can you use TileImageInt instead? (and below)

[Miranda Callahan, 2009-07-17 01:21:52]

Incorporated all of your helpful tips.

http://codereview.chromium.org/149741/diff/1043/1047
File chrome/browser/browser_theme_provider.cc (right):

http://codereview.chromium.org/149741/diff/1043/1047#newcode68
Line 68: "ntp_background_repetition";
On 2009/07/16 23:48:12, Glen Murphy wrote:
> Can you make this ntp_background_repeat (technically alignment should also be
> 'align', but it's a little late for that).

Done.

http://codereview.chromium.org/149741/diff/1043/1047#newcode279
Line 279: display_properties_.end()) {
On 2009/07/16 23:48:12, Glen Murphy wrote:
> indenting

Done.

http://codereview.chromium.org/149741/diff/1043/1047#newcode612
Line 612: if (tiling & BrowserThemeProvider::REPEAT_X)
On 2009/07/16 23:48:12, Glen Murphy wrote:
> Unlike alignment, tiling can't contain multiple values, so '==' instead of '&'
> would be clearer.

Done.

http://codereview.chromium.org/149741/diff/1043/1046
File chrome/browser/browser_theme_provider.h (right):

http://codereview.chromium.org/149741/diff/1043/1046#newcode138
Line 138: // A bitfield mask for background tiling.
On 2009/07/16 23:48:12, Glen Murphy wrote:
> Unlike alignment, tiling is exclusive (ie you can't have repeat-x and
repeat-y),
> so this shouldn't be called 'Masks' and can just be a standard enum (using
> standard base-10 ints would make this clearer, too).

Right -- started by copying the alignment, but didn't make the full switch when
I realized it was different.  Done.

http://codereview.chromium.org/149741/diff/1043/1048
File chrome/browser/dom_ui/dom_ui_theme_source.cc (right):

http://codereview.chromium.org/149741/diff/1043/1048#newcode107
Line 107: std::vector<string16> subst2;
On 2009/07/16 23:48:12, Glen Murphy wrote:
> Can you file a bug and reference it here so that we know to fix this as soon
as
> we can.

Done.

http://codereview.chromium.org/149741/diff/1043/1048#newcode134
Line 134: const std::string css_string = 
UTF16ToASCII(ReplaceStringPlaceholders(
On 2009/07/16 23:48:12, Glen Murphy wrote:
> instead of '_a' and '' css_string suffixes, can you use '' and '2' to match
> subst and subst2.

Done.

http://codereview.chromium.org/149741/diff/1043/1045
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/149741/diff/1043/1045#newcode565
Line 565: // Default tiling is no repeat.
On 2009/07/16 23:48:12, Glen Murphy wrote:
> Can you add a BrowserThemeProvider::HasCustomImage(IDR_THEME_NTP_BACKGROUND)
> check and exit early if false.

Done.

http://codereview.chromium.org/149741/diff/1043/1045#newcode574
Line 574: // ALIGN = TOP
On 2009/07/16 23:48:12, Glen Murphy wrote:
> This method is a bit large, it might be easier to break these into three
> differnet methods. (PaintThemeBackgroundTop, PaintThemeBackgroundCenter).

Unfortunately, it falls naturally into two methods, each of which is broken into
3/4 submethods (within align top and bottom, align left, right, and center --
and within each of these, the different tilings).  I broke into two largish
methods, neither of which are lovely -- but Paint looks a lot nicer.

http://codereview.chromium.org/149741/diff/1043/1045#newcode579
Line 579: if (tiling & BrowserThemeProvider::REPEAT ||
On 2009/07/16 23:48:12, Glen Murphy wrote:
> '==' and '&' (and below)

Done.

http://codereview.chromium.org/149741/diff/1043/1045#newcode582
Line 582: canvas->DrawBitmapInt(*ntp_background, x_pos, y_pos);
On 2009/07/16 23:48:12, Glen Murphy wrote:
> Can you use TileImageInt instead? (and below)

Done.

[Glen Murphy, 2009-07-17 02:14:55]

Just some tiling fiddling.

http://codereview.chromium.org/149741/diff/1051/47
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/149741/diff/1051/47#newcode646
Line 646: if (alignment & BrowserThemeProvider::ALIGN_LEFT) {
All these should be '=='

http://codereview.chromium.org/149741/diff/1051/47#newcode650
Line 650: canvas->TileImageInt(*ntp_background, x_pos, y_pos,
I think you should be able to replace some of these loops with TileImageInt,
e.g:

if (tiling == REPEAT_X)
  canvas->TileImageInt(*ntp_background, 0, 0, width(),
ntp_background->height());
else if (tiling == REPEAT)
  canvas->TileImageInt(*ntp_background, 0, 0, width(), height());
else
  canvas->DrawBitmapInt(*ntp_background, 0, 0, ntp_background->width(),
ntp_background-> height());

And for the Center and Right versions, change the x offset to be a negative
number - something like -(ntp_background->width() % width()) for right aligned.
I imagine you could do the same for vertical offset.

[Miranda Callahan, 2009-07-17 20:00:32]

http://codereview.chromium.org/149741/diff/1051/47
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/149741/diff/1051/47#newcode646
Line 646: if (alignment & BrowserThemeProvider::ALIGN_LEFT) {
On 2009/07/17 02:14:55, Glen Murphy wrote:
> All these should be '=='

But the alignment is still a bitmask, so it will often be holding another bit
value in it and therefore fail ==, I believe.

http://codereview.chromium.org/149741/diff/1051/47#newcode650
Line 650: canvas->TileImageInt(*ntp_background, x_pos, y_pos,
On 2009/07/17 02:14:55, Glen Murphy wrote:
> I think you should be able to replace some of these loops with TileImageInt,
> e.g:
> 
> if (tiling == REPEAT_X)
>   canvas->TileImageInt(*ntp_background, 0, 0, width(),
> ntp_background->height());
> else if (tiling == REPEAT)
>   canvas->TileImageInt(*ntp_background, 0, 0, width(), height());
> else
>   canvas->DrawBitmapInt(*ntp_background, 0, 0, ntp_background->width(),
> ntp_background-> height());
> 
> And for the Center and Right versions, change the x offset to be a negative
> number - something like -(ntp_background->width() % width()) for right
aligned.
> I imagine you could do the same for vertical offset.
> 

That's so much ... nicer.  Yay, SkCanvas!  Done.

[Glen Murphy, 2009-07-17 20:14:17]

LGTM with one change:

http://codereview.chromium.org/149741/diff/1070/2025
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/149741/diff/1070/2025#newcode566
Line 566: return;
I don't think you want to return here - you still need to draw the bookmark bar
itself - just use this to skip over the next Paint*Background block.

[Miranda Callahan, 2009-07-17 20:21:05]

http://codereview.chromium.org/149741/diff/1070/2025
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/149741/diff/1070/2025#newcode566
Line 566: return;
On 2009/07/17 20:14:17, Glen Murphy wrote:
> I don't think you want to return here - you still need to draw the bookmark
bar
> itself - just use this to skip over the next Paint*Background block.

Done.