Resize and save custom wallpaper to large and small size

Resize and save custom wallpaper to large and small size if original wallpaper is large.

This CL resizes user custom wallpaper if the size of wallpaper is large.
1) If size > 1366x800, scale and save a small (closest resolution to 1366x800) verison of
custom wallpaper without changing aspect ratio.
2) If size > 2560x1700, scale and save a large (closest resolution to 2560x1700) verison
of custom wallpaper without changing aspect ratio.


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

[bshe, 2012-08-28 03:49:02]

Hi Ivan.

Could you please take a look at this CL? It resizes large custom wallpaper to
two resolutions if possible.

Thanks!

[bshe, 2012-08-28 14:43:11]

+rkc FYI

I use the simple_png_encoder you refactored in this CL:
https://chromiumcodereview.appspot.com/10790084/

It looks like your CL got reverted because of interactive_ui_tests fail:
https://chromiumcodereview.appspot.com/10824006

[Ivan Korotkov, 2012-08-28 16:16:12]

LGTM with some nits

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

http://codereview.chromium.org/10883062/diff/9001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:38: const int
kSmallWallpaperMaximalWidth = 1366;
For consistency with other constants, maybe use Max instead of Maximal?

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

http://codereview.chromium.org/10883062/diff/9001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:27: ASH_EXPORT extern
const int kSmallWallpaperMaximalWidth;
Please document these.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/simple_png_encoder.cc (right):

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/simple_png_encoder.cc:16: #include <zlib.h>
This shouldn't make a big difference since you only need a single constant, but
still...

#if defined(USE_SYSTEM_ZLIB)
#include <zlib.h>
#else
#include "third_party/zlib/zlib.h"
#endif

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/simple_png_encoder.h (right):

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/simple_png_encoder.h:16: typedef
base::Callback<void(
Since this name is pretty generic, please move it inside the class and comment.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/simple_png_encoder.h:27: void Run(EncoderCallback
callback);
Please document these functions.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/simple_png_encoder.h:29: void EncodeWallpaper();
Looks like it doesn't need to be public.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:215: const char* suffix =
is_small ? "_small" : "_large";
Please move these suffices to constants.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:818: // exisit. This may
happen when the original custom wallpaper is small or
exist

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:961: double horizontal_ratio
= static_cast<double>(preferred_width) /
You actually need only the first static_cast

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:978: // TODO(bshe): Generates
cropped custom wallpaper for CENTER layout.
This TODO looks strange. What does the else branch correspond to?

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wallpaper_manager.h (right):

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:193: // original custom
wallpaper if failed. Must be executed on FILE thread.
'if failed' not necessary here
s/executed/run/

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:216: // Saves encoded
wallpaper to |path|.
Please document that this is a callback after encoding to PNG

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:220: // Resizes |wallpaper| to
a resolution which nearest to |preferred_width| and
which is

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:221: // |preferred_height|
while maintain aspect ratio. And saves the resized
maintaining

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:229: // Saves original custom
wallpaper to |path| (absolute path) in file system
on filesystem

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:230: // and starts to resize
the custom wallpaper if necessary.
starts resizing operation of

[bshe, 2012-08-28 16:48:18]

Thanks!

+sky for owners ash/*

[sky, 2012-08-28 19:39:42]

LGTM

http://codereview.chromium.org/10883062/diff/12002/ash/desktop_background/des...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/10883062/diff/12002/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.h:27: ASH_EXPORT extern
const int kSmallWallpaperMaximalWidth;
Document what these are.

[bshe, 2012-08-29 14:30:07]

Thanks for review.

ivan@ could you take another quick look before I try to land. The main thing is
I add third_party zlib to chrome/browser/chromeos/DEPS in order to include it in
header. Thanks!

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

http://codereview.chromium.org/10883062/diff/9001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:38: const int
kSmallWallpaperMaximalWidth = 1366;
On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> For consistency with other constants, maybe use Max instead of Maximal?

Done.

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

http://codereview.chromium.org/10883062/diff/9001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:27: ASH_EXPORT extern
const int kSmallWallpaperMaximalWidth;
On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> Please document these.

Done.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/simple_png_encoder.cc (right):

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/simple_png_encoder.cc:16: #include <zlib.h>
Done. Looks like I need to add third_party/zlib/zlib.h
to chrome/browser/chromeos/DEPS too.

On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> This shouldn't make a big difference since you only need a single constant,
but
> still...
> 
> #if defined(USE_SYSTEM_ZLIB)
> #include <zlib.h>
> #else
> #include "third_party/zlib/zlib.h"
> #endif

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/simple_png_encoder.h (right):

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/simple_png_encoder.h:16: typedef
base::Callback<void(
On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> Since this name is pretty generic, please move it inside the class and
comment.

Done.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/simple_png_encoder.h:27: void Run(EncoderCallback
callback);
On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> Please document these functions.

Done.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/simple_png_encoder.h:29: void EncodeWallpaper();
Moved to private.

On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> Looks like it doesn't need to be public.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:215: const char* suffix =
is_small ? "_small" : "_large";
On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> Please move these suffices to constants.

Done.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:818: // exisit. This may
happen when the original custom wallpaper is small or
On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> exist

Done.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:961: double horizontal_ratio
= static_cast<double>(preferred_width) /
On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> You actually need only the first static_cast

Done.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:978: // TODO(bshe): Generates
cropped custom wallpaper for CENTER layout.
We only support three layout settings for custom wallpaper. So it corresponds to
CENTER layout or CENTER_CROPPED layout but a small wallpaper. In that case, we
just save the original wallpaper for now (no resize or crop).
The logic is actually similar to the layout logic in OnPaint function in
ash/desktop_background/desktop_background_view.cc .

On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> This TODO looks strange. What does the else branch correspond to?

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wallpaper_manager.h (right):

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:193: // original custom
wallpaper if failed. Must be executed on FILE thread.
On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> 'if failed' not necessary here
> s/executed/run/

Done.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:216: // Saves encoded
wallpaper to |path|.
On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> Please document that this is a callback after encoding to PNG

Done.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:220: // Resizes |wallpaper| to
a resolution which nearest to |preferred_width| and
On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> which is

Done.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:221: // |preferred_height|
while maintain aspect ratio. And saves the resized
On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> maintaining

Done.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:229: // Saves original custom
wallpaper to |path| (absolute path) in file system
On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> on filesystem

Done.

http://codereview.chromium.org/10883062/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:230: // and starts to resize
the custom wallpaper if necessary.
On 2012/08/28 16:16:13, Ivan Korotkov wrote:
> starts resizing operation of

Done.

http://codereview.chromium.org/10883062/diff/12002/ash/desktop_background/des...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/10883062/diff/12002/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.h:27: ASH_EXPORT extern
const int kSmallWallpaperMaximalWidth;
On 2012/08/28 19:39:42, sky wrote:
> Document what these are.

Done.

[Ivan Korotkov, 2012-08-30 10:53:15]

LGTM, thanks for fixes

http://codereview.chromium.org/10883062/diff/11011/ash/desktop_background/des...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/10883062/diff/11011/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.h:28: // smaller than
kSmallWallpaperMaxWidth and kSmallWallpaperMaxHeight, the small
vars in |...|

http://codereview.chromium.org/10883062/diff/11011/chrome/browser/chromeos/DEPS
File chrome/browser/chromeos/DEPS (right):

http://codereview.chromium.org/10883062/diff/11011/chrome/browser/chromeos/DE...
chrome/browser/chromeos/DEPS:6: "+third_party/zlib/zlib.h", # For compression
level constants.
2 spaces

http://codereview.chromium.org/10883062/diff/11011/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/simple_png_encoder.h (right):

http://codereview.chromium.org/10883062/diff/11011/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/simple_png_encoder.h:39: // Encodes a SkBitmap to
PNG codec on worker pool.
with PNG codec?..

http://codereview.chromium.org/10883062/diff/11011/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10883062/diff/11011/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:212: std::string filename =
username + "_wallpaper.png";
This deserves a separate constant, too.

[bshe, 2012-08-30 16:39:02]

Thanks Ivan!

I have addressed all you comments.

Pre requested by saintlou. we need to use jpeg codec instead of png codec for
all custom wallpapers. So I changed the codec to jpge and renamed the file to
simple_jpge_encoder.
It is a pretty trivial change. But could you please take another quick look?

Thanks!

https://chromiumcodereview.appspot.com/10883062/diff/11011/ash/desktop_backgr...
File ash/desktop_background/desktop_background_controller.h (right):

https://chromiumcodereview.appspot.com/10883062/diff/11011/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.h:28: // smaller than
kSmallWallpaperMaxWidth and kSmallWallpaperMaxHeight, the small
On 2012/08/30 10:53:15, Ivan Korotkov wrote:
> vars in |...|

Done.

https://chromiumcodereview.appspot.com/10883062/diff/11011/chrome/browser/chr...
File chrome/browser/chromeos/DEPS (right):

https://chromiumcodereview.appspot.com/10883062/diff/11011/chrome/browser/chr...
chrome/browser/chromeos/DEPS:6: "+third_party/zlib/zlib.h", # For compression
level constants.
On 2012/08/30 10:53:15, Ivan Korotkov wrote:
> 2 spaces

Done.

https://chromiumcodereview.appspot.com/10883062/diff/11011/chrome/browser/chr...
File chrome/browser/chromeos/login/simple_png_encoder.h (right):

https://chromiumcodereview.appspot.com/10883062/diff/11011/chrome/browser/chr...
chrome/browser/chromeos/login/simple_png_encoder.h:39: // Encodes a SkBitmap to
PNG codec on worker pool.
On 2012/08/30 10:53:15, Ivan Korotkov wrote:
> with PNG codec?..

Done.

https://chromiumcodereview.appspot.com/10883062/diff/11011/chrome/browser/chr...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://chromiumcodereview.appspot.com/10883062/diff/11011/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:212: std::string filename =
username + "_wallpaper.png";
On 2012/08/30 10:53:15, Ivan Korotkov wrote:
> This deserves a separate constant, too.

Done.

[Ivan Korotkov, 2012-08-30 16:47:34]

Cool, LGTM

https://chromiumcodereview.appspot.com/10883062/diff/31001/chrome/browser/chr...
File chrome/browser/chromeos/login/simple_jpeg_encoder.cc (right):

https://chromiumcodereview.appspot.com/10883062/diff/31001/chrome/browser/chr...
chrome/browser/chromeos/login/simple_jpeg_encoder.cc:19: const int quality = 90;
Use kNaming, please.

[bshe, 2012-08-30 16:55:50]

done. Thanks!

https://chromiumcodereview.appspot.com/10883062/diff/31001/chrome/browser/chr...
File chrome/browser/chromeos/login/simple_jpeg_encoder.cc (right):

https://chromiumcodereview.appspot.com/10883062/diff/31001/chrome/browser/chr...
chrome/browser/chromeos/login/simple_jpeg_encoder.cc:19: const int quality = 90;
On 2012/08/30 16:47:34, Ivan Korotkov wrote:
> Use kNaming, please.

Done.

[commit-bot: I haz the power, 2012-08-30 16:56:53]

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

[commit-bot: I haz the power, 2012-08-30 16:56:59]

Failed to apply patch for chrome/chrome_browser.gypi:
While running patch -p1 --forward --force;
patching file chrome/chrome_browser.gypi
Hunk #1 FAILED at 766.
1 out of 1 hunk FAILED -- saving rejects to file chrome/chrome_browser.gypi.rej

[commit-bot: I haz the power, 2012-08-30 17:04:58]

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

[commit-bot: I haz the power, 2012-08-30 22:37:28]

Change committed as 154288

[Emmanuel Saint-loubert-Bié, 2012-08-30 22:53:25]

Biao,

I just added a suggestion on how to handle a TODO...

https://chromiumcodereview.appspot.com/10883062/diff/28005/chrome/browser/chr...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://chromiumcodereview.appspot.com/10883062/diff/28005/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:80: const char
kOriginalCustomWallpaperSuffix[] = "_wallpaper.png";
FWIW I have observed that the extension has no impact on the wallpaper decoding
(the process that load the file is disjoint form the decoder). 
So maybe it is best not to have any extension?

[bshe, 2012-08-31 13:30:50]

Absolutely. That's also my plan. I have noticed I can save jpeg files to .png
extension. So I will definitely remove the extension to avoid confusion. Thanks
for suggestion.

On 2012/08/30 22:53:25, Emmanuel Saint-loubert-Bié wrote:
> Biao,
> 
> I just added a suggestion on how to handle a TODO...
> 
>
https://chromiumcodereview.appspot.com/10883062/diff/28005/chrome/browser/chr...
> File chrome/browser/chromeos/login/wallpaper_manager.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10883062/diff/28005/chrome/browser/chr...
> chrome/browser/chromeos/login/wallpaper_manager.cc:80: const char
> kOriginalCustomWallpaperSuffix[] = "_wallpaper.png";
> FWIW I have observed that the extension has no impact on the wallpaper
decoding
> (the process that load the file is disjoint form the decoder). 
> So maybe it is best not to have any extension?