Fixed BubbleBorder sizing problems & added unit tests.

ui/views/bubble/bubble_border.h

 // 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.
 
 #ifndef UI_VIEWS_BUBBLE_BUBBLE_BORDER_H_
 #define UI_VIEWS_BUBBLE_BUBBLE_BORDER_H_
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "ui/views/background.h"
@@ skipping 140 matching lines @@
   void set_arrow_offset(int offset) { arrow_offset_ = offset; }
 
   // Sets the way the arrow is actually painted.  Default is PAINT_NORMAL.
   void set_paint_arrow(ArrowPaintType value) { arrow_paint_type_ = value; }
 
   // Get the desired widget bounds (in screen coordinates) given the anchor rect
   // and bubble content size; calculated from shadow and arrow image dimensions.
   virtual gfx::Rect GetBounds(const gfx::Rect& anchor_rect,
                               const gfx::Size& contents_size) const;
 
+  // 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 GetBorderThickness() const;
+  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_;
   BubbleAlignment alignment_;
   Shadow shadow_;
   internal::BorderImages* images_;
   SkColor background_color_;
   bool use_theme_background_color_;
 
   DISALLOW_COPY_AND_ASSIGN(BubbleBorder);
@@ skipping 10 matching lines @@
 
  private:
   BubbleBorder* border_;
 
   DISALLOW_COPY_AND_ASSIGN(BubbleBackground);
 };
 
 }  // namespace views
 
 #endif  // UI_VIEWS_BUBBLE_BUBBLE_BORDER_H_