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

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

Rewrites most of BrowserThemeProvider and adds a new class
BrowserThemePack. BrowserThemePack takes all the logic of generating resources
out of the BrowserThemeProvider, does all of them at theme install time
(previously, we lazily generated all the button images and a good number of
colors, which muddled logic quite a bit), and then writes all the data out into
an mmap()able file to speed startup when a theme is installed.

In addition, this changes how the GtkThemeProvider works. The GtkThemeProvider
now generates all of its images lazily and doesn't reach into the
implementation details of BrowserThemeProvider as it used to.

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

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

[Elliot Glaysher, 2009-12-09 22:13:11]

Miranda OR Glen: The majority of the patch.
Avi: Because he wanted to be on this review.
Tony: The DataPack changes at minimum, and more if you want.

[Evan Martin, 2009-12-09 22:56:57]

only data pack bits

http://codereview.chromium.org/460050/diff/7001/8005
File base/data_pack.cc (right):

http://codereview.chromium.org/460050/diff/7001/8005#newcode136
base/data_pack.cc:136: fwrite(&kFileFormatVersion, 1, sizeof(uint32_t), file);
You need to check the return value on all of these to not get compiler warnings.

You should pass in size=4 and nmemb=1 to indicate that writing three of the four
bytes is not acceptable.  (It also lets you verify success by just checking the
result == 1)

http://codereview.chromium.org/460050/diff/7001/8006
File base/data_pack.h (right):

http://codereview.chromium.org/460050/diff/7001/8006#newcode22
base/data_pack.h:22: typedef unsigned int uint32_t;
Is basictypes.h:uint32 not appropriate?

[tony, 2009-12-10 00:03:21]

http://codereview.chromium.org/460050/diff/7001/8005
File base/data_pack.cc (right):

http://codereview.chromium.org/460050/diff/7001/8005#newcode22
base/data_pack.cc:22: #pragma pack(push,1)
Should this be pack(push,4)?

http://codereview.chromium.org/460050/diff/7001/8005#newcode42
base/data_pack.cc:42: 
Can you add a compile assert to make sure this struct is 12 bytes?

http://codereview.chromium.org/460050/diff/7001/8005#newcode132
base/data_pack.cc:132: if (!file) {
nit: no braces

http://codereview.chromium.org/460050/diff/7001/8006
File base/data_pack.h (right):

http://codereview.chromium.org/460050/diff/7001/8006#newcode22
base/data_pack.h:22: typedef unsigned int uint32_t;
Can we use uint32 (from basictypes.h) instead of uint32_t or does that make gcc
unhappy?  Alternately, can we put this in basictypes.h?  Otherwise, I think this
definition will creep into other cc files by accident.

http://codereview.chromium.org/460050/diff/7001/8007
File base/data_pack_unittest.cc (right):

http://codereview.chromium.org/460050/diff/7001/8007#newcode61
base/data_pack_unittest.cc:61: resources.insert(std::make_pair(4,
base::StringPiece(four)));
Can you add a value with a gap (i.e., not all sequential values) and test that
that reads fine?

http://codereview.chromium.org/460050/diff/7001/8008
File chrome/browser/browser_theme_pack.cc (right):

http://codereview.chromium.org/460050/diff/7001/8008#newcode46
chrome/browser/browser_theme_pack.cc:46: const int kTintArraySize = 6;
Can we add tokens to the enums for these?

http://codereview.chromium.org/460050/diff/7001/8008#newcode249
chrome/browser/browser_theme_pack.cc:249: // writting to the data pack
Nit:writing

http://codereview.chromium.org/460050/diff/7001/8008#newcode250
chrome/browser/browser_theme_pack.cc:250: RepackImageCacheToImageMemory();
Can we do this on the write to disk thread?

http://codereview.chromium.org/460050/diff/7001/8009
File chrome/browser/browser_theme_pack.h (right):

http://codereview.chromium.org/460050/diff/7001/8009#newcode40
chrome/browser/browser_theme_pack.h:40: BrowserThemePack();
Maybe make BuildFromExtension and BuildFromDataPack be static methods and make
the constructor private?  It prevents stuff like calling BuildFromExtension
twice.

For testing, I think the friend class should still allow you to construct
manually.

http://codereview.chromium.org/460050/diff/7001/8009#newcode70
chrome/browser/browser_theme_pack.h:70: typedef std::map<int, SkBitmap*>
ImageCache;
Can you move the typedefs into the private section?

http://codereview.chromium.org/460050/diff/8038/8039
File app/theme_provider.h (right):

http://codereview.chromium.org/460050/diff/8038/8039#newcode10
app/theme_provider.h:10: #include "app/gfx/color_utils.h"
Nit: do we still need this?

http://codereview.chromium.org/460050/diff/8038/8045
File chrome/browser/browser_theme_pack.cc (right):

http://codereview.chromium.org/460050/diff/8038/8045#newcode48
chrome/browser/browser_theme_pack.cc:48: const int kDisplayPropertySize = 3;
These look really fragile.  Can we move these consts next to the enum
declaration so they're less likely to go out of sync?

http://codereview.chromium.org/460050/diff/8038/8045#newcode97
chrome/browser/browser_theme_pack.cc:97: { "ntp_background_alignment",
BrowserThemeProvider::NTP_BACKGROUND_ALIGNMENT },
Nit: 80 cols.

http://codereview.chromium.org/460050/diff/8038/8045#newcode110
chrome/browser/browser_theme_pack.cc:110: const int GetIntForString(const
std::string& key, StringToIntTable* table,
const?

http://codereview.chromium.org/460050/diff/8038/8045#newcode111
chrome/browser/browser_theme_pack.cc:111: int size) {
Alternately, you could put a NULL, 0 token at the end of each table.  *shrug*

http://codereview.chromium.org/460050/diff/8038/8045#newcode133
chrome/browser/browser_theme_pack.cc:133: { IDR_THEME_FRAME_OVERLAY_INACTIVE,
BrowserThemeProvider::TINT_FRAME_INACTIVE },
Nit: 80 cols.

http://codereview.chromium.org/460050/diff/8038/8045#newcode178
chrome/browser/browser_theme_pack.cc:178: RefCountedMemory* ReadFileData(const
std::string& utf8_path) {
is the path really utf8?  I think it's ascii at this point since we generate the
filename.  You could then get rid of the #if and use AppendASCII.

http://codereview.chromium.org/460050/diff/8038/8045#newcode300
chrome/browser/browser_theme_pack.cc:300: bool
BrowserThemePack::WriteToDisk(FilePath path) const {
If this is to run on a different thread, can you add a DCHECK to verify (it also
provides documentation).

http://codereview.chromium.org/460050/diff/8038/8045#newcode365
chrome/browser/browser_theme_pack.cc:365: // Check our cache of prepared images,
first.
Can you add a TODO(tony) to make this thread safe?

http://codereview.chromium.org/460050/diff/8038/8045#newcode603
chrome/browser/browser_theme_pack.cc:603: HSLShift(frame,
GetTintInternal(BrowserThemeProvider::TINT_FRAME_INACTIVE));
Nit: 80 cols

http://codereview.chromium.org/460050/diff/8038/8045#newcode607
chrome/browser/browser_theme_pack.cc:607: HSLShift(frame,
GetTintInternal(BrowserThemeProvider::TINT_FRAME_INCOGNITO));
Nit 80 cols

http://codereview.chromium.org/460050/diff/8038/8045#newcode796
chrome/browser/browser_theme_pack.cc:796: *(it->second),
GetTintInternal(BrowserThemeProvider::TINT_BACKGROUND_TAB));
Nit: 80 cols.

http://codereview.chromium.org/460050/diff/8038/8047
File chrome/browser/browser_theme_pack_unittest.cc (right):

http://codereview.chromium.org/460050/diff/8038/8047#newcode36
chrome/browser/browser_theme_pack_unittest.cc:36:
BrowserThemeProvider::GetDefaultColor(BrowserThemeProvider::COLOR_FRAME),
Nit: 80 cols.

http://codereview.chromium.org/460050/diff/8073/7012
File chrome/browser/browser_theme_provider.cc (right):

http://codereview.chromium.org/460050/diff/8073/7012#newcode225
chrome/browser/browser_theme_provider.cc:225: if (theme_pack_) {
nit: no braces

http://codereview.chromium.org/460050/diff/8073/7012#newcode467
chrome/browser/browser_theme_provider.cc:467:
UserMetrics::RecordAction("Themes_loaded", profile_);
Can we use Themes.Loaded?  I think the dashboard auto groups based on period
separators.

http://codereview.chromium.org/460050/diff/8073/7012#newcode477
chrome/browser/browser_theme_provider.cc:477:
UserMetrics::RecordAction("Themes_migrated", profile_);
Nit: Themes.Migrated

http://codereview.chromium.org/460050/diff/8073/7016
File chrome/browser/cocoa/download_shelf_controller.mm (right):

http://codereview.chromium.org/460050/diff/8073/7016#newcode118
chrome/browser/cocoa/download_shelf_controller.mm:118: useDefaultColor =
provider->GetColor(ThemeProvider::COLOR_BOOKMARK_TEXT) ==
Nit: 80 cols.

http://codereview.chromium.org/460050/diff/8073/7017
File chrome/browser/extensions/theme_installed_infobar_delegate.cc (right):

http://codereview.chromium.org/460050/diff/8073/7017#newcode32
chrome/browser/extensions/theme_installed_infobar_delegate.cc:32: if (service &&
previous_theme_id_ != new_theme_id_) {
Huh, nice catch.

http://codereview.chromium.org/460050/diff/8073/7025
File chrome/common/chrome_constants.cc (right):

http://codereview.chromium.org/460050/diff/8073/7025#newcode87
chrome/common/chrome_constants.cc:87: const FilePath::CharType
kThemePackFilename[] = FPL("CachedTheme.pak");
Nit: Can you add a space to this filename?  It seems more consistent.

[Miranda Callahan, 2009-12-10 00:53:44]

This is really awesome!  Have you stress-tested it by trying to install multiple
themes in rapid succession?

http://codereview.chromium.org/460050/diff/8038/8043
File base/data_pack.h (right):

http://codereview.chromium.org/460050/diff/8038/8043#newcode20
base/data_pack.h:20: // rest of chrome to it. We're currently breaking the
section "Interger Types"
"Integer"?

http://codereview.chromium.org/460050/diff/8038/8045
File chrome/browser/browser_theme_pack.cc (right):

http://codereview.chromium.org/460050/diff/8038/8045#newcode5
chrome/browser/browser_theme_pack.cc:5: #include "build/build_config.h"
This guy should be down with the other project includes.

http://codereview.chromium.org/460050/diff/8038/8045#newcode7
chrome/browser/browser_theme_pack.cc:7: #include
"chrome/browser/browser_theme_pack.h"
This guy needs to be the first include in the file, I believe.

http://codereview.chromium.org/460050/diff/8038/8045#newcode35
chrome/browser/browser_theme_pack.cc:35: // IDs in the DataPack that are won't
clash with the positive integer
"IDs that are in the DataPack won't..."

http://codereview.chromium.org/460050/diff/8038/8045#newcode39
chrome/browser/browser_theme_pack.cc:39: // written (i.e. chrome crashed on a
different thread while writing the pack).
cool!

http://codereview.chromium.org/460050/diff/8038/8045#newcode229
chrome/browser/browser_theme_pack.cc:229: // Builds the images. (Image building
is dependant on tints).
dependent

http://codereview.chromium.org/460050/diff/8038/8046
File chrome/browser/browser_theme_pack.h (right):

http://codereview.chromium.org/460050/diff/8038/8046#newcode133
chrome/browser/browser_theme_pack.h:133: // querrying if the tint exists.
"querying", though it actually looks cooler with two r's.

http://codereview.chromium.org/460050/diff/8073/7012
File chrome/browser/browser_theme_provider.cc (right):

http://codereview.chromium.org/460050/diff/8073/7012#newcode189
chrome/browser/browser_theme_provider.cc:189: bitmap = rb_.GetBitmapNamed(id);
Is it ok that we're no longer creating a new SkBitmap here?

http://codereview.chromium.org/460050/diff/8073/7013
File chrome/browser/browser_theme_provider.h (right):

http://codereview.chromium.org/460050/diff/8073/7013#newcode183
chrome/browser/browser_theme_provider.h:183: // Implementation of SetTheme()
(and the fallback from LoadThemePerfs() in
"LoadThemePrefs"

[Paweł Hajdan Jr., 2009-12-10 07:28:28]

Drive-by with some unit test nits.

Generally with these fixes they would be a little less likely to leave temporary
files around (and promote a better pattern), and have more useful log output in
case of failures (which is important if a test fails intermittently etc).

http://codereview.chromium.org/460050/diff/8073/7008
File base/data_pack_unittest.cc (right):

http://codereview.chromium.org/460050/diff/8073/7008#newcode50
base/data_pack_unittest.cc:50:
ASSERT_TRUE(file_util::CreateTemporaryFile(&temp_file_));
nit: Could you use ScopedTempDir instead?

http://codereview.chromium.org/460050/diff/8073/7011
File chrome/browser/browser_theme_pack_unittest.cc (right):

http://codereview.chromium.org/460050/diff/8073/7011#newcode25
chrome/browser/browser_theme_pack_unittest.cc:25: file_util::Delete(temp_file_,
false);
nit: Just please use ScopedTempDir...

http://codereview.chromium.org/460050/diff/8073/7021
File chrome/browser/gtk/gtk_theme_provider_unittest.cc (right):

http://codereview.chromium.org/460050/diff/8073/7021#newcode51
chrome/browser/gtk/gtk_theme_provider_unittest.cc:51:
EXPECT_EQ(provider_->GetColor(i), BrowserThemeProvider::GetDefaultColor(i));
nit: Could you add some logging information with << after EXPECT_EQ or
SCOPED_TRACE? Will be useful when the test fails.

[Avi (use Gerrit), 2009-12-10 19:11:08]

I see what you're doing here; awesome. Was added as FYI so I'm not doing a
review.

http://codereview.chromium.org/460050/diff/5001/5015
File chrome/browser/cocoa/download_shelf_controller.mm (right):

http://codereview.chromium.org/460050/diff/5001/5015#newcode118
chrome/browser/cocoa/download_shelf_controller.mm:118: useDefaultColor =
provider->GetColor(ThemeProvider::COLOR_BOOKMARK_TEXT) ==
80col

[Elliot Glaysher, 2009-12-10 21:46:17]

Tony: I just want to verify that the change you want me to make to the storage
of tints/colors/display properties is to have a termination entry where id is an
invalid value and only allocate enough space in the arrays for provided data.

Miranda: there's actually some sort of weird timing issue where if you install
two themes right after another, the second theme isn't installed and both themes
are deleted from disk, but I'm not worrying about it in this patch because I can
replicate it on trunk. I've modified it so my implementation doesn't blow up and
handles this case gracefully. I'll track down the issue in a followup patch.

http://codereview.chromium.org/460050/diff/7001/8005
File base/data_pack.cc (right):

http://codereview.chromium.org/460050/diff/7001/8005#newcode22
base/data_pack.cc:22: #pragma pack(push,1)
On 2009/12/10 00:03:22, tony wrote:
> Should this be pack(push,4)?

No, because we want to align to one byte boundaries. I couldn't keep
__attribute((packed)) because it's a gcc extension; the equivalent is pack(1).
pack(4) would align each variable to the next word instead of the next char.

http://codereview.chromium.org/460050/diff/7001/8008
File chrome/browser/browser_theme_pack.cc (right):

http://codereview.chromium.org/460050/diff/7001/8008#newcode250
chrome/browser/browser_theme_pack.cc:250: RepackImageCacheToImageMemory();
On 2009/12/10 00:03:22, tony wrote:
> Can we do this on the write to disk thread?

It's my intention to do this next. Currently, I think everything is threadsafe
and I know I'll need to do some locking fun to get this performant.

http://codereview.chromium.org/460050/diff/8038/8045#newcode5
chrome/browser/browser_theme_pack.cc:5: #include "build/build_config.h"
build_config.h must be the first header to define all the conditional
compilation macros.

http://codereview.chromium.org/460050/diff/8038/8045#newcode300
chrome/browser/browser_theme_pack.cc:300: bool
BrowserThemePack::WriteToDisk(FilePath path) const {
On 2009/12/10 00:03:22, tony wrote:
> If this is to run on a different thread, can you add a DCHECK to verify (it
also
> provides documentation).

I am not sure what you mean here.

http://codereview.chromium.org/460050/diff/8038/8045#newcode365
chrome/browser/browser_theme_pack.cc:365: // Check our cache of prepared images,
first.
On 2009/12/10 00:03:22, tony wrote:
> Can you add a TODO(tony) to make this thread safe?

I believe this method already is thread safe; I'm not moving the Encode() to the
worker thread (in this patch) because I know that will require some complex
locking and I want a baseline first.

http://codereview.chromium.org/460050/diff/8073/7012
File chrome/browser/browser_theme_provider.cc (right):

http://codereview.chromium.org/460050/diff/8073/7012#newcode189
chrome/browser/browser_theme_provider.cc:189: bitmap = rb_.GetBitmapNamed(id);
On 2009/12/10 00:53:44, Miranda Callahan wrote:
> Is it ok that we're no longer creating a new SkBitmap here?

Yes. In the previous implementation, the caller didn't take ownership of the
returned SkBitmap from BTP::GetBitmapNamed(). (It was placed in the
image_cache_.) This invariant holds, because the bitmap is owned by either the
theme_pack_ or by ResourceBundle.

[tony, 2009-12-10 21:59:11]

> Tony: I just want to verify that the change you want me to make to the storage
> of tints/colors/display properties is to have a termination entry where id is
an
> invalid value and only allocate enough space in the arrays for provided data.

Yes, I was just suggesting having a termination entry so you don't have to use
arraysize every time you call GetIntForString.  Your current change seems to do
this but I would use NULL rather than "" for the last entry.

http://codereview.chromium.org/460050/diff/8038/8045
File chrome/browser/browser_theme_pack.cc (right):

http://codereview.chromium.org/460050/diff/8038/8045#newcode5
chrome/browser/browser_theme_pack.cc:5: #include "build/build_config.h"
On 2009/12/10 21:46:18, Elliot Glaysher wrote:
> build_config.h must be the first header to define all the conditional
> compilation macros.

Shouldn't each header that needs these macros be including build_config.h
themselves?  You should be able to move this into the alphabetical list below.

http://codereview.chromium.org/460050/diff/8038/8045#newcode300
chrome/browser/browser_theme_pack.cc:300: bool
BrowserThemePack::WriteToDisk(FilePath path) const {
On 2009/12/10 21:46:18, Elliot Glaysher wrote:
> On 2009/12/10 00:03:22, tony wrote:
> > If this is to run on a different thread, can you add a DCHECK to verify (it
> also
> > provides documentation).
> 
> I am not sure what you mean here.

I mean add:
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
here.

http://codereview.chromium.org/460050/diff/8038/8045#newcode365
chrome/browser/browser_theme_pack.cc:365: // Check our cache of prepared images,
first.
On 2009/12/10 21:46:18, Elliot Glaysher wrote:
> On 2009/12/10 00:03:22, tony wrote:
> > Can you add a TODO(tony) to make this thread safe?
> 
> I believe this method already is thread safe; I'm not moving the Encode() to
the
> worker thread (in this patch) because I know that will require some complex
> locking and I want a baseline first.

If two threads call this method with the same id, you could create the same
bitmap for the image cache and leak one of the bitmaps.

[Elliot Glaysher, 2009-12-10 22:17:44]

On 2009/12/10 21:59:11, tony wrote:
> Yes, I was just suggesting having a termination entry so you don't have to use
> arraysize every time you call GetIntForString.  Your current change seems to
do
> this but I would use NULL rather than "" for the last entry.

OK, I was right. There was a failure of communication here.

The constants that you thought were fragile had nothing to do with the
StringToIntTable entries at the top of browser_theme_pack.cc; they define the
size, for example the tints_ array that's stored in the mmaped memory. I agree
that StringToInt translation tables should have termination values; I'm asking
if you think the per DataPack tables should have a termination values. I think
it might be a good idea; the array size could be stored in the header.

> On 2009/12/10 21:46:18, Elliot Glaysher wrote:
> > On 2009/12/10 00:03:22, tony wrote:
> > > Can you add a TODO(tony) to make this thread safe?
> > 
> > I believe this method already is thread safe; I'm not moving the Encode() to
> the
> > worker thread (in this patch) because I know that will require some complex
> > locking and I want a baseline first.
> 
> If two threads call this method with the same id, you could create the same
> bitmap for the image cache and leak one of the bitmaps.

Only one thread can call this method at a time; I meant that the parts of it
that read memory that's shared between threads (|image_memory_|) aren't written
to on either thread.

So the only part of this class that's meant to be run on a separate thread is
WriteToDisk() and thread safety is ensured by only performing read operations on
the data arrays and |image_memory_| after a BrowserThemePack has been
constructed. |image_cache_| (which is mutable) is only written to from the UI
thread because GetBitmapNamed() should only be called from the UI thread. I'll
document that explicitly.

This, BTW, is why I want to get a simple version in first before I think about
the locking needed to safely read from |image_cache_| from the other thread.

[Elliot Glaysher, 2009-12-10 22:24:13]

On 2009/12/10 21:59:11, tony wrote: 
> I mean add:
> DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
> here.

The problem with this is that I don't post this as a task from the unit tests,
tripping this DCHECK. I'll just add documentation instead.

[tony, 2009-12-10 22:24:53]

On 2009/12/10 22:17:44, Elliot Glaysher wrote:
> OK, I was right. There was a failure of communication here.
> 
> The constants that you thought were fragile had nothing to do with the
> StringToIntTable entries at the top of browser_theme_pack.cc; they define the
> size, for example the tints_ array that's stored in the mmaped memory. I agree
> that StringToInt translation tables should have termination values; I'm asking
> if you think the per DataPack tables should have a termination values. I think
> it might be a good idea; the array size could be stored in the header.

I see, you're asking about storing kTintArraySize, kColorArraySize and 
kDisplayPropertySize in the data pack header?  I don't think that's strictly
necessary because you're versioning the whole file, but I'm not opposed to it. 
Your call.

> > On 2009/12/10 21:46:18, Elliot Glaysher wrote:
> This, BTW, is why I want to get a simple version in first before I think about
> the locking needed to safely read from |image_cache_| from the other thread.

Ok, that's fine.  I just wanted a TODO so it would be clear that this is
something that I want to be able to do in the future.

http://codereview.chromium.org/460050/diff/14001/14007
File chrome/browser/browser_theme_pack.cc (right):

http://codereview.chromium.org/460050/diff/14001/14007#newcode300
chrome/browser/browser_theme_pack.cc:300: pack->tints_ =
reinterpret_cast<TintEntry*>(const_cast<char*>(pointer.data()));
Nit: 80 cols.

[Elliot Glaysher, 2009-12-11 00:32:58]

Updated. All green on the trybots.

[tony, 2009-12-11 19:14:22]

Last 2 comments, the rest is looking really good.

http://codereview.chromium.org/460050/diff/13003/12008
File chrome/browser/browser_theme_pack.h (right):

http://codereview.chromium.org/460050/diff/13003/12008#newcode43
chrome/browser/browser_theme_pack.h:43: static scoped_refptr<BrowserThemePack>
BuildFromExtension(
What does it mean to return a scoped_refptr?  Should this just return a raw
pointer and the caller can catch it in a scoped_refptr?

http://codereview.chromium.org/460050/diff/13003/12010
File chrome/browser/browser_theme_provider.cc (right):

http://codereview.chromium.org/460050/diff/13003/12010#newcode168
chrome/browser/browser_theme_provider.cc:168: FreePlatformCaches();
I don't think this is going to call the virtual method in the GTK theme
provider.  I think by the time this runs, the GTK theme provider is already gone
and we're just a BrowserThemeProvider.  Can you verify that this does the right
thing?

[Elliot Glaysher, 2009-12-11 19:35:38]

http://codereview.chromium.org/460050/diff/13003/12008
File chrome/browser/browser_theme_pack.h (right):

http://codereview.chromium.org/460050/diff/13003/12008#newcode43
chrome/browser/browser_theme_pack.h:43: static scoped_refptr<BrowserThemePack>
BuildFromExtension(
On 2009/12/11 19:14:23, tony wrote:
> What does it mean to return a scoped_refptr?

The same thing as returning any C++ object by value. operator=() will be invoked
with the returned object, and then the destructor on the returned object will be
called. scoped_refptr instances are designed to be copied from each other so
this works.

> Should this just return a raw pointer and the caller can catch it in a
scoped_refptr?

I'm against this. If you look at the BrowserThemePack::BuildFromDataPack, you'll
see a whole bunch of error early return cases where we want a smart pointer to
clean up the partially constructed BrowserThemePack after us so we don't have to
do manual cleanup. BuildFromExtension() does the same thing to make the
interface consistent.

http://codereview.chromium.org/460050/diff/13003/12010
File chrome/browser/browser_theme_provider.cc (right):

http://codereview.chromium.org/460050/diff/13003/12010#newcode168
chrome/browser/browser_theme_provider.cc:168: FreePlatformCaches();
Good catch. Fixed.

[Miranda Callahan, 2009-12-11 19:36:55]

All the hard stuff was in Tony's parts.  LGTM on the theme changing goodness.

On 2009/12/11 19:35:38, Elliot Glaysher wrote:
> http://codereview.chromium.org/460050/diff/13003/12008
> File chrome/browser/browser_theme_pack.h (right):
> 
> http://codereview.chromium.org/460050/diff/13003/12008#newcode43
> chrome/browser/browser_theme_pack.h:43: static scoped_refptr<BrowserThemePack>
> BuildFromExtension(
> On 2009/12/11 19:14:23, tony wrote:
> > What does it mean to return a scoped_refptr?
> 
> The same thing as returning any C++ object by value. operator=() will be
invoked
> with the returned object, and then the destructor on the returned object will
be
> called. scoped_refptr instances are designed to be copied from each other so
> this works.
> 
> > Should this just return a raw pointer and the caller can catch it in a
> scoped_refptr?
> 
> I'm against this. If you look at the BrowserThemePack::BuildFromDataPack,
you'll
> see a whole bunch of error early return cases where we want a smart pointer to
> clean up the partially constructed BrowserThemePack after us so we don't have
to
> do manual cleanup. BuildFromExtension() does the same thing to make the
> interface consistent.
> 
> http://codereview.chromium.org/460050/diff/13003/12010
> File chrome/browser/browser_theme_provider.cc (right):
> 
> http://codereview.chromium.org/460050/diff/13003/12010#newcode168
> chrome/browser/browser_theme_provider.cc:168: FreePlatformCaches();
> Good catch. Fixed.

[tony, 2009-12-11 19:46:50]

From offline discussion: BuildFromExtension will return just a pointer and
BuildFromDataPack will return the scoped_refptr with a comment in the header
explaining why it's like that.

With those changes, LGTM!