Google Japanese Input IME is crashing on DCHECK in debug builds of master |
||
Issue descriptionChrome Version: 62.0.3188.0 (Developer Build) unknown (32-bit) OS: Android N What steps will reproduce the problem? (1) Install the Google Japanese Input IME (2) Go to editpad.org (3) Try typing with the Japanese IME What is the expected result? Japanese characters appear underlined with a blue background. What happens instead? Chrome crashes on this DCHECK to verify that we're not adding overlapping composition DocumentMarkers: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp?q=documentmarkerlisteditor&sq=package:chromium&l=29 In release builds, we do not crash on the DCHECK, but buggy behavior may be occurring since an invariant we thought was true about composition markers (they do not overlap) is being violated.
,
Aug 7 2017
Can we ask IME team to call one set marker to specify both underline and background color to reduce IPC?
,
Aug 7 2017
I don't think this is possible. Android IMEs are built on top of the Android APIs, so they have to use BackgroundColorSpan and UnderlineSpan together if they want to set both (if there's a way to do both together, I'm not aware of it). In this particular case, where they apply to the same range, we could merge them in ImeAdapterAndroid when we convert them to CompositionUnderlines. But in general, an IME can apply multiple BackgroundColorSpans and UnderlineSpans to different regions in a way that can't be consolidated into a single marker. So I think it makes sense to support the general case. I don't think asking the authors of every single IME that uses these features to change their behavior for our benefit would be reasonable anyway. I think our strategy is generally to implement the APIs (at least, those actually used by IMEs) the same way the Android OS does.
,
Aug 7 2017
I think the right solution might be to do something like: - Rename DocumentMarker::kComposition to kImeFormattingSpan - Modify CompositionMarkerListImpl to support overlapping markers (and rename to ImeFormattingMarkerListImpl) - Rename CompositionUnderline (e.g. to IMESpan if we also want to use it for suggestion markers, or IMEFormattingSpan if not) Then InputMethodController can still keep track of the actual composition range in composition_range_, and it will be clear that the DocumentMarkers are just used for formatting.
,
Aug 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1b051d44ad4685de6d32a65f554d0a42bc0e6170 commit 1b051d44ad4685de6d32a65f554d0a42bc0e6170 Author: Ryan Landay <rlanday@chromium.org> Date: Mon Aug 28 19:59:59 2017 Rename DocumentMarkerListEditor to SortedDocumentMarkerListEditor I'm going to introduce a new class, UnsortedDocumentMarkerListEditor, for marker types that we store unsorted because they can overlap. Currently, suggestion markers are the only markers we have of this type, so the logic is implemented directly in SuggestionMarkerListImpl, but I also want to make CompositionMarkerListImpl use the same logic to fix crbug.com/752648 . Therefore, we need to rename DocumentMarkerListEditor to clarify that it should only be used for MarkerListImpls that store markers in sorted order. Bug: 752648 Change-Id: I796e8d23cb75a3c45e6c1639ad67bc266668910f Reviewed-on: https://chromium-review.googlesource.com/636181 Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Commit-Queue: Ryan Landay <rlanday@chromium.org> Cr-Commit-Position: refs/heads/master@{#497854} [modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/BUILD.gn [modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/ActiveSuggestionMarkerListImpl.cpp [modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp [modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp [delete] https://crrev.com/5302e35e0230a5661f081732b621241cfa00c4bf/third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditorTest.cpp [rename] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/SortedDocumentMarkerListEditor.cpp [rename] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/SortedDocumentMarkerListEditor.h [add] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/SortedDocumentMarkerListEditorTest.cpp [modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.cpp [modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/SuggestionMarkerListImpl.cpp [modify] https://crrev.com/1b051d44ad4685de6d32a65f554d0a42bc0e6170/third_party/WebKit/Source/core/editing/markers/TextMatchMarkerListImpl.cpp
,
Aug 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/54f103ed83effdc51456daa7212929f607b9e04d commit 54f103ed83effdc51456daa7212929f607b9e04d Author: Ryan Landay <rlanday@chromium.org> Date: Tue Aug 29 23:55:20 2017 Introduce UnsortedDocumentMarkerListEditor, part 1 This CL spins off some of SuggestionMarkerListImpl's method implementations into a new utility class, UnsortedDocumentMarkerListEditor, so we can reuse them in CompositionMarkerListImpl (which currently stores its markers in sorted order, but I want to change it to allow overlapping markers and store them in unsorted order to fix crbug.com/752648 ). Bug: 752648 Change-Id: Ia917784d3685f72e0906e77d7a57ba65199f1781 Reviewed-on: https://chromium-review.googlesource.com/636843 Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org> Commit-Queue: Ryan Landay <rlanday@chromium.org> Cr-Commit-Position: refs/heads/master@{#498291} [modify] https://crrev.com/54f103ed83effdc51456daa7212929f607b9e04d/third_party/WebKit/Source/core/editing/markers/SuggestionMarkerListImpl.cpp [modify] https://crrev.com/54f103ed83effdc51456daa7212929f607b9e04d/third_party/WebKit/Source/core/editing/markers/SuggestionMarkerListImplTest.cpp [add] https://crrev.com/54f103ed83effdc51456daa7212929f607b9e04d/third_party/WebKit/Source/core/editing/markers/UnsortedDocumentMarkerListEditor.h
,
Aug 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cb2f855cbc7b82e20387eaf9a43f6b99b6105061 commit cb2f855cbc7b82e20387eaf9a43f6b99b6105061 Author: Ryan Landay <rlanday@chromium.org> Date: Wed Aug 30 18:15:48 2017 Introduce UnsortedDocumentMarkerListEditor, part 2 In a previous CL, I moved some method implementations from SuggestionMarkerListImpl into a new class, UnsortedDocumentMarkerListEditor: https://chromium-review.googlesource.com/c/chromium/src/+/636843 In that CL, I left these method implementations in SuggestionMarkerListImpl.cpp to make the CL smaller. I also changed some test cases in SuggestionMarkerListImplTest to test UnsortedDocumentMarkerListEditor directly, and left those in SuggestionMarkerListImplTest.cpp. In this CL, I am moving these methods to UnsortedDocumentMarkerListEditor.cpp and UnsortedDocumentMarkerListEditorTest.cpp, respectively. Bug: 752648 Change-Id: Ib260fdde81c0637dfec29a5fdd786cb399e6b86f Reviewed-on: https://chromium-review.googlesource.com/639317 Commit-Queue: Ryan Landay <rlanday@chromium.org> Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Cr-Commit-Position: refs/heads/master@{#498537} [modify] https://crrev.com/cb2f855cbc7b82e20387eaf9a43f6b99b6105061/third_party/WebKit/Source/core/editing/BUILD.gn [modify] https://crrev.com/cb2f855cbc7b82e20387eaf9a43f6b99b6105061/third_party/WebKit/Source/core/editing/markers/SuggestionMarkerListImpl.cpp [modify] https://crrev.com/cb2f855cbc7b82e20387eaf9a43f6b99b6105061/third_party/WebKit/Source/core/editing/markers/SuggestionMarkerListImplTest.cpp [add] https://crrev.com/cb2f855cbc7b82e20387eaf9a43f6b99b6105061/third_party/WebKit/Source/core/editing/markers/UnsortedDocumentMarkerListEditor.cpp [add] https://crrev.com/cb2f855cbc7b82e20387eaf9a43f6b99b6105061/third_party/WebKit/Source/core/editing/markers/UnsortedDocumentMarkerListEditorTest.cpp
,
Aug 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5916acd85b148e534152b329ecc682bdcb7cd427 commit 5916acd85b148e534152b329ecc682bdcb7cd427 Author: Ryan Landay <rlanday@chromium.org> Date: Wed Aug 30 22:17:33 2017 Add UnsortedDocumentMarkerListEditor::ShiftMarkersContentIndependent() This method is not used by SuggestionMarkerListImpl, and therefore was not included in the original CLs to pull out SuggestionMarkerListImpl's methods into UnsortedDocumentMarkerListEditor. However, we do need it to implement CompositionMarkerListImpl on top of UnsortedDocumentMarkerListEditor, so I'm adding it here. Bug: 752648 Change-Id: Ibd006cdfb94a0060a9fc2c0962b92be9426e6528 Reviewed-on: https://chromium-review.googlesource.com/644145 Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org> Commit-Queue: Ryan Landay <rlanday@chromium.org> Cr-Commit-Position: refs/heads/master@{#498638} [modify] https://crrev.com/5916acd85b148e534152b329ecc682bdcb7cd427/third_party/WebKit/Source/core/editing/markers/UnsortedDocumentMarkerListEditor.cpp [modify] https://crrev.com/5916acd85b148e534152b329ecc682bdcb7cd427/third_party/WebKit/Source/core/editing/markers/UnsortedDocumentMarkerListEditorTest.cpp
,
Aug 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d36a47b56a1f0ea317f51de5124aa30d35cb6633 commit d36a47b56a1f0ea317f51de5124aa30d35cb6633 Author: Ryan Landay <rlanday@chromium.org> Date: Wed Aug 30 22:26:37 2017 Add support for overlapping markers in CompositionMarkerListImpl This CL changes CompositionMarkerListImpl from storing markers in sorted order (using methods from SortedDocumentMarkerListEditor) to storing markers in unsorted order (using methods from UnsortedDocumentMarkerListEditor), so we can officially support overlapping composition markers, as e.g. the Japanese IME is already creating. Bug: 752648 Change-Id: Ibf79a252eb891f08e9090056d241b87df3514d57 Reviewed-on: https://chromium-review.googlesource.com/644207 Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org> Commit-Queue: Ryan Landay <rlanday@chromium.org> Cr-Commit-Position: refs/heads/master@{#498643} [modify] https://crrev.com/d36a47b56a1f0ea317f51de5124aa30d35cb6633/third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp [modify] https://crrev.com/d36a47b56a1f0ea317f51de5124aa30d35cb6633/third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.h [modify] https://crrev.com/d36a47b56a1f0ea317f51de5124aa30d35cb6633/third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImplTest.cpp
,
Aug 30 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by rlanday@chromium.org
, Aug 4 2017