ash: Avoid reloading already-resized custom wallpaper.

ash: Avoid reloading already-resized custom wallpaper.

Avoid a white flash at startup caused by
DesktopBackgroundController comparing the resized version of
a custom wallpaper image against the original image, seeing
that they are different, and then resizing the original
image again.

BUG=321402
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236852

[oshima, 2013-11-22 03:13:03]

lgtm

[bshe, 2013-11-22 15:07:20]

On 2013/11/22 03:13:03, oshima wrote:
> lgtm

lgtm Thanks for fixing it.

[Daniel Erat, 2013-11-22 15:33:20]

Oshima, I have a threading question for you. :-)

https://codereview.chromium.org/81393004/diff/70001/ash/desktop_background/wa...
File ash/desktop_background/wallpaper_resizer.cc (right):

https://codereview.chromium.org/81393004/diff/70001/ash/desktop_background/wa...
ash/desktop_background/wallpaper_resizer.cc:98: const gfx::ImageSkiaRep&
image_rep = image.GetRepresentation(1.0f);
Oshima, can you comment on whether I'm doing the right thing with all of the
ImageSkia::MakeThreadSafe() calls that I'm adding to this code? Without them,
this GetRepresentation() call appears to result in failed "image_skia.cc(403)]
Check failed: CanRead()." asserts later when I run tests -- see e.g.
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds....

These images _are_ getting accessed from multiple threads (e.g. we first find
the generation ID on the browser thread, then resize the image on the blocking
pool, and then create a new image based on the resized data on the browser
thread again.

I think that the MakeThreadSafe() call in OnResizeFinished() may be extraneous
(since the resized image should only be accessed from the browser thread); I'll
try removing it.

[oshima, 2013-11-22 18:41:14]

On 2013/11/22 15:33:20, Daniel Erat wrote:
> Oshima, I have a threading question for you. :-)
> 
>
https://codereview.chromium.org/81393004/diff/70001/ash/desktop_background/wa...
> File ash/desktop_background/wallpaper_resizer.cc (right):
> 
>
https://codereview.chromium.org/81393004/diff/70001/ash/desktop_background/wa...
> ash/desktop_background/wallpaper_resizer.cc:98: const gfx::ImageSkiaRep&
> image_rep = image.GetRepresentation(1.0f);
> Oshima, can you comment on whether I'm doing the right thing with all of the
> ImageSkia::MakeThreadSafe() calls that I'm adding to this code? Without them,
> this GetRepresentation() call appears to result in failed "image_skia.cc(403)]
> Check failed: CanRead()." asserts later when I run tests -- see e.g.
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds....
> 
> These images _are_ getting accessed from multiple threads (e.g. we first find
> the generation ID on the browser thread, then resize the image on the blocking
> pool, and then create a new image based on the resized data on the browser
> thread again.
> 
> I think that the MakeThreadSafe() call in OnResizeFinished() may be extraneous
> (since the resized image should only be accessed from the browser thread);
I'll
> try removing it.

MakeThreadSafe on wallpaper image shouldn't be harmful as it should contain
fixed unscaled image,
but I'm not 100% certain. I will double check.

[oshima, 2013-11-22 19:20:06]

On 2013/11/22 18:41:14, oshima wrote:
> On 2013/11/22 15:33:20, Daniel Erat wrote:
> > Oshima, I have a threading question for you. :-)
> > 
> >
>
https://codereview.chromium.org/81393004/diff/70001/ash/desktop_background/wa...
> > File ash/desktop_background/wallpaper_resizer.cc (right):
> > 
> >
>
https://codereview.chromium.org/81393004/diff/70001/ash/desktop_background/wa...
> > ash/desktop_background/wallpaper_resizer.cc:98: const gfx::ImageSkiaRep&
> > image_rep = image.GetRepresentation(1.0f);
> > Oshima, can you comment on whether I'm doing the right thing with all of the
> > ImageSkia::MakeThreadSafe() calls that I'm adding to this code? Without
them,
> > this GetRepresentation() call appears to result in failed
"image_skia.cc(403)]
> > Check failed: CanRead()." asserts later when I run tests -- see e.g.
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds....
> > 
> > These images _are_ getting accessed from multiple threads (e.g. we first
find
> > the generation ID on the browser thread, then resize the image on the
blocking
> > pool, and then create a new image based on the resized data on the browser
> > thread again.
> > 
> > I think that the MakeThreadSafe() call in OnResizeFinished() may be
extraneous
> > (since the resized image should only be accessed from the browser thread);
> I'll
> > try removing it.
> 
> MakeThreadSafe on wallpaper image shouldn't be harmful as it should contain
> fixed unscaled image,
> but I'm not 100% certain. I will double check.

All wallpaper images are created by gfx::ImageSkia::CreateFrom1xBitmap, so
there is no issue in calling MakeThreadSafe on them.

[Daniel Erat, 2013-11-22 19:31:25]

On Fri, Nov 22, 2013 at 11:20 AM, <oshima@chromium.org> wrote:

> On 2013/11/22 18:41:14, oshima wrote:
>
>> On 2013/11/22 15:33:20, Daniel Erat wrote:
>> > Oshima, I have a threading question for you. :-)
>> >
>> >
>>
>
> https://codereview.chromium.org/81393004/diff/70001/ash/
> desktop_background/wallpaper_resizer.cc
>
>> > File ash/desktop_background/wallpaper_resizer.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/81393004/diff/70001/ash/
> desktop_background/wallpaper_resizer.cc#newcode98
>
>> > ash/desktop_background/wallpaper_resizer.cc:98: const
>> gfx::ImageSkiaRep&
>> > image_rep = image.GetRepresentation(1.0f);
>> > Oshima, can you comment on whether I'm doing the right thing with all
>> of the
>> > ImageSkia::MakeThreadSafe() calls that I'm adding to this code? Without
>>
> them,
>
>> > this GetRepresentation() call appears to result in failed
>>
> "image_skia.cc(403)]
>
>> > Check failed: CanRead()." asserts later when I run tests -- see e.g.
>> >
>>
>
> http://build.chromium.org/p/tryserver.chromium/builders/
> linux_chromeos/builds/177365.
>
>> >
>> > These images _are_ getting accessed from multiple threads (e.g. we first
>>
> find
>
>> > the generation ID on the browser thread, then resize the image on the
>>
> blocking
>
>> > pool, and then create a new image based on the resized data on the
>> browser
>> > thread again.
>> >
>> > I think that the MakeThreadSafe() call in OnResizeFinished() may be
>>
> extraneous
>
>> > (since the resized image should only be accessed from the browser
>> thread);
>> I'll
>> > try removing it.
>>
>
>  MakeThreadSafe on wallpaper image shouldn't be harmful as it should
>> contain
>> fixed unscaled image,
>> but I'm not 100% certain. I will double check.
>>
>
> All wallpaper images are created by gfx::ImageSkia::CreateFrom1xBitmap, so
> there is no issue in calling MakeThreadSafe on them.
>

We do call CreateFrom1xBitmap() in unit tests -- will calling
MakeThreadSafe() on those images be an issue? (It seems to work.)


> https://codereview.chromium.org/81393004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2013-11-22 19:43:08]

On 2013/11/22 19:31:25, Daniel Erat wrote:
> On Fri, Nov 22, 2013 at 11:20 AM, <mailto:oshima@chromium.org> wrote:
> 
> > On 2013/11/22 18:41:14, oshima wrote:
> >
> >> On 2013/11/22 15:33:20, Daniel Erat wrote:
> >> > Oshima, I have a threading question for you. :-)
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/81393004/diff/70001/ash/
> > desktop_background/wallpaper_resizer.cc
> >
> >> > File ash/desktop_background/wallpaper_resizer.cc (right):
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/81393004/diff/70001/ash/
> > desktop_background/wallpaper_resizer.cc#newcode98
> >
> >> > ash/desktop_background/wallpaper_resizer.cc:98: const
> >> gfx::ImageSkiaRep&
> >> > image_rep = image.GetRepresentation(1.0f);
> >> > Oshima, can you comment on whether I'm doing the right thing with all
> >> of the
> >> > ImageSkia::MakeThreadSafe() calls that I'm adding to this code? Without
> >>
> > them,
> >
> >> > this GetRepresentation() call appears to result in failed
> >>
> > "image_skia.cc(403)]
> >
> >> > Check failed: CanRead()." asserts later when I run tests -- see e.g.
> >> >
> >>
> >
> > http://build.chromium.org/p/tryserver.chromium/builders/
> > linux_chromeos/builds/177365.
> >
> >> >
> >> > These images _are_ getting accessed from multiple threads (e.g. we first
> >>
> > find
> >
> >> > the generation ID on the browser thread, then resize the image on the
> >>
> > blocking
> >
> >> > pool, and then create a new image based on the resized data on the
> >> browser
> >> > thread again.
> >> >
> >> > I think that the MakeThreadSafe() call in OnResizeFinished() may be
> >>
> > extraneous
> >
> >> > (since the resized image should only be accessed from the browser
> >> thread);
> >> I'll
> >> > try removing it.
> >>
> >
> >  MakeThreadSafe on wallpaper image shouldn't be harmful as it should
> >> contain
> >> fixed unscaled image,
> >> but I'm not 100% certain. I will double check.
> >>
> >
> > All wallpaper images are created by gfx::ImageSkia::CreateFrom1xBitmap, so
> > there is no issue in calling MakeThreadSafe on them.
> >
> 
> We do call CreateFrom1xBitmap() in unit tests -- will calling
> MakeThreadSafe() on those images be an issue? (It seems to work.)

No. MakeThreadSafe fetches images for all scale factors if the ImageSkia needs
to
provide images for multiple scale factors. The ImageSkia created by
CreateFrom1xBitmap
provides only one image for all scale factors, and calling MakeThreadSafe on it
simply make it
immutable.

> 
> 
> > https://codereview.chromium.org/81393004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-11-22 20:23:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/81393004/230001

[commit-bot: I haz the power, 2013-11-22 22:44:27]

Change committed as 236852