Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2692093003:
Rewrite DocumentMarkerController to use SynchronousMutationObserver (Closed)
|
Created:
3 years, 10 months ago by rlanday Modified:
3 years, 9 months ago Reviewers:
yoichio, aelias_OOO_until_Jul13, *yutak, Changwan Ryu, Stephen White, chrishtr, Xianzhu, Xiaocheng, yosin_UTC9 CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRewrite DocumentMarkerController to use SynchronousMutationObserver
This CL does a couple of things:
- Modifies CompositeEditCommand::replaceTextInNode() to use a new editing command
SetCharacterDataCommand I introduced in:
https://codereview.chromium.org/2706033007
This command allows text replacements to be reported to
SynchronousMutationNotifier as an atomic operation, instead of as a delete and
then an insert, allowing observers to respond correctly.
- Refactors DocumentMarkerController to be a SynchronousMutationObserver instead
of having custom notification logic
- Removes CompositeEditCommand::replaceTextInNodePreservingMarkers(), which is
kind of wonky and was added to support a marker type which has since been
removed
BUG=672259
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove calls to showMarkers() #Patch Set 3 : Remove unused function declaration DocumentMarkerController::markersInRangeInternal() #Patch Set 4 : Don't try to use -1 as default value for unsigned int #
Total comments: 3
Patch Set 5 : Introduce SetCharacterDataCommand #
Total comments: 19
Patch Set 6 : Rebase on separate CL to add SetCharacterDataCommand #
Total comments: 15
Patch Set 7 : Improve removal of zero-length markers, add PaintInvalidationReason #
Total comments: 5
Patch Set 8 : Make requested changes, remove now-redundant block of code #
Total comments: 1
Patch Set 9 : Remove PaintInvalidationReason (to split off into separate CL) #
Total comments: 7
Patch Set 10 : Remove unnecessary curly braces #
Total comments: 2
Patch Set 11 : Fix didShiftMarker assignment and remove removeZeroLengthMarkers() from header #
Total comments: 4
Patch Set 12 : Fix nits, make test cases more clear #
Total comments: 7
Patch Set 13 : Respond to comments, add TODO for offset bug #
Total comments: 2
Patch Set 14 : Fx tests to match current behavior of master #Depends on Patchset: Messages
Total messages: 107 (62 generated)
The CQ bit was checked by rlanday@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...
https://codereview.chromium.org/2692093003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:764: removeMarkers(node, startOffset, prevLength); There is a slight behavioral change introduced here, there are cases where we would previously remove a marker when doing a text replacement where we now keep the marker around and make it include the new text. This change didn't seem to break any test cases but it's probably something to watch out for.
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_...)
The CQ bit was checked by rlanday@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 checked by rlanday@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_...)
The CQ bit was checked by rlanday@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...
rlanday@chromium.org changed reviewers: + aelias@chromium.org, yosin@chromium.org, yutak@chromium.org
rlanday@chromium.org changed required reviewers: + yosin@chromium.org, yutak@chromium.org
This is a refactor of DocumentMarkerController to improve how it tracks updates to DocumentMarkers to support my work on adding support for Android SuggestionSpans: https://codereview.chromium.org/2650113004
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2692093003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2692093003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:4257: for (Range* range : m_ranges) Note: We'll change Range to use didUpdateCharacterData() pattern as same as SMO. Note: Relocating Range should be done before other observers. I'm still thinking how to utilize SMO for Range list but I don't have cleaner way to do so yet. https://codereview.chromium.org/2692093003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2692093003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:737: replaceTextInNode(textNode, upstream, length, rebalancedString); Let's make |replaceTextInNode()| to use |CharacterData::setData()|, which does insert/delete at once and notify didUpdateCharacterData(). It can be done by introducing SetCharacterDataCommand(). https://codereview.chromium.org/2692093003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:752: void DocumentMarkerController::shiftMarkers(Node* node, Let's make this function as didUpdateCharacterData().
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Description was changed from ========== Rewrite DocumentMarkerController to use SynchronousMutationObserver I was talking to aelias about the cleanest way to keep the DocumentMarkers for Android SuggestionSpans up-to-date. One issue I've been running into is that many text replacements get applied in two separate operations as a delete, then an insert, which makes it impossible for DocumentMarkerController to keep the markers up-to-date. aelias's idea was to add a scoped object which causes SynchronousMutationNotifier to suppress updates until the object goes out of scope. Right now the only thing it suppresses is that a delete update immediately followed by an insert update in the same spot will get reported as a single update (this is sufficient for properly updating DocumentMarkers). I added some test cases that demonstrate that this fixes a bug where a whitespace fixup operation could cause a marker to be incorrectly updated. I also added a few test cases for other marker replacement scenarios we're interested in properly handling. BUG=672259 ========== to ========== Rewrite DocumentMarkerController to use SynchronousMutationObserver I was talking to aelias about the cleanest way to keep the DocumentMarkers for Android SuggestionSpans up-to-date. One issue I've been running into is that many text replacements get applied in two separate operations as a delete, then an insert, which makes it impossible for DocumentMarkerController to keep the markers up-to-date. aelias's idea was to add a scoped object which causes SynchronousMutationNotifier to suppress updates until the object goes out of scope. Right now the only thing it suppresses is that a delete update immediately followed by an insert update in the same spot will get reported as a single update (this is sufficient for properly updating DocumentMarkers). I added some test cases that demonstrate that this fixes a bug where a whitespace fixup operation could cause a marker to be incorrectly updated. I also added a few test cases for other marker replacement scenarios we're interested in properly handling. UPDATE: yosin asked me to replace the scoped object with a new editing command, SetCharacterDataCommand, that reports replace operations as a single edit, so that's what I have starting in Patch 5. BUG=672259 ==========
Description was changed from ========== Rewrite DocumentMarkerController to use SynchronousMutationObserver I was talking to aelias about the cleanest way to keep the DocumentMarkers for Android SuggestionSpans up-to-date. One issue I've been running into is that many text replacements get applied in two separate operations as a delete, then an insert, which makes it impossible for DocumentMarkerController to keep the markers up-to-date. aelias's idea was to add a scoped object which causes SynchronousMutationNotifier to suppress updates until the object goes out of scope. Right now the only thing it suppresses is that a delete update immediately followed by an insert update in the same spot will get reported as a single update (this is sufficient for properly updating DocumentMarkers). I added some test cases that demonstrate that this fixes a bug where a whitespace fixup operation could cause a marker to be incorrectly updated. I also added a few test cases for other marker replacement scenarios we're interested in properly handling. UPDATE: yosin asked me to replace the scoped object with a new editing command, SetCharacterDataCommand, that reports replace operations as a single edit, so that's what I have starting in Patch 5. BUG=672259 ========== to ========== Rewrite DocumentMarkerController to use SynchronousMutationObserver I was talking to aelias about the cleanest way to keep the DocumentMarkers for Android SuggestionSpans up-to-date. One issue I've been running into is that many text replacements get applied in two separate operations as a delete, then an insert, which makes it impossible for DocumentMarkerController to keep the markers up-to-date. aelias's idea was to add a scoped object which causes SynchronousMutationNotifier to suppress updates until the object goes out of scope. Right now the only thing it suppresses is that a delete update immediately followed by an insert update in the same spot will get reported as a single update (this is sufficient for properly updating DocumentMarkers). I added some test cases that demonstrate that this fixes a bug where a whitespace fixup operation could cause a marker to be incorrectly updated. I also added a few test cases for other marker replacement scenarios we're interested in properly handling. UPDATE: yosin asked me to replace the scoped object with a new editing command, SetCharacterDataCommand, that reports replace operations as a single edit, so that's what I have starting in Patch 5. BUG=672259 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@yosin, I've replaced the ScopedNotificationSuppressor with a new SetCharacterDataCommand as you suggested
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yosin@chromium.org changed reviewers: + xiaochengh@chromium.org
https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp (right): https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:23: DCHECK_LE(m_offset + m_count, m_node->length()); Please add DCHECK_LT(m_offset, m_node->length()); https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:27: DCHECK(m_node); nit: We don't need to have DCHECK(m_node). |m_node| will be const member variable. https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:29: document().updateStyleAndLayoutTree(); Please add following comment before L29 // TODO(editing-dev): The use of updateStyleAndLayoutTree() // needs to be audited. See http://crbug.com/590369 for more details. https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:39: bool passwordEchoEnabled = nit: s/bool/const bool/ https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:56: DCHECK(m_node); nit: We don't need to have DCHECK(m_node). |m_node| will be const member variable. https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:58: if (!hasEditableStyle(*m_node)) Please update layout tree: // TODO(editing-dev): The use of updateStyleAndLayoutTree() // needs to be audited. See http://crbug.com/590369 for more details. document().updateStyleAndLayoutTree(); https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:63: m_node->document().updateStyleAndLayout(); Please don't call |updateStyleAndLayout()| here. https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.h (right): https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. Could you move SetCharacterDataCommand in another patch with "SetCharacterDataCommandTest.cpp" for |doApply()| and |doUnapply| https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.h:28: Could add a comment to note |doApply()| and |doUnapply()| implements of |EditCommand|'s. https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.h:29: void doApply(EditingState*) override; nit: s/override/final/ https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.h:30: void doUnapply() override; nit: s/override/final/ https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.h:32: Member<Text> m_node; nit: Could you add |const| to all member variables? We don't change them after construction. https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:705: Member<MarkerList>& list = markers[markerListIndex]; nit: s/&// https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:715: if (removedMarkers && We prefer early-return-style. if (!removeMarkers) continue; if (markerListIndex != DocumentMarker::TextMatchMarkerIndex) continue; invalidatePaintForTickmarks(node); https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:852: if (newLength == 0) { nit: no need to have "{}" https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:868: Member<MarkerList>& list = (*markers)[markerListIndex]; nit: s/&//
https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp (right): https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:58: if (!hasEditableStyle(*m_node)) On 2017/02/20 at 07:26:26, yosin_UTC9 wrote: > Please update layout tree: > > // TODO(editing-dev): The use of updateStyleAndLayoutTree() > // needs to be audited. See http://crbug.com/590369 for more details. > document().updateStyleAndLayoutTree(); Can you please explain to me what this is doing and how I know where I need to call it?
https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.h (right): https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.h:32: Member<Text> m_node; On 2017/02/20 at 07:26:27, yosin_UTC9 wrote: > nit: Could you add |const| to all member variables? > We don't change them after construction. m_previousTextForUndo can't be const since it gets set in doApply(), the others can
On 2017/02/21 at 21:16:55, rlanday wrote: > https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.h (right): > > https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.h:32: Member<Text> m_node; > On 2017/02/20 at 07:26:27, yosin_UTC9 wrote: > > nit: Could you add |const| to all member variables? > > We don't change them after construction. > > m_previousTextForUndo can't be const since it gets set in doApply(), the others can I've uploaded a separate CL to add SetCharacterDataCommand: https://codereview.chromium.org/2706033007
https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp (right): https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:58: if (!hasEditableStyle(*m_node)) On 2017/02/21 at 20:04:06, rlanday wrote: > On 2017/02/20 at 07:26:26, yosin_UTC9 wrote: > > Please update layout tree: > > > > // TODO(editing-dev): The use of updateStyleAndLayoutTree() > > // needs to be audited. See http://crbug.com/590369 for more details. > > document().updateStyleAndLayoutTree(); > > Can you please explain to me what this is doing and how I know where I need to call it? This function resolves style for all elements in the document, and calculates their sizes and positions. It also calculates how text is wrapped into lines. Most editing operations require clean layout, as they depend on how text is laid out. In most cases, you will hit a DCHECK if a call to this function is missing. https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:28: #include <algorithm> No need to change include order. https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:698: void DocumentMarkerController::removeZeroLengthMarkers() { This function may cause performance issue. It iterates the marker lists for all nodes. As a result, an editing operation can run time quadratic to the number of text nodes modified. It's better to handle empty marker removal right after adjusting the start/end offsets of a marker. https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:866: for (size_t markerListIndex = 0; Range-based for loop is preferred. https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:882: marker--; s/marker--/--marker/ Chromium prefers prefix increment & decrement: https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:894: (*marker)->setStartOffset(offset); It seems more natural to move to end of new text. New text shouldn't be marked by an existing marker unless it's in the middle of the marker. https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:925: node->layoutObject()->setShouldDoFullPaintInvalidation(); Do we really need this call? Shouldn't paint (along with style and layout) be already invalidated before entering didUpdateCharacterData?
https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:28: #include <algorithm> On 2017/02/22 at 01:49:23, Xiaocheng wrote: > No need to change include order. git cl format did this, the linter will probably complain if I undo it https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:698: void DocumentMarkerController::removeZeroLengthMarkers() { On 2017/02/22 at 01:49:23, Xiaocheng wrote: > This function may cause performance issue. It iterates the marker lists for all nodes. As a result, an editing operation can run time quadratic to the number of text nodes modified. > > It's better to handle empty marker removal right after adjusting the start/end offsets of a marker. Ah, good catch https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:894: (*marker)->setStartOffset(offset); On 2017/02/22 at 01:49:23, Xiaocheng wrote: > It seems more natural to move to end of new text. > > New text shouldn't be marked by an existing marker unless it's in the middle of the marker. I think this is how it works for DOM ranges, but I can try doing it this way for markers and see if everything works https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:925: node->layoutObject()->setShouldDoFullPaintInvalidation(); On 2017/02/22 at 01:49:23, Xiaocheng wrote: > Do we really need this call? > > Shouldn't paint (along with style and layout) be already invalidated before entering didUpdateCharacterData? I'm not really sure, I think I added this because I saw that it got called in removeMarkers()...
https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:866: for (size_t markerListIndex = 0; On 2017/02/22 at 01:49:23, Xiaocheng wrote: > Range-based for loop is preferred. I think that would be difficult given that we're removing markers in the middle of the loop...unless we want to shove them into a vector and remove them afterward?
On 2017/02/22 at 02:14:57, rlanday wrote: > https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:866: for (size_t markerListIndex = 0; > On 2017/02/22 at 01:49:23, Xiaocheng wrote: > > Range-based for loop is preferred. > > I think that would be difficult given that we're removing markers in the middle of the loop...unless we want to shove them into a vector and remove them afterward? I prefer to have two loops, one for collecting markers will be removed, and another loop for removal to utilize range-for loop. Another idea is mark markers "removed" then collect them before GC. This is inspired by [1]. [1] http://crrev.com/2669213002: Remove spelling markers in disabled and readonly form elements
https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:925: node->layoutObject()->setShouldDoFullPaintInvalidation(); On 2017/02/22 at 02:01:11, rlanday wrote: > On 2017/02/22 at 01:49:23, Xiaocheng wrote: > > Do we really need this call? > > > > Shouldn't paint (along with style and layout) be already invalidated before entering didUpdateCharacterData? > > I'm not really sure, I think I added this because I saw that it got called in removeMarkers()... Please ask wangxianzhu@, very near to you, he is THE expert of invalidation. He may have some idea about better way.
On 2017/02/22 at 02:20:42, yosin wrote: > On 2017/02/22 at 02:14:57, rlanday wrote: > > https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > > > https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:866: for (size_t markerListIndex = 0; > > On 2017/02/22 at 01:49:23, Xiaocheng wrote: > > > Range-based for loop is preferred. > > > > I think that would be difficult given that we're removing markers in the middle of the loop...unless we want to shove them into a vector and remove them afterward? > > I prefer to have two loops, one for collecting markers will be removed, and another loop for removal to utilize range-for loop. > Another idea is mark markers "removed" then collect them before GC. This is inspired by [1]. > > > [1] http://crrev.com/2669213002: Remove spelling markers in disabled and readonly form elements This patch has two nested loops: the outer loop over marker lists and the inner loop over markers. I'm talking about the outer loop. Since no marker list is removed, it's fine to use range-based for loop.
wangxianzhu@chromium.org changed reviewers: + wangxianzhu@chromium.org
https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:925: node->layoutObject()->setShouldDoFullPaintInvalidation(); On 2017/02/22 02:25:05, yosin_UTC9 wrote: > On 2017/02/22 at 02:01:11, rlanday wrote: > > On 2017/02/22 at 01:49:23, Xiaocheng wrote: > > > Do we really need this call? > > > > > > Shouldn't paint (along with style and layout) be already invalidated before > entering didUpdateCharacterData? > > > > I'm not really sure, I think I added this because I saw that it got called in > removeMarkers()... > > Please ask wangxianzhu@, very near to you, he is THE expert of invalidation. > He may have some idea about better way. Though layout may also trigger the repaint, I think we should still call it here, because: - Here we are sure the object needs paint invalidation because of change of marker; - The call is cheap, and is near no-op if the object has already been set needs paint invalidation; - Depending on others to issue paint invalidation is fragile. Other code doesn't know the requirements of markers and may miss the paint invalidation. For example, some optimization like to avoid repainting an object containing all blank characters might skip paint invalidation that markers need. I also suggest to add a PaintInvalidationReason for markers as a follow-up which will be useful for debugging and analysis.
On 2017/02/22 at 02:20:42, yosin wrote: > On 2017/02/22 at 02:14:57, rlanday wrote: > > https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > > > https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:866: for (size_t markerListIndex = 0; > > On 2017/02/22 at 01:49:23, Xiaocheng wrote: > > > Range-based for loop is preferred. > > > > I think that would be difficult given that we're removing markers in the middle of the loop...unless we want to shove them into a vector and remove them afterward? > > I prefer to have two loops, one for collecting markers will be removed, and another loop for removal to utilize range-for loop. > Another idea is mark markers "removed" then collect them before GC. This is inspired by [1]. > > > [1] http://crrev.com/2669213002: Remove spelling markers in disabled and readonly form elements I don't like the two loop idea; basically we'd have to put a list of indices into a vector, and then in the second loop we'd have to go through the list of indices in reverse and remove them from the vector (since if we go through it forward, the following indices will have been shifted). So then we'd have to use a reverse iterator to go through the loop. Is one range-for loop and one reverse iterator loop simpler than a loop using an index counter? Or we could use a deque or something that lets us push indices to the front and then we could use a range-for loop to go through them in forward order, but this also seems unnecessarily complex. I don't understand exactly what you mean by collecting the markers before GC; in the CL you linked, it looks like the markers are explicitly removed by calling spellChecker().didEndEditingOnTextField(this) in response to the text field being disabled, right?
https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:698: void DocumentMarkerController::removeZeroLengthMarkers() { On 2017/02/22 at 02:01:10, rlanday wrote: > On 2017/02/22 at 01:49:23, Xiaocheng wrote: > > This function may cause performance issue. It iterates the marker lists for all nodes. As a result, an editing operation can run time quadratic to the number of text nodes modified. > > > > It's better to handle empty marker removal right after adjusting the start/end offsets of a marker. > > Ah, good catch Wait a second, isn't it potentially quadratic anyway? It's an improvement in that we don't have to go through the lists a second time but we already have to go through them once, and removing a marker causes an O(n) shift of the vector elements wherever we do it...
The CQ bit was checked by rlanday@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...
rlanday@chromium.org changed reviewers: + senorblanco@chromium.org
rlanday@chromium.org changed required reviewers: + senorblanco@chromium.org - yosin@chromium.org
Adding @senorblanco as a reviewer for the PaintInvalidationReason addition
https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:698: void DocumentMarkerController::removeZeroLengthMarkers() { On 2017/02/22 at 22:53:01, rlanday wrote: > On 2017/02/22 at 02:01:10, rlanday wrote: > > On 2017/02/22 at 01:49:23, Xiaocheng wrote: > > > This function may cause performance issue. It iterates the marker lists for all nodes. As a result, an editing operation can run time quadratic to the number of text nodes modified. > > > > > > It's better to handle empty marker removal right after adjusting the start/end offsets of a marker. > > > > Ah, good catch > > Wait a second, isn't it potentially quadratic anyway? It's an improvement in that we don't have to go through the lists a second time but we already have to go through them once, and removing a marker causes an O(n) shift of the vector elements wherever we do it... The marker removal itself is indeed quadratic, to a different quantity (marker list length), though. My major concern is that this function unnecessarily iterates over all nodes that have marker lists, resulting in another quadratic running time (to the number of nodes). https://codereview.chromium.org/2692093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:848: for (size_t markerListIndex = 0; Can we use for (MarkerList* list : markers) It seems fine, as we do not remove |list| in the loop. https://codereview.chromium.org/2692093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:899: if ((*marker)->startOffset() == (*marker)->endOffset()) { The removal condition should be start >= end instead, as L877 moves start to the end of the new text.
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_...)
On 2017/02/22 at 23:39:28, xiaochengh wrote: > https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2692093003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:698: void DocumentMarkerController::removeZeroLengthMarkers() { > On 2017/02/22 at 22:53:01, rlanday wrote: > > On 2017/02/22 at 02:01:10, rlanday wrote: > > > On 2017/02/22 at 01:49:23, Xiaocheng wrote: > > > > This function may cause performance issue. It iterates the marker lists for all nodes. As a result, an editing operation can run time quadratic to the number of text nodes modified. > > > > > > > > It's better to handle empty marker removal right after adjusting the start/end offsets of a marker. > > > > > > Ah, good catch > > > > Wait a second, isn't it potentially quadratic anyway? It's an improvement in that we don't have to go through the lists a second time but we already have to go through them once, and removing a marker causes an O(n) shift of the vector elements wherever we do it... > > The marker removal itself is indeed quadratic, to a different quantity (marker list length), though. > > My major concern is that this function unnecessarily iterates over all nodes that have marker lists, resulting in another quadratic running time (to the number of nodes). > > https://codereview.chromium.org/2692093003/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2692093003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:848: for (size_t markerListIndex = 0; > Can we use > > for (MarkerList* list : markers) > > It seems fine, as we do not remove |list| in the loop. > > https://codereview.chromium.org/2692093003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:899: if ((*marker)->startOffset() == (*marker)->endOffset()) { > The removal condition should be start >= end instead, as L877 moves start to the end of the new text. Looks like I broke a text highlighting test case, hmm...
https://codereview.chromium.org/2692093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:848: for (size_t markerListIndex = 0; On 2017/02/22 at 23:39:28, Xiaocheng wrote: > Can we use > > for (MarkerList* list : markers) > > It seems fine, as we do not remove |list| in the loop. Oops...thought I did this https://codereview.chromium.org/2692093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:899: if ((*marker)->startOffset() == (*marker)->endOffset()) { On 2017/02/22 at 23:39:28, Xiaocheng wrote: > The removal condition should be start >= end instead, as L877 moves start to the end of the new text. Ah, good catch
The CQ bit was checked by rlanday@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...
> Looks like I broke a text highlighting test case, hmm... The test case just needed to re-baselined after I added the new PaintInvalidationReason
https://codereview.chromium.org/2692093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:899: if ((*marker)->startOffset() == (*marker)->endOffset()) { On 2017/02/23 at 01:27:28, rlanday wrote: > On 2017/02/22 at 23:39:28, Xiaocheng wrote: > > The removal condition should be start >= end instead, as L877 moves start to the end of the new text. > > Ah, good catch Actually I think the case where start > end can only happen if the replaced region contains both the start marker (in which case we move the start marker to the end of the new text) and the end marker (in which case we move the end marker to the beginning of the text), and also is not exactly equal to the region spanned by a marker (since we special-case that to put the marker around the new text). So I think this is now redundant with the block under the "if marker is contained by but not equal to the replaced range, remove the marker" comment. I'll remove the block at the top since it makes the logic more complex.
Just FYI, marker relocation isn't simple as we thought: - https://github.com/eval1749/evita/blob/master/evita/text/models/marker_set.cc - https://github.com/eval1749/evita/blob/master/evita/text/models/marker_set_te... On insertion, my marker supports splitting. On deletion, my marker supports merging. I think cost of maintaining sorted list is high and complex. It seems on-demand sorting and removal is better approach. https://codereview.chromium.org/2692093003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:900: PaintInvalidationDocumentMarkerChange); Could you move this change in another patch? This is useful for caret implementation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/02/23 at 02:15:46, yosin wrote: > Just FYI, marker relocation isn't simple as we thought: > - https://github.com/eval1749/evita/blob/master/evita/text/models/marker_set.cc > - https://github.com/eval1749/evita/blob/master/evita/text/models/marker_set_te... > > On insertion, my marker supports splitting. > On deletion, my marker supports merging. Can you please clarify what this code does? I'm not familiar with this repo.
On 2017/02/23 at 05:19:55, rlanday wrote: > On 2017/02/23 at 02:15:46, yosin wrote: > > Just FYI, marker relocation isn't simple as we thought: > > - https://github.com/eval1749/evita/blob/master/evita/text/models/marker_set.cc > > - https://github.com/eval1749/evita/blob/master/evita/text/models/marker_set_te... > > > > On insertion, my marker supports splitting. > > On deletion, my marker supports merging. > > Can you please clarify what this code does? I'm not familiar with this repo. This code does marker list management, MarketSet class, for plain text editor. MarkSet class observers text change via didDeleteAt(start, end) and didInsertBefore(start, end) (Functionality of didUpdateCharacterData() is implemented by sticky/fragile marker concept) This marker is used for implementing spelling marker and syntax highlighting in different list for ease of clearing spelling markers since syntax highlight markers are stay long. The details aren't important here. The topic is marker management for ordered set is still complex and needs to support various cases, marker overlapping, new marker covers existing markers, etc. My thought comes from experience of this code and I'm still looking for simple implementation. I remembered I spend lots of time to fix the bug of maker management because of edge cases. Since usage pattern of DMC is different of my markers, e.g. no need to expose list to JS, we can sort on demand, e.g. painting.
yosin@chromium.org changed reviewers: + yoichio@chromium.org
+yoichio@ for another eye
Description was changed from ========== Rewrite DocumentMarkerController to use SynchronousMutationObserver I was talking to aelias about the cleanest way to keep the DocumentMarkers for Android SuggestionSpans up-to-date. One issue I've been running into is that many text replacements get applied in two separate operations as a delete, then an insert, which makes it impossible for DocumentMarkerController to keep the markers up-to-date. aelias's idea was to add a scoped object which causes SynchronousMutationNotifier to suppress updates until the object goes out of scope. Right now the only thing it suppresses is that a delete update immediately followed by an insert update in the same spot will get reported as a single update (this is sufficient for properly updating DocumentMarkers). I added some test cases that demonstrate that this fixes a bug where a whitespace fixup operation could cause a marker to be incorrectly updated. I also added a few test cases for other marker replacement scenarios we're interested in properly handling. UPDATE: yosin asked me to replace the scoped object with a new editing command, SetCharacterDataCommand, that reports replace operations as a single edit, so that's what I have starting in Patch 5. BUG=672259 ========== to ========== Rewrite DocumentMarkerController to use SynchronousMutationObserver I was talking to aelias about the cleanest way to keep the DocumentMarkers for Android SuggestionSpans up-to-date. One issue I've been running into is that many text replacements get applied in two separate operations as a delete, then an insert, which makes it impossible for DocumentMarkerController to keep the markers up-to-date. aelias's idea was to add a scoped object which causes SynchronousMutationNotifier to suppress updates until the object goes out of scope. Right now the only thing it suppresses is that a delete update immediately followed by an insert update in the same spot will get reported as a single update (this is sufficient for properly updating DocumentMarkers). I added some test cases that demonstrate that this fixes a bug where a whitespace fixup operation could cause a marker to be incorrectly updated. I also added a few test cases for other marker replacement scenarios we're interested in properly handling. UPDATE: yosin asked me to replace the scoped object with a new editing command, SetCharacterDataCommand, that reports replace operations as a single edit, so that's what I have starting in Patch 5. BUG=672259 ==========
senorblanco@chromium.org changed reviewers: + chrishtr@chromium.org
platform/graphics LGTM +chrishtr as an FYI
rlanday@chromium.org changed reviewers: - senorblanco@chromium.org
rlanday@chromium.org changed required reviewers: - senorblanco@chromium.org
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Removed the PaintInvalidationReason stuff to split off into a separate CL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xiaochengh@chromium.org changed reviewers: + senorblanco@chromium.org
https://codereview.chromium.org/2692093003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:265: if (node->layoutObject()) { nit: no need to add braces https://codereview.chromium.org/2692093003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:336: if (docDirty && dstNode->layoutObject()) { nit: no need to add braces https://codereview.chromium.org/2692093003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:427: if (docDirty && node->layoutObject()) { nit: no need to add braces https://codereview.chromium.org/2692093003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:686: if (LayoutObject* layoutObject = node.layoutObject()) { nit: no need to add braces https://codereview.chromium.org/2692093003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:782: if (docDirty && node->layoutObject()) { nit: no need to add braces. https://codereview.chromium.org/2692093003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:885: didShiftMarker = true; didShiftMarker = true should be set after L863 and L876 instead. https://codereview.chromium.org/2692093003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2692093003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:155: void removeZeroLengthMarkers(); This should be removed.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
rlanday@chromium.org changed reviewers: - senorblanco@chromium.org
rlanday@chromium.org changed reviewers: + senorblanco@chromium.org
Ok, I've made those changes
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rlanday@chromium.org changed reviewers: - senorblanco@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yosin@chromium.org changed reviewers: + senorblanco@chromium.org
Getting closer. https://codereview.chromium.org/2692093003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:841: for (MarkerList::iterator marker = list->begin(); marker != list->end(); nit: s/marker/it/ or s/marker/marker_it/ since it is MarkerList::iterator. Then add: Marker& marker = *it; https://codereview.chromium.org/2692093003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:884: if (didShiftMarker) { Let's use early-return style if (!didShiftMarker) return; invalidateRectsForMarkersInNode(*node); if (!node->layoutObject()) return; node->layoutObject()->setShouldDoFullPaintInvalidation(); https://codereview.chromium.org/2692093003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:885: invalidateRectsForMarkersInNode(*node); I think we don't need to call invalidateRectsForMarkersInNode() if node->layoutObject() == nullptr. https://codereview.chromium.org/2692093003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2692093003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:140: unsigned newLength) override; nit: s/override/final/
https://codereview.chromium.org/2692093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2692093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1316: EXPECT_EQ(5u, document().markers().markers()[0]->endOffset()); It is not unclear what you check. Could you add something like: ASSERT_EQUALS(div.innerText, "text"); ASSERT_EQUALS(getMarkedText(document().markers(), 0), "text"); (you might need to impl |getMarkedText| function).
https://codereview.chromium.org/2692093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2692093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1316: EXPECT_EQ(5u, document().markers().markers()[0]->endOffset()); On 2017/02/24 at 04:52:09, yoichio wrote: > It is not unclear what you check. > Could you add something like: > ASSERT_EQUALS(div.innerText, "text"); > ASSERT_EQUALS(getMarkedText(document().markers(), 0), "text"); > (you might need to impl |getMarkedText| function). I'm going to just add a helper function in the unit test; if we want to actually add it to DocumentMarker, we'll have to do a bunch of plumbing to store a pointer to the associated Node in each DocumentMarker and keep this up to date when copying markers to other nodes, etc.
The CQ bit was checked by rlanday@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...
https://codereview.chromium.org/2692093003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2692093003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1317: controller().setCompositionFromExistingText(emptyUnderlines, 5, 9); I actually did end up changing the offsets here after adding the assertion that the string is actually " text " at this point; the offsets were previously 6 to 10 and the string was " text b". This doesn't make sense to me because this string has seven characters and offset 6 means "between the 6th and 7th characters" (counting from 1). Is the space at the beginning of the string not getting counted here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yosin@chromium.org changed reviewers: + changwan@chromium.org
+changwang@, WDYT? https://codereview.chromium.org/2692093003#msg88 https://codereview.chromium.org/2692093003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2692093003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:876: if (marker.startOffset() >= marker.endOffset()) { nit: Could you write early-continue style to reduce indentation? if (marker.startOffset() < marker.endOffset()) continue; list->remove(it - list->begin()); --it;
https://codereview.chromium.org/2692093003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2692093003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1317: controller().setCompositionFromExistingText(emptyUnderlines, 5, 9); On 2017/02/24 20:23:45, rlanday wrote: > I actually did end up changing the offsets here after adding the assertion that > the string is actually " text " at this point; the offsets were previously 6 to > 10 and the string was " text b". This doesn't make sense to me because this > string has seven characters and offset 6 means "between the 6th and 7th > characters" (counting from 1). Is the space at the beginning of the string not > getting counted here? Hmm... I think something's wrong here. The text before this line was " text blah", and you're composing and then deleting " bla", so I think the final result should be " text h". Normally the range [5,9) means starting from 5th character where the index starts from 0 and before 9th character. Could you check if you get the same test result with partially reverting the patch - with this test case and without the main logic? (Also additionally removing the marker parts in the test). If the test still fails, I don't mind you landing this with a TODO as this is perpendicular to that issue. You can also check controller().compositionText() after this line to make sure you have the correct composition. https://codereview.chromium.org/2692093003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1320: ASSERT_STREQ(div->innerHTML().utf8().data(), " text "); Normally the order is: ASSERT_STREQ(expected, actual);
https://codereview.chromium.org/2692093003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2692093003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1317: controller().setCompositionFromExistingText(emptyUnderlines, 5, 9); On 2017/02/27 at 13:55:57, Changwan Ryu wrote: > On 2017/02/24 20:23:45, rlanday wrote: > > I actually did end up changing the offsets here after adding the assertion that > > the string is actually " text " at this point; the offsets were previously 6 to > > 10 and the string was " text b". This doesn't make sense to me because this > > string has seven characters and offset 6 means "between the 6th and 7th > > characters" (counting from 1). Is the space at the beginning of the string not > > getting counted here? > > Hmm... I think something's wrong here. > The text before this line was " text blah", and you're composing and then deleting " bla", > so I think the final result should be " text h". Normally the range [5,9) means starting from 5th character where the index starts from 0 and before 9th character. > > Could you check if you get the same test result with partially reverting the patch - with this test case and without the main logic? (Also additionally removing the marker parts in the test). > If the test still fails, I don't mind you landing this with a TODO as this is perpendicular to that issue. > > You can also check controller().compositionText() after this line to make sure you have the correct composition. This must be a pre-existing issue since this test passes verbatim on master. I will file a bug.
https://codereview.chromium.org/2692093003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2692093003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1317: controller().setCompositionFromExistingText(emptyUnderlines, 5, 9); On 2017/02/27 at 18:29:09, rlanday wrote: > On 2017/02/27 at 13:55:57, Changwan Ryu wrote: > > On 2017/02/24 20:23:45, rlanday wrote: > > > I actually did end up changing the offsets here after adding the assertion that > > > the string is actually " text " at this point; the offsets were previously 6 to > > > 10 and the string was " text b". This doesn't make sense to me because this > > > string has seven characters and offset 6 means "between the 6th and 7th > > > characters" (counting from 1). Is the space at the beginning of the string not > > > getting counted here? > > > > Hmm... I think something's wrong here. > > The text before this line was " text blah", and you're composing and then deleting " bla", > > so I think the final result should be " text h". Normally the range [5,9) means starting from 5th character where the index starts from 0 and before 9th character. > > > > Could you check if you get the same test result with partially reverting the patch - with this test case and without the main logic? (Also additionally removing the marker parts in the test). > > If the test still fails, I don't mind you landing this with a TODO as this is perpendicular to that issue. > > > > You can also check controller().compositionText() after this line to make sure you have the correct composition. > > This must be a pre-existing issue since this test passes verbatim on master. I will file a bug. I think the bug is in IMC::commitText, instead of offset counting. In the removal of "Initial", IMC::commitText should convert the following space into an nbsp so that it doesn't get collapsed. It seems that the space didn't get converted, which is the bug.
https://codereview.chromium.org/2692093003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2692093003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1317: controller().setCompositionFromExistingText(emptyUnderlines, 5, 9); On 2017/02/27 at 19:31:23, Xiaocheng wrote: > On 2017/02/27 at 18:29:09, rlanday wrote: > > On 2017/02/27 at 13:55:57, Changwan Ryu wrote: > > > On 2017/02/24 20:23:45, rlanday wrote: > > > > I actually did end up changing the offsets here after adding the assertion that > > > > the string is actually " text " at this point; the offsets were previously 6 to > > > > 10 and the string was " text b". This doesn't make sense to me because this > > > > string has seven characters and offset 6 means "between the 6th and 7th > > > > characters" (counting from 1). Is the space at the beginning of the string not > > > > getting counted here? > > > > > > Hmm... I think something's wrong here. > > > The text before this line was " text blah", and you're composing and then deleting " bla", > > > so I think the final result should be " text h". Normally the range [5,9) means starting from 5th character where the index starts from 0 and before 9th character. > > > > > > Could you check if you get the same test result with partially reverting the patch - with this test case and without the main logic? (Also additionally removing the marker parts in the test). > > > If the test still fails, I don't mind you landing this with a TODO as this is perpendicular to that issue. > > > > > > You can also check controller().compositionText() after this line to make sure you have the correct composition. > > > > This must be a pre-existing issue since this test passes verbatim on master. I will file a bug. > > I think the bug is in IMC::commitText, instead of offset counting. > > In the removal of "Initial", IMC::commitText should convert the following space into an nbsp so that it doesn't get collapsed. It seems that the space didn't get converted, which is the bug. I think what's going on is that if you go through DeleteSelectionCommand (looks like the only way to do this is by going through Editor::deleteSelectionWithSmartDelete()), DeleteSelectionCommand::fixupWhitespace() gets called to convert the spaces to NBSPs if necessary. But if you try to delete text by performing the operations I'm performing in this test case, that code never gets run. Is this something we want to fix? I'm not sure what the right level to call fixupWhitespace() is. Presumably we need some sort of low-level operation that doesn't change the whitespace (CharacterData::replaceData()?), but we should probably do it in response to IME methods?
yosin@chromium.org changed reviewers: + yutak@chromium.orgco - yutak@chromium.org
yosin@chromium.org changed required reviewers: + yutak@chromium.orgco - yutak@chromium.org
Could you change description to describe current CL changes exclude history? We can visit history of this patch from link in commit log. https://codereview.chromium.org/2692093003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2692093003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1293: static String getMarkedText(DocumentMarkerController& documentMarkerController, Could you move IMCTest changes into separate CL to record current behavior? Then this CL reveal what behavior change is done by this CL. https://codereview.chromium.org/2692093003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1301: // TODO(crbug.com/696652): setCompositionFromExistingText() is counting the I would like to discuss this issue in crbug.com/696652, rather than this CL. And, I would like to focus on DMC changes for ease of reviewing.
> https://codereview.chromium.org/2692093003/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1293: static String getMarkedText(DocumentMarkerController& documentMarkerController, > Could you move IMCTest changes into separate CL to record current behavior? > Then this CL reveal what behavior change is done by this CL. As I’ve noted in crbug.com/696652, the current behavior in master is affected by a regression introduced in https://codereview.chromium.org/2675363003 which makes the fixupWhitespace() not happen properly. I think we need to fix that first before we can determine what the behavior on master really "should" be right now.
Description was changed from ========== Rewrite DocumentMarkerController to use SynchronousMutationObserver I was talking to aelias about the cleanest way to keep the DocumentMarkers for Android SuggestionSpans up-to-date. One issue I've been running into is that many text replacements get applied in two separate operations as a delete, then an insert, which makes it impossible for DocumentMarkerController to keep the markers up-to-date. aelias's idea was to add a scoped object which causes SynchronousMutationNotifier to suppress updates until the object goes out of scope. Right now the only thing it suppresses is that a delete update immediately followed by an insert update in the same spot will get reported as a single update (this is sufficient for properly updating DocumentMarkers). I added some test cases that demonstrate that this fixes a bug where a whitespace fixup operation could cause a marker to be incorrectly updated. I also added a few test cases for other marker replacement scenarios we're interested in properly handling. UPDATE: yosin asked me to replace the scoped object with a new editing command, SetCharacterDataCommand, that reports replace operations as a single edit, so that's what I have starting in Patch 5. BUG=672259 ========== to ========== Rewrite DocumentMarkerController to use SynchronousMutationObserver This CL does a couple of things: - Modifies CompositeEditCommand::replaceTextInNode() to use a new editing command SetCharacterDataCommand I introduced in: https://codereview.chromium.org/2706033007 This command allows text replacements to be reported to SynchronousMutationNotifier as an atomic operation, instead of as a delete and then an insert, allowing observers to respond correctly. - Refactors DocumentMarkerController to be a SynchronousMutationObserver instead of having custom notification logic - Removes CompositeEditCommand::replaceTextInNodePreservingMarkers(), which is kind of wonky and was added to support a marker type which has since been removed BUG=672259 ==========
The CQ bit was checked by rlanday@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...
On 2017/02/28 at 06:32:49, rlanday wrote: > > https://codereview.chromium.org/2692093003/diff/240001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1293: static String getMarkedText(DocumentMarkerController& documentMarkerController, > > Could you move IMCTest changes into separate CL to record current behavior? > > Then this CL reveal what behavior change is done by this CL. > > As I’ve noted in crbug.com/696652, the current behavior in master is affected by a regression introduced in https://codereview.chromium.org/2675363003 which makes the fixupWhitespace() not happen properly. I think we need to fix that first before we can determine what the behavior on master really "should" be right now. Ok, I spun off the tests into https://codereview.chromium.org/2721563005 and rebased this CL on top, so now you can compare how the behavior is being changed
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Message was sent while issue was closed.
closing because @yosin wants me to split this up |
