Clean up of SelectionModel

ui/gfx/selection_model.h

Index: ui/gfx/selection_model.h
===================================================================
--- ui/gfx/selection_model.h	(revision 103192)
+++ ui/gfx/selection_model.h	(working copy)
@@ -45,6 +45,14 @@
   enum CaretPlacement {
     LEADING,
     TRAILING,
+    // TRAILING_OF_PREVIOUS_GRAPHEME means cursor is visually at the trailing
+    // edge of |selection_end_|'s previous grapheme. It is used when create a

[msw, 2011/09/29 02:20:21]

Grammar: "means *that the* cursor".
Grammar: "It is used when creating" or "It is used to create".

I don't think specifics about the current use(s) in the second sentence is
helpful. I think the first sentence of this comment sufficiently describes the
purpose of this value, and the third sentence attempts to note that the caret
position and placement are resolved on SetSelectionModel(), which is important,
but it does a poor job of describing that behavior and why it's necessary.
Instead try:

TRAILING_OF_PREVIOUS_GRAPHEME means that the cursor will visually trail
|selection_end_|'s previous grapheme or lead |selection_end_| if it has no
previous grapheme. The actual caret position and placement are resolved in
SetSelectionModel, where a previous grapheme can be calculated from the current
text.

[xji, 2011/10/03 23:18:57]

it is removed.

+    // SelectionModel with non-empty selection and the selection's start is less
+    // than the selection's end. When SelectionModel is used in RenderText, in
+    // SetSelectionModel(), in case of TRAILING_OF_PREVIOUS_GRAPHEME,
+    // |caret_pos_| will be set as the index of |selection_end_|'s previous
+    // grapheme, and |caret_placement_| will be set as TRAILING.
+    TRAILING_OF_PREVIOUS_GRAPHEME,

[msw, 2011/09/29 02:20:21]

I like your old name (TRAILING_PREIVOUS_GRAPHEME) better.

   };
 
   SelectionModel();
@@ -59,21 +67,22 @@
   void set_selection_start(size_t pos) { selection_start_ = pos; }

[msw, 2011/09/29 02:20:21]

Should set_selection_start also be made private?

[xji, 2011/10/03 23:18:57]

Done.

 
   size_t selection_end() const { return selection_end_; }
-  void set_selection_end(size_t pos) { selection_end_ = pos; }
-
   size_t caret_pos() const { return caret_pos_; }
-  void set_caret_pos(size_t pos) { caret_pos_ = pos; }
-
   CaretPlacement caret_placement() const { return caret_placement_; }
-  void set_caret_placement(CaretPlacement placement) {
-    caret_placement_ = placement;
-  }
 
   bool Equals(const SelectionModel& sel) const;
 
  private:
+  friend class RenderText;

[msw, 2011/09/29 02:20:21]

I don't know that making RenderText a friend and moving setters to private is a
good change. It seems okay, but I would appreciate another reviewer's
perspective.

+
   void Init(size_t start, size_t end, size_t pos, CaretPlacement status);
 
+  void set_selection_end(size_t pos) { selection_end_ = pos; }
+  void set_caret_pos(size_t pos) { caret_pos_ = pos; }
+

[msw, 2011/09/29 02:20:21]

You can remove this blank line.

[xji, 2011/10/03 23:18:57]

Done.

+  void set_caret_placement(CaretPlacement placement) {
+    caret_placement_ = placement;
+  }

[msw, 2011/09/29 02:20:21]

You should add a blank line after this function, before the selection_start_
comment.

[xji, 2011/10/03 23:18:57]

Done.

   // Logical selection start. If there is non-empty selection, if
   // selection_start_ is less than selection_end_, the selection starts visually
   // at the leading edge of the selection_start_. If selection_start_ is greater