Clean up of SelectionModel

ui/gfx/selection_model.h

 // Copyright (c) 2011 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_GFX_SELECTION_MODEL_H_
 #define UI_GFX_SELECTION_MODEL_H_
 #pragma once
 
 #include <stdlib.h>
 
@@ skipping 27 matching lines @@
 // information to specify to which character and on which edge of the character
 // the visual cursor is bound to. For example, the visual cursor is bound to
 // the trailing side of the 2nd character 'c' when pointing to right half of
 // 'c'. And it is bound to the leading edge of the 3rd character 'D' when
 // pointing to right of 'D'.
 class UI_EXPORT SelectionModel {
  public:
   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();
   explicit SelectionModel(size_t pos);
   SelectionModel(size_t start, size_t end);
   SelectionModel(size_t end, size_t pos, CaretPlacement status);
   SelectionModel(size_t start, size_t end, size_t pos, CaretPlacement status);
 
   virtual ~SelectionModel();
 
   size_t selection_start() const { return selection_start_; }
   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
   // than selection_end_, the selection starts visually at the trailing edge of
   // selection_start_'s previous grapheme. So, we do not need extra information
   // for visual bounding.
   size_t selection_start_;
 
   // The logical cursor position that next character will be inserted into.
   // It is also the end of the selection.
   size_t selection_end_;
 
   // The following two fields are used to guide cursor visual position.
   // The index of the character that cursor is visually attached to.
   size_t caret_pos_;
   // The visual placement of the cursor, relative to its associated character.
   CaretPlacement caret_placement_;
 };
 
 }  // namespace gfx
 
 #endif  // UI_GFX_SELECTION_MODEL_H_