Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(53)

Issue 2698793003: Get rid of redundant layout tree update related to selection (Closed)

Created:
3 years, 10 months ago by yosin_UTC9
Modified:
3 years, 10 months ago
Reviewers:
tkent, yoichio
CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Get rid of redundant layout tree update related to selection This patch gets rid of redundant layout tree update regarding selection getter to recover execution speed in Speedometer performance test. Since CL[1], |FrameSelection| getters updates layout tree on-demand rather than invalidated values, e.g. |VisibleSelection| can hold invalid positions after style and DOM tree modification. However, following getters in |FrameSeleciton| don't need to clean layout tree: - affinity() - isNone() for |TextControl| - isRange() for |TextControl| - start()/end() for |TextControl| - checking editability base on selection start position; it is enough to check one of base or extent is ediable since selection can not cross editing boundary. [1] crrev.com/2680943004 Make FrameSelection to hold non-canonicalized positions BUG=692537 TEST=Speedometer performance test Review-Url: https://codereview.chromium.org/2698793003 Cr-Commit-Position: refs/heads/master@{#451270} Committed: https://chromium.googlesource.com/chromium/src/+/e8a65d35c402aa21c4ce9b07a63a109d113a6066

Patch Set 1 #

Patch Set 2 : 2017-02-16T14:57:02 #

Patch Set 3 : 2017-02-16T16:34:45 #

Patch Set 4 : 2017-02-16T17:15:58 #

Total comments: 1

Patch Set 5 : 2017-02-16T17:58:29 #

Patch Set 6 : 2017-02-16T18:19:51 Use start/end for word granularity for text control #

Patch Set 7 : 2017-02-16T19:48:39 #

Patch Set 8 : 2017-02-16T22:09:34 #

Patch Set 9 : 2017-02-17T00:37:54 FrameSelection::selectFrameElementInParentIfFullySelected() to use FS::isRange() #

Total comments: 11

Patch Set 10 : 2017-02-17T14:11:19 CANCEL DisallowTransition #

Patch Set 11 : 2017-02-17T14:18:09 DisallowTransition #

Total comments: 1

Patch Set 12 : 2017-02-17T14:28:58 #

Total comments: 2

Patch Set 13 : 2017-02-17T16:13:56 selectionTypeWithLegacyGranularity() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+148 lines, -40 lines) Patch
M third_party/WebKit/Source/core/editing/DOMSelection.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/editing/Editor.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/FrameSelection.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/FrameSelection.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +17 lines, -14 lines 0 comments Download
M third_party/WebKit/Source/core/editing/SelectionController.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +19 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/editing/SelectionTemplate.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +14 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/SelectionTemplate.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +26 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +11 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/html/TextControlElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +52 lines, -13 lines 0 comments Download

Messages

Total messages: 64 (49 generated)
yosin_UTC9
PTAL https://codereview.chromium.org/2698793003/diff/60001/third_party/WebKit/Source/core/editing/DOMSelection.cpp File third_party/WebKit/Source/core/editing/DOMSelection.cpp (left): https://codereview.chromium.org/2698793003/diff/60001/third_party/WebKit/Source/core/editing/DOMSelection.cpp#oldcode612 third_party/WebKit/Source/core/editing/DOMSelection.cpp:612: if (frame()->document() != n->document() || selection.isNone()) FrameSeleciton::isNone() requires ...
3 years, 10 months ago (2017-02-16 09:50:26 UTC) #18
yoichio
On 2017/02/16 09:50:26, yosin_UTC9 wrote: > PTAL > > https://codereview.chromium.org/2698793003/diff/60001/third_party/WebKit/Source/core/editing/DOMSelection.cpp > File third_party/WebKit/Source/core/editing/DOMSelection.cpp (left): > ...
3 years, 10 months ago (2017-02-17 02:05:13 UTC) #35
yoichio
> Since CL[1], |FrameSelection| getters updates layout tree on-demand rather than > invalidated values, e.g. ...
3 years, 10 months ago (2017-02-17 02:15:01 UTC) #36
yoichio
https://codereview.chromium.org/2698793003/diff/160001/third_party/WebKit/Source/core/editing/SelectionTemplate.cpp File third_party/WebKit/Source/core/editing/SelectionTemplate.cpp (right): https://codereview.chromium.org/2698793003/diff/160001/third_party/WebKit/Source/core/editing/SelectionTemplate.cpp#newcode137 third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:137: SelectionTemplate<Strategy>::computeEndPosition() const { Could you rename just endPosition because ...
3 years, 10 months ago (2017-02-17 02:26:12 UTC) #37
yosin_UTC9
On 2017/02/17 at 02:15:01, yoichio wrote: > > Since CL[1], |FrameSelection| getters updates layout tree ...
3 years, 10 months ago (2017-02-17 02:27:40 UTC) #38
yoichio
https://codereview.chromium.org/2698793003/diff/160001/third_party/WebKit/Source/core/html/TextControlElement.cpp File third_party/WebKit/Source/core/html/TextControlElement.cpp (right): https://codereview.chromium.org/2698793003/diff/160001/third_party/WebKit/Source/core/html/TextControlElement.cpp#newcode403 third_party/WebKit/Source/core/html/TextControlElement.cpp:403: TextFieldSelectionDirection direction) { As offline discussion, we put DocumentLifecycle::DisallowTransitionScope ...
3 years, 10 months ago (2017-02-17 04:18:59 UTC) #41
yoichio
https://codereview.chromium.org/2698793003/diff/160001/third_party/WebKit/Source/core/editing/Editor.cpp File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2698793003/diff/160001/third_party/WebKit/Source/core/editing/Editor.cpp#newcode1627 third_party/WebKit/Source/core/editing/Editor.cpp:1627: if (selection.isNone()) As commented, we should rename to computeIsNone() ...
3 years, 10 months ago (2017-02-17 04:35:16 UTC) #42
yosin_UTC9
PTAL https://codereview.chromium.org/2698793003/diff/160001/third_party/WebKit/Source/core/editing/Editor.cpp File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2698793003/diff/160001/third_party/WebKit/Source/core/editing/Editor.cpp#newcode1627 third_party/WebKit/Source/core/editing/Editor.cpp:1627: if (selection.isNone()) On 2017/02/17 at 04:35:16, yoichio wrote: ...
3 years, 10 months ago (2017-02-17 05:14:50 UTC) #45
yoichio
https://codereview.chromium.org/2698793003/diff/200001/third_party/WebKit/Source/core/html/TextControlElement.cpp File third_party/WebKit/Source/core/html/TextControlElement.cpp (right): https://codereview.chromium.org/2698793003/diff/200001/third_party/WebKit/Source/core/html/TextControlElement.cpp#newcode567 third_party/WebKit/Source/core/html/TextControlElement.cpp:567: DCHECK(isTextControl()); Can we have DocumentLifecycle::DisallowTransitionScope here?
3 years, 10 months ago (2017-02-17 05:26:38 UTC) #48
yoichio
https://codereview.chromium.org/2698793003/diff/220001/third_party/WebKit/Source/core/editing/SelectionTemplate.cpp File third_party/WebKit/Source/core/editing/SelectionTemplate.cpp (right): https://codereview.chromium.org/2698793003/diff/220001/third_party/WebKit/Source/core/editing/SelectionTemplate.cpp#newcode153 third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:153: Strategy>::computeSelectionTypeConsideringGranularity() const { Since this function doesn't use order-comparing ...
3 years, 10 months ago (2017-02-17 05:41:55 UTC) #51
yosin_UTC9
PTAL https://codereview.chromium.org/2698793003/diff/220001/third_party/WebKit/Source/core/editing/SelectionTemplate.cpp File third_party/WebKit/Source/core/editing/SelectionTemplate.cpp (right): https://codereview.chromium.org/2698793003/diff/220001/third_party/WebKit/Source/core/editing/SelectionTemplate.cpp#newcode153 third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:153: Strategy>::computeSelectionTypeConsideringGranularity() const { On 2017/02/17 at 05:41:55, yoichio ...
3 years, 10 months ago (2017-02-17 07:14:45 UTC) #56
yoichio
lgtm
3 years, 10 months ago (2017-02-17 07:23:12 UTC) #57
tkent
lgtm
3 years, 10 months ago (2017-02-17 07:24:18 UTC) #58
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698793003/240001
3 years, 10 months ago (2017-02-17 08:35:43 UTC) #61
commit-bot: I haz the power
3 years, 10 months ago (2017-02-17 09:02:32 UTC) #64
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/e8a65d35c402aa21c4ce9b07a63a...

Powered by Google App Engine
This is Rietveld 408576698