Use grammar UUID to deduplicate text effects instead of ranges.#64209
Conversation
|
EWS run on previous version of this PR (hash ec5014f) Details |
ec5014f to
d1b5223
Compare
|
EWS run on previous version of this PR (hash d1b5223) Details |
d1b5223 to
843e0ce
Compare
|
EWS run on previous version of this PR (hash 843e0ce) Details |
843e0ce to
9595fe0
Compare
|
EWS run on previous version of this PR (hash 9595fe0) Details
|
There was a problem hiding this comment.
Can grammarUUID be part of DocumentMarker::Data? It's too specific to belong at the top level addMarker.
9595fe0 to
0c1024a
Compare
|
EWS run on previous version of this PR (hash 0c1024a) Details
|
There was a problem hiding this comment.
| auto grammarUUID = grammarData ? grammarData->uuid : emptyString(); |
There was a problem hiding this comment.
This is only called when the Document is destroyed. Should we be removing UUIDs from this set somewhere else? For example, as words are deleted? Otherwise, the set grows forever.
There was a problem hiding this comment.
I did the same thing for ranges, this is not different. There's not a good way to figure out when the markers are removed, as we remove all grammar markers and re-apply them after every edit. We could set an arbitrary limit, but I don't know how else to realistically remove stale data from this list.
There was a problem hiding this comment.
I see... this is already much better then since we would have been leaking DOM nodes 😬
There was a problem hiding this comment.
We should think more about it in a follow-up. At a minimum we should probably remove from the set when the grammar marker itself is removed from the list of document markers?
There was a problem hiding this comment.
We can't do that because we remove all the document markers and then re-add them. So if we remove the UUIDs from the list, we just end up in the same state as we were before this or the previous patch :-/
There was a problem hiding this comment.
Is it possible for isGrammar to be true but for us not to have grammarData?
There was a problem hiding this comment.
It's technically possible to not have the UUID, but it's unlikely. I'm not sure what you are trying to figure out with your question?
There was a problem hiding this comment.
We could use std:get rather than std::get_if if DocumentMarkerType::Grammar will always have DocumentMarker::GrammarData. It's unrelated to whether there's a UUID or not, it's about whether that data object (which also contains the description) is always supplied/preserved when creating a grammar marker.
There was a problem hiding this comment.
I searched the code for DocumentMarkerType::Grammar, and there are a couple places that don't use DocumentMarker::GrammarData. If that's expected, then fine to keep using std::get_if. But do consider whether those call sites should also be updated.
pxlcoder
left a comment
There was a problem hiding this comment.
r+ but do consider: #64209 (comment)
0c1024a to
b4cd177
Compare
|
EWS run on previous version of this PR (hash b4cd177) Details
|
b4cd177 to
ee9be1a
Compare
|
EWS run on current version of this PR (hash ee9be1a) Details
|
https://bugs.webkit.org/show_bug.cgi?id=314018 rdar://176211949 Reviewed by Aditya Keerthi. The previous approach in 310132@main tracked applied grammar text effect ranges to avoid reapplying animations. However, the grammar checking API provides a UUID (NSGrammarUUID) for each grammar issue that is stable across re-checks of the same sentence. Using this UUID is simpler and more reliable than comparing ranges. If the UUID is not present, we fall back to always applying the effect, since we have no way to deduplicate without an identifier. Test: editing/spelling/grammar-text-effect-not-reapplied.html * LayoutTests/TestExpectations: * LayoutTests/editing/spelling/grammar-text-effect-not-reapplied-expected.txt: Added. * LayoutTests/editing/spelling/grammar-text-effect-not-reapplied.html: Added. * Source/WebCore/dom/DocumentMarker.h: (WebCore::DocumentMarker::description const): * Source/WebCore/dom/DocumentMarkerController.cpp: (WebCore::DocumentMarkerController::detach): (WebCore::DocumentMarkerController::addMarker): (WebCore::DocumentMarkerController::appliedGrammarTextEffectCount const): * Source/WebCore/dom/DocumentMarkerController.h: * Source/WebCore/editing/Editor.cpp: (WebCore::Editor::advanceToNextMisspelling): (WebCore::Editor::markAndReplaceFor): * Source/WebCore/editing/TextCheckingHelper.cpp: (WebCore::TextCheckingHelper::findUngrammaticalPhrases const): * Source/WebCore/platform/text/TextChecking.h: (WebCore::GrammarDetail::isolatedCopy): * Source/WebCore/testing/Internals.cpp: (WebCore::Internals::appliedGrammarTextEffectCount const): * Source/WebCore/testing/Internals.h: * Source/WebCore/testing/Internals.idl: * Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in: * Source/WebKit/UIProcess/ios/TextCheckerIOS.mm: (WebKit::TextChecker::checkTextOfParagraph): (WebKit::convertExtendedCheckingResults): * Source/WebKit/UIProcess/mac/TextCheckerMac.mm: (WebKit::TextChecker::checkTextOfParagraph): (WebKit::convertExtendedCheckingResults): * Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm: (core): * Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.mm: (-[LayoutTestSpellChecker setResultsFromJSValue:inContext:]): Canonical link: https://commits.webkit.org/312774@main
ee9be1a to
6b1f3f6
Compare
|
Committed 312774@main (6b1f3f6): https://commits.webkit.org/312774@main Reviewed commits have been landed. Closing PR #64209 and removing active labels. |
🛠 ios-apple
6b1f3f6
ee9be1a
🛠 win🧪 win-tests🧪 api-mac🧪 ios-wk2-wpt🧪 api-mac-debug🧪 api-ios🧪 mac-AS-debug-wk2🧪 mac-intel-wk2🛠 watch-sim