Get rid of redundant member variable SelectionEditor::m_logicalRange

Make FrameSeelction::firstRange to return proper range

This patch makes |FrameSeelction::firstRange()| to return proper range instead
of the range specified by |FrameSelection::setSelectedRange()|.

Before this patch, |FrameSelection::firstRange()| returns the range we called
"logical range" introduced by patch[1], which mitigated some situation but it
is incomplete. Patch[2] introduced complete solution and compatible with the
specification[3].

This patch is follow-up of patch[2] to remove "logical range" from Blink since
cached |Range| supersedes "logical range".

[1] crrev.com/196553003: Let FrameSelection remember the last Range set by Range.addRange()
[2] crrev.com/2653523003: Make DOMSelection cache Range
[3] https://www.w3.org/TR/selection-api/

BUG=693557
TEST=webkit_unit_tests --gtest_filter=FrameSelectionTest.FirstRange

Review-Url: https://codereview.chromium.org/2723133004
Cr-Commit-Position: refs/heads/master@{#454831}
Committed: https://chromium.googlesource.com/chromium/src/+/67adef739b13e794850c228f3677dd75614958c1

[yosin_UTC9, 2017-03-02 09:02:05]

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

[commit-bot: I haz the power, 2017-03-02 09:02:33]

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

[yosin_UTC9, 2017-03-02 10:16:53]

Description was changed from

==========
2017-03-02T18:01:04

2017-03-02T18:00:41

BUG=
==========

to

==========
Get rid of redundant member variable SelectionEditor::m_logicalRange

This patch gets rid of redundant member |SelectionEditor::m_logicalRange|,
since |m_cacheRange| supersedes it, to reduce source code size for improving
code health.

BUG=n/a
TEST=n/a; no behavior changes
==========

[yosin_UTC9, 2017-03-02 10:18:40]

yosin@chromium.org changed reviewers:
+ tkent@chromium.org, xiaochengh@chromium.org, yoichio@chromium.org

[yosin_UTC9, 2017-03-02 10:18:41]

PTAL

[yosin_UTC9, 2017-03-02 10:22:29]

Description was changed from

==========
Get rid of redundant member variable SelectionEditor::m_logicalRange

This patch gets rid of redundant member |SelectionEditor::m_logicalRange|,
since |m_cacheRange| supersedes it, to reduce source code size for improving
code health.

BUG=n/a
TEST=n/a; no behavior changes
==========

to

==========
Get rid of redundant member variable SelectionEditor::m_logicalRange

This patch gets rid of redundant member |SelectionEditor::m_logicalRange|,
since |m_cacheRange| supersedes it, to reduce source code size for improving
code health.

BUG=693557
TEST=n/a; no behavior changes
==========

[commit-bot: I haz the power, 2017-03-02 10:35:52]

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

[commit-bot: I haz the power, 2017-03-02 10:35:53]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-03-02 20:34:24]

I still don't understand why this patch is "no behavior changes"...

Could you clarify why |m_cachedRange| supercedes |m_logicalRange|?

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

https://codereview.chromium.org/2723133004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:371: return;
This patch has conflict with https://codereview.chromium.org/2724333002.

Could you make it a dependant patch? (Or just be more careful when
committing...)

[yoichio, 2017-03-03 01:54:20]

https://codereview.chromium.org/2723133004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (left):

https://codereview.chromium.org/2723133004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:370: return
m_logicalRange->cloneRange();
Could you proof logically
  m_logicalRange == firstRangeOf(computeVisibleSelectionInDOMTree()) ?
Then, this code can be removed w/o any behavior change.

[yosin_UTC9, 2017-03-03 02:27:17]

On 2017/03/02 at 20:34:24, xiaochengh wrote:
> I still don't understand why this patch is "no behavior changes"...
> 
> Could you clarify why |m_cachedRange| supersedes |m_logicalRange|?
> 

The |m_logicalRange| is introduced by patch[1] to make Selection#getRangeAt() to
*cloned* range
which added by Selection#addRange(). This is kludge of hiding canonicalization
to web developer.

However, this is incomplete; Blink failed lots of WPT test.

Then patch[2] introduces |m_cachedRange| for complete solution of
Selection#getRangeAt() and
Seleciton#addRange().

[1] crrev.com/196553003: Let FrameSelection remember the last Range set by
Range.addRange()
[2] crrev.com/2653523003: Make DOMSelection cache Range

[Xiaocheng, 2017-03-03 03:06:09]

On 2017/03/03 at 02:27:17, yosin wrote:
> On 2017/03/02 at 20:34:24, xiaochengh wrote:
> > I still don't understand why this patch is "no behavior changes"...
> > 
> > Could you clarify why |m_cachedRange| supersedes |m_logicalRange|?
> > 
> 
> The |m_logicalRange| is introduced by patch[1] to make Selection#getRangeAt()
to *cloned* range
> which added by Selection#addRange(). This is kludge of hiding canonicalization
to web developer.
> 
> However, this is incomplete; Blink failed lots of WPT test.
> 
> Then patch[2] introduces |m_cachedRange| for complete solution of
Selection#getRangeAt() and
> Seleciton#addRange().
> 
> [1] crrev.com/196553003: Let FrameSelection remember the last Range set by
Range.addRange()
> [2] crrev.com/2653523003: Make DOMSelection cache Range

This patch has no behavior change only if either of the following is true:
1. m_logicalRange == firstRangeOf(computeVisibleSelectionInDOMTree()), as Yoichi
suggested
2. SelectionEditor::firstRange can never get called when m_logicalRange is
non-null

However, it doesn't seem to be the case. A counterexample:
0. We start with no selection, which means both m_logicalRange and m_cachedRange
are null
1. InputMethodController::setSelectionOffsets updates selection, which sets
m_logicalRange non-null but doesn't touch m_cachedRange
2. Some action triggers selection canonicalization, making m_logicalRange !=
firstRangeOf(computeVisibleSelectionInDOMTree())
3. DOMSelection::getRangeAt(0) is called, which is supposed to fetch
m_logicalRange because m_cachedRange is still null

Step 1 can also be any other call site of FrameSelection::setSelectedRange

[yosin_UTC9, 2017-03-03 04:27:31]

On 2017/03/03 at 03:06:09, xiaochengh wrote:
> On 2017/03/03 at 02:27:17, yosin wrote:
> > On 2017/03/02 at 20:34:24, xiaochengh wrote:
> > > I still don't understand why this patch is "no behavior changes"...
> > > 
> > > Could you clarify why |m_cachedRange| supersedes |m_logicalRange|?
> > > 
> > 
> > The |m_logicalRange| is introduced by patch[1] to make
Selection#getRangeAt() to *cloned* range
> > which added by Selection#addRange(). This is kludge of hiding
canonicalization to web developer.
> > 
> > However, this is incomplete; Blink failed lots of WPT test.
> > 
> > Then patch[2] introduces |m_cachedRange| for complete solution of
Selection#getRangeAt() and
> > Seleciton#addRange().
> > 
> > [1] crrev.com/196553003: Let FrameSelection remember the last Range set by
Range.addRange()
> > [2] crrev.com/2653523003: Make DOMSelection cache Range
> 
> This patch has no behavior change only if either of the following is true:
> 1. m_logicalRange == firstRangeOf(computeVisibleSelectionInDOMTree()), as
Yoichi suggested
> 2. SelectionEditor::firstRange can never get called when m_logicalRange is
non-null
> 
> However, it doesn't seem to be the case. A counterexample:
> 0. We start with no selection, which means both m_logicalRange and
m_cachedRange are null
> 1. InputMethodController::setSelectionOffsets updates selection, which sets
m_logicalRange non-null but doesn't touch m_cachedRange
> 2. Some action triggers selection canonicalization, making m_logicalRange !=
firstRangeOf(computeVisibleSelectionInDOMTree())
> 3. DOMSelection::getRangeAt(0) is called, which is supposed to fetch
m_logicalRange because m_cachedRange is still null
> 
> Step 1 can also be any other call site of FrameSelection::setSelectedRange

Thanks for providing an example. This is unexpceted behavior we should stop
depending on.
I'll write test case for this.

[yosin_UTC9, 2017-03-03 10:31:12]

Description was changed from

==========
Get rid of redundant member variable SelectionEditor::m_logicalRange

This patch gets rid of redundant member |SelectionEditor::m_logicalRange|,
since |m_cacheRange| supersedes it, to reduce source code size for improving
code health.

BUG=693557
TEST=n/a; no behavior changes
==========

to

==========
Get rid of redundant member variable SelectionEditor::m_logicalRange

This patch gets rid of redundant member |SelectionEditor::m_logicalRange|,
since |m_cacheRange| supersedes it, to reduce source code size for improving
code health.

BUG=693557
TEST=webkit_unit_tests --gtest_filter=FrameSelectionTest.FirstRange
==========

[yosin_UTC9, 2017-03-03 10:34:14]

Description was changed from

==========
Get rid of redundant member variable SelectionEditor::m_logicalRange

This patch gets rid of redundant member |SelectionEditor::m_logicalRange|,
since |m_cacheRange| supersedes it, to reduce source code size for improving
code health.

BUG=693557
TEST=webkit_unit_tests --gtest_filter=FrameSelectionTest.FirstRange
==========

to

==========
Get rid of redundant member variable SelectionEditor::m_logicalRange

This patch gets rid of redundant member |SelectionEditor::m_logicalRange|,
since |m_cacheRange| supersedes it, to reduce source code size for improving
code health.

This patch also makes |FrameSelection::firstRange()| to return |Range| object
from current selection instead of "logical range" set by
|FrameSelection::setSelectedRange()|.

BUG=693557
TEST=webkit_unit_tests --gtest_filter=FrameSelectionTest.FirstRange
==========

[yosin_UTC9, 2017-03-03 10:35:15]

PTAL

Add test for checking return value of FrameSelection::firstRange(), which is
range of
current VisibleSelection.

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

https://codereview.chromium.org/2723133004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:371: return;
On 2017/03/02 at 20:34:24, Xiaocheng wrote:
> This patch has conflict with https://codereview.chromium.org/2724333002.
> 
> Could you make it a dependant patch? (Or just be more careful when
committing...)
Dependent patch doesn't work well for me; I use four work spaces, 3 on office, 1
on home and I failed on rebase dependent patches so far.
This is speculative workflow for lazy rebasing (rebase for commit; we don't sure
which patch is committed first).
For me, this is more cheap to rebase every day; I sync every morning.

[yosin_UTC9, 2017-03-03 10:36:18]

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

https://codereview.chromium.org/2723133004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:70:
EXPECT_EQ(Position(sample->nextSibling(), 0), range->startPosition())
Before this patch: 
range->startPosition() == "0123456890@3
range->endPosition() == "0123456890@6

[Xiaocheng, 2017-03-03 17:23:05]

So the current behavior is actually buggy...

Please update the patch description to emphasize that, the old design for
Selection.rangeCount and Selection.getRangeAt() based on m_logicalRange is
buggy, and this patch fixes it by letting m_cachedRange take full control over
it.

Code change lgtm.

[yoichio, 2017-03-06 01:56:00]

lgtm

[yosin_UTC9, 2017-03-06 03:22:24]

Description was changed from

==========
Get rid of redundant member variable SelectionEditor::m_logicalRange

This patch gets rid of redundant member |SelectionEditor::m_logicalRange|,
since |m_cacheRange| supersedes it, to reduce source code size for improving
code health.

This patch also makes |FrameSelection::firstRange()| to return |Range| object
from current selection instead of "logical range" set by
|FrameSelection::setSelectedRange()|.

BUG=693557
TEST=webkit_unit_tests --gtest_filter=FrameSelectionTest.FirstRange
==========

to

==========
Make FrameSeelction::firstRange to return proper range

This patch makes |FrameSeelction::firstRange()| to return proper range instead
of the range specified by |FrameSelection::setSelectedRange()|.

Before this patch, |FrameSelection::firstRange()| returns the range we called
"logical range" introduced by patch[1], which mitigated some situation but it
is incomplete. Patch[2] introduced complete solution and compatible with the
specification[3].

This patch is follow-up of patch[2] to remove "logical range" from Blink since
cached |Range| supersedes "logical range".

[1] crrev.com/196553003: Let FrameSelection remember the last Range set by
Range.addRange()
[2] crrev.com/2653523003: Make DOMSelection cache Range
[3] https://www.w3.org/TR/selection-api/

BUG=693557
TEST=webkit_unit_tests --gtest_filter=FrameSelectionTest.FirstRange
==========

[yosin_UTC9, 2017-03-06 03:22:28]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-03-06 03:22:38]

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, 2017-03-06 03:24:32]

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

[commit-bot: I haz the power, 2017-03-06 03:24:32]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[yosin_UTC9, 2017-03-06 03:43:52]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2017-03-06 03:43:56]

The CQ bit was unchecked by yosin@chromium.org

[yosin_UTC9, 2017-03-06 04:03:12]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2017-03-06 04:03:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
yoichio@chromium.org, xiaochengh@chromium.org
Link to the patchset: https://codereview.chromium.org/2723133004/#ps40001
(title: "2017-03-06T13:02:21 rebase")

[commit-bot: I haz the power, 2017-03-06 04:03:30]

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, 2017-03-06 05:34:22]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488772992052410,
"parent_rev": "615bbb898b2b62c1e1720d4278aba988662cc77b", "commit_rev":
"67adef739b13e794850c228f3677dd75614958c1"}

[commit-bot: I haz the power, 2017-03-06 05:35:10]

Description was changed from

==========
Make FrameSeelction::firstRange to return proper range

This patch makes |FrameSeelction::firstRange()| to return proper range instead
of the range specified by |FrameSelection::setSelectedRange()|.

Before this patch, |FrameSelection::firstRange()| returns the range we called
"logical range" introduced by patch[1], which mitigated some situation but it
is incomplete. Patch[2] introduced complete solution and compatible with the
specification[3].

This patch is follow-up of patch[2] to remove "logical range" from Blink since
cached |Range| supersedes "logical range".

[1] crrev.com/196553003: Let FrameSelection remember the last Range set by
Range.addRange()
[2] crrev.com/2653523003: Make DOMSelection cache Range
[3] https://www.w3.org/TR/selection-api/

BUG=693557
TEST=webkit_unit_tests --gtest_filter=FrameSelectionTest.FirstRange
==========

to

==========
Make FrameSeelction::firstRange to return proper range

This patch makes |FrameSeelction::firstRange()| to return proper range instead
of the range specified by |FrameSelection::setSelectedRange()|.

Before this patch, |FrameSelection::firstRange()| returns the range we called
"logical range" introduced by patch[1], which mitigated some situation but it
is incomplete. Patch[2] introduced complete solution and compatible with the
specification[3].

This patch is follow-up of patch[2] to remove "logical range" from Blink since
cached |Range| supersedes "logical range".

[1] crrev.com/196553003: Let FrameSelection remember the last Range set by
Range.addRange()
[2] crrev.com/2653523003: Make DOMSelection cache Range
[3] https://www.w3.org/TR/selection-api/

BUG=693557
TEST=webkit_unit_tests --gtest_filter=FrameSelectionTest.FirstRange

Review-Url: https://codereview.chromium.org/2723133004
Cr-Commit-Position: refs/heads/master@{#454831}
Committed:
https://chromium.googlesource.com/chromium/src/+/67adef739b13e794850c228f3677...
==========

[commit-bot: I haz the power, 2017-03-06 05:35:12]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/67adef739b13e794850c228f3677...