Prune layout update calls from Editor::*appliedEditing

Prune layout update calls from Editor::*appliedEditing

Editor used to update layout after applying commands for setting
frame selection. Now that setting frame selection no longer requires
clean layout, this patch removes these layout update calls.

BUG=590369
TEST=n/a; no behavior change

Review-Url: https://codereview.chromium.org/2729313002
Cr-Commit-Position: refs/heads/master@{#467890}
Committed: https://chromium.googlesource.com/chromium/src/+/63b1a036006ef1feadebfdcfd67159972deabdfa

[Xiaocheng, 2017-03-04 00:53:44]

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

[commit-bot: I haz the power, 2017-03-04 00:54:05]

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-03-04 01:03:23]

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

[commit-bot: I haz the power, 2017-03-04 01:03:24]

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

[Xiaocheng, 2017-03-04 02:26:27]

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

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

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-03-04 03:33:38]

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

[commit-bot: I haz the power, 2017-03-04 03:33:39]

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_...)

[Xiaocheng, 2017-03-06 06:42:37]

Description was changed from

==========
Prune layout update from Editor::*appliedEditing

BUG=590369
==========

to

==========
WIP Prune layout update from Editor::*appliedEditing

BUG=590369
==========

[Xiaocheng, 2017-03-17 20:20:38]

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

[commit-bot: I haz the power, 2017-03-17 20:21:08]

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-03-17 21:48:00]

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

[commit-bot: I haz the power, 2017-03-17 21:48:02]

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

[Xiaocheng, 2017-03-18 02:06:47]

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

[commit-bot: I haz the power, 2017-03-18 02:07:24]

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-03-18 04:30:26]

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

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

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-03-20 22:18:52]

Description was changed from

==========
WIP Prune layout update from Editor::*appliedEditing

BUG=590369
==========

to

==========
Prune layout update calls from Editor::*appliedEditing

Editor used to update layout after applying commands for setting
frame selection. Now that setting frame selection no longer requires
clean layout, this patch removes these layout update calls.

BUG=590369
TEST=n/a; no behavior change
==========

[Xiaocheng, 2017-03-20 22:22:32]

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

[Xiaocheng, 2017-03-20 22:22:32]

xiaochengh@chromium.org changed required reviewers:
+ dmazzoni@chromium.org, yosin@chromium.org

[Xiaocheng, 2017-03-20 22:22:33]

PTAL.

dmazzoni@: This patch passes a difference Position to
Editor::respondToChangedContent(), which is then passed to AX. Could you help me
check if such change is fine? Thanks!

[Xiaocheng, 2017-03-21 03:49:42]

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

[Xiaocheng, 2017-03-21 03:49:42]

xiaochengh@chromium.org changed required reviewers:
- yosin@chromium.org

[Xiaocheng, 2017-03-21 03:49:42]

+tkent, yoichio

[yoichio, 2017-03-21 06:41:57]

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

https://codereview.chromium.org/2729313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.cpp:887: const VisibleSelection&
passedSelection,
Could you use SelectionINDOMTree& passedSelection instead of VS?

[dmazzoni, 2017-03-21 16:09:38]

On 2017/03/20 22:22:33, Xiaocheng wrote:
> PTAL.
> 
> dmazzoni@: This patch passes a difference Position to
> Editor::respondToChangedContent(), which is then passed to AX. Could you help
me
> check if such change is fine? Thanks!

lgtm

Thanks for checking. Calling it on the selection base() instead of start() is
fine,
the AX code will still work.

[Xiaocheng, 2017-03-21 20:53:40]

Thanks for the reviews!

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

https://codereview.chromium.org/2729313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.cpp:887: const VisibleSelection&
passedSelection,
On 2017/03/21 at 06:41:57, yoichio wrote:
> Could you use SelectionINDOMTree& passedSelection instead of VS?

It doesn't seem to introduce any benefit, since EditCommand::endingSelection()
is a VS.

Anyway it's not important. I can change if you insist.

[yoichio, 2017-03-22 01:48:57]

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

https://codereview.chromium.org/2729313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.cpp:887: const VisibleSelection&
passedSelection,
Yes, no benefit from a view of semantics.
However, we're going to reduce VS usage in code.
Eventually EditCommand::endingSelection() will turn Selection.

[Xiaocheng, 2017-03-22 02:12:44]

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

https://codereview.chromium.org/2729313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.cpp:887: const VisibleSelection&
passedSelection,
On 2017/03/22 at 01:48:57, yoichio wrote:
> Yes, no benefit from a view of semantics.
> However, we're going to reduce VS usage in code.
> Eventually EditCommand::endingSelection() will turn Selection.

Sorry, I just realized that we can't change it in this patch.

VisibleSelection::asSelection() will hit DCHECK if the VS is ill-formed, which
means we need to check validity of the VS before calling VS::asSelection(). This
function does exactly the job.

[Xiaocheng, 2017-03-22 22:35:35]

Description was changed from

==========
Prune layout update calls from Editor::*appliedEditing

Editor used to update layout after applying commands for setting
frame selection. Now that setting frame selection no longer requires
clean layout, this patch removes these layout update calls.

BUG=590369
TEST=n/a; no behavior change
==========

to

==========
BLOCKED Prune layout update calls from Editor::*appliedEditing

Editor used to update layout after applying commands for setting
frame selection. Now that setting frame selection no longer requires
clean layout, this patch removes these layout update calls.

BUG=590369
TEST=n/a; no behavior change
==========

[Xiaocheng, 2017-03-22 22:36:40]

Blocked by crbug.com/702756, which I don't have a good idea to fix right now...

[yosin_UTC9, 2017-03-23 06:00:50]

On 2017/03/22 at 22:36:40, xiaochengh wrote:
> Blocked by crbug.com/702756, which I don't have a good idea to fix right
now...

I think this patch isn't blocked by crbug.com/702756, which attempts to fix
false assumption == call canonicalizaedPositionOf() with immediate child of
shadow host.

I propose to change shouldTraverseChild<EditingStrategy> == DOM version, to
check shadow host, e.g.

template <>
bool shouldTraverseChildren<EditingStarteg>() {
   if (node is shadow host)
    return false;
   ...
}

As of intention of this function, inferred from its name, it is sane to checking
shadow host.
(and display:none).

Since this patch is important, I would like to proceed.

[Xiaocheng, 2017-03-23 19:37:27]

On 2017/03/23 at 06:00:50, yosin wrote:
> On 2017/03/22 at 22:36:40, xiaochengh wrote:
> > Blocked by crbug.com/702756, which I don't have a good idea to fix right
now...
> 
> I think this patch isn't blocked by crbug.com/702756, which attempts to fix
> false assumption == call canonicalizaedPositionOf() with immediate child of
shadow host.
> 
> I propose to change shouldTraverseChild<EditingStrategy> == DOM version, to
check shadow host, e.g.
> 
> template <>
> bool shouldTraverseChildren<EditingStarteg>() {
>    if (node is shadow host)
>     return false;
>    ...
> }
> 
> As of intention of this function, inferred from its name, it is sane to
checking shadow host.
> (and display:none).
> 
> Since this patch is important, I would like to proceed.

The change to shouldTraverseChildren is reasonable, but it doesn't seem to fix
the issue.

The crash site is in mostBackwardCaretPosition, before using any
PositionIterator.

[yosin_UTC9, 2017-03-25 09:00:59]

On 2017/03/23 at 19:37:27, xiaochengh wrote:
> On 2017/03/23 at 06:00:50, yosin wrote:
> > On 2017/03/22 at 22:36:40, xiaochengh wrote:
> > > Blocked by crbug.com/702756, which I don't have a good idea to fix right
now...
> > 
> > I think this patch isn't blocked by crbug.com/702756, which attempts to fix
> > false assumption == call canonicalizaedPositionOf() with immediate child of
shadow host.
> > 
> > I propose to change shouldTraverseChild<EditingStrategy> == DOM version, to
check shadow host, e.g.
> > 
> > template <>
> > bool shouldTraverseChildren<EditingStarteg>() {
> >    if (node is shadow host)
> >     return false;
> >    ...
> > }
> > 
> > As of intention of this function, inferred from its name, it is sane to
checking shadow host.
> > (and display:none).
> > 
> > Since this patch is important, I would like to proceed.
> 
> The change to shouldTraverseChildren is reasonable, but it doesn't seem to fix
the issue.
> 
> The crash site is in mostBackwardCaretPosition, before using any
PositionIterator.

Do you think once bug[1] is fixed, can we proceed this patch?

It makes adjustPositionForBackwardIteration() to return nullptr for
PositionInFlatTree::afterNode(node) where node is not in flat tree.


[1] http://crbug.com/703052 PositonInFlatTree::toIffsetInAnchor() should handle
the case computeContainerNode() == nullptr

[yosin_UTC9, 2017-03-27 07:28:33]

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

[commit-bot: I haz the power, 2017-03-27 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...

[yosin_UTC9, 2017-03-27 07:43:36]

PS#3 failed with editing/undo/redo_correct_selection.html with crash in
PositionInFlatTree::ctor(null, 1)
PositionInFlatTree::afterAnchor(SELECT).toOffsetInAnchor()
...
SelectionEditor::updateCachedVisibleSelectionIfNeeded()

with following DOM Tree:
BODY
	DIV (editable) (focused)
		B (editable)
			#text "012"
	OPTION
		#shadow-root
*		SELECT
*			#shadow-root
*				CONTENT
afterAnchor

The test move SELECT from DIV to OPTION, this makes SELECT element no longer
participate in flat tree.

So, we should change toPositionInFlatTree() to handle this case.

[commit-bot: I haz the power, 2017-03-27 08:38:07]

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

[commit-bot: I haz the power, 2017-03-27 08:38:08]

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_...)

[yosin_UTC9, 2017-03-27 09:29:42]

On 2017/03/27 at 07:43:36, yosin_UTC9 wrote:
> PS#3 failed with editing/undo/redo_correct_selection.html with crash in
> PositionInFlatTree::ctor(null, 1)
> PositionInFlatTree::afterAnchor(SELECT).toOffsetInAnchor()
> ...
> SelectionEditor::updateCachedVisibleSelectionIfNeeded()
> 
> with following DOM Tree:
> BODY
> 	DIV (editable) (focused)
> 		B (editable)
> 			#text "012"
> 	OPTION
> 		#shadow-root
> *		SELECT
> *			#shadow-root
> *				CONTENT
> afterAnchor
> 
> The test move SELECT from DIV to OPTION, this makes SELECT element no longer
> participate in flat tree.
> 
> So, we should change toPositionInFlatTree() to handle this case.

Please see patch[1], which makes we don't have PositionInFlatTree not in flat
tree.

[1] http://crrev.com/2772233002: Make toPositionInFlatTree() to return null for
DOM position not in flat tree

[Xiaocheng, 2017-04-27 14:51:18]

Description was changed from

==========
BLOCKED Prune layout update calls from Editor::*appliedEditing

Editor used to update layout after applying commands for setting
frame selection. Now that setting frame selection no longer requires
clean layout, this patch removes these layout update calls.

BUG=590369
TEST=n/a; no behavior change
==========

to

==========
Prune layout update calls from Editor::*appliedEditing

Editor used to update layout after applying commands for setting
frame selection. Now that setting frame selection no longer requires
clean layout, this patch removes these layout update calls.

BUG=590369
TEST=n/a; no behavior change
==========

[Xiaocheng, 2017-04-27 14:52:34]

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

[commit-bot: I haz the power, 2017-04-27 14:53:15]

Dry run: CQ is trying da patch.

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

[Xiaocheng, 2017-04-27 16:06:45]

Now that the blocking issue is gone, PTAL.

[commit-bot: I haz the power, 2017-04-27 16:34:13]

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

[commit-bot: I haz the power, 2017-04-27 16:34:14]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-04-28 04:14:22]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2017-04-28 04:14:23]

lgtm

[yosin_UTC9, 2017-04-28 04:14:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2729313002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-28 04:14:54]

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-04-28 05:15:59]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493352862662330,
"parent_rev": "ea02813b6067f1ff6b52cb59f03abeed96676214", "commit_rev":
"63b1a036006ef1feadebfdcfd67159972deabdfa"}

[commit-bot: I haz the power, 2017-04-28 05:16:29]

Description was changed from

==========
Prune layout update calls from Editor::*appliedEditing

Editor used to update layout after applying commands for setting
frame selection. Now that setting frame selection no longer requires
clean layout, this patch removes these layout update calls.

BUG=590369
TEST=n/a; no behavior change
==========

to

==========
Prune layout update calls from Editor::*appliedEditing

Editor used to update layout after applying commands for setting
frame selection. Now that setting frame selection no longer requires
clean layout, this patch removes these layout update calls.

BUG=590369
TEST=n/a; no behavior change

Review-Url: https://codereview.chromium.org/2729313002
Cr-Commit-Position: refs/heads/master@{#467890}
Committed:
https://chromium.googlesource.com/chromium/src/+/63b1a036006ef1feadebfdcfd671...
==========

[commit-bot: I haz the power, 2017-04-28 05:16:30]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/63b1a036006ef1feadebfdcfd671...