views::Separator cleanup.

ui/views/controls/separator.h

Index: ui/views/controls/separator.h
diff --git a/ui/views/controls/separator.h b/ui/views/controls/separator.h
index d4689fe209748b9241bebf63b616d9227a4ccf91..d231d3a22308e1794118475543058caa96a9769a 100644
--- a/ui/views/controls/separator.h
+++ b/ui/views/controls/separator.h
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/macros.h"
+#include "base/optional.h"
 #include "ui/views/view.h"
 
 namespace views {
@@ -18,38 +19,39 @@ namespace views {
 class VIEWS_EXPORT Separator : public View {
  public:
   enum Orientation {
+    // The separator is a horizontal line that distinguishes vertically stacked
+    // elements.
     HORIZONTAL,
-    VERTICAL
+    // The separator is a vertical line that distinguishes a row of elements.
+    VERTICAL,
   };
 
   // The separator's class name.
   static const char kViewClassName[];
 
+  // The separator's thickness in dip.
+  static const int kThickness;

[sky, 2017/02/03 23:49:45]

constexpr?

[Evan Stade, 2017/02/06 16:31:27]

I don't understand this completely but this is the only way I could get the
build to link (i.e. if this is defined inline with or without constexpr then it
won't be exported for other build components).

+
   explicit Separator(Orientation orientation);
   ~Separator() override;
 
-  SkColor color() const { return color_; }
   void SetColor(SkColor color);
 
-  int size() const { return size_; }
-  // Preferred size of one axis: height for horizontal separator
-  // and width for vertical separator
-  void SetPreferredSize(int size);
+  // Preferred length: width for horizontal separator and height for vertical
+  // separator.
+  void SetPreferredLength(int length);

[sky, 2017/02/03 23:49:45]

Is there a reason you prefer length here? I like size simply because it maps
nicely with GetPreferredSize.

[Evan Stade, 2017/02/06 16:31:27]

That's exactly why I don't like size. Which dimension is it referring to?
Existing client code got it wrong, so it must not be very obvious whether "size"
means length or thickness.

[sky, 2017/02/06 17:59:21]

I don't think there is a right answer here. I prefer size.

[Evan Stade, 2017/02/06 19:03:29]

size is used to describe a multi-dimensional value. Because of the confusion
over what this meant before (it actually used to be thickness), we had a
vertical line that was being implemented as a very thick but not long horizontal
separator, i.e. instead of

|
|
|
|

it was

-
-
-
-

[sky, 2017/02/06 22:42:55]

'size' is used not inheritantly a multi-dimensional value. gfx::Size is, but
this functions takes a single int. I don't think length is more intuitive in
this case.

[Evan Stade, 2017/02/07 16:57:54]

ok, but before this cl, this "size" actually meant thickness (i.e. orthogonal to
the Orientation). Now it will mean length (i.e. parallel to the Orientation
direction). So we're changing the meaning of the function here. Which meaning is
more intuitively correlated with "size"?

In the latest patchset I've tried another tactic to make this more clear. Since
orientation is only useful if you set a preferred length/size I've combined
setting the orientation and the size, which hopefully makes it more obvious what
this function is doing, and also relieves the vast majority of callers from
needing to specify an orientation at all.

 
   // Overridden from View:
   gfx::Size GetPreferredSize() const override;
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
   void OnPaint(gfx::Canvas* canvas) override;
-  void OnNativeThemeChanged(const ui::NativeTheme* theme) override;
   const char* GetClassName() const override;
 
  private:
-  void SetColorFromNativeTheme();
-
   const Orientation orientation_;
-  SkColor color_;
-  bool color_overridden_;
-  int size_;
+  base::Optional<SkColor> overridden_color_;
+  // The length to request in GetPreferredSize().
+  int length_ = 1;
 
   DISALLOW_COPY_AND_ASSIGN(Separator);
 };