Adjust MD shelf app list icon for better centering.

Adjust MD shelf app list icon for better centering.

Also fix appearance during shelf auto-hide/show animations (for all three shelf alignments).

BUG=665894, 649549
Committed: https://crrev.com/c7f510bb94819ac12386b0624c07ef65ccc059eb
Cr-Commit-Position: refs/heads/master@{#432927}

[Evan Stade, 2016-11-16 17:57:22]

estade@chromium.org changed reviewers:
+ tdanderson@chromium.org

[Evan Stade, 2016-11-16 17:57:22]

screenshots on bug.

[Evan Stade, 2016-11-16 18:16:07]

Description was changed from

==========
Adjust MD shelf app list icon for better centering.

BUG=665894
==========

to

==========
Adjust MD shelf app list icon for better centering.

Also fix appearance during shelf auto-hide/show animations (for all three shelf
alignments).

BUG=665894,649549
==========

[tdanderson, 2016-11-16 18:19:36]

LGTM provided Sebastien is good with the appearance (it looks fine to me). Note
I am not an owner of ash/common.

https://codereview.chromium.org/2509743002/diff/1/ash/common/shelf/app_list_b...
File ash/common/shelf/app_list_button.cc (right):

https://codereview.chromium.org/2509743002/diff/1/ash/common/shelf/app_list_b...
ash/common/shelf/app_list_button.cc:156: // Paint the circular background of
AppList button.
nit: consider DCHECK(IsMd())

https://codereview.chromium.org/2509743002/diff/1/ash/common/shelf/app_list_b...
ash/common/shelf/app_list_button.cc:171: const float dsf =
canvas->UndoDeviceScaleFactor();
Do you not need a corresponding call to Restore()? UndoDeviceScaleFactor() looks
to mutate the canvas.

[Evan Stade, 2016-11-16 18:29:28]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[Evan Stade, 2016-11-16 18:29:53]

you commented on ps1, but ps2 has some interesting changes as well. Could you
take a look please?

https://codereview.chromium.org/2509743002/diff/1/ash/common/shelf/app_list_b...
File ash/common/shelf/app_list_button.cc (right):

https://codereview.chromium.org/2509743002/diff/1/ash/common/shelf/app_list_b...
ash/common/shelf/app_list_button.cc:156: // Paint the circular background of
AppList button.
On 2016/11/16 18:19:36, tdanderson wrote:
> nit: consider DCHECK(IsMd())

I like to use that as documentation but in this case the function name serves
that purpose.

https://codereview.chromium.org/2509743002/diff/1/ash/common/shelf/app_list_b...
ash/common/shelf/app_list_button.cc:171: const float dsf =
canvas->UndoDeviceScaleFactor();
On 2016/11/16 18:19:36, tdanderson wrote:
> Do you not need a corresponding call to Restore()? UndoDeviceScaleFactor()
looks
> to mutate the canvas.

we have done that a lot in the past, but Sadrul pointed out it was unnecessary
because View::Paint creates a new canvas for each view (i.e. this canvas won't
be used again after this function finishes).

[commit-bot: I haz the power, 2016-11-16 18:30:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-11-16 19:51:25]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 19:51:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tdanderson, 2016-11-16 19:53:44]

On 2016/11/16 18:29:53, Evan Stade wrote:
> you commented on ps1, but ps2 has some interesting changes as well. Could you
> take a look please?
> 
>
https://codereview.chromium.org/2509743002/diff/1/ash/common/shelf/app_list_b...
> File ash/common/shelf/app_list_button.cc (right):
> 
>
https://codereview.chromium.org/2509743002/diff/1/ash/common/shelf/app_list_b...
> ash/common/shelf/app_list_button.cc:156: // Paint the circular background of
> AppList button.
> On 2016/11/16 18:19:36, tdanderson wrote:
> > nit: consider DCHECK(IsMd())
> 
> I like to use that as documentation but in this case the function name serves
> that purpose.
> 
>
https://codereview.chromium.org/2509743002/diff/1/ash/common/shelf/app_list_b...
> ash/common/shelf/app_list_button.cc:171: const float dsf =
> canvas->UndoDeviceScaleFactor();
> On 2016/11/16 18:19:36, tdanderson wrote:
> > Do you not need a corresponding call to Restore()? UndoDeviceScaleFactor()
> looks
> > to mutate the canvas.
> 
> we have done that a lot in the past, but Sadrul pointed out it was unnecessary
> because View::Paint creates a new canvas for each view (i.e. this canvas won't
> be used again after this function finishes).

still lgtm

[Evan Stade, 2016-11-16 19:54:28]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-11-16 19:54:31]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-11-16 19:54:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 20:05:42]

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

[commit-bot: I haz the power, 2016-11-16 20:05:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Evan Stade, 2016-11-16 20:11:22]

estade@chromium.org changed reviewers:
+ derat@chromium.org

[Evan Stade, 2016-11-16 20:11:23]

+derat for owners

[tdanderson, 2016-11-16 20:20:18]

https://codereview.chromium.org/2509743002/diff/1/ash/common/shelf/app_list_b...
File ash/common/shelf/app_list_button.cc (right):

https://codereview.chromium.org/2509743002/diff/1/ash/common/shelf/app_list_b...
ash/common/shelf/app_list_button.cc:156: // Paint the circular background of
AppList button.
On 2016/11/16 18:29:53, Evan Stade wrote:
> On 2016/11/16 18:19:36, tdanderson wrote:
> > nit: consider DCHECK(IsMd())
> 
> I like to use that as documentation but in this case the function name serves
> that purpose.

Acknowledged.

https://codereview.chromium.org/2509743002/diff/1/ash/common/shelf/app_list_b...
ash/common/shelf/app_list_button.cc:171: const float dsf =
canvas->UndoDeviceScaleFactor();
On 2016/11/16 18:29:53, Evan Stade wrote:
> On 2016/11/16 18:19:36, tdanderson wrote:
> > Do you not need a corresponding call to Restore()? UndoDeviceScaleFactor()
> looks
> > to mutate the canvas.
> 
> we have done that a lot in the past, but Sadrul pointed out it was unnecessary
> because View::Paint creates a new canvas for each view (i.e. this canvas won't
> be used again after this function finishes).

Acknowledged.

[Daniel Erat, 2016-11-16 21:32:28]

lgtm

[Evan Stade, 2016-11-17 00:12:14]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-11-17 00:13:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 01:52:51]

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

[commit-bot: I haz the power, 2016-11-17 01:52:54]

Failed to apply patch for ash/common/shelf/app_list_button.cc:
While running git apply --index -p1;
  error: patch failed: ash/common/shelf/app_list_button.cc:23
  error: ash/common/shelf/app_list_button.cc: patch does not apply

Patch:       ash/common/shelf/app_list_button.cc
Index: ash/common/shelf/app_list_button.cc
diff --git a/ash/common/shelf/app_list_button.cc
b/ash/common/shelf/app_list_button.cc
index
714c37c58e80757d52de718d2259159717c07847..56d31aff5ad64ac14d25586aa6b520404447d9c1
100644
--- a/ash/common/shelf/app_list_button.cc
+++ b/ash/common/shelf/app_list_button.cc
@@ -14,7 +14,6 @@
 #include "ash/common/shelf/wm_shelf_util.h"
 #include "ash/common/wm_shell.h"
 #include "ash/public/cpp/shelf_types.h"
-#include "ash/resources/vector_icons/vector_icons.h"
 #include "base/command_line.h"
 #include "grit/ash_resources.h"
 #include "grit/ash_strings.h"
@@ -23,7 +22,6 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
-#include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/animation/ink_drop_impl.h"
 #include "ui/views/animation/square_ink_drop_ripple.h"
 #include "ui/views/painter.h"
@@ -141,55 +139,52 @@ void AppListButton::OnGestureEvent(ui::GestureEvent*
event) {
 void AppListButton::OnPaint(gfx::Canvas* canvas) {
   // Call the base class first to paint any background/borders.
   View::OnPaint(canvas);
-  ResourceBundle& rb = ResourceBundle::GetSharedInstance();
-
-  const gfx::ImageSkia& foreground_image =
-      MaterialDesignController::IsShelfMaterial()
-          ? CreateVectorIcon(kShelfAppListIcon, kShelfIconColor)
-          : *rb.GetImageNamed(IDR_ASH_SHELF_ICON_APPLIST).ToImageSkia();
 
   if (ash::MaterialDesignController::IsShelfMaterial()) {
-    PaintBackgroundMD(canvas);
-    PaintForegroundMD(canvas, foreground_image);
+    PaintMd(canvas);
   } else {
+    ResourceBundle& rb = ResourceBundle::GetSharedInstance();
+    const gfx::ImageSkia& foreground_image =
+        *rb.GetImageNamed(IDR_ASH_SHELF_ICON_APPLIST).ToImageSkia();
     PaintAppListButton(canvas, foreground_image);
   }
 
   views::Painter::PaintFocusPainter(this, canvas, focus_painter());
 }
 
-void AppListButton::PaintBackgroundMD(gfx::Canvas* canvas) {
-  // Paint the circular background of AppList button.
-  gfx::Point circle_center = GetContentsBounds().CenterPoint();
-  if (!IsHorizontalAlignment(wm_shelf_->GetAlignment()))
-    circle_center = gfx::Point(circle_center.y(), circle_center.x());
-
-  SkPaint background_paint;
-  background_paint.setColor(SkColorSetA(kShelfBaseColor, background_alpha_));
-  background_paint.setFlags(SkPaint::kAntiAlias_Flag);
-  background_paint.setStyle(SkPaint::kFill_Style);
-
-  canvas->DrawCircle(circle_center, kAppListButtonRadius, background_paint);
-}
-
-void AppListButton::PaintForegroundMD(gfx::Canvas* canvas,
-                                      const gfx::ImageSkia& foreground_image) {
-  gfx::Rect foreground_bounds(foreground_image.size());
-  gfx::Rect contents_bounds = GetContentsBounds();
-
-  if (IsHorizontalAlignment(wm_shelf_->GetAlignment())) {
-    foreground_bounds.set_x(
-        (contents_bounds.width() - foreground_bounds.width()) / 2);
-    foreground_bounds.set_y(
-        (contents_bounds.height() - foreground_bounds.height()) / 2);
-  } else {
-    foreground_bounds.set_x(
-        (contents_bounds.height() - foreground_bounds.height()) / 2);
-    foreground_bounds.set_y(
-        (contents_bounds.width() - foreground_bounds.width()) / 2);
-  }
-  canvas->DrawImageInt(foreground_image, foreground_bounds.x(),
-                       foreground_bounds.y());
+void AppListButton::PaintMd(gfx::Canvas* canvas) {
+  // During animations, width and height may not be equal. Take the greater of
+  // the two as the one that represents the normal size of the button.
+  float center = std::max(width(), height()) / 2.f;
+  gfx::PointF circle_center(center, center);
+  // For the left shelf alignment, we need to right-justify. For other shelf
+  // alignments, left/top justification (i.e. no adjustments are necessary).
+  if (SHELF_ALIGNMENT_LEFT == wm_shelf_->GetAlignment())
+    circle_center.set_x(width() - center);
+
+  // Paint the circular background.
+  SkPaint bg_paint;
+  bg_paint.setColor(SkColorSetA(kShelfBaseColor, background_alpha_));
+  bg_paint.setFlags(SkPaint::kAntiAlias_Flag);
+  bg_paint.setStyle(SkPaint::kFill_Style);
+  canvas->DrawCircle(circle_center, kAppListButtonRadius, bg_paint);
+
+  // Paint a white ring as the foreground. The ceil/dsf math assures that the
+  // ring draws sharply and is centered at all scale factors.
+  const float kRingOuterRadiusDp = 7.f;
+  const float kRingThicknessDp = 1.5f;
+  const float dsf = canvas->UndoDeviceScaleFactor();
+  circle_center.Scale(dsf);
+
+  SkPaint fg_paint;
+  fg_paint.setFlags(SkPaint::kAntiAlias_Flag);
+  fg_paint.setStyle(SkPaint::kStroke_Style);
+  fg_paint.setColor(kShelfIconColor);
+  const float thickness = std::ceil(kRingThicknessDp * dsf);
+  const float radius = std::ceil(kRingOuterRadiusDp * dsf) - thickness / 2;
+  fg_paint.setStrokeWidth(thickness);
+  // Make sure the center of the circle lands on pixel centers.
+  canvas->DrawCircle(circle_center, radius, fg_paint);
 }
 
 void AppListButton::PaintAppListButton(gfx::Canvas* canvas,

[Evan Stade, 2016-11-17 02:49:35]

estade@chromium.org changed reviewers:
+ mohsen@chromium.org

[Evan Stade, 2016-11-17 02:49:35]

Mohsen, I had to rebase in a non-trivial way on some of your changes. Could you
PTAL? The ripple seems to still work well for all shelf alignments.

[Evan Stade, 2016-11-17 02:49:41]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 02:50:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2016-11-17 03:26:56]

On 2016/11/17 at 02:49:35, estade wrote:
> Mohsen, I had to rebase in a non-trivial way on some of your changes. Could
you PTAL? The ripple seems to still work well for all shelf alignments.

LGTM. My change added GetCenterPoint() function that returns the center of
ripple/mask/background and since you're changing inside that function, all
places using it should be fine.

[Evan Stade, 2016-11-17 03:38:28]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-11-17 03:38:34]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-11-17 03:38:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, derat@chromium.org
Link to the patchset: https://codereview.chromium.org/2509743002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-17 03:39:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 06:30:43]

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

[commit-bot: I haz the power, 2016-11-17 06:30:44]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Evan Stade, 2016-11-17 18:00:27]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-11-17 18:01:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 18:48:47]

Description was changed from

==========
Adjust MD shelf app list icon for better centering.

Also fix appearance during shelf auto-hide/show animations (for all three shelf
alignments).

BUG=665894,649549
==========

to

==========
Adjust MD shelf app list icon for better centering.

Also fix appearance during shelf auto-hide/show animations (for all three shelf
alignments).

BUG=665894,649549
==========

[commit-bot: I haz the power, 2016-11-17 18:48:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-17 20:28:21]

Description was changed from

==========
Adjust MD shelf app list icon for better centering.

Also fix appearance during shelf auto-hide/show animations (for all three shelf
alignments).

BUG=665894,649549
==========

to

==========
Adjust MD shelf app list icon for better centering.

Also fix appearance during shelf auto-hide/show animations (for all three shelf
alignments).

BUG=665894,649549

Committed: https://crrev.com/c7f510bb94819ac12386b0624c07ef65ccc059eb
Cr-Commit-Position: refs/heads/master@{#432927}
==========

[commit-bot: I haz the power, 2016-11-17 20:28:22]

Patchset 4 (id:??) landed as
https://crrev.com/c7f510bb94819ac12386b0624c07ef65ccc059eb
Cr-Commit-Position: refs/heads/master@{#432927}