Use RelocatablePosition in CompositeEditCommand::moveParagraphs

Use RelocatablePosition in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when the VisiblePositions are used later. It causes
the failure of layout test:

fast/text/first-letter-bad-line-boxes-crash.html

This patch uses RelocatablePosition for the tracking purpose as
a bug fix.

BUG=581038
TEST=fast/text/first-letter-bad-line-boxes-crash.html

Committed: https://crrev.com/14bd2353e018993e07996f570cd58c2b3520d40f
Cr-Commit-Position: refs/heads/master@{#403879}

[Xiaocheng, 2016-07-05 08:43:11]

Description was changed from

==========
Use temporary Ranges in CompositeEditCommand::moveParagraphs

BUG=451440,581038
==========

to

==========
Use temporary Ranges in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when we use the VisiblePositions later.

This patch uses a pair of temporary Ranges instead, so that the
anchor nodes are always in document and the crash is fixed.

BUG=451440,581038
==========

[Xiaocheng, 2016-07-05 08:43:11]

xiaochengh@chromium.org changed reviewers:
+ yosin@chromium.org

[Xiaocheng, 2016-07-05 09:52:34]

Description was changed from

==========
Use temporary Ranges in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when we use the VisiblePositions later.

This patch uses a pair of temporary Ranges instead, so that the
anchor nodes are always in document and the crash is fixed.

BUG=451440,581038
==========

to

==========
Use temporary Ranges in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when we use the VisiblePositions later.

This patch uses a pair of temporary Ranges instead, so that the
anchor nodes are always in document and the crash is fixed.

BUG=625314
==========

[Xiaocheng, 2016-07-05 10:12:29]

PTAL.

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

Please prepare for writing test. ;-)

https://codereview.chromium.org/2127503002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
(right):

https://codereview.chromium.org/2127503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:1287:
Range* beforeParagraphTempRange = beforeParagraphPos.isNull() ? nullptr :
Range::create(document(), beforeParagraphPos, beforeParagraphPos);
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.

[yosin_UTC9, 2016-07-06 02:28:37]

On 2016/07/06 at 01:52:25, Yosi_UTC9 wrote:
> Please prepare for writing test. ;-)
> 
>
https://codereview.chromium.org/2127503002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
(right):
> 
>
https://codereview.chromium.org/2127503002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:1287:
Range* beforeParagraphTempRange = beforeParagraphPos.isNull() ? nullptr :
Range::create(document(), beforeParagraphPos, beforeParagraphPos);
> 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.

What's wrong I am!


# Sketch of RelocatablePositon
class RelocatablePositon final {
  STACK_ALLOCATED();
public:
  explicit 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();
  }

  Position position() const    
  { 
    if (!m_range)
      return Position();
    DCHECK(m_range->collapsed());
    return m_range->startPosition();
  }

private:
  const Member<Range> m_range

  DISALLOW_COPY_AND_ASSIGN();
}

[Xiaocheng, 2016-07-06 03:28:16]

On 2016/07/06 at 02:28:37, yosin wrote:
> On 2016/07/06 at 01:52:25, Yosi_UTC9 wrote:
> > Please prepare for writing test. ;-)

fast/text/first-letter-bad-line-boxes-crash.html is already serving this
purpose.

Maybe I should set this CL just for crbug.com/581038 (Layout test
fast/text/first-letter-bad-line-boxes-crash.html failed), and mark
crbug.com/625314 as a duplicate.

> > 
> >
https://codereview.chromium.org/2127503002/diff/1/third_party/WebKit/Source/c...
> > File
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
(right):
> > 
> >
https://codereview.chromium.org/2127503002/diff/1/third_party/WebKit/Source/c...
> >
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:1287:
Range* beforeParagraphTempRange = beforeParagraphPos.isNull() ? nullptr :
Range::create(document(), beforeParagraphPos, beforeParagraphPos);
> > 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());

This pattern seems to be pretty useful. Let me add it into the code base.

> > 
> > 
> > # 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.
> 
> What's wrong I am!
> 
> 
> # Sketch of RelocatablePositon
> class RelocatablePositon final {
>   STACK_ALLOCATED();
> public:
>   explicit 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();
>   }
> 
>   Position position() const    
>   { 
>     if (!m_range)
>       return Position();
>     DCHECK(m_range->collapsed());
>     return m_range->startPosition();
>   }
> 
> private:
>   const Member<Range> m_range
> 
>   DISALLOW_COPY_AND_ASSIGN();
> }

[Xiaocheng, 2016-07-06 03:31:22]

Description was changed from

==========
Use temporary Ranges in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when we use the VisiblePositions later.

This patch uses a pair of temporary Ranges instead, so that the
anchor nodes are always in document and the crash is fixed.

BUG=625314
==========

to

==========
Use temporary Ranges in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when we use the VisiblePositions later.

This patch uses a pair of temporary Ranges instead, so that the
anchor nodes are always in document and the crash is fixed.

BUG=581038
==========

[Xiaocheng, 2016-07-06 03:31:39]

Description was changed from

==========
Use temporary Ranges in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when we use the VisiblePositions later.

This patch uses a pair of temporary Ranges instead, so that the
anchor nodes are always in document and the crash is fixed.

BUG=581038
==========

to

==========
Use temporary Ranges in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when we use the VisiblePositions later.

This patch uses a pair of temporary Ranges instead, so that the
anchor nodes are always in document and the crash is fixed.

BUG=581038
TEST=fast/text/first-letter-bad-line-boxes-crash.html
==========

[Xiaocheng, 2016-07-06 05:17:01]

PTAL.

I'll upload RelocatablePosition in a separate patch if this one looks good.

[Xiaocheng, 2016-07-06 06:43:40]

xiaochengh@chromium.org changed reviewers:
+ tkent@chromium.org

[Xiaocheng, 2016-07-06 06:43:40]

+tkent@ as core/ owner.

[yosin_UTC9, 2016-07-06 06:50:30]

Seems good!

Please split this patch into RelocatablePosition w/ test and movePargaphs w/
tests.

https://codereview.chromium.org/2127503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/RelocatablePosition.cpp (right):

https://codereview.chromium.org/2127503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/RelocatablePosition.cpp:10: :
m_range(nullptr)
You don't need to initialize |Member<Range>|

https://codereview.chromium.org/2127503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/RelocatablePosition.cpp:21: if (m_range)
We prefer early-return style.

https://codereview.chromium.org/2127503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/RelocatablePosition.h (right):

https://codereview.chromium.org/2127503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/RelocatablePosition.h:19:
RelocatablePosition();
Do we really need default ctor?
Since, |RelocatablePosition| is disallow-copy-and-assign, default ctor isn't
needed.
Or, at least this bug.

https://codereview.chromium.org/2127503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/RelocatablePosition.h:28:
STACK_ALLOCATED();
nit: It seems |STACK_ALLOCATED()| usually at top.

[Xiaocheng, 2016-07-06 06:58:34]

Description was changed from

==========
Use temporary Ranges in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when we use the VisiblePositions later.

This patch uses a pair of temporary Ranges instead, so that the
anchor nodes are always in document and the crash is fixed.

BUG=581038
TEST=fast/text/first-letter-bad-line-boxes-crash.html
==========

to

==========
Use temporary Ranges in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when we use the VisiblePositions later. Specifically,
layout test fast/text/first-letter-bad-line-boxes-crash.html fails
in this way.

This patch uses a pair of temporary Ranges instead, so that the
anchor nodes are always in document and the crash is fixed.

BUG=581038
TEST=fast/text/first-letter-bad-line-boxes-crash.html
==========

[Xiaocheng, 2016-07-06 07:45:59]

Description was changed from

==========
Use temporary Ranges in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when we use the VisiblePositions later. Specifically,
layout test fast/text/first-letter-bad-line-boxes-crash.html fails
in this way.

This patch uses a pair of temporary Ranges instead, so that the
anchor nodes are always in document and the crash is fixed.

BUG=581038
TEST=fast/text/first-letter-bad-line-boxes-crash.html
==========

to

==========
Use RelocatablePosition in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when the VisiblePositions are used later. It causes
the failure of layout test:

fast/text/first-letter-bad-line-boxes-crash.html

This patch uses RelocatablePosition for the tracking purpose as
a bug fix.

BUG=581038
TEST=fast/text/first-letter-bad-line-boxes-crash.html
==========

[Xiaocheng, 2016-07-06 08:45:22]

PTAL.

I don't think this CL requires any new test case, as its purpose is for fixing
the failure on an existing layout test.

[yosin_UTC9, 2016-07-06 10:01:03]

lgtm

Since, this patch fixes regression. Excluded test covers this case.
Go ahead!

[Xiaocheng, 2016-07-06 10:02:22]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-07-06 10:02:23]

Thanks!

[commit-bot: I haz the power, 2016-07-06 10:02:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-06 10:46:00]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-06 10:47:50]

Description was changed from

==========
Use RelocatablePosition in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when the VisiblePositions are used later. It causes
the failure of layout test:

fast/text/first-letter-bad-line-boxes-crash.html

This patch uses RelocatablePosition for the tracking purpose as
a bug fix.

BUG=581038
TEST=fast/text/first-letter-bad-line-boxes-crash.html
==========

to

==========
Use RelocatablePosition in CompositeEditCommand::moveParagraphs()

Currently, CompositeEditCommand::moveParagraphs() uses a pair of
VisiblePositions to keep track of original position of the moved
paragraph after it's moved away. However, the VisiblePositions may
be invalidated if their anchor nodes are moved out of document,
causing crashes when the VisiblePositions are used later. It causes
the failure of layout test:

fast/text/first-letter-bad-line-boxes-crash.html

This patch uses RelocatablePosition for the tracking purpose as
a bug fix.

BUG=581038
TEST=fast/text/first-letter-bad-line-boxes-crash.html

Committed: https://crrev.com/14bd2353e018993e07996f570cd58c2b3520d40f
Cr-Commit-Position: refs/heads/master@{#403879}
==========

[commit-bot: I haz the power, 2016-07-06 10:47:51]

Patchset 6 (id:??) landed as
https://crrev.com/14bd2353e018993e07996f570cd58c2b3520d40f
Cr-Commit-Position: refs/heads/master@{#403879}