Use FrameSelection::selectedText where possible.

Use FrameSelection::selectedText where possible.

Substitute occurrences of selection -> range -> (text or html) conversions with
direct selection -> (text or html) conversions using existing functions.

BUG=

Committed: https://crrev.com/9c2436064ebdcb9192dfacac96944f2753a2ce78
Cr-Commit-Position: refs/heads/master@{#355247}

[kotenkov, 2015-09-30 12:48:21]

kotenkov@yandex-team.ru changed reviewers:
+ tkent@chromium.org

[kotenkov, 2015-09-30 12:48:22]

I noticed that there are some copy-pasted conversions and used existing
functions in FrameSelection to remove them. It required some minor plumbing in
VisibleSelection. Would you please take a look?

[tkent, 2015-10-01 01:59:15]

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

[tkent, 2015-10-01 01:59:26]

Delegate to yosin@.

[yosin_UTC9, 2015-10-01 06:29:09]

I love this approach. It makes our code base simpler.
Please address |normalizeRange()| nits and see
this patch doesn't break layout tests.

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

https://codereview.chromium.org/1377963004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/VisibleSelection.cpp:273: return
range.isNotNull() ? normalizeRange(range) : range;
Why do you check |range.isNotNull()|?
If |isRange()| is true, |range| can't be null.

https://codereview.chromium.org/1377963004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/VisibleSelection.cpp:1176: const
EphemeralRangeTemplate<Strategy> range(start(), end());
Do you have a case we call |toNormalizedEphemeralRange()| with empty selection?

If we have a such case, let's make |normalizeRange()| to handle null
case rather than each call site.

[kotenkov, 2015-10-02 08:27:46]

On 2015/10/01 06:29:09, Yosi_UTC9 wrote:
> I love this approach. It makes our code base simpler.
> Please address |normalizeRange()| nits and see
> this patch doesn't break layout tests.
> 
>
https://codereview.chromium.org/1377963004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/editing/VisibleSelection.cpp (right):
> 
>
https://codereview.chromium.org/1377963004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/VisibleSelection.cpp:273: return
> range.isNotNull() ? normalizeRange(range) : range;
> Why do you check |range.isNotNull()|?
> If |isRange()| is true, |range| can't be null.
> 
>
https://codereview.chromium.org/1377963004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/VisibleSelection.cpp:1176: const
> EphemeralRangeTemplate<Strategy> range(start(), end());
> Do you have a case we call |toNormalizedEphemeralRange()| with empty
selection?
> 
> If we have a such case, let's make |normalizeRange()| to handle null
> case rather than each call site.

Thanks for your comments. I moved the checks to FrameSelection.cpp, which is
logically a much better place for them.

About layout tests: there are two broken by this change and I need some advice
about what to do with them. It seems to me that changes in my patchset don't
introduce the actual logic failures, but reveals ones that were introduced
during SelectionForComposedTree transition.

The first test is editing/undo/undo-delete-boundary.html. The test deletes a
word, emulates undo command and checks whether there is a proper selection on
the page after that. The proper selection for Mac is the deleted word, the
proper selection for other systems is empty. Without my patch the test passes,
but if we open it manually, we can see the following picture:
https://yadi.sk/i/KaFjIAstjTko3. As you can see, only the 'wor' is actually
selected, but the test passes because DOMSelection uses simple EphemeralRange to
represent selection. In older versions that test would look like this
https://yadi.sk/i/I4Wy4WUCjTkfR . So when my changes are introduced this test
breaks, because DOMSelection begins to use the same representation of the
selection that is used in other places.
 
The second test is editing/text-iterator/selection-to-string-with-auto-fill.html
and the problem has the same origin. It tests that selection doesn't include the
autofill value, but the reason the test breaks has nothing to do with autofill.
Here I opened the test html and selected textarea and text surrounding it:
https://yadi.sk/i/7KNDB97fjTmGJ . In earlier versions when NodeTraversal was
used, plainText for the selected range would return 'bazquux', but now we use
ComposedTreeTraversal, the traversal sees the DIV node inside the textarea and
emits the '\n' symbol, therefore the plainText for the range would return
'baz\nquux'. The test results in two failures with my patch:
FAIL select input outer assert_equals: expected "foobar" but got "foo\nbar"
FAIL select textarea outer assert_equals: expected "bazquux" but got "baz\nquux"
This problem exists without my patch -- it is a result of
SelectionForComposedTree feature, my patch just makes it evident because the
test now shows the same result as the real copy-paste of selection in browser
would show.

I'm not sure what I should do with it, do you have any advice?

[kotenkov, 2015-10-02 08:28:07]

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

https://codereview.chromium.org/1377963004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/VisibleSelection.cpp:273: return
range.isNotNull() ? normalizeRange(range) : range;
On 2015/10/01 06:29:09, Yosi_UTC9 wrote:
> Why do you check |range.isNotNull()|?
> If |isRange()| is true, |range| can't be null.

Yes, you are right. Now I understand that I should check for |isRange()| in
|extractSelectedTextAlgorithm()| and |extractSelectedHTMLAlgorithm()|. As I did
not check it in the first patchset, I had to check for |isNotNull()| here. That
was clearly a mistake.

https://codereview.chromium.org/1377963004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/VisibleSelection.cpp:1176: const
EphemeralRangeTemplate<Strategy> range(start(), end());
On 2015/10/01 06:29:09, Yosi_UTC9 wrote:
> Do you have a case we call |toNormalizedEphemeralRange()| with empty
selection?
> 
> If we have a such case, let's make |normalizeRange()| to handle null
> case rather than each call site.

No, it seems, that we should only call |toNormalizedEphemeralRange()| if
|isRange()| is true and in that case the range cannot be null.

[yosin_UTC9, 2015-10-06 05:41:51]

Sorry for late response.

tl;dr: We need to keep current behavior of 
Selection.toString(). So, we can't use FrameSeleciton::extractSelectedText().

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

https://codereview.chromium.org/1377963004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:516: return
m_frame->selection().selectedText(TextIteratorForSelectionToString);
Since we can't change behavior Selection.toString() API at this time,
we should use DOM version of plainText().

Thus, we can't use |FrameSeleciton::selectedText()| here.
Once, we use DOM version, layout test failure will be gone.

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

https://codereview.chromium.org/1377963004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1104: if
(!visibleSelection.isRange())
I think it is easier to make |normalizeRange()| to handle null range case
rather than each call site.

[kotenkov, 2015-10-06 12:29:23]

Thanks for the clarification!
I removed changes in DOMSelection::toString(), but now I'm getting failures in
ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField:
../../third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4420: Failure
Value of: selectionAsString(frame)
  Actual: ""
Expected: "Length"

The problem is that SelectionEditor::m_selection and
SelectionEditor::m_selectionInComposedTree have different selection types while
containing the same range. The first one is RangeSelection and the second is
CaretSelection. 
This happens because the following condition in |computeSelectionType()| is
triggered when we use EditingInComposedTreeStrategy:
    if (mostBackwardCaretPosition(start) == mostBackwardCaretPosition(end))
        return CaretSelection;

Again, I'm not sure what should I do with this failure.

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

https://codereview.chromium.org/1377963004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1104: if
(!visibleSelection.isRange())
On 2015/10/06 05:41:51, Yosi_UTC9 wrote:
> I think it is easier to make |normalizeRange()| to handle null range case
> rather than each call site.

Actually, now the problem is not in |normalizeRange()| but in |createMarkup()|
-- it is not supposed to handle null position and I'm not sure whether we should
change this behavior. The check here is just for symmetry with
|extractSelectedHTMLAlgorithm()|, |plainText()| can handle empty ranges.

[kotenkov, 2015-10-15 06:37:00]

On 2015/10/06 12:29:23, kotenkov wrote:
> Thanks for the clarification!
> I removed changes in DOMSelection::toString(), but now I'm getting failures in
> ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField:
> ../../third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4420: Failure
> Value of: selectionAsString(frame)
>   Actual: ""
> Expected: "Length"
> 
> The problem is that SelectionEditor::m_selection and
> SelectionEditor::m_selectionInComposedTree have different selection types
while
> containing the same range. The first one is RangeSelection and the second is
> CaretSelection. 
> This happens because the following condition in |computeSelectionType()| is
> triggered when we use EditingInComposedTreeStrategy:
>     if (mostBackwardCaretPosition(start) == mostBackwardCaretPosition(end))
>         return CaretSelection;
> 
> Again, I'm not sure what should I do with this failure.
> 
>
https://codereview.chromium.org/1377963004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):
> 
>
https://codereview.chromium.org/1377963004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/FrameSelection.cpp:1104: if
> (!visibleSelection.isRange())
> On 2015/10/06 05:41:51, Yosi_UTC9 wrote:
> > I think it is easier to make |normalizeRange()| to handle null range case
> > rather than each call site.
> 
> Actually, now the problem is not in |normalizeRange()| but in |createMarkup()|
> -- it is not supposed to handle null position and I'm not sure whether we
should
> change this behavior. The check here is just for symmetry with
> |extractSelectedHTMLAlgorithm()|, |plainText()| can handle empty ranges.

Ping! Should I do anything about this CL, or should I just abandon all hope of
merging it? =)

[yosin_UTC9, 2015-10-15 07:45:46]

Sorry for late response.

Please run trybots if you make changes.
If you can run trybots due by permission, let me know. In that case, I run
trybots for you.

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

https://codereview.chromium.org/1377963004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1104: if
(!visibleSelection.isRange())
On 2015/10/06 at 12:29:23, kotenkov wrote:
> On 2015/10/06 05:41:51, Yosi_UTC9 wrote:
> > I think it is easier to make |normalizeRange()| to handle null range case
> > rather than each call site.
> 
> Actually, now the problem is not in |normalizeRange()| but in |createMarkup()|
-- it is not supposed to handle null position and I'm not sure whether we should
change this behavior. The check here is just for symmetry with
|extractSelectedHTMLAlgorithm()|, |plainText()| can handle empty ranges.

Let's make both |normalizeRange()| and |createMarkup()| to handle null
range case to avoid null check on each call site.

[kotenkov, 2015-10-15 17:36:25]

Now |createMarkup()| can handle null positions and I also deleted 2 redundant
check.
Still the problem with test described here:
https://codereview.chromium.org/1377963004/#msg9 remains.
What should I do with it?
I don't have try access, so would you please run trybots for me?

[yosin_UTC9, 2015-10-16 01:17:06]

Much simpler!
Running trybots and see result.
Stay tune! (^_^)b

[yosin_UTC9, 2015-10-16 08:23:04]

On 2015/10/16 at 01:17:06, Yosi_UTC9 wrote:
> Much simpler!
> Running trybots and see result.
> Stay tune! (^_^)b

Two tests are failed:
- All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/1
- All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/0

You can run these tests by
ninja -C out/Debug blink_tests
out/Debug/webkit_unit_tests
--gtest_filter=All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/*

[kotenkov, 2015-10-16 11:37:58]

On 2015/10/16 08:23:04, Yosi_UTC9 wrote:
> On 2015/10/16 at 01:17:06, Yosi_UTC9 wrote:
> > Much simpler!
> > Running trybots and see result.
> > Stay tune! (^_^)b
> 
> Two tests are failed:
> - All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/1
> - All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/0
> 
> You can run these tests by
> ninja -C out/Debug blink_tests
> out/Debug/webkit_unit_tests
>
--gtest_filter=All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/*

Let me quote here what I said in message #9:

I'm getting failures in
ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField:
../../third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4420: Failure
Value of: selectionAsString(frame)
  Actual: ""
Expected: "Length"

The problem is that SelectionEditor::m_selection and
SelectionEditor::m_selectionInComposedTree have different selection types while
containing the same range. The first one is RangeSelection and the second is
CaretSelection. 
This happens because the following condition in |computeSelectionType()| is
triggered when we use EditingInComposedTreeStrategy:
    if (mostBackwardCaretPosition(start) == mostBackwardCaretPosition(end))
        return CaretSelection;

Again, I'm not sure what should I do with this failure.

[yosin_UTC9, 2015-10-16 14:36:13]

On 2015/10/16 at 11:37:58, kotenkov wrote:
> On 2015/10/16 08:23:04, Yosi_UTC9 wrote:
> > On 2015/10/16 at 01:17:06, Yosi_UTC9 wrote:
> > > Much simpler!
> > > Running trybots and see result.
> > > Stay tune! (^_^)b
> > 
> > Two tests are failed:
> > - All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/1
> > - All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/0
> > 
> > You can run these tests by
> > ninja -C out/Debug blink_tests
> > out/Debug/webkit_unit_tests
> >
--gtest_filter=All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/*
> 
> Let me quote here what I said in message #9:
> 
> I'm getting failures in
> ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField:
> ../../third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4420: Failure
> Value of: selectionAsString(frame)
>   Actual: ""
> Expected: "Length"
> 
> The problem is that SelectionEditor::m_selection and
> SelectionEditor::m_selectionInComposedTree have different selection types
while
> containing the same range. The first one is RangeSelection and the second is
> CaretSelection. 
> This happens because the following condition in |computeSelectionType()| is
> triggered when we use EditingInComposedTreeStrategy:
>     if (mostBackwardCaretPosition(start) == mostBackwardCaretPosition(end))
>         return CaretSelection;
> 
> Again, I'm not sure what should I do with this failure.

Sorry for confusion. I understand the situation.
In this case, both selection in DOM tree and selection in composed tree should
be same,
since selected range doesn't cross tree boundaries; selection is text node in
inner editor of INPUT element, e.g.

INPUT
 Shadow
  DIV ID="inner-editor"
    "Lengthy text goes here."

input.setSelectionRange(0, 6) + Selection.addRange() calls
FrameSelection::setSelection()
fro selection in DOM then selection in composed tree is computed from selection
in DOM by
SelectionEditor::adjustVisibleSelectionInComposedTree().

I tried Ctrl+C for test case and see "Length" is in clipboard. So, in this case,
selection in composed tree points "Length", since Ctrl+C calls
FrameSelection::selectedText().

What value does selection in composed tree have, base, extent, start, end?
(I'm in home and I can't access my dev env)

This issue is existing issue rather than this patch introduces. This patch
reveals
this issue.

I'll investigate what's going on here Monday.

[yosin_UTC9, 2015-10-16 14:48:56]

On 2015/10/16 at 14:36:13, Yosi_UTC9 wrote:
> On 2015/10/16 at 11:37:58, kotenkov wrote:
> > On 2015/10/16 08:23:04, Yosi_UTC9 wrote:
> > > On 2015/10/16 at 01:17:06, Yosi_UTC9 wrote:
> > > > Much simpler!
> > > > Running trybots and see result.
> > > > Stay tune! (^_^)b
> > > 
> > > Two tests are failed:
> > > - All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/1
> > > - All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/0
> > > 
> > > You can run these tests by
> > > ninja -C out/Debug blink_tests
> > > out/Debug/webkit_unit_tests
> > >
--gtest_filter=All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/*
> > 
> > Let me quote here what I said in message #9:
> > 
> > I'm getting failures in
> > ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField:
> > ../../third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4420: Failure
> > Value of: selectionAsString(frame)
> >   Actual: ""
> > Expected: "Length"
> > 
> > The problem is that SelectionEditor::m_selection and
> > SelectionEditor::m_selectionInComposedTree have different selection types
while
> > containing the same range. The first one is RangeSelection and the second is
> > CaretSelection. 
> > This happens because the following condition in |computeSelectionType()| is
> > triggered when we use EditingInComposedTreeStrategy:
> >     if (mostBackwardCaretPosition(start) == mostBackwardCaretPosition(end))
> >         return CaretSelection;
> > 
> > Again, I'm not sure what should I do with this failure.
> 
> Sorry for confusion. I understand the situation.
> In this case, both selection in DOM tree and selection in composed tree should
be same,
> since selected range doesn't cross tree boundaries; selection is text node in
> inner editor of INPUT element, e.g.
> 
> INPUT
>  Shadow
>   DIV ID="inner-editor"
>     "Lengthy text goes here."
> 
> input.setSelectionRange(0, 6) + Selection.addRange() calls
FrameSelection::setSelection()
> fro selection in DOM then selection in composed tree is computed from
selection in DOM by
> SelectionEditor::adjustVisibleSelectionInComposedTree().
> 
> I tried Ctrl+C for test case and see "Length" is in clipboard. So, in this
case,
> selection in composed tree points "Length", since Ctrl+C calls
FrameSelection::selectedText().
> 
> What value does selection in composed tree have, base, extent, start, end?
> (I'm in home and I can't access my dev env)
> 
> This issue is existing issue rather than this patch introduces. This patch
reveals
> this issue.
> 
> I'll investigate what's going on here Monday.

Note: In move_range_selection_extent_input_field.html
  var range = input.setSelectionRange(0, 6);
  window.getSelection().addRange(range);

|range| is undefined, since input.setSelectionRange() doesn't return |Range|
object.
|getSelection().addRange(range)| throws an exception.

[yosin_UTC9, 2015-10-16 15:27:28]

On 2015/10/16 at 14:48:56, Yosi_UTC9 wrote:
> On 2015/10/16 at 14:36:13, Yosi_UTC9 wrote:
> > On 2015/10/16 at 11:37:58, kotenkov wrote:
> > > On 2015/10/16 08:23:04, Yosi_UTC9 wrote:
> > > > On 2015/10/16 at 01:17:06, Yosi_UTC9 wrote:
> > > > > Much simpler!
> > > > > Running trybots and see result.
> > > > > Stay tune! (^_^)b
> > > > 
> > > > Two tests are failed:
> > > > -
All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/1
> > > > -
All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/0
> > > > 
> > > > You can run these tests by
> > > > ninja -C out/Debug blink_tests
> > > > out/Debug/webkit_unit_tests
> > > >
--gtest_filter=All/ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField/*
> > > 
> > > Let me quote here what I said in message #9:
> > > 
> > > I'm getting failures in
> > > ParameterizedWebFrameTest.MoveRangeSelectionExtentScollsInputField:
> > > ../../third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4420: Failure
> > > Value of: selectionAsString(frame)
> > >   Actual: ""
> > > Expected: "Length"
> > > 
> > > The problem is that SelectionEditor::m_selection and
> > > SelectionEditor::m_selectionInComposedTree have different selection types
while
> > > containing the same range. The first one is RangeSelection and the second
is
> > > CaretSelection. 
> > > This happens because the following condition in |computeSelectionType()|
is
> > > triggered when we use EditingInComposedTreeStrategy:
> > >     if (mostBackwardCaretPosition(start) ==
mostBackwardCaretPosition(end))
> > >         return CaretSelection;
> > > 
> > > Again, I'm not sure what should I do with this failure.
> > 
> > Sorry for confusion. I understand the situation.
> > In this case, both selection in DOM tree and selection in composed tree
should be same,
> > since selected range doesn't cross tree boundaries; selection is text node
in
> > inner editor of INPUT element, e.g.
> > 
> > INPUT
> >  Shadow
> >   DIV ID="inner-editor"
> >     "Lengthy text goes here."
> > 
> > input.setSelectionRange(0, 6) + Selection.addRange() calls
FrameSelection::setSelection()
> > fro selection in DOM then selection in composed tree is computed from
selection in DOM by
> > SelectionEditor::adjustVisibleSelectionInComposedTree().
> > 
> > I tried Ctrl+C for test case and see "Length" is in clipboard. So, in this
case,
> > selection in composed tree points "Length", since Ctrl+C calls
FrameSelection::selectedText().
> > 
> > What value does selection in composed tree have, base, extent, start, end?
> > (I'm in home and I can't access my dev env)
> > 
> > This issue is existing issue rather than this patch introduces. This patch
reveals
> > this issue.
> > 
> > I'll investigate what's going on here Monday.
> 
> Note: In move_range_selection_extent_input_field.html
>   var range = input.setSelectionRange(0, 6);
>   window.getSelection().addRange(range);
> 
> |range| is undefined, since input.setSelectionRange() doesn't return |Range|
object.
> |getSelection().addRange(range)| throws an exception.

According layout test failure in patch set #2,
"selection-after-split-text-node.html",
toString() is FrameSelection::selectedText(), we fail to compute selection type
due by
ostBackwardCaretPosition(pos), which checks layout object associated to
position, since
|m_start| and |m_end| don't have associated layout object in these failed test
case.

[kotenkov, 2015-10-19 06:20:43]

Sorry for the delay.

> Note: In move_range_selection_extent_input_field.html
>   var range = input.setSelectionRange(0, 6);
>   window.getSelection().addRange(range);

> |range| is undefined, since input.setSelectionRange() doesn't return |Range|
> object.
> |getSelection().addRange(range)| throws an exception.

Yes, I see that too, but it seems that it doesn't affect the test in any way.

> What value does selection in composed tree have, base, extent, start, end?
> (I'm in home and I can't access my dev env)
|m_start|:
(const
blink::PositionTemplate<blink::EditingAlgorithm<blink::ComposedTreeTraversal> >)
$129 = {
  m_anchorNode = {
    m_ptr = 0x000028c8e0a24080
  }
  m_offset = 0
  m_anchorType = OffsetInAnchor
}

|m_end|:
(const
blink::PositionTemplate<blink::EditingAlgorithm<blink::ComposedTreeTraversal> >)
$130 = {
  m_anchorNode = {
    m_ptr = 0x000028c8e0a24080
  }
  m_offset = 6
  m_anchorType = OffsetInAnchor
}

Base and extent are the same as the start and end. It seems to me that
everything is okay with them and the problem is in |mostBackwardCaretPosition()|
while computing selectionType.

> According layout test failure in patch set #2,
> "selection-after-split-text-node.html",
> toString() is FrameSelection::selectedText(), we fail to compute selection
type
> due by
> ostBackwardCaretPosition(pos), which checks layout object associated to
> position, since
> |m_start| and |m_end| don't have associated layout object in these failed test
> case.

  |mostBackwardCaretPosition(m_start)| and |mostBackwardCaretPosition(m_end)|
both give the same position in composed tree selection case -- the position
before |m_start|. But there is an associated layout object with both of them.
It seems that the function fails to determine visually distinct positions. 
I'll investigate further.

[kotenkov, 2015-10-19 06:34:56]

On 2015/10/19 06:20:43, kotenkov wrote:
> Sorry for the delay.
> 
> > Note: In move_range_selection_extent_input_field.html
> >   var range = input.setSelectionRange(0, 6);
> >   window.getSelection().addRange(range);
> 
> > |range| is undefined, since input.setSelectionRange() doesn't return |Range|
> > object.
> > |getSelection().addRange(range)| throws an exception.
> 
> Yes, I see that too, but it seems that it doesn't affect the test in any way.
> 
> > What value does selection in composed tree have, base, extent, start, end?
> > (I'm in home and I can't access my dev env)
> |m_start|:
> (const
> blink::PositionTemplate<blink::EditingAlgorithm<blink::ComposedTreeTraversal>
>)
> $129 = {
>   m_anchorNode = {
>     m_ptr = 0x000028c8e0a24080
>   }
>   m_offset = 0
>   m_anchorType = OffsetInAnchor
> }
> 
> |m_end|:
> (const
> blink::PositionTemplate<blink::EditingAlgorithm<blink::ComposedTreeTraversal>
>)
> $130 = {
>   m_anchorNode = {
>     m_ptr = 0x000028c8e0a24080
>   }
>   m_offset = 6
>   m_anchorType = OffsetInAnchor
> }
> 
> Base and extent are the same as the start and end. It seems to me that
> everything is okay with them and the problem is in
|mostBackwardCaretPosition()|
> while computing selectionType.
> 
> > According layout test failure in patch set #2,
> > "selection-after-split-text-node.html",
> > toString() is FrameSelection::selectedText(), we fail to compute selection
> type
> > due by
> > ostBackwardCaretPosition(pos), which checks layout object associated to
> > position, since
> > |m_start| and |m_end| don't have associated layout object in these failed
test
> > case.
> 
>   |mostBackwardCaretPosition(m_start)| and |mostBackwardCaretPosition(m_end)|
> both give the same position in composed tree selection case -- the position
> before |m_start|. But there is an associated layout object with both of them.
> It seems that the function fails to determine visually distinct positions. 
> I'll investigate further.

Now I see that |endsOfNodeAreVisuallyDistinctPositions(Node* node)| returns
false for my Text node, which seems incorrect to me.

[yosin_UTC9, 2015-10-19 08:29:50]

On 2015/10/19 at 06:34:56, kotenkov wrote:
> On 2015/10/19 06:20:43, kotenkov wrote:
> > Sorry for the delay.
> > 
> > > Note: In move_range_selection_extent_input_field.html
> > >   var range = input.setSelectionRange(0, 6);
> > >   window.getSelection().addRange(range);
> > 
> > > |range| is undefined, since input.setSelectionRange() doesn't return
|Range|
> > > object.
> > > |getSelection().addRange(range)| throws an exception.
> > 
> > Yes, I see that too, but it seems that it doesn't affect the test in any
way.
> > 
> > > What value does selection in composed tree have, base, extent, start, end?
> > > (I'm in home and I can't access my dev env)
> > |m_start|:
> > (const
> >
blink::PositionTemplate<blink::EditingAlgorithm<blink::ComposedTreeTraversal> >)
> > $129 = {
> >   m_anchorNode = {
> >     m_ptr = 0x000028c8e0a24080
> >   }
> >   m_offset = 0
> >   m_anchorType = OffsetInAnchor
> > }
> > 
> > |m_end|:
> > (const
> >
blink::PositionTemplate<blink::EditingAlgorithm<blink::ComposedTreeTraversal> >)
> > $130 = {
> >   m_anchorNode = {
> >     m_ptr = 0x000028c8e0a24080
> >   }
> >   m_offset = 6
> >   m_anchorType = OffsetInAnchor
> > }
> > 
> > Base and extent are the same as the start and end. It seems to me that
> > everything is okay with them and the problem is in
|mostBackwardCaretPosition()|
> > while computing selectionType.
> > 
> > > According layout test failure in patch set #2,
> > > "selection-after-split-text-node.html",
> > > toString() is FrameSelection::selectedText(), we fail to compute selection
> > type
> > > due by
> > > ostBackwardCaretPosition(pos), which checks layout object associated to
> > > position, since
> > > |m_start| and |m_end| don't have associated layout object in these failed
test
> > > case.
> > 
> >   |mostBackwardCaretPosition(m_start)| and
|mostBackwardCaretPosition(m_end)|
> > both give the same position in composed tree selection case -- the position
> > before |m_start|. But there is an associated layout object with both of
them.
> > It seems that the function fails to determine visually distinct positions. 
> > I'll investigate further.
> 
> Now I see that |endsOfNodeAreVisuallyDistinctPositions(Node* node)| returns
false for my Text node, which seems incorrect to me.

The text node in inner editor of INPUT element has layout object but it doesn't
have InlineTextBox, textNode->layoutObject()->m_firstTextBox/m_lastTextBox are
nullptr,
because of input.setSelectionRange() is called DocumentLifeCycle::StyleClean,
node has LayoutObject for style but not have InlineBox objects which is attached
by layout
phase.

There are two options:
1. Call document().updateLayoutIgnorePendingStylesheets() in
HTMLTextFormControlElement::setSelectionRange() to avoid using
VisibleSelection::setWithoutValidation().
2. Compute selection type on demand with cache.

#1 is ease and preferred way, since we would like to deprecate
VisibleSelection::setWithoutValidation(), since it is dangerous.

At first glance, #2 is desired way, if we also defer start/end calculation too
== need more code changes.

Note: unit test like below is passed since we update layout tree:
TEST_F(FrameSelectionTest, InputSetSelectionRange)
{
    const char* bodyContent = "<input value=123456>";
    setBodyContent(bodyContent);
    updateLayoutAndStyleForPainting();

    HTMLInputElement* input =
toHTMLInputElement(document().querySelector("input",
ASSERT_NO_EXCEPTION).get());
    input->setSelectionRange(2, 4);
    Node* valueNode = input->innerEditorElement()->firstChild();

    EXPECT_EQ(Position(valueNode, 2), visibleSelectionInDOMTree().start());
    EXPECT_EQ(Position(valueNode, 4), visibleSelectionInDOMTree().end());
    EXPECT_EQ(RangeSelection, visibleSelectionInDOMTree().selectionType());

    EXPECT_EQ(PositionInComposedTree(valueNode, 2),
visibleSelectionInComposedTree().start());
    EXPECT_EQ(PositionInComposedTree(valueNode, 4),
visibleSelectionInComposedTree().end());
    EXPECT_EQ(RangeSelection, visibleSelectionInComposedTree().selectionType());
}

[kotenkov, 2015-10-19 14:24:43]

On 2015/10/19 08:29:50, Yosi_UTC9 wrote:
> On 2015/10/19 at 06:34:56, kotenkov wrote:
> > On 2015/10/19 06:20:43, kotenkov wrote:
> > > Sorry for the delay.
> > > 
> > > > Note: In move_range_selection_extent_input_field.html
> > > >   var range = input.setSelectionRange(0, 6);
> > > >   window.getSelection().addRange(range);
> > > 
> > > > |range| is undefined, since input.setSelectionRange() doesn't return
> |Range|
> > > > object.
> > > > |getSelection().addRange(range)| throws an exception.
> > > 
> > > Yes, I see that too, but it seems that it doesn't affect the test in any
> way.
> > > 
> > > > What value does selection in composed tree have, base, extent, start,
end?
> > > > (I'm in home and I can't access my dev env)
> > > |m_start|:
> > > (const
> > >
> blink::PositionTemplate<blink::EditingAlgorithm<blink::ComposedTreeTraversal>
>)
> > > $129 = {
> > >   m_anchorNode = {
> > >     m_ptr = 0x000028c8e0a24080
> > >   }
> > >   m_offset = 0
> > >   m_anchorType = OffsetInAnchor
> > > }
> > > 
> > > |m_end|:
> > > (const
> > >
> blink::PositionTemplate<blink::EditingAlgorithm<blink::ComposedTreeTraversal>
>)
> > > $130 = {
> > >   m_anchorNode = {
> > >     m_ptr = 0x000028c8e0a24080
> > >   }
> > >   m_offset = 6
> > >   m_anchorType = OffsetInAnchor
> > > }
> > > 
> > > Base and extent are the same as the start and end. It seems to me that
> > > everything is okay with them and the problem is in
> |mostBackwardCaretPosition()|
> > > while computing selectionType.
> > > 
> > > > According layout test failure in patch set #2,
> > > > "selection-after-split-text-node.html",
> > > > toString() is FrameSelection::selectedText(), we fail to compute
selection
> > > type
> > > > due by
> > > > ostBackwardCaretPosition(pos), which checks layout object associated to
> > > > position, since
> > > > |m_start| and |m_end| don't have associated layout object in these
failed
> test
> > > > case.
> > > 
> > >   |mostBackwardCaretPosition(m_start)| and
> |mostBackwardCaretPosition(m_end)|
> > > both give the same position in composed tree selection case -- the
position
> > > before |m_start|. But there is an associated layout object with both of
> them.
> > > It seems that the function fails to determine visually distinct positions.

> > > I'll investigate further.
> > 
> > Now I see that |endsOfNodeAreVisuallyDistinctPositions(Node* node)| returns
> false for my Text node, which seems incorrect to me.
> 
> The text node in inner editor of INPUT element has layout object but it
doesn't
> have InlineTextBox, textNode->layoutObject()->m_firstTextBox/m_lastTextBox are
> nullptr,
> because of input.setSelectionRange() is called DocumentLifeCycle::StyleClean,
> node has LayoutObject for style but not have InlineBox objects which is
attached
> by layout
> phase.
> 
> There are two options:
> 1. Call document().updateLayoutIgnorePendingStylesheets() in
> HTMLTextFormControlElement::setSelectionRange() to avoid using
> VisibleSelection::setWithoutValidation().
> 2. Compute selection type on demand with cache.
> 
> #1 is ease and preferred way, since we would like to deprecate
> VisibleSelection::setWithoutValidation(), since it is dangerous.
> 
> At first glance, #2 is desired way, if we also defer start/end calculation too
> == need more code changes.
> 
> Note: unit test like below is passed since we update layout tree:
> TEST_F(FrameSelectionTest, InputSetSelectionRange)
> {
>     const char* bodyContent = "<input value=123456>";
>     setBodyContent(bodyContent);
>     updateLayoutAndStyleForPainting();
> 
>     HTMLInputElement* input =
> toHTMLInputElement(document().querySelector("input",
> ASSERT_NO_EXCEPTION).get());
>     input->setSelectionRange(2, 4);
>     Node* valueNode = input->innerEditorElement()->firstChild();
> 
>     EXPECT_EQ(Position(valueNode, 2), visibleSelectionInDOMTree().start());
>     EXPECT_EQ(Position(valueNode, 4), visibleSelectionInDOMTree().end());
>     EXPECT_EQ(RangeSelection, visibleSelectionInDOMTree().selectionType());
> 
>     EXPECT_EQ(PositionInComposedTree(valueNode, 2),
> visibleSelectionInComposedTree().start());
>     EXPECT_EQ(PositionInComposedTree(valueNode, 4),
> visibleSelectionInComposedTree().end());
>     EXPECT_EQ(RangeSelection,
visibleSelectionInComposedTree().selectionType());
> }

I made it the easy way -- by updating layout. All webkit_unit_tests passed
locally, would you please run try jobs for me to check it?

As for the hard way -- I can make it if you tell me how to do it in some more
detail.

[yosin_UTC9, 2015-10-20 04:05:12]

On 2015/10/19 at 14:24:43, kotenkov wrote:
> On 2015/10/19 08:29:50, Yosi_UTC9 wrote:
> > On 2015/10/19 at 06:34:56, kotenkov wrote:
> > > On 2015/10/19 06:20:43, kotenkov wrote:
> > > > Sorry for the delay.
> > > > 
> > > > > Note: In move_range_selection_extent_input_field.html
> > > > >   var range = input.setSelectionRange(0, 6);
> > > > >   window.getSelection().addRange(range);
> > > > 
> > > > > |range| is undefined, since input.setSelectionRange() doesn't return
> > |Range|
> > > > > object.
> > > > > |getSelection().addRange(range)| throws an exception.
> > > > 
> > > > Yes, I see that too, but it seems that it doesn't affect the test in any
> > way.
> > > > 
> > > > > What value does selection in composed tree have, base, extent, start,
end?
> > > > > (I'm in home and I can't access my dev env)
> > > > |m_start|:
> > > > (const
> > > >
> >
blink::PositionTemplate<blink::EditingAlgorithm<blink::ComposedTreeTraversal> >)
> > > > $129 = {
> > > >   m_anchorNode = {
> > > >     m_ptr = 0x000028c8e0a24080
> > > >   }
> > > >   m_offset = 0
> > > >   m_anchorType = OffsetInAnchor
> > > > }
> > > > 
> > > > |m_end|:
> > > > (const
> > > >
> >
blink::PositionTemplate<blink::EditingAlgorithm<blink::ComposedTreeTraversal> >)
> > > > $130 = {
> > > >   m_anchorNode = {
> > > >     m_ptr = 0x000028c8e0a24080
> > > >   }
> > > >   m_offset = 6
> > > >   m_anchorType = OffsetInAnchor
> > > > }
> > > > 
> > > > Base and extent are the same as the start and end. It seems to me that
> > > > everything is okay with them and the problem is in
> > |mostBackwardCaretPosition()|
> > > > while computing selectionType.
> > > > 
> > > > > According layout test failure in patch set #2,
> > > > > "selection-after-split-text-node.html",
> > > > > toString() is FrameSelection::selectedText(), we fail to compute
selection
> > > > type
> > > > > due by
> > > > > ostBackwardCaretPosition(pos), which checks layout object associated
to
> > > > > position, since
> > > > > |m_start| and |m_end| don't have associated layout object in these
failed
> > test
> > > > > case.
> > > > 
> > > >   |mostBackwardCaretPosition(m_start)| and
> > |mostBackwardCaretPosition(m_end)|
> > > > both give the same position in composed tree selection case -- the
position
> > > > before |m_start|. But there is an associated layout object with both of
> > them.
> > > > It seems that the function fails to determine visually distinct
positions. 
> > > > I'll investigate further.
> > > 
> > > Now I see that |endsOfNodeAreVisuallyDistinctPositions(Node* node)|
returns
> > false for my Text node, which seems incorrect to me.
> > 
> > The text node in inner editor of INPUT element has layout object but it
doesn't
> > have InlineTextBox, textNode->layoutObject()->m_firstTextBox/m_lastTextBox
are
> > nullptr,
> > because of input.setSelectionRange() is called
DocumentLifeCycle::StyleClean,
> > node has LayoutObject for style but not have InlineBox objects which is
attached
> > by layout
> > phase.
> > 
> > There are two options:
> > 1. Call document().updateLayoutIgnorePendingStylesheets() in
> > HTMLTextFormControlElement::setSelectionRange() to avoid using
> > VisibleSelection::setWithoutValidation().
> > 2. Compute selection type on demand with cache.
> > 
> > #1 is ease and preferred way, since we would like to deprecate
> > VisibleSelection::setWithoutValidation(), since it is dangerous.
> > 
> > At first glance, #2 is desired way, if we also defer start/end calculation
too
> > == need more code changes.
> > 
> > Note: unit test like below is passed since we update layout tree:
> > TEST_F(FrameSelectionTest, InputSetSelectionRange)
> > {
> >     const char* bodyContent = "<input value=123456>";
> >     setBodyContent(bodyContent);
> >     updateLayoutAndStyleForPainting();
> > 
> >     HTMLInputElement* input =
> > toHTMLInputElement(document().querySelector("input",
> > ASSERT_NO_EXCEPTION).get());
> >     input->setSelectionRange(2, 4);
> >     Node* valueNode = input->innerEditorElement()->firstChild();
> > 
> >     EXPECT_EQ(Position(valueNode, 2), visibleSelectionInDOMTree().start());
> >     EXPECT_EQ(Position(valueNode, 4), visibleSelectionInDOMTree().end());
> >     EXPECT_EQ(RangeSelection, visibleSelectionInDOMTree().selectionType());
> > 
> >     EXPECT_EQ(PositionInComposedTree(valueNode, 2),
> > visibleSelectionInComposedTree().start());
> >     EXPECT_EQ(PositionInComposedTree(valueNode, 4),
> > visibleSelectionInComposedTree().end());
> >     EXPECT_EQ(RangeSelection,
visibleSelectionInComposedTree().selectionType());
> > }
> 
> I made it the easy way -- by updating layout. All webkit_unit_tests passed
locally, would you please run try jobs for me to check it?
> 
> As for the hard way -- I can make it if you tell me how to do it in some more
detail.

Running trybots. I hope we pass them.

Here is rough idea of the hard way, lazy VisibleSelection:

Fact: VisbileSelection is defined by base and extent positions
Fact: getters in VisibleSelection returns values based on canonicalized
base/extent postiions
Fact: Canonicalization requires layout tree updated, e.g.
DocumentLifeCylce:LayoutClean

Thus, we can make getters to do canonicalization on demand and we can cache the
result.

Sketch:

VisibleSelection::VsibileSeleciton(const Position& base, const Position& extent)
  : m_originalBase(base), m_originalExtent(extent), m_canonicalized(false) {
}

Position VisibleSelection::base() const {
  canonicalizeIfNeeded();
  return m_base;
}

void VisibleSelection::canonicalizeIfNeeded() const {
  // TODO(yosin): We may want to check documentTreeVersion for re-validate();
  if (m_canonicalized)
    return;
  validate(); // No changes except for computing from m_originalBase and
m_orignalExtent
  m_canonicalized = true;
}

class VisibleSelection final {
  ...
 private:
  Position m_originalBase;
  Position m_originalExtent;
  mutable Position m_base, m_extent, m_start, m_end;
  mutable m_canonicalized;
}


Caveat: Lazy VisibleSeleciton changes behavior, some layout tests will fail.
But, I think these failed test are wrong and we should adjust them.

[yosin_UTC9, 2015-10-20 12:21:48]

lgtm

All bots are passed!

[yosin_UTC9, 2015-10-20 12:21:55]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-20 12:22:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377963004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377963004/80001

[commit-bot: I haz the power, 2015-10-20 15:16:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-20 15:16:40]

Dry run: This issue passed the CQ dry run.

[kotenkov, 2015-10-20 15:25:36]

On 2015/10/20 12:21:48, Yosi_UTC9 wrote:
> lgtm
> 
> All bots are passed!

So we need an approve from tkent now to get approve from all needed owners?

As for the lazy selection, I assume that we are going to implement it in another
patch?
If so, should I do it myself or will you take it from here? (I would be glad to
do it myself)

[tkent, 2015-10-20 23:11:32]

lgtm

https://codereview.chromium.org/1377963004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/1377963004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp:357:
document().updateLayoutIgnorePendingStylesheets();
nit: Add a comment why we need to update layout.

[yosin_UTC9, 2015-10-21 01:04:14]

On 2015/10/20 at 15:25:36, kotenkov wrote:
> On 2015/10/20 12:21:48, Yosi_UTC9 wrote:
> > lgtm
> > 
> > All bots are passed!
> 
> So we need an approve from tkent now to get approve from all needed owners?
> 
> As for the lazy selection, I assume that we are going to implement it in
another patch?
> If so, should I do it myself or will you take it from here? (I would be glad
to do it myself)
I filed lazy selection into http://crbug.com/545746
I don't have time to do this at this time as of my team priority.
If you're interested, please take it. (^_^)b

[kotenkov, 2015-10-21 05:53:45]

I have added comments. Checking the commit checkbox now.

[kotenkov, 2015-10-21 05:53:53]

The CQ bit was checked by kotenkov@yandex-team.ru

[kotenkov, 2015-10-21 05:53:53]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/1377963004/#ps100001
(title: "Add comments.")

[commit-bot: I haz the power, 2015-10-21 05:54:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377963004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377963004/100001

[commit-bot: I haz the power, 2015-10-21 07:01:03]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-21 07:01:49]

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

[tkent, 2015-10-22 22:45:44]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1417363002/ by tkent@chromium.org.

The reason for reverting is: Regressed speedometer significantly. 
crbug.com/546248
.

[yosin_UTC9, 2015-10-23 01:41:44]

On 2015/10/22 at 22:45:44, tkent wrote:
> A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1417363002/ by tkent@chromium.org.
> 
> The reason for reverting is: Regressed speedometer significantly. 
crbug.com/546248
> .

document().updateLayoutIgnorePendingStylesheets();
in HTMLTextFOrmControllerElement::setSelectionRange() causes speed regression.

Before going lazy selection, what about introducing:
FrameSelection::setSelectionInTextFormControl(HTMLTextFormControl& control, int
base, int extent, bool isDiractional, ...)
 redirect to SelectionEdtior

SelectionEditor::setSelectionInTextFormControl(...)
 Does setWithoutValidation() for both DOM and composed tree VisibleSelection.

This is optimization for selection on text control. When we associate
FrameSeleciton to text control, we know inner editor
shows everything we don't need to do canonicalization.

[tkent, 2015-10-23 01:59:26]

On 2015/10/22 at 22:45:44, tkent wrote:
> The reason for reverting is: Regressed speedometer significantly. 
crbug.com/546248
> .

blink_perf.dom textarea-dom (crbug.com/546250) too.