Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2131103002:
Ensure that TextIterator::m_pastEndNode is not skipped in advance() (Closed)
|
Created:
4 years, 5 months ago by Xiaocheng Modified:
4 years, 4 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure that TextIterator::m_pastEndNode is not skipped in advance()
In the current implemetation of |TextIterator::advance()|, when facing a
node that is neither a shadow root nor a laid-out node, the node and its
entire subtree are directly skipped, which is incorrect if the ending
position is in the subtree.
This CL fixes the issue by setting |m_pastEndNode| as the first non-skippable
node after the ending position. It also ensures that |m_pastEndNode| is
only used for checking loop ending condition, and replaces the other usage
with an equivalent implementation.
BUG=630921
TEST=webkit_unit_tests --gtest_filter=TextIteratorTest.EndingConditionWithDisplayNone*
Committed: https://crrev.com/32dad58c08112ed4a04505675aca27d37d2ce755
Cr-Commit-Position: refs/heads/master@{#408071}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Make sure m_pastEndNode is not skipped #
Total comments: 10
Patch Set 3 : comments addressed #
Total comments: 6
Patch Set 4 : Check offsetInCharacters #
Total comments: 2
Patch Set 5 : revise unit test #
Messages
Total messages: 34 (21 generated)
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
PTAL. I hope the extra check is not going to introduce performance regression. Btw, SimplifiedBackwardsTextIterator doesn't seem to have this bug.
https://codereview.chromium.org/2131103002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2131103002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:287: return; Discuss off line. Summary: - return in else-clause isn't beautiful - Calling isDescendantOf() in loop should be avoid; isDescendantOf() isn't expensive but O(n) where n is tree depth of node - Shrinking iteration range seems to be good and done in initialize() function https://codereview.chromium.org/2131103002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp (right): https://codereview.chromium.org/2131103002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp:503: setBodyContent("<div style=\"display: none\"><span>hello</span>world</div>Lorem ipsum dolor sit amet."); nit: Let's use single-quote to avoid using backslash.
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Text iterator should check ending condition when skipping non-laid-out nodes In the current implemetation of |TextIterator::advance()|, when facing a node that is neither a shadow root nor a laid-out node, the node and its entire subtree are directly skipped, which is incorrect if the ending position is in the subtree. This CL fixes the issue by adding a check of the above condition. BUG=597628 TEST=webkit_unit_tests --gtest_filter=TextIteratorTest.EndingConditionWithDisplayNone ========== to ========== Ensure that TextIterator::m_pastEndNode is not skipped in advance() In the current implemetation of |TextIterator::advance()|, when facing a node that is neither a shadow root nor a laid-out node, the node and its entire subtree are directly skipped, which is incorrect if the ending position is in the subtree. This CL fixes the issue by setting |m_pastEndNode| as the first non-skippable node after the ending position. It also ensures that |m_pastEndNode| is only used for checking loop ending condition, and replaces the other usage with an equivalent implementation. BUG=630921 TEST=webkit_unit_tests --gtest_filter=TextIteratorTest.EndingConditionWithDisplayNone ==========
Description was changed from ========== Ensure that TextIterator::m_pastEndNode is not skipped in advance() In the current implemetation of |TextIterator::advance()|, when facing a node that is neither a shadow root nor a laid-out node, the node and its entire subtree are directly skipped, which is incorrect if the ending position is in the subtree. This CL fixes the issue by setting |m_pastEndNode| as the first non-skippable node after the ending position. It also ensures that |m_pastEndNode| is only used for checking loop ending condition, and replaces the other usage with an equivalent implementation. BUG=630921 TEST=webkit_unit_tests --gtest_filter=TextIteratorTest.EndingConditionWithDisplayNone ========== to ========== Ensure that TextIterator::m_pastEndNode is not skipped in advance() In the current implemetation of |TextIterator::advance()|, when facing a node that is neither a shadow root nor a laid-out node, the node and its entire subtree are directly skipped, which is incorrect if the ending position is in the subtree. This CL fixes the issue by setting |m_pastEndNode| as the first non-skippable node after the ending position. It also ensures that |m_pastEndNode| is only used for checking loop ending condition, and replaces the other usage with an equivalent implementation. BUG=630921 TEST=webkit_unit_tests --gtest_filter=TextIteratorTest.EndingConditionWithDisplayNone* ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL at patch 2, whose idea is to set |m_pastEndNode| to the first past-end node that can't be skipped.
https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:89: return (node.layoutObject() || (node.isShadowRoot() && node.shadowHost()->layoutObject())); nit: We don't need to have outer most parenthesis. https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:99: for (Node* next = Strategy::childAt(rangeEndContainer, rangeEndOffset); next; next = Strategy::nextSibling(*next)) { Q: Is it better to use |Strategy::childrenOf(const Node&)|? Note: we have |NodeTraversal::childrendOf(const Node&)|, but not for |FlatTreeTraversal|. WDYT? int index = 0; for (Node* child : Strategy::childrenOf(*rangeEndContainer)) { if (index < rangeEndOffset) continue; if (notSkipping(*child)) return child; ++index; } https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:104: for (const Node* node = &rangeEndContainer; node; ) { We should use |Strategy::inclusiveAncestorsOf()| here. Note: We need to introduce Flat Tree version. https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:371: if (parentNode == m_endContainer && m_endOffset > 0 && Strategy::childAt(*m_endContainer, m_endOffset - 1) == m_node) Strategy::childAt(parent, k) isn't cheap, O(k), and |m_endContainer| and |m_endOffset| are constant values. Is it better to hold it in member variable? https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp (right): https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp:505: Position end(document().querySelector("span", ASSERT_NO_EXCEPTION), 0); FYI: ASSERT_NO_EXCEPTION will be an optional by crret.com/2178243002
Thanks for the review. An updated patch will be uploaded after the landing of crrev.com/2178243002 https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:89: return (node.layoutObject() || (node.isShadowRoot() && node.shadowHost()->layoutObject())); On 2016/07/26 at 02:04:12, Yosi_UTC9 wrote: > nit: We don't need to have outer most parenthesis. Done (but not uploaded yet). https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:99: for (Node* next = Strategy::childAt(rangeEndContainer, rangeEndOffset); next; next = Strategy::nextSibling(*next)) { On 2016/07/26 at 02:04:12, Yosi_UTC9 wrote: > Q: Is it better to use |Strategy::childrenOf(const Node&)|? > Note: we have |NodeTraversal::childrendOf(const Node&)|, but not for |FlatTreeTraversal|. > WDYT? > > int index = 0; > for (Node* child : Strategy::childrenOf(*rangeEndContainer)) { > if (index < rangeEndOffset) > continue; > if (notSkipping(*child)) > return child; > ++index; > } I prefer keeping it as is, at least for now. FlatTreeTraversal doesn't have any function that returns a range, and it would take some amount of time to introduce them. We may put it in the backlog and do it when there's time, though. https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:104: for (const Node* node = &rangeEndContainer; node; ) { On 2016/07/26 at 02:04:12, Yosi_UTC9 wrote: > We should use |Strategy::inclusiveAncestorsOf()| here. > Note: We need to introduce Flat Tree version. Same as to the previous comment. https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:371: if (parentNode == m_endContainer && m_endOffset > 0 && Strategy::childAt(*m_endContainer, m_endOffset - 1) == m_node) On 2016/07/26 at 02:04:12, Yosi_UTC9 wrote: > Strategy::childAt(parent, k) isn't cheap, O(k), and |m_endContainer| and |m_endOffset| are constant values. > Is it better to hold it in member variable? It seems better to store it. Done. https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp (right): https://codereview.chromium.org/2131103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp:505: Position end(document().querySelector("span", ASSERT_NO_EXCEPTION), 0); On 2016/07/26 at 02:04:12, Yosi_UTC9 wrote: > FYI: ASSERT_NO_EXCEPTION will be an optional by crret.com/2178243002 Noted. I'll wait for the landing of that patch.
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
https://codereview.chromium.org/2131103002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2131103002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:208: m_endNode = endContainer && endOffset > 0 ? Strategy::childAt(*endContainer, endOffset - 1) : nullptr; We should check whether |endContainer.offsetInCharacters()| or not, like what |pastLastNode()| does. In theory, m_endNode == previousNodeOf(pastLastNode(Position(container, offset)) See Position::nodeAsRange() https://codereview.chromium.org/2131103002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp (right): https://codereview.chromium.org/2131103002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp:526: Can we have a test which iterate characters then stops before display:none?
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
PTAL. https://codereview.chromium.org/2131103002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2131103002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:208: m_endNode = endContainer && endOffset > 0 ? Strategy::childAt(*endContainer, endOffset - 1) : nullptr; On 2016/07/26 at 08:40:43, Yosi_UTC9 wrote: > We should check whether |endContainer.offsetInCharacters()| or not, like what |pastLastNode()| does. > In theory, m_endNode == previousNodeOf(pastLastNode(Position(container, offset)) > See Position::nodeAsRange() Done. https://codereview.chromium.org/2131103002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp (right): https://codereview.chromium.org/2131103002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp:526: On 2016/07/26 at 08:40:43, Yosi_UTC9 wrote: > Can we have a test which iterate characters then stops before display:none? The ending check of TextIterator::advance() is trickier than just a comparison with |m_pastEndNode|. Haven't been able to construct such a test case where the current implementation fails, though.
https://codereview.chromium.org/2131103002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp (right): https://codereview.chromium.org/2131103002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp:526: On 2016/07/27 at 02:07:33, Xiaocheng wrote: > On 2016/07/26 at 08:40:43, Yosi_UTC9 wrote: > > Can we have a test which iterate characters then stops before display:none? > > The ending check of TextIterator::advance() is trickier than just a comparison with |m_pastEndNode|. Haven't been able to construct such a test case where the current implementation fails, though. So do you mean below case doesn't crash? const char* bodyContent = "abcd<div style='display: none'><span id=..."; I expect we have "abc" from TextIterator. https://codereview.chromium.org/2131103002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp (right): https://codereview.chromium.org/2131103002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp:519: Node* bInShadowTree = shadowRoot->firstChild()->firstChild(); Add 'id="end"' then Node* bInShadowTree = shadowRoot->getElementById("end"); for better readability.
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. https://codereview.chromium.org/2131103002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp (right): https://codereview.chromium.org/2131103002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp:526: On 2016/07/27 at 02:17:23, Yosi_UTC9 wrote: > On 2016/07/27 at 02:07:33, Xiaocheng wrote: > > On 2016/07/26 at 08:40:43, Yosi_UTC9 wrote: > > > Can we have a test which iterate characters then stops before display:none? > > > > The ending check of TextIterator::advance() is trickier than just a comparison with |m_pastEndNode|. Haven't been able to construct such a test case where the current implementation fails, though. > > So do you mean below case doesn't crash? > > const char* bodyContent = "abcd<div style='display: none'><span id=..."; > > I expect we have "abc" from TextIterator. It doesn't crash or produce any weird result. There are quite a bunch of heuristics to stop the text iterator, and it's tricky to get through all of them. https://codereview.chromium.org/2131103002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp (right): https://codereview.chromium.org/2131103002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp:519: Node* bInShadowTree = shadowRoot->firstChild()->firstChild(); On 2016/07/27 at 02:17:23, Yosi_UTC9 wrote: > Add 'id="end"' then > Node* bInShadowTree = shadowRoot->getElementById("end"); > for better readability. Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaochengh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Ensure that TextIterator::m_pastEndNode is not skipped in advance() In the current implemetation of |TextIterator::advance()|, when facing a node that is neither a shadow root nor a laid-out node, the node and its entire subtree are directly skipped, which is incorrect if the ending position is in the subtree. This CL fixes the issue by setting |m_pastEndNode| as the first non-skippable node after the ending position. It also ensures that |m_pastEndNode| is only used for checking loop ending condition, and replaces the other usage with an equivalent implementation. BUG=630921 TEST=webkit_unit_tests --gtest_filter=TextIteratorTest.EndingConditionWithDisplayNone* ========== to ========== Ensure that TextIterator::m_pastEndNode is not skipped in advance() In the current implemetation of |TextIterator::advance()|, when facing a node that is neither a shadow root nor a laid-out node, the node and its entire subtree are directly skipped, which is incorrect if the ending position is in the subtree. This CL fixes the issue by setting |m_pastEndNode| as the first non-skippable node after the ending position. It also ensures that |m_pastEndNode| is only used for checking loop ending condition, and replaces the other usage with an equivalent implementation. BUG=630921 TEST=webkit_unit_tests --gtest_filter=TextIteratorTest.EndingConditionWithDisplayNone* Committed: https://crrev.com/32dad58c08112ed4a04505675aca27d37d2ce755 Cr-Commit-Position: refs/heads/master@{#408071} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/32dad58c08112ed4a04505675aca27d37d2ce755 Cr-Commit-Position: refs/heads/master@{#408071} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
