Cache gfx::RenderText instances in views::Label.

ui/views/controls/label.h

Index: ui/views/controls/label.h
diff --git a/ui/views/controls/label.h b/ui/views/controls/label.h
index 603323aed0ee2f359a0f2188bdc6bcafcb94e46c..597692cdffe2e007405e92156d4aa8605025f22a 100644
--- a/ui/views/controls/label.h
+++ b/ui/views/controls/label.h
@@ -5,16 +5,10 @@
 #ifndef UI_VIEWS_CONTROLS_LABEL_H_
 #define UI_VIEWS_CONTROLS_LABEL_H_
 
-#include <string>
-#include <vector>
-
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
-#include "base/strings/string16.h"
-#include "third_party/skia/include/core/SkColor.h"
-#include "ui/gfx/font_list.h"
-#include "ui/gfx/shadow_value.h"
-#include "ui/gfx/text_constants.h"
+#include "base/memory/scoped_vector.h"
+#include "ui/gfx/render_text.h"
 #include "ui/views/view.h"
 
 namespace views {
@@ -34,11 +28,16 @@ class VIEWS_EXPORT Label : public View {
   ~Label() override;
 
   // Gets or sets the fonts used by this label.
-  const gfx::FontList& font_list() const { return font_list_; }
+  const gfx::FontList& font_list() const {
+    return render_text_ ? render_text_->font_list() : render_data_->font_list;
+  }
+
   virtual void SetFontList(const gfx::FontList& font_list);
 
   // Get or set the label text.
-  const base::string16& text() const { return text_; }
+  const base::string16& text() const {
+    return render_text_ ? render_text_->text() : render_data_->text;
+  }
   virtual void SetText(const base::string16& text);
 
   // Enables or disables auto-color-readability (enabled by default).  If this
@@ -61,7 +60,9 @@ class VIEWS_EXPORT Label : public View {
 
   // Set drop shadows underneath the text.
   void SetShadows(const gfx::ShadowValues& shadows);
-  const gfx::ShadowValues& shadows() const { return shadows_; }
+  const gfx::ShadowValues& shadows() const {
+    return render_text_ ? render_text_->shadows() : render_data_->shadows;
+  }
 
   // Sets whether subpixel rendering is used; the default is true, but this
   // feature also requires an opaque background color.
@@ -69,7 +70,9 @@ class VIEWS_EXPORT Label : public View {
 
   // Sets the horizontal alignment; the argument value is mirrored in RTL UI.
   void SetHorizontalAlignment(gfx::HorizontalAlignment alignment);
-  gfx::HorizontalAlignment GetHorizontalAlignment() const;
+  gfx::HorizontalAlignment horizontal_alignment() const {
+    return horizontal_alignment_;
+  }
 
   // Get or set the distance in pixels between baselines of multi-line text.
   // Default is 0, indicating the distance between lines should be the standard
@@ -83,15 +86,16 @@ class VIEWS_EXPORT Label : public View {
 
   // Get or set if the label text should be obscured before rendering (e.g.
   // should "Password!" display as "*********"); default is false.
-  bool obscured() const { return obscured_; }
+  bool obscured() const { return render_text_->obscured(); }

[msw, 2015/02/05 20:16:42]

Shouldn't this fall back on RenderData?

[Jun Mukai, 2015/02/12 21:43:04]

Right, thanks for pointing (I've removed render_data_ so it's okay now)

   void SetObscured(bool obscured);
 
   // Sets whether multi-line text can wrap mid-word; the default is false.
   void SetAllowCharacterBreak(bool allow_character_break);
 
   // Sets the eliding or fading behavior, applied as necessary. The default is
-  // to elide at the end. Eliding is not well supported for multi-line labels.
+  // to elide at the end. Eliding is not well-supported for multi-line labels.
   void SetElideBehavior(gfx::ElideBehavior elide_behavior);
+  gfx::ElideBehavior elide_behavior() const { return elide_behavior_; }
 
   // Sets the tooltip text.  Default behavior for a label (single-line) is to
   // show the full text if it is wider than its bounds.  Calling this overrides
@@ -125,21 +129,17 @@ class VIEWS_EXPORT Label : public View {
   gfx::Size GetPreferredSize() const override;
   gfx::Size GetMinimumSize() const override;
   int GetHeightForWidth(int w) const override;
+  void Layout() override;
   const char* GetClassName() const override;
   View* GetTooltipHandlerForPoint(const gfx::Point& point) override;
   bool CanProcessEventsWithinSubtree() const override;
   void GetAccessibleState(ui::AXViewState* state) override;
   bool GetTooltipText(const gfx::Point& p,
                       base::string16* tooltip) const override;
+  void OnEnabledChanged() override;
 
  protected:
-  // Called by Paint to paint the text.
-  void PaintText(gfx::Canvas* canvas,
-                 const base::string16& text,
-                 const gfx::Rect& text_bounds,
-                 int flags);
-
-  virtual gfx::Size GetTextSize() const;
+  void PaintText(gfx::Canvas* canvas);
 
   SkColor disabled_color() const { return actual_disabled_color_; }
 
@@ -148,56 +148,51 @@ class VIEWS_EXPORT Label : public View {
   void OnPaint(gfx::Canvas* canvas) override;
   void OnNativeThemeChanged(const ui::NativeTheme* theme) override;
   void OnDeviceScaleFactorChanged(float device_scale_factor) override;
+  void VisibilityChanged(View* starting_from, bool is_visible) override;
 
  private:
-  struct DrawStringParams {
-    DrawStringParams() : flags(0) {}
-
+  // Lightweight data to keep the information to recover RenderText.
+  struct RenderData {

[msw, 2015/02/05 20:16:42]

I don't think this is the best way to address the memory issue, but it *might*
be okay for now. Instead, could RenderText::ResetLayout() free its layout data
and expose that function publicly for competent users? That might have adverse
performance effects if it's called too frequently before the layout *is*
actually needed, but it seems like a balancing act that some classes, such as
views::Label, might be able to perform correctly.

[Jun Mukai, 2015/02/12 21:43:04]

Done, I've realized your point really after I put a few more field into this
object.

     base::string16 text;
-    gfx::Rect bounds;
-    int flags;
+    gfx::FontList font_list;
+    gfx::ShadowValues shadows;
+    bool obscured;
+    RenderData(const gfx::RenderText& render_text);

[msw, 2015/02/05 20:16:42]

Functions should be listed before member variables.

[Jun Mukai, 2015/02/19 22:01:17]

the struct itself was removed.

+    ~RenderData();
+    scoped_ptr<gfx::RenderText> CreateRenderText();
   };
 
-  // These tests call CalculateDrawStringParams in order to verify the
-  // calculations done for drawing text.
-  FRIEND_TEST_ALL_PREFIXES(LabelTest, DrawSingleLineString);
-  FRIEND_TEST_ALL_PREFIXES(LabelTest, DrawMultiLineString);
-  FRIEND_TEST_ALL_PREFIXES(LabelTest, DrawSingleLineStringInRTL);
-  FRIEND_TEST_ALL_PREFIXES(LabelTest, DrawMultiLineStringInRTL);
-  FRIEND_TEST_ALL_PREFIXES(LabelTest, DirectionalityFromText);
-  FRIEND_TEST_ALL_PREFIXES(LabelTest, DisableSubpixelRendering);
-
-  // Sets both |text_| and |layout_text_| to appropriate values, taking
-  // the label's 'obscured' status into account.
-  void SetTextInternal(const base::string16& text);
-
   void Init(const base::string16& text, const gfx::FontList& font_list);
 
-  void RecalculateColors();
+  void ResetLayout();
 
-  // Returns where the text is drawn, in the receivers coordinate system.
-  gfx::Rect GetTextBounds() const;
+  // Get the text broken into lines as needed to fit the given |width|.
+  std::vector<base::string16> GetLinesForWidth(int width) const;
 
-  int ComputeDrawStringFlags() const;
+  // Get the natural text size, unelided and only wrapped on newlines.
+  gfx::Size GetTextSize() const;
 
-  gfx::Rect GetAvailableRect() const;
-
-  // Returns parameters to be used for the DrawString call. Returned value is a
-  // weak pointer, owned by and scoped to the label.
-  const DrawStringParams* CalculateDrawStringParams() const;
+  void RecalculateColors();
 
   // Updates any colors that have not been explicitly set from the theme.
   void UpdateColorsFromTheme(const ui::NativeTheme* theme);
 
-  // Resets |cached_heights_|, |cached_heights_cursor_|, |cached_draw_params_|
-  // and mark |text_size_valid_| as false.
-  void ResetLayoutCache();
-
   bool ShouldShowDefaultTooltip() const;
 
-  base::string16 text_;
-  base::string16 layout_text_;
-  gfx::FontList font_list_;
+  // An un-elided and single-line RenderText object used for preferred sizing.
+  scoped_ptr<gfx::RenderText> render_text_;
+
+  // The RenderText instances used to display elided and multi-line text.
+  ScopedVector<gfx::RenderText> lines_;
+
+  // This field is for memory optimization: render_text_ is memory-heavy because

[msw, 2015/02/05 20:16:42]

nit: |render_text_| to match below, or RenderText.

[Jun Mukai, 2015/02/19 22:01:17]

comment was removed.

+  // it holds several data for layouts. Therefore in case it's not visible, use

[msw, 2015/02/05 20:16:42]

nit: s/several data for layouts/layout data/
also: s/Therefore in case it's not visible/When the label is not visible/

[Jun Mukai, 2015/02/19 22:01:17]

comment was removed.

+  // this struct instead of |render_text_| itself.
+  scoped_ptr<RenderData> render_data_;
+
+  gfx::HorizontalAlignment horizontal_alignment_;
+  gfx::ElideBehavior elide_behavior_;
+
   SkColor requested_enabled_color_;
   SkColor actual_enabled_color_;
   SkColor requested_disabled_color_;
@@ -211,26 +206,14 @@ class VIEWS_EXPORT Label : public View {
 
   bool subpixel_rendering_enabled_;
   bool auto_color_readability_;
-  mutable gfx::Size text_size_;
-  mutable bool text_size_valid_;
-  int line_height_;
-  bool multi_line_;
-  bool obscured_;
-  bool allow_character_break_;
-  gfx::ElideBehavior elide_behavior_;
-  gfx::HorizontalAlignment horizontal_alignment_;
   base::string16 tooltip_text_;
   bool handles_tooltips_;
   // Whether to collapse the label when it's not visible.
   bool collapse_when_hidden_;
-  gfx::ShadowValues shadows_;
-
-  // The cached heights to avoid recalculation in GetHeightForWidth().
-  mutable std::vector<gfx::Size> cached_heights_;
-  mutable int cached_heights_cursor_;
-
-  // The cached results of CalculateDrawStringParams().
-  mutable DrawStringParams cached_draw_params_;
+  bool allow_character_break_;
+  bool multi_line_;
+  int line_height_;
+  int max_width_;
 
   // TODO(vadimt): Remove is_first_paint_text_ before crbug.com/431326 is
   // closed.