Update WM shadows for MD.

ui/wm/core/shadow.cc

Index: ui/wm/core/shadow.cc
diff --git a/ui/wm/core/shadow.cc b/ui/wm/core/shadow.cc
index 78853435c6cc81720b66c08c0bf0c2268b8b885b..eda7f78f2ecd201302915aa11d6ef7ce89f756bf 100644
--- a/ui/wm/core/shadow.cc
+++ b/ui/wm/core/shadow.cc
@@ -4,11 +4,14 @@
 
 #include "ui/wm/core/shadow.h"
 
-#include "third_party/skia/include/core/SkBitmap.h"
+#include "base/lazy_instance.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/compositor/layer.h"
 #include "ui/compositor/scoped_layer_animation_settings.h"
-#include "ui/resources/grit/ui_resources.h"
+#include "ui/gfx/geometry/insets.h"
+#include "ui/gfx/image/image_skia_operations.h"
+
+namespace wm {
 
 namespace {
 
@@ -16,18 +19,6 @@ namespace {
 // inactive/active shadow.
 const float kInactiveShadowAnimationOpacity = 0.2f;
 
-// Shadow aperture for different styles.
-// Note that this may be greater than interior inset to allow shadows with
-// curved corners that extend inwards beyond a window's borders.
-const int kActiveInteriorAperture = 134;
-const int kInactiveInteriorAperture = 134;
-const int kSmallInteriorAperture = 9;
-
-// Interior inset for different styles.
-const int kActiveInteriorInset = 64;
-const int kInactiveInteriorInset = 64;
-const int kSmallInteriorInset = 4;
-
 // Rounded corners are overdrawn on top of the window's content layer,
 // we need to exclude them from the occlusion area.
 const int kRoundedCornerRadius = 2;
@@ -35,39 +26,43 @@ const int kRoundedCornerRadius = 2;
 // Duration for opacity animation in milliseconds.
 const int kShadowAnimationDurationMs = 100;
 
-int GetShadowApertureForStyle(wm::Shadow::Style style) {
-  switch (style) {
-    case wm::Shadow::STYLE_ACTIVE:
-      return kActiveInteriorAperture;
-    case wm::Shadow::STYLE_INACTIVE:
-      return kInactiveInteriorAperture;
-    case wm::Shadow::STYLE_SMALL:
-      return kSmallInteriorAperture;
-  }
-  return 0;
-}
-
-int GetInteriorInsetForStyle(wm::Shadow::Style style) {
-  switch (style) {
-    case wm::Shadow::STYLE_ACTIVE:
-      return kActiveInteriorInset;
-    case wm::Shadow::STYLE_INACTIVE:
-      return kInactiveInteriorInset;
-    case wm::Shadow::STYLE_SMALL:
-      return kSmallInteriorInset;
+struct ShadowDetails {
+  gfx::ShadowValues values;
+  gfx::ImageSkia image;

[James Cook, 2016/12/05 23:38:35]

nit: document either here or above struct what this image is (or rename)

[Evan Stade, 2016/12/06 00:53:21]

Done.

+};
+
+// Map from elevation to a cached shadow.
+typedef std::map<int, ShadowDetails> ShadowDetailsMap;
+base::LazyInstance<ShadowDetailsMap> g_shadow_cache = LAZY_INSTANCE_INITIALIZER;
+
+const ShadowDetails& GetDetailsForElevation(int elevation) {
+  auto insertion =
+      g_shadow_cache.Get().insert(std::make_pair(elevation, ShadowDetails()));

[James Cook, 2016/12/05 23:38:35]

optional: would emplace work here?

[Evan Stade, 2016/12/06 00:53:21]

yes it would if map::emplace were allowed :~(

"std::map::emplace() is not yet available on all libstdc++ versions we support"
- https://chromium-cpp.appspot.com/

[James Cook, 2016/12/06 18:17:51]

I learned something new today!

+  ShadowDetails* shadow = &insertion.first->second;
+  // If there was an insertion, initialize the new ShadowDetails.
+  if (insertion.second) {

[James Cook, 2016/12/05 23:38:35]

I think this would be clearer if it did std::map::find() or std::map::count()
rather than an insert that sometimes fails. If you used find() and you were
particularly concerned about performance you could use insert-with-hint.

[Evan Stade, 2016/12/06 00:53:21]

Done.

+    // To match the CSS notion of blur (spread outside the bounding box) to the
+    // Skia notion of blur (spread outside and inside the bounding box), we have
+    // to double the designer-provided blur values.
+    const int kBlurCorrection = 2;
+    // "Key shadow": y offset is elevation and blur is twice the elevation.
+    shadow->values.emplace_back(gfx::Vector2d(0, elevation),
+                                kBlurCorrection * elevation * 2,
+                                SkColorSetA(SK_ColorBLACK, 0x3d));
+    // "Ambient shadow": no offset and blur matches the elevation.
+    shadow->values.emplace_back(gfx::Vector2d(), kBlurCorrection * elevation,
+                                SkColorSetA(SK_ColorBLACK, 0x1f));
+    shadow->image = gfx::ImageSkiaOperations::CreateShadowNinebox(
+        shadow->values, kRoundedCornerRadius);
   }
-  return 0;
+  return *shadow;
 }
 
 }  // namespace
 
-namespace wm {
+Shadow::Shadow() : style_(STYLE_ACTIVE) {}

[James Cook, 2016/12/05 23:38:35]

optional: init in header?

[Evan Stade, 2016/12/06 00:53:21]

Done.

 
-Shadow::Shadow() : style_(STYLE_ACTIVE), interior_inset_(0) {
-}
-
-Shadow::~Shadow() {
-}
+Shadow::~Shadow() {}
 
 void Shadow::Init(Style style) {
   style_ = style;
@@ -149,59 +144,53 @@ void Shadow::OnImplicitAnimationsCompleted() {
 }
 
 void Shadow::UpdateImagesForStyle() {
-  ResourceBundle& res = ResourceBundle::GetSharedInstance();
-  gfx::Image image;
-  switch (style_) {
-    case STYLE_ACTIVE:
-      image = res.GetImageNamed(IDR_AURA_SHADOW_ACTIVE);

[James Cook, 2016/12/05 23:38:35]

Can you file a bug about updating Mustash to use the new shadow approach? Code
is in ash/mus/shadow.cc. Then we can delete the assets. Label Proj-Mustash-Mash
on the bug should be sufficient.

[Evan Stade, 2016/12/06 00:53:21]

Done.

-      break;
-    case STYLE_INACTIVE:
-      image = res.GetImageNamed(IDR_AURA_SHADOW_INACTIVE);
-      break;
-    case STYLE_SMALL:
-      image = res.GetImageNamed(IDR_WINDOW_BUBBLE_SHADOW_SMALL);
-      break;
-    default:
-      NOTREACHED() << "Unhandled style " << style_;
-      break;
-  }
-
-  shadow_layer_->UpdateNinePatchLayerImage(image.AsImageSkia());
-  image_size_ = image.Size();
-  interior_inset_ = GetInteriorInsetForStyle(style_);
-
-  // Image sizes may have changed.
+  const ShadowDetails& details = GetDetailsForElevation(ElevationForStyle());
+  shadow_layer_->UpdateNinePatchLayerImage(details.image);
   UpdateLayerBounds();
 }
 
 void Shadow::UpdateLayerBounds() {
-  // Update bounds based on content bounds and interior inset.
+  const ShadowDetails& details = GetDetailsForElevation(ElevationForStyle());
+  const gfx::Insets margins = gfx::ShadowValue::GetMargin(details.values);

[James Cook, 2016/12/05 23:38:35]

nit: maybe comment that these insets push the bounds outward?

(Or do I have that backwards?)

[Evan Stade, 2016/12/06 00:53:21]

done

   gfx::Rect layer_bounds = content_bounds_;
-  layer_bounds.Inset(-interior_inset_, -interior_inset_);
+  layer_bounds.Inset(margins);
   layer()->SetBounds(layer_bounds);
-  shadow_layer_->SetBounds(gfx::Rect(layer_bounds.size()));
-
-  // Update the shadow aperture and border for style. Note that border is in
-  // layer space and it cannot exceed the bounds of the layer.
-  int aperture = GetShadowApertureForStyle(style_);
-  int aperture_x = std::min(aperture, layer_bounds.width() / 2);
-  int aperture_y = std::min(aperture, layer_bounds.height() / 2);
-  gfx::Rect aperture_rect(aperture_x, aperture_y,
-                          image_size_.width() - aperture_x * 2,
-                          image_size_.height() - aperture_y * 2);
-
-  shadow_layer_->UpdateNinePatchLayerAperture(aperture_rect);
-  shadow_layer_->UpdateNinePatchLayerBorder(
-      gfx::Rect(aperture_x, aperture_y, aperture_x * 2, aperture_y * 2));
-
-  // The content bounds in the shadow's layer space are offsetted by
-  // |interior_inset_|. The occlusion area also has to be shrunk to allow
-  // rounded corners overdrawing on top of the window's content.
-  gfx::Rect content_bounds(interior_inset_ + kRoundedCornerRadius,
-                           interior_inset_ + kRoundedCornerRadius,
-                           content_bounds_.width() - 2 * kRoundedCornerRadius,
-                           content_bounds_.height() - 2 * kRoundedCornerRadius);
-  shadow_layer_->UpdateNinePatchOcclusion(content_bounds);
+  gfx::Rect shadow_layer_bounds(layer_bounds.size());
+  shadow_layer_->SetBounds(shadow_layer_bounds);
+
+  // Occlude the region inside the bounding box. Occlusion uses shadow layer
+  // space.
+  gfx::Rect occlusion_bounds = shadow_layer_bounds;
+  occlusion_bounds.Inset(-margins + gfx::Insets(kRoundedCornerRadius));
+  shadow_layer_->UpdateNinePatchOcclusion(occlusion_bounds);
+
+  // The ninebox grid is defined in terms of the image size. We have to take
+  // into account the inner blur as well as outer blur and offset.
+  gfx::Rect aperture(details.image.size());
+  gfx::Insets blur_region = gfx::ShadowValue::GetBlurRegion(details.values) +
+                            gfx::Insets(kRoundedCornerRadius);
+  aperture.Inset(blur_region);
+  shadow_layer_->UpdateNinePatchLayerAperture(aperture);
+
+  // The border is more or less the same inset as the aperture, but can be no
+  // larger than the shadow layer.
+  gfx::Rect border_rect(blur_region.left(), blur_region.top(),
+                        blur_region.width(), blur_region.height());
+  border_rect.ClampToCenteredSize(shadow_layer_bounds.size());
+  shadow_layer_->UpdateNinePatchLayerBorder(border_rect);
+}

[James Cook, 2016/12/05 23:38:35]

I'm having a hard time following this function. Maybe a little ascii art picture
would help? Or link to a drawing?

[Evan Stade, 2016/12/06 00:53:21]

no kidding. Figuring this out cost me a lot of hair. I think the best docs are
in nine_patch_layer.h. I do happen to think that API is pretty darn confusing
(especially the fact that the border is a rect instead of Insets), which is
probably the reason for the excessive length of the comments in that file. I'm
hoping to discuss improvements to it with somebody. I started that conversation
on a separate CL already: https://codereview.chromium.org/2543473005/

I created this jsfiddle to help me reason about it (and define what Sebastien
really wanted): http://jsfiddle.net/evanstade/40acqxwp/2/ --- do you think that
is helpful documentation?

I also elaborated some of the text comments in this function.

[James Cook, 2016/12/06 18:17:51]

The new docs here help. I think adding a note about nine_patch_layer.h and a
link to your jsfiddle would help too.

[Evan Stade, 2016/12/07 01:56:42]

done, except included a CSS snippet instead of the fiddle.

+
+int Shadow::ElevationForStyle() {
+  switch (style_) {
+    case STYLE_ACTIVE:
+      return 24;
+    case STYLE_INACTIVE:
+      return 8;
+    case STYLE_SMALL:
+      return 6;
+  }
+  NOTREACHED();
+  return 0;
 }
 
 }  // namespace wm