Load 2x resources on demand

Load 2x resources on demand
 Currently, this always load 1x. This will be fixed in follow up CL to use currently used scale factor.

* Added thread check to ImageSkiaStorage using NonThreadSafe.
 This is important as GetRepresentation may change the stage
now even for the Image obtained from ResourceBundle.
* Added SetReadOnly to protect read only resources (ones in ResourceBundle) from being modified by accident.
* Addded MakeThreadSafe to guarantee that it can be safely accessed from multiple threads.

BUG=141351, 144367
TEST=covered by tests. Also run with --force-device-scale-factor=2 and --load-2x-resources and make sure chrome still uses 2x resources where available.

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

[sky, 2012-08-08 14:41:24]

http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.cc (right):

http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:349: // Check to see if the image is already
in the cache.
I believe this code can be invoked on any thread, which implies the code you're
adding and related code needs to be thread safe. Is that right? If so, it has
pretty far reaching ramifications for ImageSkia and ImageSkiaSource.

[oshima, 2012-08-08 15:12:15]

http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.cc (right):

http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:349: // Check to see if the image is already
in the cache.
On 2012/08/08 14:41:24, sky wrote:
> I believe this code can be invoked on any thread, which implies the code
you're
> adding and related code needs to be thread safe. Is that right? If so, it has
> pretty far reaching ramifications for ImageSkia and ImageSkiaSource.

This method is still thread safe. However, returned Image isn't thread safe, and
it's been thread unsafe before this CL because Image.GetRepresentation isn't
thread safe.
Does Image need to be thread safe? If so that's a requirement we need to address
separately.

Do you know where we're using this in non UI thread?

[sky, 2012-08-08 16:10:48]

On Wed, Aug 8, 2012 at 8:12 AM,  <oshima@chromium.org> wrote:
>
>
http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
> File ui/base/resource/resource_bundle.cc (right):
>
>
http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
> ui/base/resource/resource_bundle.cc:349: // Check to see if the image is
> already in the cache.
> On 2012/08/08 14:41:24, sky wrote:
>>
>> I believe this code can be invoked on any thread, which implies the
>
> code you're
>>
>> adding and related code needs to be thread safe. Is that right? If so,
>
> it has
>>
>> pretty far reaching ramifications for ImageSkia and ImageSkiaSource.
>
>
> This method is still thread safe. However, returned Image isn't thread
> safe, and
> it's been thread unsafe before this CL because Image.GetRepresentation
> isn't thread safe.

Ya, that's what I meant.

> Does Image need to be thread safe? If so that's a requirement we need to
> address separately.
>
> Do you know where we're using this in non UI thread?

This I don't know. The only place I can think that might use images
off the ui thread is theme caching, but I'm not sure. Tony might know.
I suspect you're going to have to do some investigation.

  -Scott

>
> http://codereview.chromium.org/10820049/

[pkotwicz, 2012-08-08 16:40:26]

The code in BrowserThemePack does in fact use images from the resource bundle
off the UI thread. If that's the only place we do this, then its fixable.

[bshe, 2012-08-08 17:26:12]

http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.cc (right):

http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:349: // Check to see if the image is already
in the cache.
Not sure if related, for wallpapers, I believe we also use this function in a
WorkerPool thread. 

On 2012/08/08 15:12:16, oshima wrote:
> On 2012/08/08 14:41:24, sky wrote:
> > I believe this code can be invoked on any thread, which implies the code
> you're
> > adding and related code needs to be thread safe. Is that right? If so, it
has
> > pretty far reaching ramifications for ImageSkia and ImageSkiaSource.
> 
> This method is still thread safe. However, returned Image isn't thread safe,
and
> it's been thread unsafe before this CL because Image.GetRepresentation isn't
> thread safe.
> Does Image need to be thread safe? If so that's a requirement we need to
address
> separately.
> 
> Do you know where we're using this in non UI thread?

[oshima, 2012-08-09 00:29:25]

Thank you guys for info. I'll investigate thread safety issue and will get back
to you.

On 2012/08/08 17:26:12, bshe wrote:
>
http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
> File ui/base/resource/resource_bundle.cc (right):
> 
>
http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
> ui/base/resource/resource_bundle.cc:349: // Check to see if the image is
already
> in the cache.
> Not sure if related, for wallpapers, I believe we also use this function in a
> WorkerPool thread. 
> 
> On 2012/08/08 15:12:16, oshima wrote:
> > On 2012/08/08 14:41:24, sky wrote:
> > > I believe this code can be invoked on any thread, which implies the code
> > you're
> > > adding and related code needs to be thread safe. Is that right? If so, it
> has
> > > pretty far reaching ramifications for ImageSkia and ImageSkiaSource.
> > 
> > This method is still thread safe. However, returned Image isn't thread safe,
> and
> > it's been thread unsafe before this CL because Image.GetRepresentation isn't
> > thread safe.
> > Does Image need to be thread safe? If so that's a requirement we need to
> address
> > separately.
> > 
> > Do you know where we're using this in non UI thread?

[oshima, 2012-08-09 07:38:33]

I looked into both cases, and they seems to be fine.

1) Theme Pack: BrowserThemePack generates image reps for scale_factors_ in UI
thread, and then
   repack the same image reps in FILE thread. Since all image reps are already
obtained, all retrieval operations
   will be just read, so this should be thread safe. We need to be careful about
not to unload reps from these images
   when we implement unloading. I'll include this requirement to the design.
2) Wallpaper: the image for wall paper is used only after it's loaded. That is,
only one thread touch the image
   at a time, so this is safe.

For both cases, ResourceBundle::GetImageNamed has to be thread safe, but the
Image class do not have to be thread safe.

I'm going to investigate other cases.

On 2012/08/09 00:29:25, oshima wrote:
> Thank you guys for info. I'll investigate thread safety issue and will get
back
> to you.
> 
> On 2012/08/08 17:26:12, bshe wrote:
> >
>
http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
> > File ui/base/resource/resource_bundle.cc (right):
> > 
> >
>
http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
> > ui/base/resource/resource_bundle.cc:349: // Check to see if the image is
> already
> > in the cache.
> > Not sure if related, for wallpapers, I believe we also use this function in
a
> > WorkerPool thread. 
> > 
> > On 2012/08/08 15:12:16, oshima wrote:
> > > On 2012/08/08 14:41:24, sky wrote:
> > > > I believe this code can be invoked on any thread, which implies the code
> > > you're
> > > > adding and related code needs to be thread safe. Is that right? If so,
it
> > has
> > > > pretty far reaching ramifications for ImageSkia and ImageSkiaSource.
> > > 
> > > This method is still thread safe. However, returned Image isn't thread
safe,
> > and
> > > it's been thread unsafe before this CL because Image.GetRepresentation
isn't
> > > thread safe.
> > > Does Image need to be thread safe? If so that's a requirement we need to
> > address
> > > separately.
> > > 
> > > Do you know where we're using this in non UI thread?

[pkotwicz, 2012-08-09 16:01:47]

I think that BrowserThemePack::RepackImages can also generate image reps. Only
some of the image reps are generated on the UI thread before the data pack is
written to disk.

[sky, 2012-08-09 16:35:01]

I think we have two options here:
. Ideally we only need access on the ui thread, in which case we add a
DCHECK to that effect and say this code isn't thread safe.
. All this code needs to be thread safe.

  -Scott

On Thu, Aug 9, 2012 at 12:38 AM,  <oshima@chromium.org> wrote:
> I looked into both cases, and they seems to be fine.
>
> 1) Theme Pack: BrowserThemePack generates image reps for scale_factors_ in
> UI
> thread, and then
>    repack the same image reps in FILE thread. Since all image reps are
> already
> obtained, all retrieval operations
>    will be just read, so this should be thread safe. We need to be careful
> about
> not to unload reps from these images
>    when we implement unloading. I'll include this requirement to the design.
> 2) Wallpaper: the image for wall paper is used only after it's loaded. That
> is,
> only one thread touch the image
>    at a time, so this is safe.
>
> For both cases, ResourceBundle::GetImageNamed has to be thread safe, but the
> Image class do not have to be thread safe.
>
> I'm going to investigate other cases.
>
>
> On 2012/08/09 00:29:25, oshima wrote:
>>
>> Thank you guys for info. I'll investigate thread safety issue and will get
>
> back
>>
>> to you.
>
>
>> On 2012/08/08 17:26:12, bshe wrote:
>> >
>
>
>
http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
>>
>> > File ui/base/resource/resource_bundle.cc (right):
>> >
>> >
>
>
>
http://codereview.chromium.org/10820049/diff/7004/ui/base/resource/resource_b...
>>
>> > ui/base/resource/resource_bundle.cc:349: // Check to see if the image is
>> already
>> > in the cache.
>> > Not sure if related, for wallpapers, I believe we also use this function
>> > in
>
> a
>>
>> > WorkerPool thread.
>> >
>> > On 2012/08/08 15:12:16, oshima wrote:
>> > > On 2012/08/08 14:41:24, sky wrote:
>> > > > I believe this code can be invoked on any thread, which implies the
>> > > > code
>> > > you're
>> > > > adding and related code needs to be thread safe. Is that right? If
>> > > > so,
>
> it
>>
>> > has
>> > > > pretty far reaching ramifications for ImageSkia and ImageSkiaSource.
>> > >
>> > > This method is still thread safe. However, returned Image isn't thread
>
> safe,
>>
>> > and
>> > > it's been thread unsafe before this CL because Image.GetRepresentation
>
> isn't
>>
>> > > thread safe.
>> > > Does Image need to be thread safe? If so that's a requirement we need
>> > > to
>> > address
>> > > separately.
>> > >
>> > > Do you know where we're using this in non UI thread?
>
>
>
>
> http://codereview.chromium.org/10820049/

[oshima, 2012-08-10 01:08:10]

On 2012/08/09 16:01:47, pkotwicz wrote:
> I think that BrowserThemePack::RepackImages can also generate image reps. Only
> some of the image reps are generated on the UI thread before the data pack is
> written to disk.

I somehow misread the code, and yes It does seems to generate images, which
means
it's already broken and violates the comment in borwser_theme_pack.h.
I think we need to move it to UI thread.

There is another thing that came to my mind while I'm looking. I believe theme
is syncable.
What happens/how this works when I create theme on Mac/ChromeOS (with supported
scale factor CL),
and sync it on Win/Linux (and vice versa)?

[sky, 2012-08-10 16:10:51]

On Thu, Aug 9, 2012 at 6:08 PM,  <oshima@chromium.org> wrote:
> On 2012/08/09 16:01:47, pkotwicz wrote:
>>
>> I think that BrowserThemePack::RepackImages can also generate image reps.
>> Only
>> some of the image reps are generated on the UI thread before the data pack
>> is
>> written to disk.
>
>
> I somehow misread the code, and yes It does seems to generate images, which
> means
> it's already broken and violates the comment in borwser_theme_pack.h.
> I think we need to move it to UI thread.
>
> There is another thing that came to my mind while I'm looking. I believe
> theme
> is syncable.
> What happens/how this works when I create theme on Mac/ChromeOS (with
> supported
> scale factor CL),
> and sync it on Win/Linux (and vice versa)?

I'm not sure, but I think you're just syncing the url of the theme,
not the actual theme data. I could be totally wrong.

  -Scott

[oshima, 2012-08-23 07:23:13]

Scott, I added the code to detect multiple thread access and fixed a few places
that violates this. I tested theme, extensions and few others and things seems
to be working. I also added CHECK so that we can catch it on dev/beta release. I
filed a blocker bug to remove it before m23 release.

Peter, can you answer my question below?

http://codereview.chromium.org/10820049/diff/29018/chrome/browser/themes/brow...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/10820049/diff/29018/chrome/browser/themes/brow...
chrome/browser/themes/browser_theme_pack.cc:557:
RepackImages(images_on_file_thread_, &reencoded_images);
Peter, is it ok to delete images in image_on_file_thread_? I think they're not
used once saved to disk?

[oshima, 2012-08-23 14:21:08]

http://codereview.chromium.org/10820049/diff/31002/ui/gfx/image/image_skia.cc
File ui/gfx/image/image_skia.cc (right):

http://codereview.chromium.org/10820049/diff/31002/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:108: // TODO(oshima): Make this DCHECK for M23.
crbug.com/144367.
Looks like NonThreadSafe is no-op in release build, so this won't fire on
release build. I'll change this to DCHECK.

[sky, 2012-08-23 16:32:32]

http://codereview.chromium.org/10820049/diff/31005/chrome/browser/themes/brow...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/10820049/diff/31005/chrome/browser/themes/brow...
chrome/browser/themes/browser_theme_pack.cc:441: for (ImageCache::iterator it =
pack->images_on_file_thread_.begin();
This is too error prone. I think we should make it so ImageSkia CHECKs your on
the UI thread always. If some one needs to use it off the UI thread they should
copy out the bitmaps.

http://codereview.chromium.org/10820049/diff/31005/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

http://codereview.chromium.org/10820049/diff/31005/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:128: // When the soruce is available, generates all
ImageReps for
soruce -> source

[oshima, 2012-08-23 16:57:31]

http://codereview.chromium.org/10820049/diff/31005/chrome/browser/themes/brow...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/10820049/diff/31005/chrome/browser/themes/brow...
chrome/browser/themes/browser_theme_pack.cc:441: for (ImageCache::iterator it =
pack->images_on_file_thread_.begin();
On 2012/08/23 16:32:32, sky wrote:
> This is too error prone. I think we should make it so ImageSkia CHECKs your on
> the UI thread always.

Yes it's error prone and that's why I added NonThreadSafe and
DCHECK(CalledOnValidThread())
in ImageSkia. That's how this has been detected.

Unfortunately, we can't check if it's UI thread (In fact, I tried it first)
because this is ui:: component and can't depend on content. Also, this is used
in renderer process where there is no UI thread.

> If some one needs to use it off the UI thread they should
> copy out the bitmaps.

images_on_file_thread_ IS copy of image_on_ui_thread_. This is to make sure
that images_on_file_thread_ has all bitmaps so that FILE thread do not have to
load them. Alternative way is to detach these ImageSkia from UI thread so that
FILE thread can load them. This is also safe because they're copy and won't be
used
by UI thread after this.

http://codereview.chromium.org/10820049/diff/31005/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

http://codereview.chromium.org/10820049/diff/31005/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:128: // When the soruce is available, generates all
ImageReps for
On 2012/08/23 16:32:32, sky wrote:
> soruce -> source

Done.

[pkotwicz, 2012-08-23 16:58:48]

http://codereview.chromium.org/10820049/diff/18025/chrome/browser/themes/brow...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/10820049/diff/18025/chrome/browser/themes/brow...
chrome/browser/themes/browser_theme_pack.cc:447: }
Why don't you make images_on_ui_thread_ have images for all supported scale
factors and then do a copy to images_on_file_thread_?
Also you don't need the code below: pack->GenerateImageReps() anymore.

[oshima, 2012-08-23 17:16:50]

http://codereview.chromium.org/10820049/diff/18025/chrome/browser/themes/brow...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/10820049/diff/18025/chrome/browser/themes/brow...
chrome/browser/themes/browser_theme_pack.cc:447: }
On 2012/08/23 16:58:48, pkotwicz wrote:
> Why don't you make images_on_ui_thread_ have images for all supported scale
> factors and then do a copy to images_on_file_thread_?

Because images in UI thread only needs scales that are in use.
It'd be waste of memory to load them all. That's why I was asking if we can
release images_on_file_thread_ after it's written.

> Also you don't need the code below: pack->GenerateImageReps() anymore.

Ah, you're right. Thank you for the catch! Done.

[sky, 2012-08-23 19:44:09]

Can we add checks to ResourceBundle that loading only happens on the UI thread?

http://codereview.chromium.org/10820049/diff/18026/chrome/browser/themes/brow...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/10820049/diff/18026/chrome/browser/themes/brow...
chrome/browser/themes/browser_theme_pack.cc:439: // Make sure the
images_on_file_thread_ has bitmaps for supported
|s around images_on_file_thread_

http://codereview.chromium.org/10820049/diff/18026/chrome/browser/themes/brow...
chrome/browser/themes/browser_theme_pack.cc:442: it !=
pack->images_on_file_thread_.end(); ++it) {
I still think this should copy off the images rather than use ImageSkia.

http://codereview.chromium.org/10820049/diff/18026/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.cc (right):

http://codereview.chromium.org/10820049/diff/18026/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:66: class
ResourceBundle::ResourceBundleImageSource : public gfx::ImageSkiaSource {
Add a description.

http://codereview.chromium.org/10820049/diff/18026/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:129: int resource_id_;
Make these two const.

[pkotwicz, 2012-08-23 22:20:33]

http://codereview.chromium.org/10820049/diff/18028/chrome/browser/themes/brow...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/10820049/diff/18028/chrome/browser/themes/brow...
chrome/browser/themes/browser_theme_pack.cc:1124: // Attempt to generate image
reps for all supported scale factors.
With your change, this for loop should be dead code. Can you remove it?

[oshima, 2012-08-24 16:45:34]

Scott, I added read only mode, and updated places that access ImageSkia from non
UI thread to use read only ImageSkia. For deep copy, I don't yet find the case
that I need it (where one thread modifying while others are accessing the same
ImageSkia), but added it anyway for wallpaper_manager.

This is not ready for check in (there are a few more check failures I need to
fix, and missing tests, which I'm working on), but I want to know if this
approach looks good to you. Can you take a look look at this?

http://codereview.chromium.org/10820049/diff/18026/chrome/browser/themes/brow...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/10820049/diff/18026/chrome/browser/themes/brow...
chrome/browser/themes/browser_theme_pack.cc:439: // Make sure the
images_on_file_thread_ has bitmaps for supported
On 2012/08/23 19:44:09, sky wrote:
> |s around images_on_file_thread_

Done.

http://codereview.chromium.org/10820049/diff/18026/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.cc (right):

http://codereview.chromium.org/10820049/diff/18026/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:66: class
ResourceBundle::ResourceBundleImageSource : public gfx::ImageSkiaSource {
On 2012/08/23 19:44:09, sky wrote:
> Add a description.

Done.

http://codereview.chromium.org/10820049/diff/18026/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:129: int resource_id_;
On 2012/08/23 19:44:09, sky wrote:
> Make these two const.

Done.

http://codereview.chromium.org/10820049/diff/18028/chrome/browser/themes/brow...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/10820049/diff/18028/chrome/browser/themes/brow...
chrome/browser/themes/browser_theme_pack.cc:1124: // Attempt to generate image
reps for all supported scale factors.
On 2012/08/23 22:20:33, pkotwicz wrote:
> With your change, this for loop should be dead code. Can you remove it?

Done.

[sky, 2012-08-24 18:25:48]

This seems reasonable, but I have one thought below.

http://codereview.chromium.org/10820049/diff/25013/ui/gfx/image/image_skia.cc
File ui/gfx/image/image_skia.cc (right):

http://codereview.chromium.org/10820049/diff/25013/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:240: storage_->AssertModify();
One other thought, should we take a copy on write approach? That is, if
AddRepresentation is invoked we clone the storage? Seems like that would make
things less fragile.

http://codereview.chromium.org/10820049/diff/25013/ui/gfx/image/image_skia.cc...
ui/gfx/image/image_skia.cc:285: storage_->SetReadOnly();
Should this invoke DeleteSource?

[oshima, 2012-08-24 19:06:45]

https://chromiumcodereview.appspot.com/10820049/diff/25013/ui/gfx/image/image...
File ui/gfx/image/image_skia.cc (right):

https://chromiumcodereview.appspot.com/10820049/diff/25013/ui/gfx/image/image...
ui/gfx/image/image_skia.cc:240: storage_->AssertModify();
On 2012/08/24 18:25:48, sky wrote:
> One other thought, should we take a copy on write approach? That is, if
> AddRepresentation is invoked we clone the storage? Seems like that would make
> things less fragile.

Doing so in a thread safe way require locking and will make it more complicated
IMO. Another issue is that the current semantics of shallow
copy (which is used very often) is that the copy shares the storage,
and copy on write changes this semantics. I'm afraid that it will break
a lot of places.

https://chromiumcodereview.appspot.com/10820049/diff/25013/ui/gfx/image/image...
ui/gfx/image/image_skia.cc:285: storage_->SetReadOnly();
On 2012/08/24 18:25:48, sky wrote:
> Should this invoke DeleteSource?

Maybe: I kept it separated because I wanted to make the image returned by
ResourceBundle read only,
but still allow source to generate the image. Bad use will be caught by DCHECK()
in ImageSkiaSource::FindRepresentation, and such code should use
EnsureRepsForSupportedScaleFactors & DeleteSource.

I now think that it's probably not intuitive and am thinking of introducing two
read only mode,
one that prevent modification by client (thus keep source),
and another one to make it thread safe (thus, generate images and remove
source).
What do you think?

[oshima, 2012-08-28 09:33:08]

Added SetReadOnly/MakeThreadSafe/DeepCopy to ImageSkia and updated
related code. PTAL.

[sky, 2012-08-28 17:18:14]

LGTM

http://codereview.chromium.org/10820049/diff/42001/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

http://codereview.chromium.org/10820049/diff/42001/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:72: // the ImageSkiaRep instances that this ImageSkia
currently have.
have -> has

http://codereview.chromium.org/10820049/diff/42001/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:73: // This can be safely passed to and/ manipulated
by another thread.
This -> 'The returned value'
'and/' -> 'and'

[oshima, 2012-08-28 21:23:34]

http://codereview.chromium.org/10820049/diff/42001/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

http://codereview.chromium.org/10820049/diff/42001/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:72: // the ImageSkiaRep instances that this ImageSkia
currently have.
On 2012/08/28 17:18:14, sky wrote:
> have -> has

Done.

http://codereview.chromium.org/10820049/diff/42001/ui/gfx/image/image_skia.h#...
ui/gfx/image/image_skia.h:73: // This can be safely passed to and/ manipulated
by another thread.
On 2012/08/28 17:18:14, sky wrote:
> This -> 'The returned value'
> 'and/' -> 'and'

Done.

[commit-bot: I haz the power, 2012-08-28 21:25:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/10820049/35021

[commit-bot: I haz the power, 2012-08-29 03:46:29]

Change committed as 153846