Fixed BubbleBorder sizing problems & added unit tests.

ui/views/bubble/bubble_border.h

Index: ui/views/bubble/bubble_border.h
diff --git a/ui/views/bubble/bubble_border.h b/ui/views/bubble/bubble_border.h
index 15f0f85f60ddcae2a44a1f373ecc4dbb1320866f..3cdf02d336c6665bf437026823aa7a73e25df703 100644
--- a/ui/views/bubble/bubble_border.h
+++ b/ui/views/bubble/bubble_border.h
@@ -158,26 +158,45 @@ class VIEWS_EXPORT BubbleBorder : public Border {
   virtual gfx::Rect GetBounds(const gfx::Rect& anchor_rect,
                               const gfx::Size& contents_size) const;
 
-  // Get the border exterior thickness, including stroke and shadow, in pixels.
+  // Get the arrow thickness.

[msw, 2014/08/19 21:35:39]

Expanding unit tests shouldn't expand BubbleBorder's API, especially the public
surface. Instead of adding all these public accessors that aren't otherwise
needed, you should add a private BorderImages* GetImagesForTest() accessor that
just returns images_ (for direct access to BorderImages metrics), and a new
private GetArrowSize, then add FRIEND_TEST_ALL_PREFIXES for the needed tests.

[bruthig, 2014/08/20 15:01:28]

The reason I didn't add a BorderImages* GetImagesForTest() originally is because
the images_ type 'BorderImages' is a struct defined in the .cc file and not
accessible in the tests.  I thought about moving the definition to the .h file
but it was in the 'internal' namespace and I wasn't sure if that was the right
thing to do or not.  If I moved it to the .h file it would also require
#includes for gfx::ImageSkia.

I was unfamiliar with the FRIEND_TEST_ALL_PREFIXES, thanks for pointing it out
:)

Initially I wanted to create an abstraction for the BorderImages type so that I
could inject a test double that didn't actually depend on the ResourceBundle and
didn't actually require a Painter, but breaking the Painter dependency would
have required inverting the painting ownership from the BubbleBorder to the
BorderImages.  Ultimately I had decided this was too large a change for right
now and had a questionable amount of return on investment.

Perhaps the BorderImages definition should be added to an internal.h file which
can be included in the tests.

Thoughts?

[msw, 2014/08/20 20:41:02]

Yeah, move the internal::BorderImages decl to this bubble_border.h.

[bruthig, 2014/08/22 19:45:04]

Done.

+  int GetArrowThickness() const;
+
+  // Get the top arrow width.
+  int GetTopArrowWidth() const;
+
+  // Get the border thickness.
   int GetBorderThickness() const;
 
+  // Get the border interior thickness.
+  int GetBorderInteriorThickness() const;
+
+  // Get the border exterior thickness, including stroke and shadow, in pixels.
+  int GetBorderExteriorThickness() const;

[msw, 2014/08/19 21:35:39]

nit: revert this name change when you remove the accessors.

[bruthig, 2014/08/22 19:45:04]

Done.

+
   // Returns the corner radius of the current image set.
   int GetBorderCornerRadius() const;
 
   // Gets the arrow offset to use.
   int GetArrowOffset(const gfx::Size& border_size) const;
 
+  // Gets the arrow size to use.
+  int GetArrowSize() const;
+
   // Overridden from Border:
   virtual void Paint(const View& view, gfx::Canvas* canvas) OVERRIDE;
   virtual gfx::Insets GetInsets() const OVERRIDE;
   virtual gfx::Size GetMinimumSize() const OVERRIDE;
 
  private:
+  friend class BubbleBorderTest;
+
   gfx::Size GetSizeForContentsSize(const gfx::Size& contents_size) const;
   gfx::ImageSkia* GetArrowImage() const;
   gfx::Rect GetArrowRect(const gfx::Rect& bounds) const;
   void DrawArrow(gfx::Canvas* canvas, const gfx::Rect& arrow_bounds) const;
 
+  // The border and arrow stroke size used in image assets, in pixels.
+  static const int kStroke;

[msw, 2014/08/19 21:35:39]

statics should be declared before functions as per the style-guide. Move this
between "friend class BubbleBorderTest" and "...GetSizeForContentsSize..." with
a blank line above and one below).

[bruthig, 2014/08/22 19:45:04]

Done.

   Arrow arrow_;
   int arrow_offset_;
   ArrowPaintType arrow_paint_type_;