Use RelocatablePosition in CompositeEditCommand::moveParagraphs

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

 /*
  * Copyright (C) 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 1264 matching lines @@
             startIndex = 0;
             if (startInParagraph)
                 startIndex = TextIterator::rangeLength(startOfParagraphToMove.toParentAnchoredPosition(), visibleStart.toParentAnchoredPosition(), true);
 
             endIndex = 0;
             if (endInParagraph)
                 endIndex = TextIterator::rangeLength(startOfParagraphToMove.toParentAnchoredPosition(), visibleEnd.toParentAnchoredPosition(), true);
         }
     }
 
-    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.
     Position start = mostForwardCaretPosition(startOfParagraphToMove.deepEquivalent());
     Position end = mostBackwardCaretPosition(endOfParagraphToMove.deepEquivalent());
 
     // FIXME: This is an inefficient way to preserve style on nodes in the paragraph to move. It
     // shouldn't matter though, since moved paragraphs will usually be quite small.
     DocumentFragment* fragment = startOfParagraphToMove.deepEquivalent() != endOfParagraphToMove.deepEquivalent() ?
         createFragmentFromMarkup(document(), createMarkup(start.parentAnchoredEquivalent(), end.parentAnchoredEquivalent(), DoNotAnnotateForInterchange, ConvertBlocksToInlines::Convert, DoNotResolveURLs, constrainingAncestor), "") : nullptr;
@@ skipping 23 matching lines @@
         return;
     DCHECK(destination.deepEquivalent().inShadowIncludingDocument()) << destination;
 
     // Add a br if pruning an empty block level element caused a collapse. For example:
     // foo^
     // <div>bar</div>
     // baz
     // 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);
         if (editingState->isAborted())
             return;
         // Need an updateLayout here in case inserting the br has split a text node.
         document().updateStyleAndLayoutIgnorePendingStylesheets();
     }
 
     destinationIndex = TextIterator::rangeLength(Position::firstPositionInNode(document().documentElement()), destination.toParentAnchoredPosition(), true);
@@ skipping 276 matching lines @@
 }
 
 DEFINE_TRACE(CompositeEditCommand)
 {
     visitor->trace(m_commands);
     visitor->trace(m_composition);
     EditCommand::trace(visitor);
 }
 
 } // namespace blink