Make DOMSelection cache Range

Make DOMSelection cache Range

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 Canary Version 57.0.2985.0 : 1199 Pass
 This Patch                 : 1895 Pass

DOMSelection::addRange caches the passed Range if no selection and 
 cached merged Range if there is already selection.

SelectionEditor clear the cached Range if selection is changed.

DOMSelection::getRangeAt return the cached Range if exists. If it doesn't,
 create and recache it.

BUG=490206
Review-Url: https://codereview.chromium.org/2653523003
Cr-Commit-Position: refs/heads/master@{#449258}
Committed: https://chromium.googlesource.com/chromium/src/+/0ea3c20f46d0ce2139eaf03bba6a3dd79429c786

[yoichio, 2017-01-23 08:37:54]

Description was changed from

==========
[WIP] getRange cache

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

to

==========
[WIP] getRange cache

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

[yoichio, 2017-01-24 07:13:55]

Patchset #2 (id:20001) has been deleted

[yoichio, 2017-01-24 07:33:18]

Patchset #3 (id:60001) has been deleted

[yoichio, 2017-01-24 07:38:58]

Description was changed from

==========
[WIP] getRange cache

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

to

==========
[WIP] getRange cache

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 before: 1199 Pass
 after : 2052 Pass

This fails on LayoutTests/editing/selection/addRange-merging.html which
 is js-test-base so I convert it to w3c harness.

BUG=683066
==========

[yoichio, 2017-01-24 07:39:14]

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

[commit-bot: I haz the power, 2017-01-24 07:39:21]

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

[yoichio, 2017-01-24 08:27:01]

Description was changed from

==========
[WIP] getRange cache

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 before: 1199 Pass
 after : 2052 Pass

This fails on LayoutTests/editing/selection/addRange-merging.html which
 is js-test-base so I convert it to w3c harness.

BUG=683066
==========

to

==========
[WIP] getRange cache

This patch improves selection WPT score.

Path #2 and #3:
For example selection/addRange-00.html of 2784 tests,
 before: 1199 Pass
 after : 2052 Pass
This fails on LayoutTests/editing/selection/addRange-merging.html which
 is js-test-base so I convert it to w3c harness.



BUG=683066
==========

[yoichio, 2017-01-24 08:31:02]

Description was changed from

==========
[WIP] getRange cache

This patch improves selection WPT score.

Path #2 and #3:
For example selection/addRange-00.html of 2784 tests,
 before: 1199 Pass
 after : 2052 Pass
This fails on LayoutTests/editing/selection/addRange-merging.html which
 is js-test-base so I convert it to w3c harness.



BUG=683066
==========

to

==========
[WIP] getRange cache

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 Canary: 1199 Pass

Patch #2 and #3:
 2052 Pass
This fails on LayoutTests/editing/selection/addRange-merging.html which
 is js-test-base so I convert it to w3c harness.

Patch #4:
 1895 Pass

BUG=683066
==========

[yoichio, 2017-01-24 08:32:48]

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

[commit-bot: I haz the power, 2017-01-24 08:32:57]

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

[yoichio, 2017-01-24 09:09:21]

Description was changed from

==========
[WIP] getRange cache

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 Canary: 1199 Pass

Patch #2 and #3:
 2052 Pass
This fails on LayoutTests/editing/selection/addRange-merging.html which
 is js-test-base so I convert it to w3c harness.

Patch #4:
 1895 Pass

BUG=683066
==========

to

==========
[WIP] getRange cache

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 Canary Version 57.0.2985.0: 1199 Pass

Patch #2 and #3:
 2052 Pass
This fails when addRange if Selection already has a Range
(see LayoutTests/editing/selection/addRange-merging.html)

Patch #4:
 1895 Pass

BUG=683066
==========

[commit-bot: I haz the power, 2017-01-24 10:14:41]

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

[commit-bot: I haz the power, 2017-01-24 10:14:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yoichio, 2017-01-25 07:33:06]

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

[commit-bot: I haz the power, 2017-01-25 07:33:26]

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

[yoichio, 2017-01-25 07:41:26]

Description was changed from

==========
[WIP] getRange cache

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 Canary Version 57.0.2985.0: 1199 Pass

Patch #2 and #3:
 2052 Pass
This fails when addRange if Selection already has a Range
(see LayoutTests/editing/selection/addRange-merging.html)

Patch #4:
 1895 Pass

BUG=683066
==========

to

==========
[WIP] Make DOMSelection cache Range

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 Canary Version 57.0.2985.0 : 1199 Pass
 This Patch                 : 1895 Pass

DOMSelection::addRange caches the passed Range if no selection and 
 cached merged Range if there is already selection.

SelectionEditor clear the cached Range if selection is changed.

DOMSelection::getRangeAt return the cached Range if exists. If it doesn't,
 create and recache it.

BUG=683066
==========

[yoichio, 2017-01-25 07:43:48]

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

[yoichio, 2017-01-25 07:43:49]

This CL needs to refine Layouttests/external/wpt/selection/*-expected.txt

Could you tell me an easy way to do that?(Just copy from result HTML page?)

[tkent, 2017-01-25 07:46:07]

On 2017/01/25 at 07:43:49, yoichio wrote:
> This CL needs to refine Layouttests/external/wpt/selection/*-expected.txt
> 
> Could you tell me an easy way to do that?(Just copy from result HTML page?)

% run-webkit-tests external/wpt/selection --reset

[tkent, 2017-01-25 07:59:12]

https://codereview.chromium.org/2653523003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/2653523003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:422: if
(!areRangesEqual(newRange, m_Range))
m_Range is always nullptr. Is this code equivalent to

if (Range* newRange = createRangeFromSelectionEditor())
    m_range = newRange;

right?

https://codereview.chromium.org/2653523003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/DOMSelection.h (right):

https://codereview.chromium.org/2653523003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.h:126: Member<Range>
m_Range;
m_Range -> m_range

https://codereview.chromium.org/2653523003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2653523003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:84: static void
MarkDOMSelectionDirty(const VisibleSelection& selection) {
Mark -> mark

[yoichio, 2017-01-25 08:53:00]

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

[commit-bot: I haz the power, 2017-01-25 08:53:14]

Dry run: 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-01-25 10:37:18]

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

[commit-bot: I haz the power, 2017-01-25 10:37:20]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tkent, 2017-01-25 23:55:58]

https://codereview.chromium.org/2653523003/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/dom/ranges/Range-mutations-appendChild-expected.txt
(right):

https://codereview.chromium.org/2653523003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/dom/ranges/Range-mutations-appendChild-expected.txt:13:
FAIL testDiv.appendChild(testDiv.lastChild), with selected range collapsed at
(testDiv.lastChild, 0) assert_equals: Wrong start container expected Element
node <div id="test"><p id="a">Äb̈c̈d̈ëf̈g̈ḧ
Some tests has new FAIL lines.  Are they expected?

[yosin_UTC9, 2017-01-26 08:12:49]

https://codereview.chromium.org/2653523003/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/dom/ranges/Range-mutations-appendChild-expected.txt
(right):

https://codereview.chromium.org/2653523003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/dom/ranges/Range-mutations-appendChild-expected.txt:13:
FAIL testDiv.appendChild(testDiv.lastChild), with selected range collapsed at
(testDiv.lastChild, 0) assert_equals: Wrong start container expected Element
node <div id="test"><p id="a">Äb̈c̈d̈ëf̈g̈ḧ
On 2017/01/25 at 23:55:58, tkent wrote:
> Some tests has new FAIL lines.  Are they expected?

It seems this test case attempt to set position in comment to selection, and
Firefox move it to DIV enclosing comment?

https://codereview.chromium.org/2653523003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2653523003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:88:
DCHECK(selection.start().anchorNode());
nit: We don't need to have this DCHECK() because following statement uses it.

https://codereview.chromium.org/2653523003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:93:
DCHECK(selection.end().anchorNode());
nit: We don't need to have this DCHECK() because following statement uses it.

[yosin_UTC9, 2017-01-26 08:28:04]

https://codereview.chromium.org/2653523003/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/dom/ranges/Range-mutations-appendChild-expected.txt
(right):

https://codereview.chromium.org/2653523003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/dom/ranges/Range-mutations-appendChild-expected.txt:13:
FAIL testDiv.appendChild(testDiv.lastChild), with selected range collapsed at
(testDiv.lastChild, 0) assert_equals: Wrong start container expected Element
node <div id="test"><p id="a">Äb̈c̈d̈ëf̈g̈ḧ
On 2017/01/26 at 08:12:49, Yosi_UTC9 wrote:
> On 2017/01/25 at 23:55:58, tkent wrote:
> > Some tests has new FAIL lines.  Are they expected?
> 
> It seems this test case attempt to set position in comment to selection, and
Firefox move it to DIV enclosing comment?

Humm, Firefox can set a position in comment:
https://jsfiddle.net/fuxfs01L/2/

[yoichio, 2017-01-27 08:33:04]

It seems "--reset" flag rewrites tests expectation even it is marked Failure
 in TestExpectations(e.g. external/wpt/webstorage/document-domain-expected.txt)
Is that O.K or how do you avoid that?

https://codereview.chromium.org/2653523003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/2653523003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:422: if
(!areRangesEqual(newRange, m_Range))
On 2017/01/25 07:59:12, tkent wrote:
> m_Range is always nullptr. Is this code equivalent to
> 
> if (Range* newRange = createRangeFromSelectionEditor())
>     m_range = newRange;
> 
> right?

Right.

https://codereview.chromium.org/2653523003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/DOMSelection.h (right):

https://codereview.chromium.org/2653523003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.h:126: Member<Range>
m_Range;
On 2017/01/25 07:59:12, tkent wrote:
> m_Range -> m_range

Done.

https://codereview.chromium.org/2653523003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2653523003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:84: static void
MarkDOMSelectionDirty(const VisibleSelection& selection) {
On 2017/01/25 07:59:12, tkent wrote:
> Mark -> mark

Done.

https://codereview.chromium.org/2653523003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2653523003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:88:
DCHECK(selection.start().anchorNode());
On 2017/01/26 08:12:49, Yosi_UTC9 wrote:
> nit: We don't need to have this DCHECK() because following statement uses it.

Done.

https://codereview.chromium.org/2653523003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:93:
DCHECK(selection.end().anchorNode());
On 2017/01/26 08:12:49, Yosi_UTC9 wrote:
> nit: We don't need to have this DCHECK() because following statement uses it.

Done.

[yoichio, 2017-01-27 08:33:34]

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

[commit-bot: I haz the power, 2017-01-27 08:33:51]

Dry run: 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-01-27 10:02:52]

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

[commit-bot: I haz the power, 2017-01-27 10:02:54]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yosin_UTC9, 2017-01-27 10:19:35]

https://codereview.chromium.org/2653523003/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2653523003/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:88:
selection.start().anchorNode()->treeScope().getSelection()->markRangeDirty();
We want to have |TreeScope::getSelectionIfExists()| to avoid creating
|DOMSelection| it isn't used.

[tkent, 2017-01-27 11:12:38]

On 2017/01/27 at 08:33:04, yoichio wrote:
> It seems "--reset" flag rewrites tests expectation even it is marked Failure
>  in TestExpectations(e.g.
external/wpt/webstorage/document-domain-expected.txt)
> Is that O.K or how do you avoid that?

You should run "run-webkit-tests external/wpt/selection external/wpt/dom
--reset" to do rebaseline only for affected tests.
Or revert unnecessary -expected.txt; For example, "git checkout HEAD^ --
third_party/WebKit/LayoutTests/external/wpt/webstorage/document-domain-expected.txt"

[yoichio, 2017-02-07 09:44:24]

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

[commit-bot: I haz the power, 2017-02-07 09:44:44]

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

[yoichio, 2017-02-07 09:54:26]

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

[commit-bot: I haz the power, 2017-02-07 09:54:47]

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-02-07 10:04:30]

https://codereview.chromium.org/2653523003/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2653523003/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:85: while (treeScope)
{
Better to use for-statement;

for (TreeScope* runner = treeScope; runner; runner = runner->parentTreeScope())
{
  if (!runner->hasSeelection())
    continue;
  treeScope()->getSelection()->markRangeDirty();
}

[commit-bot: I haz the power, 2017-02-07 11:35:21]

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

[commit-bot: I haz the power, 2017-02-07 11:35:23]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tkent, 2017-02-08 00:38:24]

https://codereview.chromium.org/2653523003/diff/170001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/selection/extend-00-expected.txt
(right):

https://codereview.chromium.org/2653523003/diff/170001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/selection/extend-00-expected.txt:531:
FAIL extend() with range 1 [paras[0].firstChild, 0, paras[0].firstChild, 0] and
point 1 [paras[0].firstChild, 0] assert_not_equals: extend() must replace any
existing range with a new one, not mutate the existing one got disallowed value
object ""
PASS->FAIL in extend-00.html are not regressions, but a defect of this CL causes
them.

Now we pass these tests because Selection.getRangeAt() always returns new Range.
With this CL, DOMSelection::extend() needs to clear DOMSelection::m_range,
however it seems it doesn't happen.  Due to early-return in
DOMSelection::extend() or FrameSelection::setSelectionAlgorithm()?

[yoichio, 2017-02-08 01:01:30]

I'm wondering why following tests are failed:
external/wpt/service-workers/service-worker/navigation-redirect.https.html
external/wpt/selection/addRange-36.html
external/wpt/selection/addRange-00.html
external/wpt/selection/addRange-16.html
external/wpt/selection/addRange-32.html
external/wpt/selection/addRange-28.html
external/wpt/selection/addRange-04.html

[tkent, 2017-02-08 01:12:31]

https://codereview.chromium.org/2653523003/diff/170001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/dom/ranges/Range-mutations-appendChild-expected.txt
(right):

https://codereview.chromium.org/2653523003/diff/170001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/dom/ranges/Range-mutations-appendChild-expected.txt:13:
FAIL testDiv.appendChild(testDiv.lastChild), with selected range collapsed at
(testDiv.lastChild, 0) assert_equals: Wrong start container expected Element
node <div id="test"><p id="a">Äb̈c̈d̈ëf̈g̈ḧ
PASS->FAIL changes in Range-mutations-appendChild.html and
Range-mutations-dataChange.html are expected.  I think it's ok to proceed this
CL without resolving them.

It seems we have an existing bug around Comment handling. Without this CL,
"selected range" tests are affected by the normalization, and they pass
accidentally.  With this CL, getRangeAt(0) correctly returns non-normalized
Range, and it expose the bug.
Note that the result of "selected range" test is now same as the result of
"unselected range". It's reasonable.

[tkent, 2017-02-08 01:13:59]

On 2017/02/08 at 01:01:30, yoichio wrote:
> I'm wondering why following tests are failed:
> external/wpt/service-workers/service-worker/navigation-redirect.https.html

Because you changed navigation-redirect.https-expected.txt unnecessarily?

> external/wpt/selection/addRange-36.html
> external/wpt/selection/addRange-00.html
> external/wpt/selection/addRange-16.html
> external/wpt/selection/addRange-32.html
> external/wpt/selection/addRange-28.html
> external/wpt/selection/addRange-04.html

Because you didn't upload updated -expected.txt for them.

[tkent, 2017-02-08 01:15:56]

I think I investigated all of PASS->FAILs.
In summary,
 - We don't need to care about them in wpt/dom/ranges/*
 - We should fix them in wpt/selection/extend-00.html

[tkent, 2017-02-08 01:47:04]

https://codereview.chromium.org/2653523003/diff/170001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/dom/ranges/Range-mutations-appendChild-expected.txt
(right):

https://codereview.chromium.org/2653523003/diff/170001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/dom/ranges/Range-mutations-appendChild-expected.txt:13:
FAIL testDiv.appendChild(testDiv.lastChild), with selected range collapsed at
(testDiv.lastChild, 0) assert_equals: Wrong start container expected Element
node <div id="test"><p id="a">Äb̈c̈d̈ëf̈g̈ḧ
On 2017/02/08 at 01:12:31, tkent wrote:
> PASS->FAIL changes in Range-mutations-appendChild.html and
Range-mutations-dataChange.html are expected.  I think it's ok to proceed this
CL without resolving them.
> 
> It seems we have an existing bug around Comment handling. Without this CL,
"selected range" tests are affected by the normalization, and they pass
accidentally.  With this CL, getRangeAt(0) correctly returns non-normalized
Range, and it expose the bug.
> Note that the result of "selected range" test is now same as the result of
"unselected range". It's reasonable.

I found the root cause of the bug.  Will fix.

[yoichio, 2017-02-08 06:05:36]

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

[commit-bot: I haz the power, 2017-02-08 06:05:56]

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

[yoichio, 2017-02-08 06:06:21]

Description was changed from

==========
[WIP] Make DOMSelection cache Range

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 Canary Version 57.0.2985.0 : 1199 Pass
 This Patch                 : 1895 Pass

DOMSelection::addRange caches the passed Range if no selection and 
 cached merged Range if there is already selection.

SelectionEditor clear the cached Range if selection is changed.

DOMSelection::getRangeAt return the cached Range if exists. If it doesn't,
 create and recache it.

BUG=683066
==========

to

==========
Make DOMSelection cache Range

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 Canary Version 57.0.2985.0 : 1199 Pass
 This Patch                 : 1895 Pass

DOMSelection::addRange caches the passed Range if no selection and 
 cached merged Range if there is already selection.

SelectionEditor clear the cached Range if selection is changed.

DOMSelection::getRangeAt return the cached Range if exists. If it doesn't,
 create and recache it.

BUG=683066
==========

[yoichio, 2017-02-08 06:09:33]

I fixed DOMSelection::extend issue but there are many diff in 
selection/extend-*-expected.txt.
They consist of few of "FAIL to PASS" and much still FAILs with other messages.

Also update SelectionEditor.cpp::markRangeDirty readability.

[yoichio, 2017-02-08 06:10:10]

Patchset #11 (id:190001) has been deleted

[yoichio, 2017-02-08 06:10:25]

Patchset #10 (id:180001) has been deleted

[yoichio, 2017-02-08 06:10:38]

Patchset #8 (id:160001) has been deleted

[tkent, 2017-02-08 06:16:50]

lgtm.

This CL doesn't need to update the following file. Please revert the change.
third_party/WebKit/LayoutTests/external/wpt/html/browsers/the-window-object/security-window/window-security-expected.txt

https://codereview.chromium.org/2653523003/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2653523003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2351: 
Tips:

If you want to add new entries to TestExpectations file, you should avoid to add
to the bottom of the file because many people try to update the bottom, and it's
easy to cause patch conflict.

[yosin_UTC9, 2017-02-08 06:20:26]

https://codereview.chromium.org/2653523003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2653523003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:97:
markRangeDirty(&startScope);
We should reset all ranges in whole tree-of-tree not only ancestors of start and
end.

https://codereview.chromium.org/2653523003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:139:
markDOMSelectionDirty(m_selection);
Why do we need to call markDOMSelectionDirty() twice?

[commit-bot: I haz the power, 2017-02-08 07:19:20]

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

[commit-bot: I haz the power, 2017-02-08 07:19:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yoichio, 2017-02-08 07:41:36]

As offline discussion, we cache only selection is of
 root document.

[yoichio, 2017-02-08 09:04:25]

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

[yoichio, 2017-02-08 09:04:26]

Patchset #11 (id:220001) has been deleted

[commit-bot: I haz the power, 2017-02-08 09:04:43]

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-02-08 09:20:25]

Please hold |Range| object in |SelectionEditor| rather than |DOMSelection|. We
don't need to hold |Range| object for |ShadowRoot|.

Here is pseudo code:

Range* DOMSelection::getRangeAT(int index) {
  if (index == 0 && m_treeScope == m_treeScope.document())
    return m_frame.selection().getRangeAt(0);
  ... current code ...
}

FrameSelection::getRange(int index) { return m_selectionEditor->getRangeAt(0); }

void DOMSelection::addRange(Range* range) {
  if (m_treeScope == m_treeScope.document())
   return m_frame.selection().addRange(range);
  ... set selection from |range|.
}

 FrameSelection::addRange(Range* range) {
  m_selectionEditor->addRange(range);
  ... set selection from Range
}


void DOMSelection::removeAllRange() {
  m_frame.selection().removeAllRange()
}

We also want to make |FrameSelection::firstRange()| always returns cloned-Range.
Since we don't need to have logical Range anymore.

[commit-bot: I haz the power, 2017-02-08 11:03:40]

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

[commit-bot: I haz the power, 2017-02-08 11:03:42]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tkent, 2017-02-09 00:21:36]

https://codereview.chromium.org/2653523003/diff/230001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/selection/selectAllChildren-expected.txt
(right):

https://codereview.chromium.org/2653523003/diff/230001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/selection/selectAllChildren-expected.txt:398:
FAIL Range 1 [paras[0].firstChild, 0, paras[0].firstChild, 0], node 1
paras[0].firstChild assert_not_equals: selectAllChildren must replace any
existing range, not mutate it got disallowed value object ""
Probably this is a similar problem as extend().

[tkent, 2017-02-09 00:34:34]

Description was changed from

==========
Make DOMSelection cache Range

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 Canary Version 57.0.2985.0 : 1199 Pass
 This Patch                 : 1895 Pass

DOMSelection::addRange caches the passed Range if no selection and 
 cached merged Range if there is already selection.

SelectionEditor clear the cached Range if selection is changed.

DOMSelection::getRangeAt return the cached Range if exists. If it doesn't,
 create and recache it.

BUG=683066
==========

to

==========
Make DOMSelection cache Range

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 Canary Version 57.0.2985.0 : 1199 Pass
 This Patch                 : 1895 Pass

DOMSelection::addRange caches the passed Range if no selection and 
 cached merged Range if there is already selection.

SelectionEditor clear the cached Range if selection is changed.

DOMSelection::getRangeAt return the cached Range if exists. If it doesn't,
 create and recache it.

BUG=490206
==========

[yoichio, 2017-02-09 05:57:58]

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

[yoichio, 2017-02-09 05:58:18]

Patchset #12 (id:240001) has been deleted

[commit-bot: I haz the power, 2017-02-09 05:58:32]

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

[yoichio, 2017-02-09 05:59:06]

On 2017/02/08 09:20:25, yosin_BlinkOn_slow wrote:
> Please hold |Range| object in |SelectionEditor| rather than |DOMSelection|. We
> don't need to hold |Range| object for |ShadowRoot|.
> 
Done.

[yoichio, 2017-02-09 05:59:16]

https://codereview.chromium.org/2653523003/diff/230001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/selection/selectAllChildren-expected.txt
(right):

https://codereview.chromium.org/2653523003/diff/230001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/selection/selectAllChildren-expected.txt:398:
FAIL Range 1 [paras[0].firstChild, 0, paras[0].firstChild, 0], node 1
paras[0].firstChild assert_not_equals: selectAllChildren must replace any
existing range, not mutate it got disallowed value object ""
On 2017/02/09 00:21:35, tkent wrote:
> Probably this is a similar problem as extend().

Done.

[yoichio, 2017-02-09 06:11:02]

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

[yoichio, 2017-02-09 06:11:02]

Patchset #12 (id:250001) has been deleted

[commit-bot: I haz the power, 2017-02-09 06:11:35]

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-02-09 06:52:06]

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:423: if
(!isTreeScopeDocument())
If we have |cachedRangeIfDocumentSlection()| and
|cacheRangeIfDocumentSelection()|

if (Range* cachedRange = cachedRangeIfDocumentSelection())
  return range;
return cacheRangeIfDocumentSelection(createRangeFromSelectionEditor());

It seems |cachedRangeXXX()| and |cacheRangeXXX() are very similar, we should
have
another name like |rememberRangeIfDocumentSelection()| or another to have visual
difference.

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:449: bool
DOMSelection::isTreeScopeDocument() const {
nit: s/isTreeScopeDocument/isSelectionForDocument/

It is hard to see relationship between DOMSelection and TreeScope.

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:464:
frame()->selection().clearDocumentCachedRange();
Let's add |DCHECK(isTreeScopeDocument())|.

It seems |if (isTreeScopeDocument()) ...| pattern is verbose.

|clearCachedRangeIfDocument()| is simpler.

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:503: if
(isTreeScopeDocument())
|cacheRangeIfDocumentSlection(newRange)| is simpler.

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:28: #include
"core/editing/DOMSelection.h"
nit: SelectionEditor should not depend to DOMSelection.

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionEditor.h (right):

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.h:85: void
cacheRangeOfDocument(Range* range) { m_cachedRange = range; }
Let's move an implementation to "SelectionEdtior.cpp" because of this function
is not time critical and we'll extend this function to support multiple |Range|.

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.h:86: Range*
documentCachedRange() const { return m_cachedRange; }
Let's move an implementation to "SelectionEdtior.cpp" because of this function
is not time critical and we'll extend this function to support multiple |Range|.

[commit-bot: I haz the power, 2017-02-09 07:20:27]

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

[commit-bot: I haz the power, 2017-02-09 07:20:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoichio, 2017-02-09 09:11:33]

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

[commit-bot: I haz the power, 2017-02-09 09:11:47]

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

[yoichio, 2017-02-09 09:28:19]

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

[commit-bot: I haz the power, 2017-02-09 09:28:37]

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

[yoichio, 2017-02-09 09:28:57]

Patchset #13 (id:270001) has been deleted

[yoichio, 2017-02-09 09:29:15]

Patchset #13 (id:280001) has been deleted

[yoichio, 2017-02-09 09:31:20]

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:423: if
(!isTreeScopeDocument())
On 2017/02/09 06:52:06, yosin_UTC9 wrote:
> If we have |cachedRangeIfDocumentSlection()| and
> |cacheRangeIfDocumentSelection()|
> 
> if (Range* cachedRange = cachedRangeIfDocumentSelection())
>   return range;
> return cacheRangeIfDocumentSelection(createRangeFromSelectionEditor());
> 
> It seems |cachedRangeXXX()| and |cacheRangeXXX() are very similar, we should
> have
> another name like |rememberRangeIfDocumentSelection()| or another to have
visual
> difference.

Done.

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:449: bool
DOMSelection::isTreeScopeDocument() const {
On 2017/02/09 06:52:06, yosin_UTC9 wrote:
> nit: s/isTreeScopeDocument/isSelectionForDocument/
> 
> It is hard to see relationship between DOMSelection and TreeScope.
> 

Done.

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:464:
frame()->selection().clearDocumentCachedRange();
On 2017/02/09 06:52:06, yosin_UTC9 wrote:
> Let's add |DCHECK(isTreeScopeDocument())|.
> 
> It seems |if (isTreeScopeDocument()) ...| pattern is verbose.
> 
> |clearCachedRangeIfDocument()| is simpler.

Done.

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:503: if
(isTreeScopeDocument())
On 2017/02/09 06:52:06, yosin_UTC9 wrote:
> |cacheRangeIfDocumentSlection(newRange)| is simpler.

Done.

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:28: #include
"core/editing/DOMSelection.h"
On 2017/02/09 06:52:06, yosin_UTC9 wrote:
> nit: SelectionEditor should not depend to DOMSelection.

Done.

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionEditor.h (right):

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.h:85: void
cacheRangeOfDocument(Range* range) { m_cachedRange = range; }
On 2017/02/09 06:52:06, yosin_UTC9 wrote:
> Let's move an implementation to "SelectionEdtior.cpp" because of this function
> is not time critical and we'll extend this function to support multiple
|Range|.

Done.

https://codereview.chromium.org/2653523003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.h:86: Range*
documentCachedRange() const { return m_cachedRange; }
On 2017/02/09 06:52:06, yosin_UTC9 wrote:
> Let's move an implementation to "SelectionEdtior.cpp" because of this function
> is not time critical and we'll extend this function to support multiple
|Range|.

Done.

[yosin_UTC9, 2017-02-09 09:32:10]

lgtm w/ small nits

https://codereview.chromium.org/2653523003/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/2653523003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:451: 
nit: Please get rid of an extra blank line.

https://codereview.chromium.org/2653523003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:458: 
nit: Please get rid of an extra blank line.

https://codereview.chromium.org/2653523003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:465: 
nit: Please get rid of an extra blank line.

[yosin_UTC9, 2017-02-09 09:35:49]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2017-02-09 09:35:50]

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/2653523003/#ps300001
(title: "nit")

[yoichio, 2017-02-09 09:36:04]

https://codereview.chromium.org/2653523003/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/2653523003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:451: 
On 2017/02/09 09:32:09, yosin_UTC9 wrote:
> nit: Please get rid of an extra blank line.

Done.

https://codereview.chromium.org/2653523003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:458: 
On 2017/02/09 09:32:09, yosin_UTC9 wrote:
> nit: Please get rid of an extra blank line.

Done.

https://codereview.chromium.org/2653523003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/DOMSelection.cpp:465: 
On 2017/02/09 09:32:09, yosin_UTC9 wrote:
> nit: Please get rid of an extra blank line.

Done.

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

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-02-09 11:15:58]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1486632949830200,
"parent_rev": "706a34d2abe59db6e34b212c1ff2de64ddc7904c", "commit_rev":
"0ea3c20f46d0ce2139eaf03bba6a3dd79429c786"}

[commit-bot: I haz the power, 2017-02-09 11:16:30]

Description was changed from

==========
Make DOMSelection cache Range

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 Canary Version 57.0.2985.0 : 1199 Pass
 This Patch                 : 1895 Pass

DOMSelection::addRange caches the passed Range if no selection and 
 cached merged Range if there is already selection.

SelectionEditor clear the cached Range if selection is changed.

DOMSelection::getRangeAt return the cached Range if exists. If it doesn't,
 create and recache it.

BUG=490206
==========

to

==========
Make DOMSelection cache Range

This patch improves selection WPT score.
For example selection/addRange-00.html of 2784 tests,
 Canary Version 57.0.2985.0 : 1199 Pass
 This Patch                 : 1895 Pass

DOMSelection::addRange caches the passed Range if no selection and 
 cached merged Range if there is already selection.

SelectionEditor clear the cached Range if selection is changed.

DOMSelection::getRangeAt return the cached Range if exists. If it doesn't,
 create and recache it.

BUG=490206

Review-Url: https://codereview.chromium.org/2653523003
Cr-Commit-Position: refs/heads/master@{#449258}
Committed:
https://chromium.googlesource.com/chromium/src/+/0ea3c20f46d0ce2139eaf03bba6a...
==========

[commit-bot: I haz the power, 2017-02-09 11:16:32]

Committed patchset #14 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/0ea3c20f46d0ce2139eaf03bba6a...