Try 2: Completely redo how themes are stored on disk and processed at install time.

Try 2: Completely redo how themes are stored on disk and processed at install time.

Same as previous patch, except we now have a
BrowserThemeProvider::GetDefaultDisplayProperty() so we don't have UMRs in
ntp_resource_cache.cc.

BUG=24493, 21121
TEST=All the new unit tests pass. All the complex theme startup tests go faster.

Previous Review URL: http://codereview.chromium.org/460050

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

[Elliot Glaysher, 2009-12-14 20:14:03]

This fixes the UMRs. I also noticed that while this patch doesn't increase
complex_theme NTP performance, it does speed up warm startup.

[tony, 2009-12-14 20:19:57]

LGTM

http://codereview.chromium.org/499004/diff/1/11
File chrome/browser/browser_theme_provider.h (right):

http://codereview.chromium.org/499004/diff/1/11#newcode152
chrome/browser/browser_theme_provider.h:152: // Returns true and sets the
default property to |result| for valid display
Nit: This comment makes it sounds like default property is assigned the value of
result, rather than the other way around.

[Miranda Callahan, 2009-12-14 20:47:50]

Some annoying little nits on second reading; otherwise LGTM!

http://codereview.chromium.org/499004/diff/1/7
File chrome/browser/browser_theme_pack.cc (right):

http://codereview.chromium.org/499004/diff/1/7#newcode202
chrome/browser/browser_theme_pack.cc:202: // floaing point value.
floating

http://codereview.chromium.org/499004/diff/1/7#newcode261
chrome/browser/browser_theme_pack.cc:261: // writing to the data pack
. at end of sentence

http://codereview.chromium.org/499004/diff/1/8
File chrome/browser/browser_theme_pack.h (right):

http://codereview.chromium.org/499004/diff/1/8#newcode44
chrome/browser/browser_theme_pack.h:44: // Builds the theme pack from a
previously WriteToDisk(). This operation
"from a previous WriteToDisk() operation", or "from a previously performed
WriteToDisk()"

http://codereview.chromium.org/499004/diff/1/9
File chrome/browser/browser_theme_pack_unittest.cc (right):

http://codereview.chromium.org/499004/diff/1/9#newcode177
chrome/browser/browser_theme_pack_unittest.cc:177: // If we specify the
underline color, it shouldn't try to generate one.x
an x crept in here.  terminate its existence!

[Avi (use Gerrit), 2010-02-12 21:58:05]

I'm rather confused by what happened here re Mac theming.

Things like GetNSColor() and GetNSColorTint() explicitly state in their contract
"If a theme does not provide [an color|tint with that id], this function will
return nil." which is explicitly broken with this change. It looks like this was
papered over with one-off fixes to the download shelf controller, and that was
done directly with the theme functions rather than going through GTMTheme. :(

Since the Mac drawing code often relies on getting the nil to know when to draw
the default colors (which are different than the Windows/Linux ones), I'm still
not sure why this works at all.

[Elliot Glaysher, 2010-02-12 22:28:08]

On Fri, Feb 12, 2010 at 1:58 PM,  <avi@chromium.org> wrote:
> I'm rather confused by what happened here re Mac theming.
>
> Things like GetNSColor() and GetNSColorTint() explicitly state in their
> contract
> "If a theme does not provide [an color|tint with that id], this function
> will
> return nil." which is explicitly broken with this change. It looks like this
> was
> papered over with one-off fixes to the download shelf controller, and that
> was
> done directly with the theme functions rather than going through GTMTheme.
> :(

The current code (abridged) for GetNSColor is:

NSColor* BrowserThemeProvider::GetNSColor(int id) const {
  /* CODE CUT: Checks the nscolor_cache_ */

  SkColor sk_color;
  if (theme_pack_.get() && theme_pack_->GetColor(id, &sk_color)) {
    NSColor* color = /* ... */

    // We loaded successfully.  Cache the color.
    if (color) {
      nscolor_cache_[id] = [color retain];
      return color;
    }
  }

  return nil;
}

We only return a color (i) If we have a custom theme installed
(theme_pack_) and (ii) that theme contains specifies that color id.
Otherwise nil is returned. Note that we do the same thing in
BrowserThemeProvider::GetColor(), but fallback to
BrowserThemeProvider::GetDefaultColor() if the theme doesn't specify a
new color.

I think the confusion comes from BrowserThemePack::Get*()'s
documentation being wrong. I will prepare a patch.

-- Elliot