Skip to content

Writing Tools test CompositionWithMultipleUndosAndRestarts fails on mac.#30894

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
megangardner:eng/Writing-Tools-test-CompositionWithMultipleUndosAndRestarts-fails-on-mac-
Jul 22, 2024
Merged

Writing Tools test CompositionWithMultipleUndosAndRestarts fails on mac.#30894
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
megangardner:eng/Writing-Tools-test-CompositionWithMultipleUndosAndRestarts-fails-on-mac-

Conversation

@megangardner
Copy link
Contributor

@megangardner megangardner commented Jul 17, 2024

c59eb74

Writing Tools test CompositionWithMultipleUndosAndRestarts fails on mac.
https://bugs.webkit.org/show_bug.cgi?id=276700
rdar://131891447

Reviewed by Richard Robinson.

This test was failing because we get the final set of text to replace, we were
not comparing it against the previously replaced range correctly. The previous
range was adjusted to the the actual range of the text before it was stored
but we were comparing that against the full range of text, which was incorrect
and also if the replaced text was shorter, it would cause a debug assertion
which then caused the web process to crash and the completion handlers to not
be called and crashed the UI process as well.

This adds a way to have the completion handlers be called and not do any work, so
the UI process won't crash as well, and also allows for the handler to replace
the text without running the animation again, as the final replace is always the
same as the second to last replace, and there is nothing to animate for that final
replace, so we skip the animation step.

* Source/WebCore/page/ChromeClient.h:
(WebCore::ChromeClient::addSourceTextAnimation):
* Source/WebCore/page/writing-tools/WritingToolsController.mm:
(WebCore::WritingToolsController::compositionSessionDidReceiveTextWithReplacementRange):
* Source/WebCore/page/writing-tools/WritingToolsTypes.h:
* Source/WebKit/DerivedSources-input.xcfilelist:
* Source/WebKit/DerivedSources.make:
* Source/WebKit/Scripts/webkit/messages.py:
(headers_for_type):
* Source/WebKit/Shared/TextAnimationTypes.serialization.in: Renamed from Source/WebKit/Shared/TextAnimationType.serialization.in.
* Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _enableSourceTextAnimationAfterElementWithID:]):
(-[WKWebView _enableFinalTextAnimationForElementWithID:]):
* Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:
* Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:
(WebKit::WebPageProxy::addTextAnimationForAnimationID):
(WebKit::WebPageProxy::callCompletionHandlerForAnimationID):
* Source/WebKit/UIProcess/PageClient.h:
* Source/WebKit/UIProcess/TextAnimationTypes.h: Renamed from Source/WebKit/UIProcess/TextAnimationType.h.
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/WebPageProxy.messages.in:
* Source/WebKit/UIProcess/WebPageProxyInternals.h:
* Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView callCompletionHandlerForAnimationID:]):
* Source/WebKit/UIProcess/mac/WKTextAnimationManager.h:
* Source/WebKit/UIProcess/mac/WKTextAnimationManager.mm:
(-[WKTextAnimationManager addTextAnimationForAnimationID:withData:]):
(-[WKTextAnimationManager restoreTextAnimationType]):
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:
* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:
(WebKit::WebChromeClient::addSourceTextAnimation):
* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:
* Source/WebKit/WebProcess/WebPage/Cocoa/TextAnimationController.h:
* Source/WebKit/WebProcess/WebPage/Cocoa/TextAnimationController.mm:
(WebKit::remainingCharacterRange):
(WebKit::TextAnimationController::addInitialTextAnimation):
(WebKit::TextAnimationController::addSourceTextAnimation):
(WebKit::TextAnimationController::addDestinationTextAnimation):
* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::addTextAnimationForAnimationID):
(WebKit::WebPage::addSourceTextAnimation):
* Source/WebKit/WebProcess/WebPage/WebPage.h:

Canonical link: https://commits.webkit.org/281219@main

fee736c

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac 🧪 api-wpe
✅ 🧪 webkitpy ✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2
✅ 🛠 tv
✅ 🛠 tv-sim
✅ 🛠 watch
loading 🛠 watch-sim

@megangardner megangardner requested a review from cdumez as a code owner July 17, 2024 03:24
@megangardner megangardner self-assigned this Jul 17, 2024
@megangardner megangardner added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Jul 17, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 17, 2024
@rr-codes
Copy link
Contributor

as also allows

should be

and also allows

in the commit message

@megangardner megangardner removed the merging-blocked Applied to prevent a change from being merged label Jul 18, 2024
@megangardner megangardner force-pushed the eng/Writing-Tools-test-CompositionWithMultipleUndosAndRestarts-fails-on-mac- branch from 83c6835 to c589b7d Compare July 18, 2024 19:09
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 18, 2024
@megangardner megangardner removed the merging-blocked Applied to prevent a change from being merged label Jul 18, 2024
@megangardner megangardner force-pushed the eng/Writing-Tools-test-CompositionWithMultipleUndosAndRestarts-fails-on-mac- branch from c589b7d to d0d08d3 Compare July 18, 2024 20:06
@rr-codes
Copy link
Contributor

@megangardner megangardner force-pushed the eng/Writing-Tools-test-CompositionWithMultipleUndosAndRestarts-fails-on-mac- branch from d0d08d3 to 539350b Compare July 19, 2024 04:22
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 19, 2024
@megangardner megangardner removed the merging-blocked Applied to prevent a change from being merged label Jul 19, 2024
@megangardner megangardner force-pushed the eng/Writing-Tools-test-CompositionWithMultipleUndosAndRestarts-fails-on-mac- branch from 539350b to d3d76a2 Compare July 19, 2024 19:22
@megangardner megangardner force-pushed the eng/Writing-Tools-test-CompositionWithMultipleUndosAndRestarts-fails-on-mac- branch from d3d76a2 to fb19112 Compare July 19, 2024 19:28
@megangardner megangardner added the merge-queue Applied to send a pull request to merge-queue label Jul 19, 2024
@webkit-commit-queue webkit-commit-queue added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels Jul 19, 2024
@megangardner megangardner removed the merging-blocked Applied to prevent a change from being merged label Jul 22, 2024
@megangardner megangardner force-pushed the eng/Writing-Tools-test-CompositionWithMultipleUndosAndRestarts-fails-on-mac- branch from fb19112 to fee736c Compare July 22, 2024 19:42
@megangardner megangardner added the merge-queue Applied to send a pull request to merge-queue label Jul 22, 2024
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Writing-Tools-test-CompositionWithMultipleUndosAndRestarts-fails-on-mac- branch from fee736c to 9b9eae7 Compare July 22, 2024 23:24
https://bugs.webkit.org/show_bug.cgi?id=276700
rdar://131891447

Reviewed by Richard Robinson.

This test was failing because we get the final set of text to replace, we were
not comparing it against the previously replaced range correctly. The previous
range was adjusted to the the actual range of the text before it was stored
but we were comparing that against the full range of text, which was incorrect
and also if the replaced text was shorter, it would cause a debug assertion
which then caused the web process to crash and the completion handlers to not
be called and crashed the UI process as well.

This adds a way to have the completion handlers be called and not do any work, so
the UI process won't crash as well, and also allows for the handler to replace
the text without running the animation again, as the final replace is always the
same as the second to last replace, and there is nothing to animate for that final
replace, so we skip the animation step.

* Source/WebCore/page/ChromeClient.h:
(WebCore::ChromeClient::addSourceTextAnimation):
* Source/WebCore/page/writing-tools/WritingToolsController.mm:
(WebCore::WritingToolsController::compositionSessionDidReceiveTextWithReplacementRange):
* Source/WebCore/page/writing-tools/WritingToolsTypes.h:
* Source/WebKit/DerivedSources-input.xcfilelist:
* Source/WebKit/DerivedSources.make:
* Source/WebKit/Scripts/webkit/messages.py:
(headers_for_type):
* Source/WebKit/Shared/TextAnimationTypes.serialization.in: Renamed from Source/WebKit/Shared/TextAnimationType.serialization.in.
* Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _enableSourceTextAnimationAfterElementWithID:]):
(-[WKWebView _enableFinalTextAnimationForElementWithID:]):
* Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:
* Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:
(WebKit::WebPageProxy::addTextAnimationForAnimationID):
(WebKit::WebPageProxy::callCompletionHandlerForAnimationID):
* Source/WebKit/UIProcess/PageClient.h:
* Source/WebKit/UIProcess/TextAnimationTypes.h: Renamed from Source/WebKit/UIProcess/TextAnimationType.h.
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/WebPageProxy.messages.in:
* Source/WebKit/UIProcess/WebPageProxyInternals.h:
* Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView callCompletionHandlerForAnimationID:]):
* Source/WebKit/UIProcess/mac/WKTextAnimationManager.h:
* Source/WebKit/UIProcess/mac/WKTextAnimationManager.mm:
(-[WKTextAnimationManager addTextAnimationForAnimationID:withData:]):
(-[WKTextAnimationManager restoreTextAnimationType]):
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:
* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:
(WebKit::WebChromeClient::addSourceTextAnimation):
* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:
* Source/WebKit/WebProcess/WebPage/Cocoa/TextAnimationController.h:
* Source/WebKit/WebProcess/WebPage/Cocoa/TextAnimationController.mm:
(WebKit::remainingCharacterRange):
(WebKit::TextAnimationController::addInitialTextAnimation):
(WebKit::TextAnimationController::addSourceTextAnimation):
(WebKit::TextAnimationController::addDestinationTextAnimation):
* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::addTextAnimationForAnimationID):
(WebKit::WebPage::addSourceTextAnimation):
* Source/WebKit/WebProcess/WebPage/WebPage.h:

Canonical link: https://commits.webkit.org/281219@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Writing-Tools-test-CompositionWithMultipleUndosAndRestarts-fails-on-mac- branch from 9b9eae7 to c59eb74 Compare July 22, 2024 23:26
@webkit-commit-queue
Copy link
Collaborator

Committed 281219@main (c59eb74): https://commits.webkit.org/281219@main

Reviewed commits have been landed. Closing PR #30894 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit c59eb74 into WebKit:main Jul 22, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants