Use RelocatablePosition in CompositeEditCommand::moveParagraphs

third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp

Index: third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
diff --git a/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp b/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
index c0d9030727ba5011dc26aa0840f4266a819d1eea..67d34185a4e3ec38b8c529daa7ff10fe30a8f1ef 100644
--- a/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
+++ b/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
@@ -1282,8 +1282,12 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
         }
     }
 
-    VisiblePosition beforeParagraph = previousPositionOf(startOfParagraphToMove, CannotCrossEditingBoundary);
-    VisiblePosition afterParagraph = nextPositionOf(endOfParagraphToMove, CannotCrossEditingBoundary);
+    // Create a temporary Range to keep track of the "hole" left by moving away the paragraph.
+    Position beforeParagraphPos = previousPositionOf(startOfParagraphToMove, CannotCrossEditingBoundary).deepEquivalent();
+    Range* beforeParagraphTempRange = beforeParagraphPos.isNull() ? nullptr : Range::create(document(), beforeParagraphPos, beforeParagraphPos);

[yosin_UTC9, 2016/07/06 01:52:25]

The rule is when we use temporary range, it should be scoped.

Thus, we need to have stack allocated TemporaryRange class which calls
Range::dispose() in dtor.

In this use case, we need to combine TemporaryRange and WTF::Optional<T> ==
base::Optional<T> == std::optional<T> in c++17
However, we decided not to introduce TemporaryRange at early stage of Oilpan
project, since it makes
Oilpan code looks ugly.

Give that, I propose to introduce RelocatablePosition, or another good name,
which can represent null position
and encapsulates usage of Range in that class.

If we have RelocatablePosition, we could write:

// Note: Blink prefers full noun rather than abbreviation, e.g. "Position" over
"Pos".
const RelocatablePosition beforParagraphPosition =
previousPositionOf(startOfParagraphToMove,
CannotCrossEditingBoundary).deepEquivalent();
const RelocatablePosition afterParagraphPosition =
nextPositionOf(endOfParagraphToMove,
CannotCrossEditingBoundary).deepEquivalent();
...
const VisiblePosition beforeParagraph =
createVisiblePosition(beforeParagraphPosition.posiiton());
const VisiblePosition afterParagraph =
createVisiblePosition(afterParagraphPosition.posiiton());


# Sketch of RelocatablePositon
class RelocatablePositon final {
  STACK_ALLOCATED();
public:
  RelocatablePositon();
  RelocatablePositon(const Position& position) : m_position(position)
  {
     if (m_position.isNull())
       return;
     m_range = Range::crate(position, position);
  }
  ~RelocatablePositon()
  {
    if (!m_range)
      return;
    m_range->dispose();
  }

  const Position& position() const { return m_position; }

private:
  Position m_position;
  Member<Range> m_range

  DISALLOW_COPY_AND_ASSIGN();
}


I expect other places of execCommand implementations uses RelocatablePosition.

+
+    Position afterParagraphPos = nextPositionOf(endOfParagraphToMove, CannotCrossEditingBoundary).deepEquivalent();
+    Range* afterParagraphTempRange = afterParagraphPos.isNull() ? nullptr : Range::create(document(), afterParagraphPos, afterParagraphPos);
 
     // We upstream() the end and downstream() the start so that we don't include collapsed whitespace in the move.
     // When we paste a fragment, spaces after the end and before the start are treated as though they were rendered.
@@ -1327,8 +1331,8 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
     // Imagine moving 'bar' to ^. 'bar' will be deleted and its div pruned. That would
     // cause 'baz' to collapse onto the line with 'foobar' unless we insert a br.
     // Must recononicalize these two VisiblePositions after the pruning above.
-    beforeParagraph = createVisiblePosition(beforeParagraph.deepEquivalent());
-    afterParagraph = createVisiblePosition(afterParagraph.deepEquivalent());
+    VisiblePosition beforeParagraph = beforeParagraphTempRange ? createVisiblePosition(beforeParagraphTempRange->startPosition()) : VisiblePosition();
+    VisiblePosition afterParagraph = afterParagraphTempRange ? createVisiblePosition(afterParagraphTempRange->startPosition()) : VisiblePosition();
     if (beforeParagraph.isNotNull() && (!isEndOfParagraph(beforeParagraph) || beforeParagraph.deepEquivalent() == afterParagraph.deepEquivalent())) {
         // FIXME: Trim text between beforeParagraph and afterParagraph if they aren't equal.
         insertNodeAt(HTMLBRElement::create(document()), beforeParagraph.deepEquivalent(), editingState);