Invalidate all wallpaper cache if display configuration changed

Invalidate all wallpaper cache if display configuration changed


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

[bshe, 2014-03-10 16:40:38]

Hi Nikita.

Do you mind to take a look at this CL? It should fix the failing test in multi
profile mode.

THanks

[Nikita (slow), 2014-03-11 16:34:49]

lgtm

https://codereview.chromium.org/190993003/diff/1/ash/desktop_background/deskt...
File ash/desktop_background/desktop_background_controller.cc (right):

https://codereview.chromium.org/190993003/diff/1/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_controller.cc:441:
UpdateWallpaper(true);
nit: /* clear cache */

https://codereview.chromium.org/190993003/diff/1/ash/desktop_background/user_...
File ash/desktop_background/user_wallpaper_delegate.h (right):

https://codereview.chromium.org/190993003/diff/1/ash/desktop_background/user_...
ash/desktop_background/user_wallpaper_delegate.h:34: // current display's
resolution.
nit: |clear_cache| parameter description

https://codereview.chromium.org/190993003/diff/1/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://codereview.chromium.org/190993003/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:635:
wallpaper_manager->UpdateWallpaper(false);
nit: /* clear_cache */

https://codereview.chromium.org/190993003/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/wallpaper_manager.cc (left):

https://codereview.chromium.org/190993003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/wallpaper_manager.cc:832: ClearWallpaperCache();
What's the difference?

[bshe, 2014-03-11 22:52:34]

Thanks Nikita.

Do you mind to take another look?

https://codereview.chromium.org/190993003/diff/1/ash/desktop_background/deskt...
File ash/desktop_background/desktop_background_controller.cc (right):

https://codereview.chromium.org/190993003/diff/1/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_controller.cc:441:
UpdateWallpaper(true);
On 2014/03/11 16:34:50, Nikita Kostylev wrote:
> nit: /* clear cache */

Done.

https://codereview.chromium.org/190993003/diff/1/ash/desktop_background/user_...
File ash/desktop_background/user_wallpaper_delegate.h (right):

https://codereview.chromium.org/190993003/diff/1/ash/desktop_background/user_...
ash/desktop_background/user_wallpaper_delegate.h:34: // current display's
resolution.
On 2014/03/11 16:34:50, Nikita Kostylev wrote:
> nit: |clear_cache| parameter description

Done.

https://codereview.chromium.org/190993003/diff/1/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://codereview.chromium.org/190993003/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:635:
wallpaper_manager->UpdateWallpaper(false);
On 2014/03/11 16:34:50, Nikita Kostylev wrote:
> nit: /* clear_cache */

Done.

https://codereview.chromium.org/190993003/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/wallpaper_manager.cc (left):

https://codereview.chromium.org/190993003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/wallpaper_manager.cc:832: ClearWallpaperCache();
ClearWallpaperCache is probably misnamed. It will not clear the wallpaper cache
for logged in users in multi profile mode. This is preferred as we want a smooth
animation when switching users at multi profile mode.

However, if UpdateWallpaper is called after a large screen is hooked up. At
multi profile mode, we still need to invalidate all wallpaper caches. Otherwise,
when we switch user, the cached small resolution wallpaper is used. But we are
expecting large resolution wallpapers. And that's why the test failed.

If UpdateWallpaper is called by wallpaper private api, we don't want to
invalidate all the caches.

 
On 2014/03/11 16:34:50, Nikita Kostylev wrote:
> What's the difference?

[Nikita (slow), 2014-03-12 07:34:40]

lgtm

[bshe, 2014-03-12 14:47:55]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2014-03-12 14:48:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/190993003/40001

[commit-bot: I haz the power, 2014-03-12 15:19:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-12 15:19:30]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[bshe, 2014-03-13 03:27:11]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2014-03-13 03:27:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/190993003/80001

[commit-bot: I haz the power, 2014-03-13 03:47:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 03:47:40]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[bshe, 2014-03-13 15:34:19]

+oshima for the following files:

ash/default_user_wallpaper_delegate.cc
ash/default_user_wallpaper_delegate.h
ash/test/test_user_wallpaper_delegate.cc
ash/test/test_user_wallpaper_delegate.h
chrome/browser/ui/ash/user_wallpaper_delegate_win.cc

Thanks!

[oshima, 2014-03-13 20:13:39]

a couple of questions.

https://codereview.chromium.org/190993003/diff/80001/ash/desktop_background/u...
File ash/desktop_background/user_wallpaper_delegate.h (right):

https://codereview.chromium.org/190993003/diff/80001/ash/desktop_background/u...
ash/desktop_background/user_wallpaper_delegate.h:36: // need to load a different
resolution wallpaper. All the previous wallpaper
we need to clear the cache only if one of displays' pixel size becomes bigger
than one stored in the cache.

Do we really clearing the cache for all display change?

Can you also mentioned about memory vs disk cache? I assume this is in memory
cache and won't clear disk cache right?

https://codereview.chromium.org/190993003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/190993003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:1261:
wallpaper_cache_[user_id] = wallpaper.image();
what would be an user_id when chrome is in login screen?

[bshe, 2014-03-13 22:16:46]

Thanks for review! Would you mind to take another look?

https://codereview.chromium.org/190993003/diff/80001/ash/desktop_background/u...
File ash/desktop_background/user_wallpaper_delegate.h (right):

https://codereview.chromium.org/190993003/diff/80001/ash/desktop_background/u...
ash/desktop_background/user_wallpaper_delegate.h:36: // need to load a different
resolution wallpaper. All the previous wallpaper
Sorry. The comment is not misleading. UpdateWallpeper is only called when
displays' pixel size is larger than previous stored value.

On 2014/03/13 20:13:39, oshima wrote:
> we need to clear the cache only if one of displays' pixel size becomes bigger
> than one stored in the cache.
> 
> Do we really clearing the cache for all display change?
> 
> Can you also mentioned about memory vs disk cache? I assume this is in memory
> cache and won't clear disk cache right?

https://codereview.chromium.org/190993003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/190993003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wallpaper_manager.cc:1261:
wallpaper_cache_[user_id] = wallpaper.image();
The user id would be the email of the selected user. If no user is selected
(GAIA login flow or new user login page), it is UserManager::kSignInUser.

On 2014/03/13 20:13:39, oshima wrote:
> what would be an user_id when chrome is in login screen?

[oshima, 2014-03-13 22:23:40]

https://codereview.chromium.org/190993003/diff/100001/ash/desktop_background/...
File ash/desktop_background/user_wallpaper_delegate.h (right):

https://codereview.chromium.org/190993003/diff/100001/ash/desktop_background/...
ash/desktop_background/user_wallpaper_delegate.h:37: // previous small
resolution wallpaper cache should be cleared.
I think the comment is still not clear because screen size changes when you
change the UI scaling or rotate. Can you explicitly say the display's native
resolution (or pixel resolution) has changed?

[bshe, 2014-03-13 23:19:04]

Thanks

https://codereview.chromium.org/190993003/diff/100001/ash/desktop_background/...
File ash/desktop_background/user_wallpaper_delegate.h (right):

https://codereview.chromium.org/190993003/diff/100001/ash/desktop_background/...
ash/desktop_background/user_wallpaper_delegate.h:37: // previous small
resolution wallpaper cache should be cleared.
Used native resolution to be more specific.

On 2014/03/13 22:23:40, oshima wrote:
> I think the comment is still not clear because screen size changes when you
> change the UI scaling or rotate. Can you explicitly say the display's native
> resolution (or pixel resolution) has changed?

[oshima, 2014-03-13 23:36:07]

lgtm

Thanks!

[bshe, 2014-03-13 23:36:53]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2014-03-13 23:37:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/190993003/120001

[commit-bot: I haz the power, 2014-03-14 00:26:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/190993003/120001

[commit-bot: I haz the power, 2014-03-14 01:50:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-14 01:50:18]

Retried try job too often on linux_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[bshe, 2014-03-17 15:32:52]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2014-03-17 15:37:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/190993003/120001

[commit-bot: I haz the power, 2014-03-17 16:18:55]

Change committed as 257440