Make FrameSelection to hold non-canonicalized positions

Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

|SelectionEditor| utilizes |SynchronousMutationObserver| to relocate
|m_selectionInDOMTree| instead of |FrameSelection|. Before this patch
|FrameSelection| relocates |VisibleSelection| with dirty layout tree then
sets |FrameSelection::setSelection()|. To void cyclic reference between
|FrameSelection| and |SelectionEditor|, we could not move relocation part to
|SelectionEditor|.

This patch also updates 
|FrameSelection::updatePostionAfterAdoptingTextNodesMerged()| to handle
|PositonAnchorType|.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2680943004
Cr-Original-Original-Commit-Position: refs/heads/master@{#449928}
Committed: https://chromium.googlesource.com/chromium/src/+/157413286770a7ac5a24c446a30c08f749738276
Review-Url: https://codereview.chromium.org/2680943004
Cr-Original-Commit-Position: refs/heads/master@{#450280}
Committed: https://chromium.googlesource.com/chromium/src/+/17c84b2b6519c821dc319e79f2a7a4508508e20f
Review-Url: https://codereview.chromium.org/2680943004
Cr-Commit-Position: refs/heads/master@{#450370}
Committed: https://chromium.googlesource.com/chromium/src/+/d892f9592860691ae9a782c12260c94ed6bd1a63

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

Description was changed from

==========
2017-02-09T14:04:22

2017-02-08T19:09:51

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

to

==========
2017-02-09T14:04:22

2017-02-08T19:09:51

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yosin_UTC9, 2017-02-09 05:06:49]

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

[commit-bot: I haz the power, 2017-02-09 05:07: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 05:48:48]

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

[commit-bot: I haz the power, 2017-02-09 05:49:30]

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:07:13]

Description was changed from

==========
2017-02-09T14:04:22

2017-02-08T19:09:51

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
==========

[yosin_UTC9, 2017-02-09 06:14:30]

Description was changed from

==========
Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
==========

to

==========
Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yosin_UTC9, 2017-02-09 06:16:45]

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

[yosin_UTC9, 2017-02-09 06:16:46]

PTAL

changwang@, PTAL for ImeTest.java changes.

[Changwan Ryu, 2017-02-09 06:26:24]

On 2017/02/09 06:16:46, yosin_BlinkOn_slow wrote:
> PTAL
> 
> changwang@, PTAL for ImeTest.java changes.

ImeTest.java lgtm

[yosin_UTC9, 2017-02-09 09:21:13]

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

[yosin_UTC9, 2017-02-09 09:21:13]

+xiaochengh@, it seems he is now settled. (^_^)

[yoichio, 2017-02-10 05:24:52]

https://codereview.chromium.org/2680943004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2680943004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:94:
const_cast<SelectionEditor*>(this)->updateCachedVisibleSelectionIfNeeded();
Why do you const_cast the same type?

https://codereview.chromium.org/2680943004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleSelection.cpp (right):

https://codereview.chromium.org/2680943004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleSelection.cpp:485: if
(m_granularity != WordGranularity)
Does this change relate?

[yosin_UTC9, 2017-02-10 06:37:33]

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

[commit-bot: I haz the power, 2017-02-10 06:37:51]

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-10 06:38:42]

https://codereview.chromium.org/2680943004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2680943004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:94:
const_cast<SelectionEditor*>(this)->updateCachedVisibleSelectionIfNeeded();
On 2017/02/10 at 05:24:52, yoichio wrote:
> Why do you const_cast the same type?

This is const member function, |this| is type |const SelectionEditor*|.
To call non-const function, we need to have |const_cast<T>|.

https://codereview.chromium.org/2680943004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleSelection.cpp (right):

https://codereview.chromium.org/2680943004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleSelection.cpp:485: if
(m_granularity != WordGranularity)
On 2017/02/10 at 05:24:52, yoichio wrote:
> Does this change relate?

Yes, we call |validate()| function not only changing selection but also layout
and style changes.

Suppose:
1. Double click to word set |m_hasTrailingWhitespace| true
2. Some layout or style changes
3. Call |FrameSelection::computeVisibleSelection()| computes |VisibleSeleciton|
again with calling |VS::validate()|.

[commit-bot: I haz the power, 2017-02-10 07:00:06]

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

[commit-bot: I haz the power, 2017-02-10 07:00:07]

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

[yosin_UTC9, 2017-02-10 07:44:15]

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

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

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

[tkent, 2017-02-10 08:47:41]

to be honest, I don't understand the logic change in this CL. Added general
comments.

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

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:122: // TODO(yosin):
We should replace // |visibleSelection<EditingStrategy>()| to
nit: Looks the second "//" is unnecessary.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:134: //
|visibleSelection<EditingInFlatTreeStrategy>()| to
nit |to| -> |with| or |by|

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:356: // TODO(yosin):
We should remove |computePositionForChildrenRemoval()| to
nit: remove -> move

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:397: // being removed,
nit: Text wrapping looks weird.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:408: // TODO(yosin):
We should remove |updatePositionAfterAdoptingTextReplacement()|
nit: remove -> move

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:409: // to //
"SelectionEditor.cpp" since it used only in
nit: the second '//' looks unnecessary.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:454: void
SelectionEditor::didUpdateCharacterData(CharacterData* node,
nit: Add TODO comment about moving it to SelectionEditor.cpp.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:471: // TODO(yosin):
We should remove |updatePostionAfterAdoptingTextNodesMerged()|
nit: remove -> move

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:472: // to //
"SelectionEditor.cpp" since it used only in
nit: the second '//' looks unnecessary.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:527: // TODO(yosin):
We should remove |updatePostionAfterAdoptingTextNodeSplit()|
nit: remove -> move

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:528: // to //
"SelectionEditor.cpp" since it used only in
nit: the second '//' looks unnecessary.

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

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:269:
EXPECT_EQ(Position(), selection().start())
Why isn't it Position(HTML,0)?

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:106:
const_cast<SelectionEditor*>(this)->updateCachedVisibleSelectionIfNeeded();
Can we make updateCachedVisibleSelectionIfNeeded() const? It should update only
mutable data members.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/SelectionEditor.h (right):

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionEditor.h:136: mutable uint64_t
m_styleVersion = static_cast<uint64_t>(-1);
Why don't you make it int64_t?

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

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

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

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-02-10 10:12:24]

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

[commit-bot: I haz the power, 2017-02-10 10:12:40]

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-10 10:13:21]

PTAL

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

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:122: // TODO(yosin):
We should replace // |visibleSelection<EditingStrategy>()| to
On 2017/02/10 at 08:47:40, tkent wrote:
> nit: Looks the second "//" is unnecessary.

Done.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:134: //
|visibleSelection<EditingInFlatTreeStrategy>()| to
On 2017/02/10 at 08:47:41, tkent wrote:
> nit |to| -> |with| or |by|

Done.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:356: // TODO(yosin):
We should remove |computePositionForChildrenRemoval()| to
On 2017/02/10 at 08:47:40, tkent wrote:
> nit: remove -> move

Done.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:397: // being removed,
On 2017/02/10 at 08:47:40, tkent wrote:
> nit: Text wrapping looks weird.

Done.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:408: // TODO(yosin):
We should remove |updatePositionAfterAdoptingTextReplacement()|
On 2017/02/10 at 08:47:41, tkent wrote:
> nit: remove -> move

Done.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:409: // to //
"SelectionEditor.cpp" since it used only in
On 2017/02/10 at 08:47:40, tkent wrote:
> nit: the second '//' looks unnecessary.

Done.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:454: void
SelectionEditor::didUpdateCharacterData(CharacterData* node,
On 2017/02/10 at 08:47:40, tkent wrote:
> nit: Add TODO comment about moving it to SelectionEditor.cpp.

Done.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:471: // TODO(yosin):
We should remove |updatePostionAfterAdoptingTextNodesMerged()|
On 2017/02/10 at 08:47:41, tkent wrote:
> nit: remove -> move

Done.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:472: // to //
"SelectionEditor.cpp" since it used only in
On 2017/02/10 at 08:47:40, tkent wrote:
> nit: the second '//' looks unnecessary.

Done.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:527: // TODO(yosin):
We should remove |updatePostionAfterAdoptingTextNodeSplit()|
On 2017/02/10 at 08:47:40, tkent wrote:
> nit: remove -> move

Done.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:528: // to //
"SelectionEditor.cpp" since it used only in
On 2017/02/10 at 08:47:41, tkent wrote:
> nit: the second '//' looks unnecessary.

Done.

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

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:269:
EXPECT_EQ(Position(), selection().start())
On 2017/02/10 at 08:47:41, tkent wrote:
> Why isn't it Position(HTML,0)?
My reasoning was wrong. I forgot to consider HEAD element.

Positoin() is correct since 
  VisiblePosition(Postion(After, HEAD)) == Position() and
  VisiblePosition(Postion(HTML, 1)) == Position().
// I think this is an issue of VP canonicalization, VP(After, HEAD) should be
VP(After, HEAD)


as L275's EXPECT_EQ(Position(), selection().start())

DOM tree before removal is:
#document
  HTML
    HEAD (editable)
    BODY (editable)
      STYLE id="sample" (editable)

DOM tree after removal:
#document (editable)
  HTML (editable)
    HEAD (editable)

Before removal: selection is STYLE,0
After removal: selection is After HEAD; this is identical to HTML,1

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:106:
const_cast<SelectionEditor*>(this)->updateCachedVisibleSelectionIfNeeded();
On 2017/02/10 at 08:47:41, tkent wrote:
> Can we make updateCachedVisibleSelectionIfNeeded() const? It should update
only mutable data members.

Done.

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/SelectionEditor.h (right):

https://codereview.chromium.org/2680943004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionEditor.h:136: mutable uint64_t
m_styleVersion = static_cast<uint64_t>(-1);
On 2017/02/10 at 08:47:41, tkent wrote:
> Why don't you make it int64_t?

As same as Document::m_styleVersion

[commit-bot: I haz the power, 2017-02-10 11:52:28]

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

[commit-bot: I haz the power, 2017-02-10 11:52:30]

Dry run: This issue passed the CQ dry run.

[tkent, 2017-02-13 00:07:31]

I have no additional comments. I'll approve this after l-g-t-m by yoichio@ or
xiaochengh@.

[yoichio, 2017-02-13 04:38:19]

I recommend you splitting the cl into
 1. Having SelectionEditor cached VS
 2. change FrameSelection not to use VS

https://codereview.chromium.org/2680943004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):

https://codereview.chromium.org/2680943004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:94:
const_cast<SelectionEditor*>(this)->updateCachedVisibleSelectionIfNeeded();
On 2017/02/10 06:38:41, yosin_UTC9 wrote:
> On 2017/02/10 at 05:24:52, yoichio wrote:
> > Why do you const_cast the same type?
> 
> This is const member function, |this| is type |const SelectionEditor*|.
> To call non-const function, we need to have |const_cast<T>|.
I prefer const-clean code flow.
Could you rm |const| from this function or somehow?

[yoichio, 2017-02-13 05:38:39]

Could you add description about SynchronousMutationObsderver implementation
change
 both in FrameSelection and SelectionEditor?

[yosin_UTC9, 2017-02-13 05:53:03]

Description was changed from

==========
Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

|SelectionEditor| utilizes |SynchronousMutationObserver| to relocate
|m_selectionInDOMTree| instead of |FrameSelection|. Before this patch
|FrameSelection| relocates |VisibleSelection| with dirty layout tree then
sets |FrameSelection::setSelection()|. To void cyclic reference between
|FrameSelection| and |SelectionEditor|, we could not move relocation part to
|SelectionEditor|.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yosin_UTC9, 2017-02-13 05:55:12]

On 2017/02/13 at 05:38:39, yoichio wrote:
> Could you add description about SynchronousMutationObsderver implementation
change
>  both in FrameSelection and SelectionEditor?

Update description.

I tried to move relocation part to |SelectionEditor| when |SelectionEditor|
introduced.
However, current implementation requires to call
|FrameSelection::setSelection()| at
the end of relocation. To avoid having back pointer to |FrameSelection| from
|SelectionEditor|, I gave up to move relocation part to |SelectionEditor|.

[yoichio, 2017-02-13 06:37:43]

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:246: if
(!selection().isNone() && !(options & DoNotSetFocus)) {
I prefer m_selectionEditor->computeVisibleSelectionInDOMTree() to 
 avoid confusing
 because this is a function changing "selection".

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:475: static Position
updatePostionAfterAdoptingTextNodesMerged(
How do you change this function and why?

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:477: const Text&
mergedNode,
Just |Text* mergedNode| to avoid const_cast?

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

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:98:
SelectionEditor::computeVisibleSelectionInFlatTree() const {
Remove const declaration.

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:161: Node&
nodeToBeRemoved) {
Why don't you use uodatePositionForNodeRemoval 
 as FrameSelection::nodeWillBeRemoved originally uses it?

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

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.h:57: bool
hasEditableStyle() const;
Please declare at original line if not intended.

[yosin_UTC9, 2017-02-13 07:02:47]

Description was changed from

==========
Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

|SelectionEditor| utilizes |SynchronousMutationObserver| to relocate
|m_selectionInDOMTree| instead of |FrameSelection|. Before this patch
|FrameSelection| relocates |VisibleSelection| with dirty layout tree then
sets |FrameSelection::setSelection()|. To void cyclic reference between
|FrameSelection| and |SelectionEditor|, we could not move relocation part to
|SelectionEditor|.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

|SelectionEditor| utilizes |SynchronousMutationObserver| to relocate
|m_selectionInDOMTree| instead of |FrameSelection|. Before this patch
|FrameSelection| relocates |VisibleSelection| with dirty layout tree then
sets |FrameSelection::setSelection()|. To void cyclic reference between
|FrameSelection| and |SelectionEditor|, we could not move relocation part to
|SelectionEditor|.

This patch also updates 
|FrameSelection::updatePostionAfterAdoptingTextNodesMerged()| to handle
|PositonAnchorType|.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yosin_UTC9, 2017-02-13 07:28:31]

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

[yosin_UTC9, 2017-02-13 07:28:41]

PTAL

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:246: if
(!selection().isNone() && !(options & DoNotSetFocus)) {
On 2017/02/13 at 06:37:42, yoichio wrote:
> I prefer m_selectionEditor->computeVisibleSelectionInDOMTree() to 
>  avoid confusing
>  because this is a function changing "selection".

Done. 
BTW, I expect to change later to minimize size of patch.

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:475: static Position
updatePostionAfterAdoptingTextNodesMerged(
On 2017/02/13 at 06:37:42, yoichio wrote:
> How do you change this function and why?

Existing function doesn't consider |PositionAnchorType|.
This function handles |PositionAnchorType| correctly.

I update description about this change.

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:477: const Text&
mergedNode,
On 2017/02/13 at 06:37:42, yoichio wrote:
> Just |Text* mergedNode| to avoid const_cast?

I would like to keep this |const_cast| since |didMergeTextNodes()| takes |const
Text&| 
and introduce const Text&, int)| in following patch.

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

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:98:
SelectionEditor::computeVisibleSelectionInFlatTree() const {
On 2017/02/13 at 06:37:42, yoichio wrote:
> Remove const declaration.

I would like to keep "const" since term "computeXXX()" doesn't
imply causing side effect into object. Cache is considered as
implementation details and callers doesn't care about cache.

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:161: Node&
nodeToBeRemoved) {
On 2017/02/13 at 06:37:42, yoichio wrote:
> Why don't you use uodatePositionForNodeRemoval 
>  as FrameSelection::nodeWillBeRemoved originally uses it?

Good catch. I implemented by myself when I need to change
it. But, original updatePositionForNodeRemoval() works
fine. I switch to it.

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

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.h:57: bool
hasEditableStyle() const;
On 2017/02/13 at 06:37:42, yoichio wrote:
> Please declare at original line if not intended.

Done.

[commit-bot: I haz the power, 2017-02-13 07:28: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-02-13 07:37:41]

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

[commit-bot: I haz the power, 2017-02-13 07:37:42]

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

[yosin_UTC9, 2017-02-13 07:40:03]

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

[yosin_UTC9, 2017-02-13 07:40:04]

Patchset #7 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-02-13 07:40:16]

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-13 08:18:04]

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:475: static Position
updatePostionAfterAdoptingTextNodesMerged(
On 2017/02/13 07:28:40, yosin_UTC9 wrote:
> On 2017/02/13 at 06:37:42, yoichio wrote:
> > How do you change this function and why?
> 
> Existing function doesn't consider |PositionAnchorType|.
> This function handles |PositionAnchorType| correctly.
> 
> I update description about this change.
If this change is a kind of bugfixing, could you do it later combining 
|const Text& mergedNode| type change below?

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:477: const Text&
mergedNode,
On 2017/02/13 07:28:40, yosin_UTC9 wrote:
> On 2017/02/13 at 06:37:42, yoichio wrote:
> > Just |Text* mergedNode| to avoid const_cast?
> 
> I would like to keep this |const_cast| since |didMergeTextNodes()| takes
|const
> Text&| 
> and introduce const Text&, int)| in following patch.

Acknowledged.

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

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:98:
SelectionEditor::computeVisibleSelectionInFlatTree() const {
On 2017/02/13 07:28:40, yosin_UTC9 wrote:
> On 2017/02/13 at 06:37:42, yoichio wrote:
> > Remove const declaration.
> 
> I would like to keep "const" since term "computeXXX()" doesn't
> imply causing side effect into object. Cache is considered as
> implementation details and callers doesn't care about cache.
Then I prefer changing its name. Caller side developers may believe object state
doesn't change over const method calling and its 
important to understand and debug.

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:210: void
SelectionEditor::nodeWillBeRemoved(Node& nodeToBeRemoved) {
I prefer setting this function after |FrameSelection::nodeWillBeRemoved|
 to emphasize this replaces |FrameSelection::respondToNodeModification|
 like |nodeChildrenWillBeRemoved|.

[tkent, 2017-02-13 08:33:27]

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

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:98:
SelectionEditor::computeVisibleSelectionInFlatTree() const {
On 2017/02/13 at 08:18:03, yoichio wrote:
> On 2017/02/13 07:28:40, yosin_UTC9 wrote:
> > On 2017/02/13 at 06:37:42, yoichio wrote:
> > > Remove const declaration.
> > 
> > I would like to keep "const" since term "computeXXX()" doesn't
> > imply causing side effect into object. Cache is considered as
> > implementation details and callers doesn't care about cache.
> Then I prefer changing its name. Caller side developers may believe object
state doesn't change over const method calling and its 
> important to understand and debug.

Maybe updateCachedVisibleSelectionIfNeeded should be a const function if it
touch only mutable data members.

[yosin_UTC9, 2017-02-13 09:01:43]

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

[commit-bot: I haz the power, 2017-02-13 09:01:53]

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

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

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

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

[yosin_UTC9, 2017-02-13 09:19:13]

On 2017/02/13 at 08:33:27, tkent wrote:
>
https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/SelectionEditor.cpp (right):
> 
>
https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/SelectionEditor.cpp:98:
SelectionEditor::computeVisibleSelectionInFlatTree() const {
> On 2017/02/13 at 08:18:03, yoichio wrote:
> > On 2017/02/13 07:28:40, yosin_UTC9 wrote:
> > > On 2017/02/13 at 06:37:42, yoichio wrote:
> > > > Remove const declaration.
> > > 
> > > I would like to keep "const" since term "computeXXX()" doesn't
> > > imply causing side effect into object. Cache is considered as
> > > implementation details and callers doesn't care about cache.
> > Then I prefer changing its name. Caller side developers may believe object
state doesn't change over const method calling and its 
> > important to understand and debug.
> 
> Maybe updateCachedVisibleSelectionIfNeeded should be a const function if it
touch only mutable data members.
SelectionEditor::updateCachedVisibleSelectionIfNeeded() is now |const| to avoid
|const_cast<SelectionEditor*>()|

[yosin_UTC9, 2017-02-13 09:19:26]

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

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

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-13 09:24:14]

PTAL

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:475: static Position
updatePostionAfterAdoptingTextNodesMerged(
On 2017/02/13 at 08:18:03, yoichio wrote:
> On 2017/02/13 07:28:40, yosin_UTC9 wrote:
> > On 2017/02/13 at 06:37:42, yoichio wrote:
> > > How do you change this function and why?
> > 
> > Existing function doesn't consider |PositionAnchorType|.
> > This function handles |PositionAnchorType| correctly.
> > 
> > I update description about this change.
> If this change is a kind of bugfixing, could you do it later combining 
> |const Text& mergedNode| type change below?

No this is required change to pass "selection-after-text-normalize.html"
When we use original version, I got following:

FAIL ^foo | baz
LayoutTests/editing/selection/selection-after-text-normalize.html:50:9)
	 expected ^foo|baz,
	 but got  ^foobaz|,
	 sameupto ^foo

This is the reason why I changed this and SMO::didMergeTextNode() takes
    const Text& mergedNode,
    const NodeWithIndex& nodeToBeRemovedWithIndex,
    unsigned offset
instead of
   const Text& nodeToBeRemoved,
   unsigned offset

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

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:98:
SelectionEditor::computeVisibleSelectionInFlatTree() const {
On 2017/02/13 at 08:18:03, yoichio wrote:
> On 2017/02/13 07:28:40, yosin_UTC9 wrote:
> > On 2017/02/13 at 06:37:42, yoichio wrote:
> > > Remove const declaration.
> > 
> > I would like to keep "const" since term "computeXXX()" doesn't
> > imply causing side effect into object. Cache is considered as
> > implementation details and callers doesn't care about cache.
> Then I prefer changing its name. Caller side developers may believe object
state doesn't change over const method calling and its 
> important to understand and debug.

No. I don't want to call this function to
computeVisibleSelectionWithUpdatingCache().
This is kind of exposing too much implementation details.

|computeVS()| is "logical const". 
I think [1] is an good article for "mutable" and "const".
[1]
http://stackoverflow.com/questions/105014/does-the-mutable-keyword-have-any-p...

https://codereview.chromium.org/2680943004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.cpp:210: void
SelectionEditor::nodeWillBeRemoved(Node& nodeToBeRemoved) {
On 2017/02/13 at 08:18:04, yoichio wrote:
> I prefer setting this function after |FrameSelection::nodeWillBeRemoved|
>  to emphasize this replaces |FrameSelection::respondToNodeModification|
>  like |nodeChildrenWillBeRemoved|.

Done. Move to "FrameSelection.cpp"

[yoichio, 2017-02-13 09:26:42]

lgtm

[tkent, 2017-02-13 09:39:26]

lgtm

[yosin_UTC9, 2017-02-13 09:53:46]

The CQ bit was unchecked by yosin@chromium.org

[yosin_UTC9, 2017-02-13 09:53:49]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2017-02-13 09:53:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
changwan@chromium.org
Link to the patchset: https://codereview.chromium.org/2680943004/#ps180001
(title: "2017-02-13T18:18:28 Rebase")

[commit-bot: I haz the power, 2017-02-13 09:54:10]

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-13 10:55:16]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1486979629203900,
"parent_rev": "4f85155e4b7ae25e1dfb21cf686890fb8634eb6f", "commit_rev":
"157413286770a7ac5a24c446a30c08f749738276"}

[commit-bot: I haz the power, 2017-02-13 10:55:50]

Description was changed from

==========
Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

|SelectionEditor| utilizes |SynchronousMutationObserver| to relocate
|m_selectionInDOMTree| instead of |FrameSelection|. Before this patch
|FrameSelection| relocates |VisibleSelection| with dirty layout tree then
sets |FrameSelection::setSelection()|. To void cyclic reference between
|FrameSelection| and |SelectionEditor|, we could not move relocation part to
|SelectionEditor|.

This patch also updates 
|FrameSelection::updatePostionAfterAdoptingTextNodesMerged()| to handle
|PositonAnchorType|.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

|SelectionEditor| utilizes |SynchronousMutationObserver| to relocate
|m_selectionInDOMTree| instead of |FrameSelection|. Before this patch
|FrameSelection| relocates |VisibleSelection| with dirty layout tree then
sets |FrameSelection::setSelection()|. To void cyclic reference between
|FrameSelection| and |SelectionEditor|, we could not move relocation part to
|SelectionEditor|.

This patch also updates 
|FrameSelection::updatePostionAfterAdoptingTextNodesMerged()| to handle
|PositonAnchorType|.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2680943004
Cr-Commit-Position: refs/heads/master@{#449928}
Committed:
https://chromium.googlesource.com/chromium/src/+/157413286770a7ac5a24c446a30c...
==========

[commit-bot: I haz the power, 2017-02-13 10:55:52]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/157413286770a7ac5a24c446a30c...

[Garrett Casto, 2017-02-13 17:32:36]

A revert of this CL (patchset #9 id:180001) has been created in
https://codereview.chromium.org/2694823002/ by gcasto@chromium.org.

The reason for reverting is: This patch looks like it is causing failures in
editing/execCommand/move-up-down-should-skip-hidden-elements.html on Windows 7
(https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win7....

[yosin_UTC9, 2017-02-14 06:29:13]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-02-14 06:29:30]

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

[yosin_UTC9, 2017-02-14 06:32:13]

On 2017/02/13 at 17:32:36, gcasto wrote:
> A revert of this CL (patchset #9 id:180001) has been created in
https://codereview.chromium.org/2694823002/ by gcasto@chromium.org.
> 
> The reason for reverting is: This patch looks like it is causing failures in
editing/execCommand/move-up-down-should-skip-hidden-elements.html on Windows 7
(https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win7....

It seems this is caused by Win7 bots slowness rather than causing regression by
this CL.
The test verifies execution speed regression. So, I convert the test to
Performance Test[1].
So, no more bots breakages are expected.

Re-landing now.

[1] http://crrev.com/2689183003 Convert layout test
move-up-down-should-skip-hidden-elements.html to performance test

[commit-bot: I haz the power, 2017-02-14 06:34:42]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487053753243070,
"parent_rev": "13cff56419e163e6c4c2a2e1e98e4bdb6087651b", "commit_rev":
"17c84b2b6519c821dc319e79f2a7a4508508e20f"}

[commit-bot: I haz the power, 2017-02-14 06:35:42]

Description was changed from

==========
Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

|SelectionEditor| utilizes |SynchronousMutationObserver| to relocate
|m_selectionInDOMTree| instead of |FrameSelection|. Before this patch
|FrameSelection| relocates |VisibleSelection| with dirty layout tree then
sets |FrameSelection::setSelection()|. To void cyclic reference between
|FrameSelection| and |SelectionEditor|, we could not move relocation part to
|SelectionEditor|.

This patch also updates 
|FrameSelection::updatePostionAfterAdoptingTextNodesMerged()| to handle
|PositonAnchorType|.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2680943004
Cr-Commit-Position: refs/heads/master@{#449928}
Committed:
https://chromium.googlesource.com/chromium/src/+/157413286770a7ac5a24c446a30c...
==========

to

==========
Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

|SelectionEditor| utilizes |SynchronousMutationObserver| to relocate
|m_selectionInDOMTree| instead of |FrameSelection|. Before this patch
|FrameSelection| relocates |VisibleSelection| with dirty layout tree then
sets |FrameSelection::setSelection()|. To void cyclic reference between
|FrameSelection| and |SelectionEditor|, we could not move relocation part to
|SelectionEditor|.

This patch also updates 
|FrameSelection::updatePostionAfterAdoptingTextNodesMerged()| to handle
|PositonAnchorType|.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2680943004
Cr-Original-Commit-Position: refs/heads/master@{#449928}
Committed:
https://chromium.googlesource.com/chromium/src/+/157413286770a7ac5a24c446a30c...
Review-Url: https://codereview.chromium.org/2680943004
Cr-Commit-Position: refs/heads/master@{#450280}
Committed:
https://chromium.googlesource.com/chromium/src/+/17c84b2b6519c821dc319e79f2a7...
==========

[commit-bot: I haz the power, 2017-02-14 06:35:43]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/17c84b2b6519c821dc319e79f2a7...

[tyoshino (SeeGerritForStatus), 2017-02-14 08:07:59]

It looks this patch caused the following layout tests failures:

fast/dom/delete-contents.html
editing/deleting/delete-br-001.html
external/wpt/selection/collapse-30.html
external/wpt/selection/collapse-00.html
editing/deleting/delete-character-003.html
external/wpt/selection/collapseToStartEnd.html

according to the blame lists of the following builds:

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty...
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build...
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.11/build...
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.11%20(re...
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.9/builds...

[tyoshino (SeeGerritForStatus), 2017-02-14 08:16:14]

A revert of this CL (patchset #9 id:180001) has been created in
https://codereview.chromium.org/2691243002/ by tyoshino@chromium.org.

The reason for reverting is: See
https://codereview.chromium.org/2680943004/#msg70.

[yosin_UTC9, 2017-02-14 09:38:21]

On 2017/02/14 at 08:16:14, tyoshino wrote:
> A revert of this CL (patchset #9 id:180001) has been created in
https://codereview.chromium.org/2691243002/ by tyoshino@chromium.org.
> 
> The reason for reverting is: See
https://codereview.chromium.org/2680943004/#msg70.

Committing again.

Update test expectations to adopt [1] since [1] is committed before PS#9
committed.


[1] http://crrev.com/2694043004 Selection API: anchorNode, anchorOffset,
focusNode, focusOffset, isCollapsed, and type should be based on a registered
Range

[yosin_UTC9, 2017-02-14 09:38:28]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2017-02-14 09:38:29]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yoichio@chromium.org, changwan@chromium.org
Link to the patchset: https://codereview.chromium.org/2680943004/#ps200001
(title: "2017-02-14T18:34:24 Update test expectation for crrev.com/2694043004")

[commit-bot: I haz the power, 2017-02-14 09:38:55]

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

[yosin_UTC9, 2017-02-14 10:09:53]

https://codereview.chromium.org/2680943004/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/selection/collapseToStartEnd-expected.txt
(right):

https://codereview.chromium.org/2680943004/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/selection/collapseToStartEnd-expected.txt:30:
FAIL Range 5 [paras[1].firstChild, 0, paras[1].firstChild, 0] collapseToStart()
assert_equals: focusNode must equal the original start node expected Text node
"Ijklmnop
Before this patch, SelectionEditor::m_cachedRange contains a Range from
Selection#addRange() and SelectionEditor::m_selection holds
Position("Qrstuvwx",0) which is visible canonicalized of Position("Ijklmnop",
0).
Then Selection#collapseToStart() calls setSelection(Position("Qrstuvwx",0),
Position("Qrstuvwx",0)), which equals to Selection::m_selection.

After this patch, Selection::Editor::m_selection holds Position("Ijklmnop", 0)
and which doesn't equal to Position("Qrstuvwx",0).

It seems that we should change DOMSelection#collapseTo{Start,End}() to use
non-canonicalized selection FrameSelection::selectionInDOMTree() instead of
FrameSelection::selection() == computeVisibleSelection().

[yosin_UTC9, 2017-02-14 12:34:47]

The CQ bit was unchecked by yosin@chromium.org

[yosin_UTC9, 2017-02-14 12:38:46]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2017-02-14 12:38:47]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yoichio@chromium.org, changwan@chromium.org
Link to the patchset: https://codereview.chromium.org/2680943004/#ps220001
(title: "2017-02-14T21:37:22 Update test expectation for crrev.com/2694043004
for auto rebase")

[commit-bot: I haz the power, 2017-02-14 12:39:08]

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

[yosin_UTC9, 2017-02-14 14:27:31]

The CQ bit was unchecked by yosin@chromium.org

[yosin_UTC9, 2017-02-14 14:27:42]

Patchset #11 (id:220001) has been deleted

[yosin_UTC9, 2017-02-14 14:27:49]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-02-14 14:28:15]

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

[yosin_UTC9, 2017-02-14 14:29:41]

The CQ bit was unchecked by yosin@chromium.org

[yosin_UTC9, 2017-02-14 14:34:48]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2017-02-14 14:34:48]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yoichio@chromium.org, changwan@chromium.org
Link to the patchset: https://codereview.chromium.org/2680943004/#ps240001
(title: "2014-02-14T23:34:22")

[commit-bot: I haz the power, 2017-02-14 14:35:14]

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-14 15:56:06]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1487082888104510,
"parent_rev": "18375370aff250f59f5ef624ecf686d860377ece", "commit_rev":
"d892f9592860691ae9a782c12260c94ed6bd1a63"}

[commit-bot: I haz the power, 2017-02-14 15:56:44]

Description was changed from

==========
Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

|SelectionEditor| utilizes |SynchronousMutationObserver| to relocate
|m_selectionInDOMTree| instead of |FrameSelection|. Before this patch
|FrameSelection| relocates |VisibleSelection| with dirty layout tree then
sets |FrameSelection::setSelection()|. To void cyclic reference between
|FrameSelection| and |SelectionEditor|, we could not move relocation part to
|SelectionEditor|.

This patch also updates 
|FrameSelection::updatePostionAfterAdoptingTextNodesMerged()| to handle
|PositonAnchorType|.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2680943004
Cr-Original-Commit-Position: refs/heads/master@{#449928}
Committed:
https://chromium.googlesource.com/chromium/src/+/157413286770a7ac5a24c446a30c...
Review-Url: https://codereview.chromium.org/2680943004
Cr-Commit-Position: refs/heads/master@{#450280}
Committed:
https://chromium.googlesource.com/chromium/src/+/17c84b2b6519c821dc319e79f2a7...
==========

to

==========
Make FrameSelection to hold non-canonicalized DOM positions

This patch makes |FrameSelection| to hold non-canonicalized DOM positions in
|SelectionEditor| to align Selection API specification[1] for improving
interoperatbility[2].

Before this patch we holds selection as |VisibleSelection| as canonicalized
DOM positions. This behavior is not align with Selection API specification[1]
then the most complained issue of Blink from editing-tf@w3c.

The heart of this patch is holding selection as |SelectionInDOMTree| and
compute |VisibleSelection| on-demand with cache of computed |VisibleSelection|.

|VisibleSelection| cache is invalidate each DOM tree change and style change
since canonicalization referes CSS style properties, e.g display, visibility,
-webkit-user-modify, etc, and layout dimension.

|SelectionEditor| utilizes |SynchronousMutationObserver| to relocate
|m_selectionInDOMTree| instead of |FrameSelection|. Before this patch
|FrameSelection| relocates |VisibleSelection| with dirty layout tree then
sets |FrameSelection::setSelection()|. To void cyclic reference between
|FrameSelection| and |SelectionEditor|, we could not move relocation part to
|SelectionEditor|.

This patch also updates 
|FrameSelection::updatePostionAfterAdoptingTextNodesMerged()| to handle
|PositonAnchorType|.

# Highlight of changes
## FrameCaret
- Compute caret position after "layout clean" rather than each selection change
to align rendering pipeline.

## CharacterData
Changes timing of notifying character data update for ease of relocation of
positions.

## FrameSelection
- Move |m_isHandleVisible| to |SelectionInDOMTree| as follow-up of [5].
- Move selection relocation to |SelectionEditor|; following patch will move
implementations to "SelectionEditor.cpp"

## SelecitonEdtior
- Make it to hold |SelectionInDOMTree| with relocation at DOM mutation.
- Caching |VisibleSelection|

# Brief description of test expectation changes:
## ImeTest.java:
This patch gets rid of redundant selection change event from
 - |testImePaste|,
 - |testContentEditableEvents_DeleteSurroundingText|
 - |testInputTextEvents_DeleteSurroundingText|

## LayoutTests
Before this patch, Blink uses |VisibleSelection| when it sets even if style and
layout changed. This is wrong and unexpected behavior since positions in
|VisibleSelection| can no longer be canonicalized positions. This patch changes
this behavior to return "sane" canonicalized positions with clean style and
layout tree.

This patch is the result of many attempts. Previous changes can be found in
[3][4].

[1] https://www.w3.org/TR/selection-api/ W3C Selection API
[2] https://goo.gl/9v1zOK Improving Interoperatbility of Selection
[3] http://crrev.com/1958093002
[4] http://crrev.com/2637013002
[5] http://crrev.com/2651803007 Added isHandleVisible to |SelectionTemplate|

BUG=139552, 603684, 605499, 606499, 625533, 644648, 679991
TEST=See changes in this patch
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2680943004
Cr-Original-Original-Commit-Position: refs/heads/master@{#449928}
Committed:
https://chromium.googlesource.com/chromium/src/+/157413286770a7ac5a24c446a30c...
Review-Url: https://codereview.chromium.org/2680943004
Cr-Original-Commit-Position: refs/heads/master@{#450280}
Committed:
https://chromium.googlesource.com/chromium/src/+/17c84b2b6519c821dc319e79f2a7...
Review-Url: https://codereview.chromium.org/2680943004
Cr-Commit-Position: refs/heads/master@{#450370}
Committed:
https://chromium.googlesource.com/chromium/src/+/d892f9592860691ae9a782c12260...
==========

[commit-bot: I haz the power, 2017-02-14 15:56:46]

Committed patchset #11 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/d892f9592860691ae9a782c12260...