views::Separator cleanup.

ui/views/controls/separator.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/controls/separator.h"
 
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/gfx/canvas.h"
 #include "ui/native_theme/native_theme.h"
 
 namespace views {
 
 // static
 const char Separator::kViewClassName[] = "Separator";
 
-// The separator size in pixels.
-const int kSeparatorSize = 1;
+// static
+const int Separator::kThickness = 1;
 
-Separator::Separator(Orientation orientation)
-    : orientation_(orientation),
-      color_overridden_(false),
-      size_(kSeparatorSize) {
-  SetColorFromNativeTheme();
-}
+Separator::Separator() : preferred_size_(kThickness, kThickness) {}
 
-Separator::~Separator() {
-}
+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;
 }
 
 void Separator::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   node_data->role = ui::AX_ROLE_SPLITTER;
 }
 
 void Separator::OnPaint(gfx::Canvas* canvas) {
-  canvas->FillRect(GetContentsBounds(), color_);
-}
+  SkColor color = overridden_color_
+                      ? *overridden_color_
+                      : GetNativeTheme()->GetSystemColor(
+                            ui::NativeTheme::kColorId_SeparatorColor);
 
-void Separator::OnNativeThemeChanged(const ui::NativeTheme* theme) {
-  if (!color_overridden_)
-    SetColorFromNativeTheme();
+  // 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 {
   return kViewClassName;
 }
 
 }  // namespace views