New issue
Advanced search Search tips

Issue 648949 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Task

Blocked on:
issue 749442

Blocking:
issue 657237
issue 647219



Sign in to add a comment

VisiblePositions should be used only when valid

Project Member Reported by xiaoche...@chromium.org, Sep 21 2016

Issue description

VisiblePositions are short-living objects that are created with clean layout and invalidated as soon as there is a DOM or style change.

Unfortunately, there are many clients that store a VisiblePosition, do some mutation and then inspect the VisiblePosition's property. We should stop doing that.
 
Blockedon: 650164
VisibleUnits' |inSameBlock()| is called with invalid VisiblePosition. However, the invalidity is due to Element::cloneElementWithoutChildren() increasing the DOM tree version, which is a bug.
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 27 2016

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

commit 58d3e91f04c7eb7847dade2b3f427da26dfd7379
Author: xiaochengh <xiaochengh@chromium.org>
Date: Tue Sep 27 01:04:32 2016

Mark paragraph-related functions deprecated in VisibleUnits

This patch marks the following functions in VisibleUnits.cpp deprecated
because they accept invalid VisiblePositions as input:
- startOfParagraph
- endOfParagraph
- isStartOfParagraph
- isEndOfParagraph
- inSameParagraph
- startOfNextParagraph

This patch also creates the non-deprecated versions of these functions
that requires valid VisiblePositions as input.

Calls of these functions in editing/commands/ are switched to the deprecated
functions. Calls in other places have ensured valid VisiblePositions as input,
and hence, are calling the non-deprecated functions after this patch.

BUG=648949

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

[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/VisibleUnits.h
[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/commands/ApplyBlockElementCommand.cpp
[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/commands/ApplyStyleCommand.cpp
[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/commands/FormatBlockCommand.cpp
[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/commands/IndentOutdentCommand.cpp
[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/commands/InsertLineBreakCommand.cpp
[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp
[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/commands/InsertParagraphSeparatorCommand.cpp
[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/commands/InsertTextCommand.cpp
[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp
[modify] https://crrev.com/58d3e91f04c7eb7847dade2b3f427da26dfd7379/third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 27 2016

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

commit 1985713ab7e640a5d5de02a579e10aed85eae34a
Author: xiaochengh <xiaochengh@chromium.org>
Date: Tue Sep 27 01:08:02 2016

Add VisiblePosition validity checks to VisibleUnits.cpp

BUG=648949

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

[modify] https://crrev.com/1985713ab7e640a5d5de02a579e10aed85eae34a/third_party/WebKit/Source/core/editing/VisibleUnits.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 27 2016

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

commit 172391bd511c3871ff8c7bcd73fa9cb5c1d12043
Author: xiaochengh <xiaochengh@chromium.org>
Date: Tue Sep 27 07:37:52 2016

Ensure valid VisiblePosition input for inSameBlock

This patch postpones the creation of |visiblePos| in
|InsertParagraphSeparatorCommand::doApply|  so that |visiblePos|
does not get invalidated by the creation of |blockToInsert|.

Note: ideally, the creation and modification of an out-of-document
element (in this case, |blockToInsert|) shouldn't invalidate
VisiblePositions, but this is still hard to fix in the current
implementation. See  crbug.com/650164 .

BUG=648949

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

[modify] https://crrev.com/172391bd511c3871ff8c7bcd73fa9cb5c1d12043/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
[modify] https://crrev.com/172391bd511c3871ff8c7bcd73fa9cb5c1d12043/third_party/WebKit/Source/core/editing/commands/InsertParagraphSeparatorCommand.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 27 2016

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

commit 7b0d88beaef6c050b97d2aacd3310be4ef6fd4c0
Author: xiaochengh <xiaochengh@chromium.org>
Date: Tue Sep 27 10:45:03 2016

Ensure valid VisiblePosition input for character{Before,After}

This patch also drive-by eliminates a |createVisiblePositionDeprecated|
in |CompositeEditCommand::prepareWhitespaceAtPositionForSplit|.

BUG=648949

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

[modify] https://crrev.com/7b0d88beaef6c050b97d2aacd3310be4ef6fd4c0/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
[modify] https://crrev.com/7b0d88beaef6c050b97d2aacd3310be4ef6fd4c0/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
[modify] https://crrev.com/7b0d88beaef6c050b97d2aacd3310be4ef6fd4c0/third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp

Blockedon: -650164
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 28 2016

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

commit 4ebc66c03af35f9294189171d84ebf7c4ba348a3
Author: xiaochengh <xiaochengh@chromium.org>
Date: Wed Sep 28 01:20:09 2016

Mark calls of {previous,next}PositionOf with invalid VisiblePosition deprecated

This patch marks functions {previous,next}PositionOf in VisibleUnits.cpp
deprecated because they take invalid VisiblePositions as input. This patch
also creates the non-deprecated versions of these functions that requires
valid VisiblePositions as input.

Callers passing invalid VisiblePositions have been switched to using the
deprecated functions. Calls in other places have ensured valid VisiblePositions
as input, and hence, are calling the non-deprecated functions after this patch.

BUG=648949

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

[modify] https://crrev.com/4ebc66c03af35f9294189171d84ebf7c4ba348a3/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
[modify] https://crrev.com/4ebc66c03af35f9294189171d84ebf7c4ba348a3/third_party/WebKit/Source/core/editing/VisibleUnits.h
[modify] https://crrev.com/4ebc66c03af35f9294189171d84ebf7c4ba348a3/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
[modify] https://crrev.com/4ebc66c03af35f9294189171d84ebf7c4ba348a3/third_party/WebKit/Source/core/editing/commands/IndentOutdentCommand.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 3 2016

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

commit 0895689a9cf3613ea07cd1a05ce4c6d32d6d411c
Author: xiaochengh <xiaochengh@chromium.org>
Date: Mon Oct 03 09:32:07 2016

Make DragCaretController store PositionWithAffinity instead of VisiblePosition

DragCaretController is an on-stack object that stores a VisiblePosition and may
inspect properties of the VisiblePosition even after it is invalidated. This
patch fixes it by making DragCaretController store a PositionWithAffinity
instead.

This patch is a preparation of pruning createVisibleSelectionDeprecated from
DragController: https://codereview.chromium.org/2386083002

BUG=648949, 651373 

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

[modify] https://crrev.com/0895689a9cf3613ea07cd1a05ce4c6d32d6d411c/third_party/WebKit/Source/core/editing/DragCaretController.cpp
[modify] https://crrev.com/0895689a9cf3613ea07cd1a05ce4c6d32d6d411c/third_party/WebKit/Source/core/editing/DragCaretController.h

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 7 2016

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

commit cf43a0661cc568c57f073904d76b989b363b23ea
Author: xiaochengh <xiaochengh@chromium.org>
Date: Fri Oct 07 05:36:48 2016

Prune deprecated functions from TypingCommand

BUG=648949, 651373 

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

[modify] https://crrev.com/cf43a0661cc568c57f073904d76b989b363b23ea/third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 7 2016

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

commit d1947c7fd01731ba28e72137e99d9cf78c7ab0b6
Author: xiaochengh <xiaochengh@chromium.org>
Date: Fri Oct 07 09:19:23 2016

Prune deprecated functions from InsertListCommand

BUG= 647219 ,648949, 651373 

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

[modify] https://crrev.com/d1947c7fd01731ba28e72137e99d9cf78c7ab0b6/third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 11 2016

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

commit 498e9e19dffeb563c02167e40da7a4424eaba02e
Author: xiaochengh <xiaochengh@chromium.org>
Date: Tue Oct 11 08:32:06 2016

Prune deprecated functions from InsertTextCommand

BUG= 647219 ,648949, 651373 

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

[modify] https://crrev.com/498e9e19dffeb563c02167e40da7a4424eaba02e/third_party/WebKit/Source/core/editing/commands/InsertTextCommand.cpp

Project Member

Comment 13 by bugdroid1@chromium.org, Oct 12 2016

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

commit 7850cba844bb06053c9d6d553700240d15978af4
Author: xiaochengh <xiaochengh@chromium.org>
Date: Wed Oct 12 05:01:16 2016

Recalculate |visiblePos| for InsertParagraphSeparatorCommand::doApply() after mutations

This patch does the following change to the above mentioned function that,
|visiblePos| is always recalculated as |createVisiblePosition(insertionPosition)|
before being used, so that it is always valid at those places.

BUG=648949

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

[modify] https://crrev.com/7850cba844bb06053c9d6d553700240d15978af4/third_party/WebKit/Source/core/editing/commands/InsertParagraphSeparatorCommand.cpp

Currently, there is a lot of usages of invalidated VisiblePositions in editing/commands. To get rid of them:

1. EditCommand should stop storing VisibleSelections as its starting & ending selection. Instead, it should store a somehow lightweight class like SelectionInDOMTree

2. Currently EditCommands (and also a bunch of functions in VisibleUnits) are abusing VisiblePositions. Actually, they only require canonical positions (probably with affinity) as input. We should make them pass canonical positions instead of VPs.

3. Eliminate all VPs stored on heap as they will definitely become invalidated over time.

4. All stack allocated VPs should be stored as const references. New VPs should be created after DOM/style changes.
Project Member

Comment 16 by bugdroid1@chromium.org, Oct 12 2016

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

commit c7a2dfb0e4118e118fdbd2bd566aa3fa77a7d665
Author: xiaochengh <xiaochengh@chromium.org>
Date: Wed Oct 12 14:03:10 2016

Prune deprecated functions from VisibleSelection and VisibleUnits

This patch prunes all deprecated functions from VisibleSelection and
VisibleUnits because they have no callers any more.

The following functions are removed:
- VisibleSelection::visible{Start,End}Deprecated
- createVisibleSelectionDeprecated (all overloads)
- paragraph-related deprecated functions (in VisibleUnits)
- {previous,next}PositionOf (in VisibleUnits)

BUG= 647219 ,648949, 651373 

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

[modify] https://crrev.com/c7a2dfb0e4118e118fdbd2bd566aa3fa77a7d665/third_party/WebKit/Source/core/editing/VisibleSelection.cpp
[modify] https://crrev.com/c7a2dfb0e4118e118fdbd2bd566aa3fa77a7d665/third_party/WebKit/Source/core/editing/VisibleSelection.h
[modify] https://crrev.com/c7a2dfb0e4118e118fdbd2bd566aa3fa77a7d665/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
[modify] https://crrev.com/c7a2dfb0e4118e118fdbd2bd566aa3fa77a7d665/third_party/WebKit/Source/core/editing/VisibleUnits.h

Blocking: 657237
Project Member

Comment 18 by bugdroid1@chromium.org, Oct 26 2016

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

commit d7c76ec65a164e8b0d1bcede4f6763026ce29042
Author: yosin <yosin@chromium.org>
Date: Wed Oct 26 04:04:48 2016

Introduce EditCommand::setEndingSelection() taking SelectionInDOMTree

This patch introduces |SelectionInDOMTree| version of |setEndingSelection()| in
|EditCommand| class as a preparation of getting rid of overloads and update
layout calls for improving code health.

Following patch will introduce SelectionInUndoStep which is intended for
restore selection after DOM tree mutation at undo/redo time.

BUG=648949,  646323 
TEST=n/a; no behavior changes

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

[modify] https://crrev.com/d7c76ec65a164e8b0d1bcede4f6763026ce29042/third_party/WebKit/Source/core/editing/commands/EditCommand.cpp
[modify] https://crrev.com/d7c76ec65a164e8b0d1bcede4f6763026ce29042/third_party/WebKit/Source/core/editing/commands/EditCommand.h

Labels: -Type-Bug Type-Task
Owner: ----
Status: Available (was: Assigned)
Blockedon: 749442
Labels: Pri-3
Project Member

Comment 22 by sheriffbot@chromium.org, Oct 4

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Available (was: Untriaged)

Sign in to add a comment