reset VisibleSelection at the Undo command.

reset VisibleSelection  at the Undo command.

Sice UndoManager doesn't manage DOM mutations outside of execCommand, the Undo command sometimes set broken VisibleSelection which is valid when it was recorded to the Undo steps but selection.deleteFromDocument broke that in this test case.

This CL validates VisibleSelection when it is used to restore Selection at Undo.
The main target VisibleSelection to be validate is that has any VisiblePosition which is not in the document already or which offset violates its nodes length because of "Unmagaed" DOM mutations.

BUG=369759
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175647

[yosin_UTC9, 2014-06-04 01:17:30]

https://codereview.chromium.org/315493004/diff/1/Source/core/editing/Editor.cpp
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/315493004/diff/1/Source/core/editing/Editor.c...
Source/core/editing/Editor.cpp:738: newSelection =
VisibleSelection(newSelection.base(), newSelection.extent(),
newSelection.affinity(), newSelection.isDirectional());
Q: Why do we create |VisibleSelection| from invalid positions?

[yoichio, 2014-06-04 02:01:46]

https://codereview.chromium.org/315493004/diff/1/Source/core/editing/Editor.cpp
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/315493004/diff/1/Source/core/editing/Editor.c...
Source/core/editing/Editor.cpp:738: newSelection =
VisibleSelection(newSelection.base(), newSelection.extent(),
newSelection.affinity(), newSelection.isDirectional());
On 2014/06/04 01:17:30, yosi wrote:
> Q: Why do we create |VisibleSelection| from invalid positions?
It's OK to create null VisibleSelection if there are invalid positions.
However, some of them have valid information like anchor Node and the
VisibleSelection constructor creates new valid VisibleSelection even from the
invalid Positions through the canonicalization.
I think it's better to recycle VisibleSelection from the information than to
reset VisibleSelection.

[yosin_UTC9, 2014-06-04 02:20:58]

https://codereview.chromium.org/315493004/diff/1/Source/core/editing/Editor.cpp
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/315493004/diff/1/Source/core/editing/Editor.c...
Source/core/editing/Editor.cpp:738: newSelection =
VisibleSelection(newSelection.base(), newSelection.extent(),
newSelection.affinity(), newSelection.isDirectional());
On 2014/06/04 02:01:46, yoichio wrote:
> On 2014/06/04 01:17:30, yosi wrote:
> > Q: Why do we create |VisibleSelection| from invalid positions?
> It's OK to create null VisibleSelection if there are invalid positions.
> However, some of them have valid information like anchor Node and the
> VisibleSelection constructor creates new valid VisibleSelection even from the
> invalid Positions through the canonicalization.
> I think it's better to recycle VisibleSelection from the information than to
> reset VisibleSelection.

I think it is better to reset rather than set unexpected value.
yutak@, How do you think?

[yoichio, 2014-06-05 03:02:15]

ping?

[Yuta Kitamura, 2014-06-05 08:37:55]

https://codereview.chromium.org/315493004/diff/1/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/315493004/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:551: unsigned Range::lengthOfContentsInNode(const
Node* node)
I don't think this function should be exposed as a member of
Range. It should probably belong to Node.

https://codereview.chromium.org/315493004/diff/1/Source/core/editing/Editor.cpp
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/315493004/diff/1/Source/core/editing/Editor.c...
Source/core/editing/Editor.cpp:738: newSelection =
VisibleSelection(newSelection.base(), newSelection.extent(),
newSelection.affinity(), newSelection.isDirectional());
On 2014/06/04 02:20:57, yosi wrote:
> On 2014/06/04 02:01:46, yoichio wrote:
> > On 2014/06/04 01:17:30, yosi wrote:
> > > Q: Why do we create |VisibleSelection| from invalid positions?
> > It's OK to create null VisibleSelection if there are invalid positions.
> > However, some of them have valid information like anchor Node and the
> > VisibleSelection constructor creates new valid VisibleSelection even from
the
> > invalid Positions through the canonicalization.
> > I think it's better to recycle VisibleSelection from the information than to
> > reset VisibleSelection.
> 
> I think it is better to reset rather than set unexpected value.
> yutak@, How do you think?

First of all, in my view this line is very hard to understand, since it seems to
create another VisibleSelection with the exact same set of arguments. It's
totally
not obvious that it resets out-of-index positions.

My take is that VisibleSelection should be responsible for making sure it is in
a valid state. In that light, it should be VisibleSelection's copy constructor
to check the validity of Positions, not Editor.

If changing VisibleSelection's copy constructor is not feasible (because it
slows
down the performance or breaks compatibility), then VisibleSelection should have
a member function that corrects the positions, and lines 736-738 should be
replaced with something like:

    VisibleSelection newSelection(
        cmd->startingSelection().ensureValidPositions());

[yoichio, 2014-06-06 04:49:22]

https://codereview.chromium.org/315493004/diff/1/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/315493004/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:551: unsigned Range::lengthOfContentsInNode(const
Node* node)
On 2014/06/05 08:37:55, Yuta Kitamura wrote:
> I don't think this function should be exposed as a member of
> Range. It should probably belong to Node.

Done.

https://codereview.chromium.org/315493004/diff/1/Source/core/editing/Editor.cpp
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/315493004/diff/1/Source/core/editing/Editor.c...
Source/core/editing/Editor.cpp:738: newSelection =
VisibleSelection(newSelection.base(), newSelection.extent(),
newSelection.affinity(), newSelection.isDirectional());
> If changing VisibleSelection's copy constructor is not feasible (because it
> slows
> down the performance or breaks compatibility), then VisibleSelection should
have
> a member function that corrects the positions, and lines 736-738 should be
> replaced with something like:
> 
>     VisibleSelection newSelection(
>         cmd->startingSelection().ensureValidPositions());
VisibleSelection is a DISALLOW_ALLOCATION class. It means that its instances are
always only on stack and you must copy an object if you need another variable of
that.
Thus to insert ::valid() to the copy constructor may make performance down.
Additionally, VisibleSelection::valid() change a VisibleSelection which has
'valid' Positions because of the  valid() behavior to the notorious base,
extend, start and end relationship.

As conclusion, I add VisibleSelection::validatePositionsIfNeeded to validate
each position if its position is invalid.

[yosin_UTC9, 2014-06-06 05:02:07]

Just FYI.

On 2014/06/06 04:49:22, yoichio wrote:
> VisibleSelection is a DISALLOW_ALLOCATION class. It means that its instances
are
> always only on stack and you must copy an object if you need another variable
of
> that.

DISALLOW_ALLOCATION() means the class can't be constructed with |new| operator.
It can be on heap as member variable. For this case, VisibleSelection is member
of FrameSeleciton.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Yuta Kitamura, 2014-06-06 08:53:14]

lgtm

[yoichio, 2014-06-06 08:54:12]

The CQ bit was checked by yoichio@chromium.org

[commit-bot: I haz the power, 2014-06-06 08:55:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoichio@chromium.org/315493004/20001

[commit-bot: I haz the power, 2014-06-06 08:59:29]

Change committed as 175647