New issue
Advanced search Search tips

Issue 694413 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 590369
issue 679991



Sign in to add a comment

mostForwardCaretPosition() and mostBackwardCaretPosition() should have DCHECK of clean layout

Project Member Reported by yosin@chromium.org, Feb 21 2017

Issue description

We should have DCHECK of clean layout in nmostForwardCaretPosition() and mostBackwardCaretPosition() since these functions uses layout tree.

Note: Before patch[1], we called them during removing node then we could not have 
DCHECK clean layout tree.

There are some call sites calling them with dirty layout tree revealed by
patch[2]:

Forward
- editing/style/make-text-writing-direction-inline-win.html
- editing/input/div-first-child-rule-input.html
- editing/style/make-text-writing-direction-inline-mac.html
- fast/forms/8250.html

Backward
- editing/input/div-first-child-rule-textarea.html
- editing/inserting/insert_html_as_plain_text.html
- editing/pasteboard/drop-text-events.html



[1] http://crrev.com/2680943004 Make FrameSelection to hold non-canonicalized positions
[2] http://crrev.com/2703323002: Add assertion of layout tree clean in mostBackwardCaretPosition/mostForwardCaretPosition()
 

Comment 1 by yosin@chromium.org, Feb 21 2017

Blocking: 590369
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/52407bff3797a14717d3bd8fcd5c0e3a56489640

commit 52407bff3797a14717d3bd8fcd5c0e3a56489640
Author: yosin <yosin@chromium.org>
Date: Wed Feb 22 01:22:03 2017

Add clean layout DCHECK to mostBackwardCaretPosition()

This patch adds clean layout |DCHECK()| to |mostBackwardCaretPosition()|, and
fixes call sites calling it with dirty layout tree, to make sure we don't call
it with dirty layout tree.

We could not have this |DCHECK()| since we call |mostBackwardCaretPosition()|
during node removal, but patch[1] made not to do so.

This patch is similar to patch[1], which is for |mostForwardCaretPosition()|.

This patch also makes |VisibleSelection::toNormalizedEphemeralRange()| to
use |needsLayoutTreeUpdate()| since |Document::needsLayoutTreeUpdate()| returns
false for "editing/inserting/insert_html_as_plain_text.html".

[1] http://crrev.com/2680943004 Make FrameSelection to hold non-canonicalized positions
[2] http://crrev.com/2706163002 Add clean layout DCHECK to mostForwardCaretPosition()

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

Review-Url: https://codereview.chromium.org/2705113003
Cr-Commit-Position: refs/heads/master@{#451869}

[modify] https://crrev.com/52407bff3797a14717d3bd8fcd5c0e3a56489640/third_party/WebKit/Source/core/editing/VisibleSelection.cpp
[modify] https://crrev.com/52407bff3797a14717d3bd8fcd5c0e3a56489640/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
[modify] https://crrev.com/52407bff3797a14717d3bd8fcd5c0e3a56489640/third_party/WebKit/Source/core/editing/commands/ApplyStyleCommand.cpp
[modify] https://crrev.com/52407bff3797a14717d3bd8fcd5c0e3a56489640/third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp

Project Member

Comment 3 by bugdroid1@chromium.org, Feb 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/22a030be0c75c7a39b938d51d04bcc259bf9fe17

commit 22a030be0c75c7a39b938d51d04bcc259bf9fe17
Author: yosin <yosin@chromium.org>
Date: Wed Feb 22 01:30:02 2017

Add clean layout DCHECK to mostForwardCaretPosition()

This patch adds clean layout |DCHECK()| to |mostForwardCaretPosition()|, and
fixes call sites calling it with dirty layout tree, to make sure we don't call
it with dirty layout tree.

We could not have this |DCHECK()| since we call |mostForwardCaretPosition()|
during node removal, but patch[1] made not to do so.

This patch is similar to patch[2], which is for |mostBackwardCaretPosition()|.

[1] http://crrev.com/2680943004 Make FrameSelection to hold non-canonicalized positions
[2] http://crrev.com/2705113003 Add clean layout DCHECK to mostBackwardCaretPosition()

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

Review-Url: https://codereview.chromium.org/2706163002
Cr-Commit-Position: refs/heads/master@{#451874}

[modify] https://crrev.com/22a030be0c75c7a39b938d51d04bcc259bf9fe17/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
[modify] https://crrev.com/22a030be0c75c7a39b938d51d04bcc259bf9fe17/third_party/WebKit/Source/core/editing/commands/ApplyStyleCommand.cpp
[modify] https://crrev.com/22a030be0c75c7a39b938d51d04bcc259bf9fe17/third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp

Comment 4 by yosin@chromium.org, Feb 22 2017

Status: Fixed (was: Available)

Comment 5 by yosin@chromium.org, Feb 22 2017

Blocking: 679991

Sign in to add a comment