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

Issue 1189543002: Do not traverse nodes not having a layout object while searching editable visible position. (Closed)

Created:
5 years, 6 months ago by changseok
Modified:
5 years, 4 months ago
CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, yoichio
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Do not traverse nodes not having a layout object while searching editable visible position. This is a merge of http://trac.webkit.org/changeset/185287 Typing is slow in Gmail on iPads by Ryosuke Niwa <rniwa@webkit.org>; Performance drop in GMail was caused by nextCandidate and nextVisuallyDistinctCandidate traversing through each character in a text node without a layout object. Skip any node that doesn't have a layout object in both of those functions and corresponding previous* functions. It's fine to skip unrendered nodes in PositionIterator because only other clients of PositionIterator are Position::upstream and Position::downstream and they don't care about un-rendered nodes either. BUG=497525 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200236

Patch Set 1 #

Patch Set 2 : Added a perf test #

Patch Set 3 : Split out the test #

Total comments: 2

Patch Set 4 : Replace FIXMEs with TODOs #

Patch Set 5 : Rebased #

Patch Set 6 : Fixed a build failure #

Patch Set 7 : Fix a loop issue caused by document-fragment #

Total comments: 3

Patch Set 8 : Removed conditions, !node->isDocumentFragment() #

Patch Set 9 : Resolve conflicts on PositionIterator.cpp #

Patch Set 10 : Resolved conflicts. #

Total comments: 4

Patch Set 11 : Rebased #

Patch Set 12 : Fix infinite loop cuased by accessing shadow dom. #

Patch Set 13 : Udated cl #

Patch Set 14 : Fix a crash caused by m_anchorNode being null #

Total comments: 2

Patch Set 15 : Updated patch #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -8 lines) Patch
M Source/core/dom/PositionIterator.cpp View 1 2 3 4 5 6 7 8 9 10 12 14 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/editing/htmlediting.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +24 lines, -6 lines 0 comments Download

Messages

Total messages: 72 (19 generated)
changseok
5 years, 6 months ago (2015-06-15 03:59:55 UTC) #2
eae
Is there a test for this?
5 years, 6 months ago (2015-06-15 15:26:04 UTC) #3
leviw_travelin_and_unemployed
This is a reasonable change. Can you update the description? We don't run Blink on ...
5 years, 6 months ago (2015-06-15 19:00:46 UTC) #4
changseok
On 2015/06/15 15:26:04, eae wrote: > Is there a test for this? No. There is ...
5 years, 6 months ago (2015-06-16 03:19:49 UTC) #5
changseok
On 2015/06/15 19:00:46, leviw wrote: > This is a reasonable change. Can you update the ...
5 years, 6 months ago (2015-06-16 03:21:38 UTC) #6
leviw_travelin_and_unemployed
On 2015/06/16 at 03:21:38, shivamidow wrote: > On 2015/06/15 19:00:46, leviw wrote: > > This ...
5 years, 6 months ago (2015-06-16 17:09:51 UTC) #7
changseok
On 2015/06/16 17:09:51, leviw wrote: > You can keep the old subject line in the ...
5 years, 6 months ago (2015-06-17 06:52:03 UTC) #8
changseok
On 2015/06/17 06:52:03, changseok wrote: > On 2015/06/16 17:09:51, leviw wrote: > > You can ...
5 years, 6 months ago (2015-06-17 09:31:23 UTC) #9
changseok
Would you comment on the updated cl?
5 years, 6 months ago (2015-06-21 14:18:25 UTC) #10
leviw_travelin_and_unemployed
Let's pull the perf test out and land it separately and in advance so we ...
5 years, 6 months ago (2015-06-22 20:14:46 UTC) #11
changseok
On 2015/06/22 20:14:46, leviw wrote: > Let's pull the perf test out and land it ...
5 years, 6 months ago (2015-06-23 07:19:34 UTC) #12
leviw_travelin_and_unemployed
LGTM with nit addressed now that the perf test has been landed and had time ...
5 years, 6 months ago (2015-06-25 00:15:20 UTC) #13
changseok
Thanks! https://codereview.chromium.org/1189543002/diff/40001/Source/core/editing/htmlediting.cpp File Source/core/editing/htmlediting.cpp (right): https://codereview.chromium.org/1189543002/diff/40001/Source/core/editing/htmlediting.cpp#newcode233 Source/core/editing/htmlediting.cpp:233: // FIXME: Use PositionIterator instead. On 2015/06/25 00:15:20, ...
5 years, 6 months ago (2015-06-25 03:49:39 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189543002/60001
5 years, 6 months ago (2015-06-25 04:03:11 UTC) #17
commit-bot: I haz the power
Exceeded global retry quota
5 years, 6 months ago (2015-06-25 04:14:16 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189543002/80001
5 years, 6 months ago (2015-06-25 04:33:34 UTC) #22
commit-bot: I haz the power
Exceeded global retry quota
5 years, 6 months ago (2015-06-25 04:42:05 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189543002/100001
5 years, 6 months ago (2015-06-25 05:24:31 UTC) #27
commit-bot: I haz the power
Exceeded global retry quota
5 years, 6 months ago (2015-06-25 09:02:46 UTC) #29
changseok
On 2015/06/25 09:02:46, commit-bot: I haz the power wrote: > Exceeded global retry quota There ...
5 years, 6 months ago (2015-06-26 05:29:30 UTC) #30
leviw_travelin_and_unemployed
https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/htmlediting.cpp File Source/core/editing/htmlediting.cpp (right): https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/htmlediting.cpp#newcode261 Source/core/editing/htmlediting.cpp:261: if (!node->isDocumentFragment() && !node->layoutObject()) Can you help me understand ...
5 years, 5 months ago (2015-06-29 20:17:56 UTC) #31
changseok
https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/htmlediting.cpp File Source/core/editing/htmlediting.cpp (right): https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/htmlediting.cpp#newcode261 Source/core/editing/htmlediting.cpp:261: if (!node->isDocumentFragment() && !node->layoutObject()) On 2015/06/29 20:17:56, leviw wrote: ...
5 years, 5 months ago (2015-06-30 05:50:58 UTC) #32
leviw_travelin_and_unemployed
+yosin@ and tkent@ The CL you mention shouldn't have had any behavior changes.
5 years, 5 months ago (2015-06-30 22:17:48 UTC) #34
yosin_UTC9
I suggest to this CL to split into - htmlediting.cpp - PositionIterator.cpp Checking layout object ...
5 years, 5 months ago (2015-07-01 01:50:12 UTC) #35
yosin_UTC9
On 2015/07/01 01:50:12, Yosi_UTC9 wrote: > I suggest to this CL to split into > ...
5 years, 5 months ago (2015-07-01 02:12:00 UTC) #36
changseok
On 2015/07/01 02:12:00, Yosi_UTC9 wrote: > On 2015/07/01 01:50:12, Yosi_UTC9 wrote: > > I suggest ...
5 years, 5 months ago (2015-07-01 07:55:28 UTC) #37
yosin_UTC9
On 2015/07/01 07:55:28, changseok wrote: > On 2015/07/01 02:12:00, Yosi_UTC9 wrote: > > On 2015/07/01 ...
5 years, 5 months ago (2015-07-01 08:53:52 UTC) #38
changseok
On 2015/07/01 08:53:52, Yosi_UTC9 wrote: > > Do you mean removing !node->isDocumentFragment() will just be ...
5 years, 5 months ago (2015-07-03 09:08:01 UTC) #39
changseok
On 2015/07/03 09:08:01, changseok wrote: > On 2015/07/01 08:53:52, Yosi_UTC9 wrote: > > > Do ...
5 years, 5 months ago (2015-07-05 11:22:47 UTC) #40
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189543002/180001
5 years, 5 months ago (2015-07-07 23:04:37 UTC) #43
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/69592) (exceeded global ...
5 years, 5 months ago (2015-07-08 00:02:37 UTC) #45
changseok
> Still waiting for the following processes to finish: > /b/build/slave/linux_layout/build/src/out/Release/webkit_unit_tests --gtest_filter=StyledMarkupSerializerTest.AcrossShadow --single-process-tests --test-launcher-output=/tmp/.org.chromium.Chromium.LuPXcC/test_results.xml > ...
5 years, 5 months ago (2015-07-17 09:09:35 UTC) #46
yosin_UTC9
On 2015/07/17 09:09:35, changseok wrote: > > Still waiting for the following processes to finish: ...
5 years, 5 months ago (2015-07-17 11:48:52 UTC) #47
yosin_UTC9
I'm thinking to get rid of DOM tree version of PositionIterator and next/previousXXX and use ...
5 years, 5 months ago (2015-07-17 12:00:21 UTC) #48
changseok
On 2015/07/17 11:48:52, Yosi_UTC9 wrote: > On 2015/07/17 09:09:35, changseok wrote: > > > Still ...
5 years, 4 months ago (2015-07-28 05:00:16 UTC) #49
changseok
> I'm thinking to get rid of DOM tree version of PositionIterator and > next/previousXXX ...
5 years, 4 months ago (2015-07-28 05:05:38 UTC) #50
changseok
On 2015/07/28 05:05:38, changseok wrote: > > I'm thinking to get rid of DOM tree ...
5 years, 4 months ago (2015-07-29 04:24:27 UTC) #51
yosin_UTC9
On 2015/07/29 04:24:27, changseok wrote: > On 2015/07/28 05:05:38, changseok wrote: > > > I'm ...
5 years, 4 months ago (2015-07-29 05:55:31 UTC) #52
changseok
On 2015/07/29 05:55:31, Yosi_UTC9 wrote: > On 2015/07/29 04:24:27, changseok wrote: > > On 2015/07/28 ...
5 years, 4 months ago (2015-07-30 07:36:05 UTC) #53
changseok
On 2015/07/30 07:36:05, changseok wrote: > On 2015/07/29 05:55:31, Yosi_UTC9 wrote: > > On 2015/07/29 ...
5 years, 4 months ago (2015-07-30 08:16:01 UTC) #54
yosin_UTC9
LGTM Thanks so much!
5 years, 4 months ago (2015-07-30 12:38:43 UTC) #55
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189543002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1189543002/240001
5 years, 4 months ago (2015-07-30 12:39:02 UTC) #58
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/72059)
5 years, 4 months ago (2015-07-30 13:35:54 UTC) #60
changseok
> editing/selection/DOMSelection-DocumentType.html > editing/undo/orphaned-selection-crash-bug32823-3.html > fast/dom/shadow/mouse-click-mouseup-listener-update-distribution-crash.html > fast/dom/shadow/gesture-tap-mouseup-listener-update-distribution-crash.html > editing/pasteboard/smart-paste-003-trailing-whitespace.html There was a crash caused ...
5 years, 4 months ago (2015-07-31 05:31:50 UTC) #61
changseok
> PositionIteratorAlgorithm<Strategy>::deprecatedComputePosition(). Err, it should be PositionIteratorAlgorithm<Strategy>::computePosition().
5 years, 4 months ago (2015-07-31 05:33:53 UTC) #62
yosin_UTC9
On 2015/07/31 05:33:53, changseok wrote: > > PositionIteratorAlgorithm<Strategy>::deprecatedComputePosition(). > > Err, it should be PositionIteratorAlgorithm<Strategy>::computePosition(). ...
5 years, 4 months ago (2015-07-31 06:35:01 UTC) #63
yosin_UTC9
https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/PositionIterator.cpp File Source/core/dom/PositionIterator.cpp (right): https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/PositionIterator.cpp#newcode74 Source/core/dom/PositionIterator.cpp:74: if (m_anchorNode && Strategy::hasChildren(*m_anchorNode)) Could you remove |m_anchorNode|? |computePosition()| ...
5 years, 4 months ago (2015-07-31 06:37:01 UTC) #64
changseok
On 2015/07/31 06:37:01, Yosi_UTC9 wrote: > https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/PositionIterator.cpp > File Source/core/dom/PositionIterator.cpp (right): > > https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/PositionIterator.cpp#newcode74 > ...
5 years, 4 months ago (2015-07-31 08:40:11 UTC) #65
yosin_UTC9
On 2015/07/31 08:40:11, changseok wrote: > On 2015/07/31 06:37:01, Yosi_UTC9 wrote: > > > https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/PositionIterator.cpp ...
5 years, 4 months ago (2015-08-06 02:01:53 UTC) #66
changseok
On 2015/08/06 02:01:53, Yosi_UTC9 wrote: > On 2015/07/31 08:40:11, changseok wrote: > > On 2015/07/31 ...
5 years, 4 months ago (2015-08-08 14:21:21 UTC) #67
yosin_UTC9
lgtm Thanks for long iteration!
5 years, 4 months ago (2015-08-10 06:07:43 UTC) #68
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189543002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1189543002/280001
5 years, 4 months ago (2015-08-10 06:07:56 UTC) #71
commit-bot: I haz the power
5 years, 4 months ago (2015-08-10 07:15:56 UTC) #72
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200236

Powered by Google App Engine
This is Rietveld 408576698