Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a separate animation ID from the session ID in all cases, including initial animation. #30009

Conversation

megangardner
Copy link
Contributor

@megangardner megangardner commented Jun 20, 2024

c5c0cb2

Use a separate animation ID from the session ID in all cases, including initial animation.
https://bugs.webkit.org/show_bug.cgi?id=275700
rdar://130217887

Reviewed by Richard Robinson.

Perviously, I was using the sessionUUID for the first animation. This has made it difficult
to write clean and compartmentalized code, and caused issues where the wrong UUID was used.
This patch separated these into two separate concepts, and added mapping storage in the animation
controller. I also clean up some of the naming for the clean up code, and generally make the code
easier to understand.

* Source/WebCore/page/ChromeClient.h:
(WebCore::ChromeClient::removeTextAnimationForAnimationID):
(WebCore::ChromeClient::removeTransparentMarkersForSessionID):
(WebCore::ChromeClient::removeInitialTextAnimation):
(WebCore::ChromeClient::addInitialTextAnimation):
(WebCore::ChromeClient::removeTextAnimationForID): Deleted.
(WebCore::ChromeClient::cleanUpTextAnimationsForSessionID): Deleted.
* Source/WebCore/page/writing-tools/WritingToolsController.mm:
(WebCore::WritingToolsController::didBeginWritingToolsSession):
(WebCore::WritingToolsController::proofreadingSessionDidReceiveSuggestions):
(WebCore::WritingToolsController::compositionSessionDidReceiveTextWithReplacementRange):
(WebCore::WritingToolsController::didEndWritingToolsSession):
* Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _addTextAnimationForAnimationID:withData:]):
(-[WKWebView _removeTextAnimationForAnimationID:]):
(-[WKWebView didBeginWritingToolsSession:contexts:]):
(-[WKWebView _enableSourceTextAnimationAfterElementWithID:]):
(-[WKWebView _enableFinalTextAnimationForElementWithID:]):
(-[WKWebView _disableTextAnimationWithUUID:]):
(-[WKWebView _addTextAnimationTypeForID:withData:]): Deleted.
(-[WKWebView _removeTextAnimationForID:]): Deleted.
(-[WKWebView beginWritingToolsAnimationForSessionWithUUID:]): Deleted.
(-[WKWebView endWritingToolsAnimationForSessionWithUUID:]): Deleted.
* Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h:
* Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.h:
* Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:
(WebKit::PageClientImplCocoa::addTextAnimationForAnimationID):
(WebKit::PageClientImplCocoa::removeTextAnimationForAnimationID):
(WebKit::PageClientImplCocoa::addTextAnimationTypeForID): Deleted.
(WebKit::PageClientImplCocoa::removeTextAnimationForID): Deleted.
* Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:
(WebKit::WebPageProxy::addTextAnimationForAnimationID):
(WebKit::WebPageProxy::removeTextAnimationForAnimationID):
(WebKit::WebPageProxy::addTextAnimationTypeForID): Deleted.
(WebKit::WebPageProxy::removeTextAnimationForID): Deleted.
* Source/WebKit/UIProcess/PageClient.h:
* Source/WebKit/UIProcess/WKSTextAnimationManager.h:
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/WebPageProxy.messages.in:
* Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:
* Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView addTextAnimationForAnimationID:withStyleType:]):
(-[WKContentView removeTextAnimationForAnimationID:]):
(-[WKContentView addTextAnimationTypeForID:withStyleType:]): Deleted.
(-[WKContentView removeTextAnimationForID:]): Deleted.
* Source/WebKit/UIProcess/mac/WKTextAnimationManager.h:
* Source/WebKit/UIProcess/mac/WKTextAnimationManager.mm:
(-[WKTextAnimationManager addTextAnimationForAnimationID:withData:]):
(-[WKTextAnimationManager removeTextAnimationForAnimationID:]):
(-[WKTextAnimationManager restoreTextAnimationType]):
(-[WKTextAnimationManager addTextAnimationTypeForID:withData:]): Deleted.
(-[WKTextAnimationManager removeTextAnimationForID:]): Deleted.
* Source/WebKit/UIProcess/mac/WebViewImpl.h:
* Source/WebKit/UIProcess/mac/WebViewImpl.mm:
(WebKit::WebViewImpl::addTextAnimationForAnimationID):
(WebKit::WebViewImpl::removeTextAnimationForAnimationID):
(WebKit::WebViewImpl::addTextAnimationTypeForID): Deleted.
(WebKit::WebViewImpl::removeTextAnimationForID): Deleted.
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:
* Source/WebKit/WebKitSwift/TextAnimation/TextAnimationManager.swift:
(beginEffect(for:style:)):
(endEffect(for:)):
* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:
(WebKit::WebChromeClient::removeTextAnimationForAnimationID):
(WebKit::WebChromeClient::removeTransparentMarkersForSessionID):
(WebKit::WebChromeClient::removeInitialTextAnimation):
(WebKit::WebChromeClient::addInitialTextAnimation):
(WebKit::WebChromeClient::removeTextAnimationForID): Deleted.
(WebKit::WebChromeClient::cleanUpTextAnimationsForSessionID): Deleted.
* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:
* Source/WebKit/WebProcess/WebPage/Cocoa/TextAnimationController.h: Renamed from Source/WebKit/WebProcess/WebPage/TextAnimationController.h.
* Source/WebKit/WebProcess/WebPage/Cocoa/TextAnimationController.mm:
(WebKit::TextAnimationController::contextRangeForTextAnimationID const):
(WebKit::TextAnimationController::removeTransparentMarkersForSessionID):
(WebKit::TextAnimationController::removeInitialTextAnimation):
(WebKit::TextAnimationController::addInitialTextAnimation):
(WebKit::TextAnimationController::addSourceTextAnimation):
(WebKit::TextAnimationController::addDestinationTextAnimation):
(WebKit::TextAnimationController::updateUnderlyingTextVisibilityForTextAnimationID):
(WebKit::TextAnimationController::createTextIndicatorForTextAnimationID):
(WebKit::TextAnimationController::contextRangeForTextAnimationType const): Deleted.
(WebKit::TextAnimationController::cleanUpTextAnimationsForSessionID): Deleted.
* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::addTextAnimationForAnimationID):
(WebKit::WebPage::removeTextAnimationForAnimationID):
(WebKit::WebPage::removeTransparentMarkersForSessionID):
(WebKit::WebPage::removeInitialTextAnimation):
(WebKit::WebPage::addInitialTextAnimation):
(WebKit::WebPage::addTextAnimationTypeForID): Deleted.
(WebKit::WebPage::removeTextAnimationForID): Deleted.
(WebKit::WebPage::cleanUpTextAnimationsForSessionID): Deleted.
* Source/WebKit/WebProcess/WebPage/WebPage.h:

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

0f4399f

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
βœ… πŸ§ͺ 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
βœ… πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  tv
βœ… πŸ›  tv-sim
βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@megangardner megangardner self-assigned this Jun 20, 2024
@megangardner megangardner added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Jun 20, 2024
@pxlcoder
Copy link
Member

Typo in commit title: seperate -> separate

@megangardner megangardner force-pushed the eng/Use-a-seperate-animation-ID-from-the-session-ID-in-all-cases-including-initial-animation- branch from 77a0607 to 7b837a2 Compare June 20, 2024 19:48
@megangardner megangardner changed the title Use a seperate animation ID from the session ID in all cases, including initial animation. Use a separate animation ID from the session ID in all cases, including initial animation. Jun 20, 2024
@megangardner megangardner force-pushed the eng/Use-a-seperate-animation-ID-from-the-session-ID-in-all-cases-including-initial-animation- branch from 7b837a2 to f87eafe Compare June 20, 2024 23:34
@megangardner megangardner added safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks merge-queue Applied to send a pull request to merge-queue and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks merge-queue Applied to send a pull request to merge-queue labels Jun 20, 2024
@megangardner megangardner force-pushed the eng/Use-a-seperate-animation-ID-from-the-session-ID-in-all-cases-including-initial-animation- branch from f87eafe to 0f4399f Compare June 24, 2024 22:54
@megangardner megangardner added the merge-queue Applied to send a pull request to merge-queue label Jun 24, 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 Jun 25, 2024
@megangardner megangardner added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels Jun 25, 2024
…ng initial animation.

https://bugs.webkit.org/show_bug.cgi?id=275700
rdar://130217887

Reviewed by Richard Robinson.

Perviously, I was using the sessionUUID for the first animation. This has made it difficult
to write clean and compartmentalized code, and caused issues where the wrong UUID was used.
This patch separated these into two separate concepts, and added mapping storage in the animation
controller. I also clean up some of the naming for the clean up code, and generally make the code
easier to understand.

* Source/WebCore/page/ChromeClient.h:
(WebCore::ChromeClient::removeTextAnimationForAnimationID):
(WebCore::ChromeClient::removeTransparentMarkersForSessionID):
(WebCore::ChromeClient::removeInitialTextAnimation):
(WebCore::ChromeClient::addInitialTextAnimation):
(WebCore::ChromeClient::removeTextAnimationForID): Deleted.
(WebCore::ChromeClient::cleanUpTextAnimationsForSessionID): Deleted.
* Source/WebCore/page/writing-tools/WritingToolsController.mm:
(WebCore::WritingToolsController::didBeginWritingToolsSession):
(WebCore::WritingToolsController::proofreadingSessionDidReceiveSuggestions):
(WebCore::WritingToolsController::compositionSessionDidReceiveTextWithReplacementRange):
(WebCore::WritingToolsController::didEndWritingToolsSession):
* Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _addTextAnimationForAnimationID:withData:]):
(-[WKWebView _removeTextAnimationForAnimationID:]):
(-[WKWebView didBeginWritingToolsSession:contexts:]):
(-[WKWebView _enableSourceTextAnimationAfterElementWithID:]):
(-[WKWebView _enableFinalTextAnimationForElementWithID:]):
(-[WKWebView _disableTextAnimationWithUUID:]):
(-[WKWebView _addTextAnimationTypeForID:withData:]): Deleted.
(-[WKWebView _removeTextAnimationForID:]): Deleted.
(-[WKWebView beginWritingToolsAnimationForSessionWithUUID:]): Deleted.
(-[WKWebView endWritingToolsAnimationForSessionWithUUID:]): Deleted.
* Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h:
* Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.h:
* Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:
(WebKit::PageClientImplCocoa::addTextAnimationForAnimationID):
(WebKit::PageClientImplCocoa::removeTextAnimationForAnimationID):
(WebKit::PageClientImplCocoa::addTextAnimationTypeForID): Deleted.
(WebKit::PageClientImplCocoa::removeTextAnimationForID): Deleted.
* Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:
(WebKit::WebPageProxy::addTextAnimationForAnimationID):
(WebKit::WebPageProxy::removeTextAnimationForAnimationID):
(WebKit::WebPageProxy::addTextAnimationTypeForID): Deleted.
(WebKit::WebPageProxy::removeTextAnimationForID): Deleted.
* Source/WebKit/UIProcess/PageClient.h:
* Source/WebKit/UIProcess/WKSTextAnimationManager.h:
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/WebPageProxy.messages.in:
* Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:
* Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView addTextAnimationForAnimationID:withStyleType:]):
(-[WKContentView removeTextAnimationForAnimationID:]):
(-[WKContentView addTextAnimationTypeForID:withStyleType:]): Deleted.
(-[WKContentView removeTextAnimationForID:]): Deleted.
* Source/WebKit/UIProcess/mac/WKTextAnimationManager.h:
* Source/WebKit/UIProcess/mac/WKTextAnimationManager.mm:
(-[WKTextAnimationManager addTextAnimationForAnimationID:withData:]):
(-[WKTextAnimationManager removeTextAnimationForAnimationID:]):
(-[WKTextAnimationManager restoreTextAnimationType]):
(-[WKTextAnimationManager addTextAnimationTypeForID:withData:]): Deleted.
(-[WKTextAnimationManager removeTextAnimationForID:]): Deleted.
* Source/WebKit/UIProcess/mac/WebViewImpl.h:
* Source/WebKit/UIProcess/mac/WebViewImpl.mm:
(WebKit::WebViewImpl::addTextAnimationForAnimationID):
(WebKit::WebViewImpl::removeTextAnimationForAnimationID):
(WebKit::WebViewImpl::addTextAnimationTypeForID): Deleted.
(WebKit::WebViewImpl::removeTextAnimationForID): Deleted.
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:
* Source/WebKit/WebKitSwift/TextAnimation/TextAnimationManager.swift:
(beginEffect(for:style:)):
(endEffect(for:)):
* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:
(WebKit::WebChromeClient::removeTextAnimationForAnimationID):
(WebKit::WebChromeClient::removeTransparentMarkersForSessionID):
(WebKit::WebChromeClient::removeInitialTextAnimation):
(WebKit::WebChromeClient::addInitialTextAnimation):
(WebKit::WebChromeClient::removeTextAnimationForID): Deleted.
(WebKit::WebChromeClient::cleanUpTextAnimationsForSessionID): Deleted.
* Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:
* Source/WebKit/WebProcess/WebPage/Cocoa/TextAnimationController.h: Renamed from Source/WebKit/WebProcess/WebPage/TextAnimationController.h.
* Source/WebKit/WebProcess/WebPage/Cocoa/TextAnimationController.mm:
(WebKit::TextAnimationController::contextRangeForTextAnimationID const):
(WebKit::TextAnimationController::removeTransparentMarkersForSessionID):
(WebKit::TextAnimationController::removeInitialTextAnimation):
(WebKit::TextAnimationController::addInitialTextAnimation):
(WebKit::TextAnimationController::addSourceTextAnimation):
(WebKit::TextAnimationController::addDestinationTextAnimation):
(WebKit::TextAnimationController::updateUnderlyingTextVisibilityForTextAnimationID):
(WebKit::TextAnimationController::createTextIndicatorForTextAnimationID):
(WebKit::TextAnimationController::contextRangeForTextAnimationType const): Deleted.
(WebKit::TextAnimationController::cleanUpTextAnimationsForSessionID): Deleted.
* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::addTextAnimationForAnimationID):
(WebKit::WebPage::removeTextAnimationForAnimationID):
(WebKit::WebPage::removeTransparentMarkersForSessionID):
(WebKit::WebPage::removeInitialTextAnimation):
(WebKit::WebPage::addInitialTextAnimation):
(WebKit::WebPage::addTextAnimationTypeForID): Deleted.
(WebKit::WebPage::removeTextAnimationForID): Deleted.
(WebKit::WebPage::cleanUpTextAnimationsForSessionID): Deleted.
* Source/WebKit/WebProcess/WebPage/WebPage.h:

Canonical link: https://commits.webkit.org/280319@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Use-a-seperate-animation-ID-from-the-session-ID-in-all-cases-including-initial-animation- branch from 0f4399f to c5c0cb2 Compare June 25, 2024 05:38
@webkit-commit-queue
Copy link
Collaborator

Committed 280319@main (c5c0cb2): https://commits.webkit.org/280319@main

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

@webkit-commit-queue webkit-commit-queue merged commit c5c0cb2 into WebKit:main Jun 25, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 25, 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
5 participants