[ash-md] WIP Added wallpaper color caching.

[ash-md] WIP Added wallpaper color caching.

Extracting the color from the wallpaper is an expensive operation. This change
adds a cache to prevent wasting cycles.

TODO:
- Use a unique image identifier. File path is most likely candidate.
- Determine where the cache should be initialized
- Implement cache purging
- Add tests

BUG=595010
TEST=TODO

[bruthig, 2017-03-28 13:19:52]

Description was changed from

==========
[ash-md] WIP Added wallpaper color caching.

Extracting the color from the wallpaper is an expensive operation. This change
adds a cache to prevent wasting cycles.

BUG=595010
TEST=TODO
==========

to

==========
[ash-md] WIP Added wallpaper color caching.

Extracting the color from the wallpaper is an expensive operation. This change
adds a cache to prevent wasting cycles.

TODO:
- Use a unique image identifier. File path is most likely candidate.
- Determine where the cache should be initialized
- Implement cache purging
- Add tests

BUG=595010
TEST=TODO
==========

[bruthig, 2017-03-28 13:32:21]

bruthig@chromium.org changed reviewers:
+ bshe@chromium.org, sky@chromium.org

[bruthig, 2017-03-28 13:32:21]

Hi All,

This CL still has some outstanding work to be done but I have some higher level
design questions that I'm looking for your help on.

Specifically:
- Any objections to the (PrefBased)WallpaperColorCache design?
- Where should the cache be initialized? (Currently done in ash_init.cc)
- Where should the cache prefs be registered? (Currently done in
browser_prefs.cc)
- Any objections to piping in the wallpaper file path as the key for the cache?
Or are there better alternatives?
- Any objections to a simple purge strategy for the cache? i.e. max size and
purging the oldest records on SetColor().

Thanks,
Ben

[jonross, 2017-03-28 14:33:26]

jonross@chromium.org changed reviewers:
+ jonross@chromium.org

[jonross, 2017-03-28 14:33:30]

https://codereview.chromium.org/2772313004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/2772313004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:394:
ash::Shell::GetInstance()->wallpaper_controller()->SetWallpaperColorCache(
This pattern is the older method of having chrome inject delegates into ash.

This wouldn't work in the mash world. However it is not blocked by the
preferences changes which would delay access to the preference service via ash.

https://codereview.chromium.org/2772313004/diff/20001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2772313004/diff/20001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:440:
wallpaper::PrefBasedWallpaperColorCache::RegisterPrefs(registry);
I would merge this with WallpaperManagerBase::RegisterPrefs

https://codereview.chromium.org/2772313004/diff/20001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_init.cc (right):

https://codereview.chromium.org/2772313004/diff/20001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_init.cc:109:
ash::Shell::GetInstance()->wallpaper_controller()->SetWallpaperColorCache(
This may be more appropriate in the chromeos::WallpaperManager.

Ideally, once the pref service is ready, this would not be needed at all. The
ash::WallpaperController would be able to instantiate the color cache itself.

https://codereview.chromium.org/2772313004/diff/20001/components/wallpaper/pr...
File components/wallpaper/pref_based_wallpaper_color_cache.cc (right):

https://codereview.chromium.org/2772313004/diff/20001/components/wallpaper/pr...
components/wallpaper/pref_based_wallpaper_color_cache.cc:66: "%s.%u.%s",
kWallpaperImagesNodeKey, image_id, kColorNodeKey);
This key seems more complex than is required.

Would not the image_id be sufficient?

https://codereview.chromium.org/2772313004/diff/20001/components/wallpaper/pr...
components/wallpaper/pref_based_wallpaper_color_cache.cc:70: if
(info_dict->GetString(color_key, &color_str)) {
You can store / extract integers directly as keys in DictionaryValue::SetInteger
/ GetInteger

[bruthig, 2017-03-28 15:18:51]

https://codereview.chromium.org/2772313004/diff/20001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_init.cc (right):

https://codereview.chromium.org/2772313004/diff/20001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_init.cc:109:
ash::Shell::GetInstance()->wallpaper_controller()->SetWallpaperColorCache(
On 2017/03/28 14:33:28, jonross wrote:
> This may be more appropriate in the chromeos::WallpaperManager.
> 
> Ideally, once the pref service is ready, this would not be needed at all. The
> ash::WallpaperController would be able to instantiate the color cache itself.

I tried to initialize the cache in the WallpaperManager in a few places but the
ash::Shell was not available yet I think.

[jonross, 2017-03-28 18:24:35]

https://codereview.chromium.org/2772313004/diff/20001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_init.cc (right):

https://codereview.chromium.org/2772313004/diff/20001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_init.cc:109:
ash::Shell::GetInstance()->wallpaper_controller()->SetWallpaperColorCache(
On 2017/03/28 15:18:50, bruthig wrote:
> On 2017/03/28 14:33:28, jonross wrote:
> > This may be more appropriate in the chromeos::WallpaperManager.
> > 
> > Ideally, once the pref service is ready, this would not be needed at all.
The
> > ash::WallpaperController would be able to instantiate the color cache
itself.
> 
> I tried to initialize the cache in the WallpaperManager in a few places but
the
> ash::Shell was not available yet I think.

That's a bit scary. A lot of login screen code relies heavily on the ash::Shell.

However if there is a weird startup order, this spot could work for setting up
this connection.

[bshe, 2017-03-28 21:33:26]

https://codereview.chromium.org/2772313004/diff/20001/components/wallpaper/pr...
File components/wallpaper/pref_based_wallpaper_color_cache_unittest.cc (right):

https://codereview.chromium.org/2772313004/diff/20001/components/wallpaper/pr...
components/wallpaper/pref_based_wallpaper_color_cache_unittest.cc:59:
EXPECT_FALSE(cache_.GetColor(image_1, &color));
why do you want to purge the cache? I imagine you could use it when next time
the user login or when different user use the same wallpaper?

[bruthig, 2017-03-28 21:35:51]

https://codereview.chromium.org/2772313004/diff/20001/components/wallpaper/pr...
File components/wallpaper/pref_based_wallpaper_color_cache_unittest.cc (right):

https://codereview.chromium.org/2772313004/diff/20001/components/wallpaper/pr...
components/wallpaper/pref_based_wallpaper_color_cache_unittest.cc:59:
EXPECT_FALSE(cache_.GetColor(image_1, &color));
On 2017/03/28 21:33:26, bshe wrote:
> why do you want to purge the cache? I imagine you could use it when next time
> the user login or when different user use the same wallpaper?

I don't plan to purge all entries in it.  Some should persist, but I don't want
it to grow unbounded.

[bshe, 2017-03-29 01:38:08]

Also, it looks like that you have two parameters (LumaRange and SaturationRange)
when calculate color. Are these pre user setting or global setting? If it is pre
user setting, using wallpaper id might not be enough to get the calculated
color?

https://codereview.chromium.org/2772313004/diff/20001/components/wallpaper/pr...
File components/wallpaper/pref_based_wallpaper_color_cache_unittest.cc (right):

https://codereview.chromium.org/2772313004/diff/20001/components/wallpaper/pr...
components/wallpaper/pref_based_wallpaper_color_cache_unittest.cc:59:
EXPECT_FALSE(cache_.GetColor(image_1, &color));
On 2017/03/28 21:35:51, bruthig wrote:
> On 2017/03/28 21:33:26, bshe wrote:
> > why do you want to purge the cache? I imagine you could use it when next
time
> > the user login or when different user use the same wallpaper?
> 
> I don't plan to purge all entries in it.  Some should persist, but I don't
want
> it to grow unbounded.
I see. Although from the unit test, it looks like you invalidate previous
calculated color when a
new wallpaper displayed?

[bruthig, 2017-03-29 01:43:48]

On 2017/03/29 01:38:08, bshe wrote:
> Also, it looks like that you have two parameters (LumaRange and
SaturationRange)
> when calculate color. Are these pre user setting or global setting? If it is
pre
> user setting, using wallpaper id might not be enough to get the calculated
> color?

The plan is for these to be global settings.  Although we may need to add
special handling for users who have used non-default command line values. Or we
could incorporate that into the pref structure.  I'd rather shelve that convo to
settle the higher level design issues though.

https://codereview.chromium.org/2772313004/diff/20001/components/wallpaper/pr...
File components/wallpaper/pref_based_wallpaper_color_cache_unittest.cc (right):

https://codereview.chromium.org/2772313004/diff/20001/components/wallpaper/pr...
components/wallpaper/pref_based_wallpaper_color_cache_unittest.cc:59:
EXPECT_FALSE(cache_.GetColor(image_1, &color));
On 2017/03/29 01:38:08, bshe wrote:
> On 2017/03/28 21:35:51, bruthig wrote:
> > On 2017/03/28 21:33:26, bshe wrote:
> > > why do you want to purge the cache? I imagine you could use it when next
> time
> > > the user login or when different user use the same wallpaper?
> > 
> > I don't plan to purge all entries in it.  Some should persist, but I don't
> want
> > it to grow unbounded.
> I see. Although from the unit test, it looks like you invalidate previous
> calculated color when a
> new wallpaper displayed?

This test was written under the assumption the cache was initialized with a max
size of 1.

[bshe, 2017-03-29 15:18:18]

On 2017/03/29 01:43:48, bruthig wrote:
> On 2017/03/29 01:38:08, bshe wrote:
> > Also, it looks like that you have two parameters (LumaRange and
> SaturationRange)
> > when calculate color. Are these pre user setting or global setting? If it is
> pre
> > user setting, using wallpaper id might not be enough to get the calculated
> > color?
> 
> The plan is for these to be global settings.  Although we may need to add
> special handling for users who have used non-default command line values. Or
we
> could incorporate that into the pref structure.  I'd rather shelve that convo
to
> settle the higher level design issues though.

Hmm. If user could change it through non-default command line(assuming they can
do it though about://flags),
how do we know which value they use before login? We might want to include
user's hash id into the key for
retrieving color value.

> 
>
https://codereview.chromium.org/2772313004/diff/20001/components/wallpaper/pr...
> File components/wallpaper/pref_based_wallpaper_color_cache_unittest.cc
(right):
> 
>
https://codereview.chromium.org/2772313004/diff/20001/components/wallpaper/pr...
> components/wallpaper/pref_based_wallpaper_color_cache_unittest.cc:59:
> EXPECT_FALSE(cache_.GetColor(image_1, &color));
> On 2017/03/29 01:38:08, bshe wrote:
> > On 2017/03/28 21:35:51, bruthig wrote:
> > > On 2017/03/28 21:33:26, bshe wrote:
> > > > why do you want to purge the cache? I imagine you could use it when next
> > time
> > > > the user login or when different user use the same wallpaper?
> > > 
> > > I don't plan to purge all entries in it.  Some should persist, but I don't
> > want
> > > it to grow unbounded.
> > I see. Although from the unit test, it looks like you invalidate previous
> > calculated color when a
> > new wallpaper displayed?
> 
> This test was written under the assumption the cache was initialized with a
max
> size of 1.
Got you. Thanks for explaining.