Tracing showed that the resource bundle lock was held for long periods of tim...

Tracing showed that the resource bundle lock was held for long periods of time, as much as 3.5ms.  Altough the lock is not contended in this case, be more precise about when we need to hold the lock.  Also clean up a static initialization to use LazyInstance.

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

[Dean McNamee, 2008-11-03 15:38:57]

Doing some lock analysis.  Not a huge deal here, but I saw the lock being held
129 times with a duration > 0.1ms.  Since we are loading the bitmaps on the UI
thread, there is no contention.  Since it looks like the loading / PNG decoding
is slow, maybe someday we'd want to preload these on another thread, so I think
making the lock more fine grained makes sense.

A left a note about a truncated comment, because I wasn't sure what it was
supposed to mean.

Additionally now if loading fails, we cache a null pointer.  Should we instead
cache the empty bitmap?

[tony, 2008-11-03 16:32:14]

LGTM.  If you sync to ToT, you should see code to cache an empty red square
bitmap.  I think we want to keep that.

On 2008/11/03 15:38:57, Dean McNamee wrote:
> Doing some lock analysis.  Not a huge deal here, but I saw the lock being held
> 129 times with a duration > 0.1ms.  Since we are loading the bitmaps on the UI
> thread, there is no contention.  Since it looks like the loading / PNG
decoding
> is slow, maybe someday we'd want to preload these on another thread, so I
think
> making the lock more fine grained makes sense.
> 
> A left a note about a truncated comment, because I wasn't sure what it was
> supposed to mean.
> 
> Additionally now if loading fails, we cache a null pointer.  Should we instead
> cache the empty bitmap?

[tony, 2008-11-03 16:32:35]

LGTM

http://codereview.chromium.org/9246/diff/1/2
File chrome/common/resource_bundle.cc (right):

http://codereview.chromium.org/9246/diff/1/2#newcode159
Line 159: found = true;
How does this work with two variables named found of different types?

http://codereview.chromium.org/9246/diff/1/2#newcode181
Line 181: // TO REVIEWER: this comment looks truncated, what should it be?
I think the comment is supposed to apply to the code that was added in r3922.  I
don't see that in your diff, maybe you need to re-sync?  Anyway, the comment can
probably be removed as it's removed from the actual code it describes.

[tony, 2008-11-03 17:16:34]

LGTM!

On 2008/11/03 16:32:35, tony wrote:
> LGTM
> 
> http://codereview.chromium.org/9246/diff/1/2
> File chrome/common/resource_bundle.cc (right):
> 
> http://codereview.chromium.org/9246/diff/1/2#newcode159
> Line 159: found = true;
> How does this work with two variables named found of different types?
> 
> http://codereview.chromium.org/9246/diff/1/2#newcode181
> Line 181: // TO REVIEWER: this comment looks truncated, what should it be?
> I think the comment is supposed to apply to the code that was added in r3922. 
I
> don't see that in your diff, maybe you need to re-sync?  Anyway, the comment
can
> probably be removed as it's removed from the actual code it describes.

[brettw, 2008-11-03 19:12:46]

LGTM