views::Separator cleanup.

ui/views/controls/separator.cc

Index: ui/views/controls/separator.cc
diff --git a/ui/views/controls/separator.cc b/ui/views/controls/separator.cc
index 5ec611832bd1aa7758a4d86702fb74c4a781b7a9..4216682d6dc57e6821341046a052cd8dfc31628f 100644
--- a/ui/views/controls/separator.cc
+++ b/ui/views/controls/separator.cc
@@ -13,43 +13,39 @@ namespace views {
 // static
 const char Separator::kViewClassName[] = "Separator";
 
-// The separator size in pixels.
-const int kSeparatorSize = 1;
-
-Separator::Separator(Orientation orientation)
-    : orientation_(orientation),
-      color_overridden_(false),
-      size_(kSeparatorSize) {
-  SetColorFromNativeTheme();
-}
+// static
+const int Separator::kThickness = 1;
 
-Separator::~Separator() {
-}
+Separator::Separator() : preferred_size_(kThickness, kThickness) {}
+
+Separator::~Separator() {}
 
 void Separator::SetColor(SkColor color) {
-  color_ = color;
-  color_overridden_ = true;
+  overridden_color_ = color;
   SchedulePaint();
 }
 
-void Separator::SetPreferredSize(int size) {
-  if (size != size_) {
-    size_ = size;
-    PreferredSizeChanged();
-  }
+void Separator::SetPreferredWidth(int width) {
+  if (width == preferred_size_.width())

[sky, 2017/02/09 03:16:09]

WDYT of calling a common (private) SetPreferredSize. I'm specifically suggesting
the two functions become something like:

SetPreferredSize(gfx::Size(width, preferred_size_.height()));

The early out and other code moves to said function.

[Evan Stade, 2017/02/09 15:46:11]

I was on the fence about creating a shared private function. The early out is
actually a bit silly given how this is used (it's not now and unlikely to ever
be called more than once, much less many times).

As far as allowing callers to set both height and width, this is purely
philosophical since no callers actually want to do so right now (in fact we
don't even have any callers of SetPreferredWidth at the moment). But I erred on
the side of not supporting something that doesn't yet have a use case, because
adding a new behavior such as a double-wide separator IMO should have extra
friction (i.e. updating this class).

We could always circumvent this issue by removing SetPreferredWidth and adding a
note that says "add it if you need it".

[sky, 2017/02/09 20:32:50]

I'm ok with only what we need and no note. If someone needs the functionality
they'll figure they need to add it.

+    return;
+
+  preferred_size_ = gfx::Size(width, kThickness);
+  PreferredSizeChanged();
 }
 
-void Separator::SetColorFromNativeTheme() {
-  color_ = GetNativeTheme()->GetSystemColor(
-      ui::NativeTheme::kColorId_SeparatorColor);
+void Separator::SetPreferredHeight(int height) {
+  if (height == preferred_size_.height())
+    return;
+
+  preferred_size_ = gfx::Size(kThickness, height);
+  PreferredSizeChanged();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Separator, View overrides:
 
 gfx::Size Separator::GetPreferredSize() const {
-  gfx::Size size =
-      orientation_ == HORIZONTAL ? gfx::Size(1, size_) : gfx::Size(size_, 1);
+  gfx::Size size = preferred_size_;
   gfx::Insets insets = GetInsets();
   size.Enlarge(insets.width(), insets.height());
   return size;
@@ -60,12 +56,15 @@ void Separator::GetAccessibleNodeData(ui::AXNodeData* node_data) {
 }
 
 void Separator::OnPaint(gfx::Canvas* canvas) {
-  canvas->FillRect(GetContentsBounds(), color_);
-}
-
-void Separator::OnNativeThemeChanged(const ui::NativeTheme* theme) {
-  if (!color_overridden_)
-    SetColorFromNativeTheme();
+  SkColor color = overridden_color_
+                      ? *overridden_color_
+                      : GetNativeTheme()->GetSystemColor(
+                            ui::NativeTheme::kColorId_SeparatorColor);
+
+  // The separator fills its bounds, but avoid filling partial pixels.
+  float dsf = canvas->UndoDeviceScaleFactor();
+  gfx::RectF contents = gfx::ScaleRect(gfx::RectF(GetContentsBounds()), dsf);
+  canvas->FillRect(gfx::ToEnclosedRect(contents), color);
 }
 
 const char* Separator::GetClassName() const {