Remove use of index in wallpaper picker code and some refactor

Remove use of index in wallpaper picker code and some refactor


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

[bshe, 2012-11-16 02:20:32]

This CL removes most of the old wallpaper picker code. I have removed the
disable_new_wallpaper_picker flag in a previous CL. So it should be fine to
remove all these files.
Also there are some code refactor in ash/* to remove default wallpaper index as
suggested by sky@.

+sky for ash/*

+nkostylev for chromeos/*

+jhawkins for resources/* and ui/webui/*

Thanks!

[sky, 2012-11-16 03:22:06]

Any chance we could split these out to remove first, then cleanup?
That way I have no problem with TBRing removal.

  -Scott

On Thu, Nov 15, 2012 at 6:20 PM,  <bshe@chromium.org> wrote:
> Reviewers: sky, Nikita Kostylev, James Hawkins,
>
> Message:
> This CL removes most of the old wallpaper picker code. I have removed the
> disable_new_wallpaper_picker flag in a previous CL. So it should be fine to
> remove all these files.
> Also there are some code refactor in ash/* to remove default wallpaper index
> as
> suggested by sky@.
>
> +sky for ash/*
>
> +nkostylev for chromeos/*
>
> +jhawkins for resources/* and ui/webui/*
>
> Thanks!
>
> Description:
> Remove old wallpaper picker code and some code refactor
>
>
> BUG=141431
>
>
> Please review this at http://codereview.chromium.org/11415015/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M ash/ash.gyp
>   M ash/desktop_background/desktop_background_controller.h
>   M ash/desktop_background/desktop_background_controller.cc
>   D ash/desktop_background/desktop_background_resources.h
>   D ash/desktop_background/desktop_background_resources.cc
>   M ash/desktop_background/desktop_background_view.h
>   M ash/desktop_background/user_wallpaper_delegate.h
>   M ash/shell.cc
>   M ash/shell/content_client/shell_browser_main_parts.cc
>   M ash/shell_factory.h
>   M chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc
>   M chrome/browser/chromeos/extensions/wallpaper_private_api.h
>   M chrome/browser/chromeos/extensions/wallpaper_private_api.cc
>   M chrome/browser/chromeos/login/mock_user_manager.h
>   M chrome/browser/chromeos/login/user.cc
>   M chrome/browser/chromeos/login/user_manager.h
>   M chrome/browser/chromeos/login/user_manager_impl.h
>   M chrome/browser/chromeos/login/user_manager_impl.cc
>   M chrome/browser/chromeos/login/wallpaper_manager.h
>   M chrome/browser/chromeos/login/wallpaper_manager.cc
>   M chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc
>   D chrome/browser/resources/options/chromeos/set_wallpaper_options.css
>   D chrome/browser/resources/options/chromeos/set_wallpaper_options.html
>   D chrome/browser/resources/options/chromeos/set_wallpaper_options.js
>   M chrome/browser/resources/options/options.html
>   D chrome/browser/ui/webui/options/chromeos/set_wallpaper_options_handler.h
>   D
> chrome/browser/ui/webui/options/chromeos/set_wallpaper_options_handler.cc
>   D chrome/browser/ui/webui/options/chromeos/wallpaper_thumbnail_source.h
>   D chrome/browser/ui/webui/options/chromeos/wallpaper_thumbnail_source.cc
>   M chrome/browser/ui/webui/options/options_ui.cc
>   M chrome/chrome_browser_ui.gypi
>
>

[Nikita (slow), 2012-11-16 14:46:26]

I agree with Scott. CL should contain just cleanup otherwise some aspects of
refactoring could be overlooked.

[bshe, 2012-11-16 15:29:03]

I have created a new patch here https://codereview.chromium.org/11308068/ to
remove all old files. After that one is landed, the remaining diff is my
refactor. The refactor is mostly in ash/*. I will ping again once this CL is
ready for review.
Sorry for trouble.

[bshe, 2012-11-19 17:26:53]

I have uploaded my CL which removes unused files. This CL is mainly code
refactor(most in ash/*) now. Could you guys please take a look? Thanks!

+sky for ash/* related
+nikita for chromeos/login/* related

[Nikita (slow), 2012-11-19 17:45:38]

chromeos/login lgtm

[sky, 2012-11-19 22:21:19]

https://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/des...
File ash/desktop_background/desktop_background_controller.cc (right):

https://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:83: int wallpaper_idr;
Can this use WallpaperInfo?

https://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:90: class
DesktopBackgroundController::WallpaperOperation
If this loads, shouldn't the name be WallpaperLoader or something?

https://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:99:
wo->LoadingWallpaper();
Add DCHECK not on UI thread. Similarly naming this to make it clear it isn't
invoked on the UI thread would be nice.

https://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:102: void
LoadingWallpaper() {
Does this need to be public?

https://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:112: int idr() {
how come you removed the const?

https://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:203:
DCHECK(GetWallpaperIDR() != info.idr);
DCHECK_NE
How come this is a DCHECK? Isn't it possible for someone to invoke twice?

https://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/des...
File ash/desktop_background/desktop_background_controller.h (right):

https://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.h:27: CENTER,
For enums that exist in the ash namespace they need to be named more
specifically. Eg WALLPAPER_LAYOUT_CENTER and WALLPAPER_RESOLUTION_LARGE.

https://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.h:100: void
SetDefaultWallpaper(WallpaperInfo info);
const WallpaperInfo&

[bshe, 2012-11-20 00:33:59]

Done. Thanks for review!
Could you please take another look?

http://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:83: int wallpaper_idr;
On 2012/11/19 22:21:19, sky wrote:
> Can this use WallpaperInfo?

Done.

http://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:90: class
DesktopBackgroundController::WallpaperOperation
On 2012/11/19 22:21:19, sky wrote:
> If this loads, shouldn't the name be WallpaperLoader or something?

Done.

http://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:99:
wo->LoadingWallpaper();
On 2012/11/19 22:21:19, sky wrote:
> Add DCHECK not on UI thread. Similarly naming this to make it clear it isn't
> invoked on the UI thread would be nice.

Done.

http://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:102: void
LoadingWallpaper() {
On 2012/11/19 22:21:19, sky wrote:
> Does this need to be public?

Done.

http://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:112: int idr() {
On 2012/11/19 22:21:19, sky wrote:
> how come you removed the const?

Done.

http://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:203:
DCHECK(GetWallpaperIDR() != info.idr);
I have moved the logic to prevent reloading same wallpaper to
wallpaper_manager.cc file. And this function is called from wallpaper manager
for chrome. So I add a DCHECK here. I am expecting that we shouldn't try to call
this twice on the same wallpaper from wallpaper manager.

For ash_shell in ash/shell/content_client/shell_browser_main_parts.cc. We are
calling it from function PreMainMessageLoopRun, it looks like we should only
call it once as well?




On 2012/11/19 22:21:19, sky wrote:
> DCHECK_NE
> How come this is a DCHECK? Isn't it possible for someone to invoke twice?

http://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:27: CENTER,
On 2012/11/19 22:21:19, sky wrote:
> For enums that exist in the ash namespace they need to be named more
> specifically. Eg WALLPAPER_LAYOUT_CENTER and WALLPAPER_RESOLUTION_LARGE.

Done.

http://codereview.chromium.org/11415015/diff/6010/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:100: void
SetDefaultWallpaper(WallpaperInfo info);
On 2012/11/19 22:21:19, sky wrote:
> const WallpaperInfo&

Done.

[sky, 2012-11-20 03:13:41]

LGTM