Change Aura desktop background behavior.

Change Aura desktop background behavior.


BUG=116298
TEST=none

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

[Daniel Erat, 2012-02-29 21:41:40]

https://chromiumcodereview.appspot.com/9557001/diff/1/ash/desktop_background/...
File ash/desktop_background/desktop_background_view.cc (right):

https://chromiumcodereview.appspot.com/9557001/diff/1/ash/desktop_background/...
ash/desktop_background/desktop_background_view.cc:26:
wallpaper_.buildMipMap(false);
DrawBitmapInt()'s comment suggests calling this, but just out of curiosity, what
happens if you remove it?

https://chromiumcodereview.appspot.com/9557001/diff/1/ash/desktop_background/...
ash/desktop_background/desktop_background_view.cc:53:
static_cast<int>(static_cast<double>(width()) / vertical_ratio),
nit: round this?

https://chromiumcodereview.appspot.com/9557001/diff/1/ash/desktop_background/...
ash/desktop_background/desktop_background_view.cc:57:
static_cast<int>(static_cast<double>(height()) / horizontal_ratio));
nit: round?

https://chromiumcodereview.appspot.com/9557001/diff/1/ash/desktop_background/...
ash/desktop_background/desktop_background_view.cc:68: // Center the wallpaper
and crop to the destination rectangle.
have you tested whether it's okay for the destination coordinates to fall
outside of the destination bounds (i.e. will skia do the clipping for you)?  if
so, you could probably just do:

  canvas->DrawBitmapInt(
      wallpaper_,
      (width() - wallpaper_.width()) / 2,
      (height() - wallpaper_.height()) / 2);

[Emmanuel Saint-loubert-Bié, 2012-02-29 22:04:16]

Hi Dan,

Thank for your comments. I have one question about the rounding (see my comment
below).

Thanks

-- Emmanuel

https://chromiumcodereview.appspot.com/9557001/diff/1/ash/desktop_background/...
File ash/desktop_background/desktop_background_view.cc (right):

https://chromiumcodereview.appspot.com/9557001/diff/1/ash/desktop_background/...
ash/desktop_background/desktop_background_view.cc:26:
wallpaper_.buildMipMap(false);
Nothing as far as I can tell. I added that line based on the same comment you
read.

https://chromiumcodereview.appspot.com/9557001/diff/1/ash/desktop_background/...
ash/desktop_background/desktop_background_view.cc:53:
static_cast<int>(static_cast<double>(width()) / vertical_ratio),
Do you mean using floor() or similar function and applying to the "double"
resulting from the division?

https://chromiumcodereview.appspot.com/9557001/diff/1/ash/desktop_background/...
ash/desktop_background/desktop_background_view.cc:68: // Center the wallpaper
and crop to the destination rectangle.
Ha yes that works too, much clearer. I applied your suggestion.

[Daniel Erat, 2012-02-29 22:10:40]

https://chromiumcodereview.appspot.com/9557001/diff/1/ash/desktop_background/...
File ash/desktop_background/desktop_background_view.cc (right):

https://chromiumcodereview.appspot.com/9557001/diff/1/ash/desktop_background/...
ash/desktop_background/desktop_background_view.cc:53:
static_cast<int>(static_cast<double>(width()) / vertical_ratio),
On 2012/02/29 22:04:16, Emmanuel Saint-loubert wrote:
> Do you mean using floor() or similar function and applying to the "double"
> resulting from the division?

i meant doing static_cast<int>(round(...)) on the result of the division.  the
static_cast<int> that you're currently doing is pretty much the same as calling
floor(), but i think it'd be more accurate to round.

https://chromiumcodereview.appspot.com/9557001/diff/6001/ash/desktop_backgrou...
File ash/desktop_background/desktop_background_view.cc (right):

https://chromiumcodereview.appspot.com/9557001/diff/6001/ash/desktop_backgrou...
ash/desktop_background/desktop_background_view.cc:69: gfx::Rect
wallpaper_centered_rect = wallpaper_rect.Center(gfx::Size(
you don't need this anymore

[Emmanuel Saint-loubert-Bié, 2012-02-29 22:32:58]

Hi Dan,

Yes the previous impl would have been like a floor(), so I added round() per you
suggestion.

Thanks,

-- Emmanuel

https://chromiumcodereview.appspot.com/9557001/diff/6001/ash/desktop_backgrou...
File ash/desktop_background/desktop_background_view.cc (right):

https://chromiumcodereview.appspot.com/9557001/diff/6001/ash/desktop_backgrou...
ash/desktop_background/desktop_background_view.cc:69: gfx::Rect
wallpaper_centered_rect = wallpaper_rect.Center(gfx::Size(
On 2012/02/29 22:10:40, Daniel Erat wrote:
> you don't need this anymore

Done.

[Daniel Erat, 2012-02-29 22:34:43]

lgtm

[commit-bot: I haz the power, 2012-02-29 22:39:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/saintlou@chromium.org/9557001/8001

[commit-bot: I haz the power, 2012-03-01 16:21:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/saintlou@chromium.org/9557001/4003

[tfarina, 2012-03-01 17:24:11]

http://codereview.chromium.org/9557001/diff/4003/ash/desktop_background/deskt...
File ash/desktop_background/desktop_background_view.cc (right):

http://codereview.chromium.org/9557001/diff/4003/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_view.cc:75: else {
nit: else should be in the of the previous line.

[commit-bot: I haz the power, 2012-03-01 17:49:12]

Change committed as 124424