WKWebView.URL should not need not parse the URL string every time it is called#62522
Conversation
|
EWS run on previous version of this PR (hash 150f7ed) Details
|
macOS Safer C++ Build #92237 (150f7ed)❌ Found 8 failing files with 19 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
150f7ed to
72eddd1
Compare
|
EWS run on previous version of this PR (hash 72eddd1) Details |
macOS Safer C++ Build #92245 (72eddd1)❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
72eddd1 to
aa25b31
Compare
|
EWS run on previous version of this PR (hash aa25b31) Details
|
| @@ -86,7 +86,7 @@ - (NSString *)title | |||
|
|
|||
| - (NSURL *)URL | |||
| { | |||
| return [NSURL _web_URLWithWTFString:protect(protect(*_page)->pageLoadState())->activeURL()]; | |||
| return protect(protect(*_page)->pageLoadState())->activeURL().createNSURL().autorelease(); | |||
There was a problem hiding this comment.
Why do we need protect() here? Other places doesn't use it.
| @@ -86,7 +86,7 @@ - (NSString *)title | |||
|
|
|||
| - (NSURL *)URL | |||
| { | |||
| return [NSURL _web_URLWithWTFString:protect(protect(*_page)->pageLoadState())->activeURL()]; | |||
| return protect(protect(*_page)->pageLoadState())->activeURL().createNSURL().autorelease(); | |||
There was a problem hiding this comment.
The premise of this change is that these methods are called repeatedly, and if so we still will do repetitive unnecessary work, creating a new NSURL every time. I could imagine storing a String/NSURL pair and returning that same NSURL unless the string has changed. This should be even more effective than the change here at speeding up this method.
We could do either or both improvements.
A separate decision is whether to autorelease the NSURL each time. In theory that would no longer be needed but there is a small risk that callers accidentally rely on the NSURL lifetime continuing past the lifetime of the owning object or past a future call to the same method returning a different URL.
We could create a C++ object implementing this idiom for reuse, and do this for all the NSURL-returning functions.
There was a problem hiding this comment.
I'm fixing a radar where we're spinning in the URLParser when calling WKWebView.URL so it made sense for me to focus on avoiding calling the URLParser. That said, it is true that we also recreate a NSURL every time and this can be avoided too. I'll take a look.
There was a problem hiding this comment.
Some things to consider: Maybe this is obvious but memoizing the NSURL would also avoid calling the parser. Creating an NSURL from a WebKit URL also involves URL parsing, although perhaps it's more efficient than WebKit's parsing or maybe it's got a laziness mechanism.
There was a problem hiding this comment.
Not calling autorelease() caused some crashes in the tests so I'm going to keep autoreleasing for now, especially considering that this is likely going to get cherry-picked to a branch.
aa25b31 to
fbab551
Compare
|
EWS run on previous version of this PR (hash fbab551) Details
|
fbab551 to
c17138e
Compare
|
EWS run on current version of this PR (hash c17138e) Details
|
…is called https://bugs.webkit.org/show_bug.cgi?id=312009 rdar://174434204 Reviewed by Darin Adler. Update PageLoadState to store urls as URL, not String. Otherwise, APIs like WKWebView.URL have to keep re-parsing the URL string every thing they're called. * Source/WTF/wtf/URL.cpp: (WTF::aboutBlankURL): (WTF::aboutSrcDocURL): * Source/WTF/wtf/URL.h: * Source/WebKit/UIProcess/API/C/WKPage.cpp: (WKPageSetPageUIClient): (WKPageCopyPendingAPIRequestURL): * Source/WebKit/UIProcess/API/C/mac/WKPagePrivateMac.mm: (-[WKObservablePageState URL]): (-[WKObservablePageState unreachableURL]): * Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm: (-[WKWebView URL]): (-[WKWebView _unreachableURL]): (-[WKWebView _committedURL]): * Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h: * Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp: (PageLoadStateObserver::didChangeActiveURL): (webkitWebViewWillStartLoad): (webkitWebViewLoadChanged): (webkitWebViewGetLoadDecisionForIcons): (webkitWebViewUpdatePageIcons): (webkitWebViewSetIcon): * Source/WebKit/UIProcess/API/gtk/WebKitRemoteInspectorProtocolHandler.cpp: * Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp: (webkitWebViewScriptDialog): * Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp: (WebKit::WebAutomationSession::buildBrowsingContextForPage): (WebKit::WebAutomationSession::addSingleCookie): (WebKit::WebAutomationSession::deleteAllCookies): * Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.h: * Source/WebKit/UIProcess/Cocoa/UIDelegate.mm: (WebKit::UIDelegate::UIClient::decidePolicyForGeolocationPermissionRequest): (WebKit::UIDelegate::UIClient::shouldAllowDeviceOrientationAndMotionAccess): * Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm: (WebKit::WebPageProxy::shouldAllowAutoFillForCellularIdentifiers const): * Source/WebKit/UIProcess/Inspector/socket/RemoteInspectorProtocolHandler.cpp: * Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp: (WebKit::NetworkProcessProxy::deleteWebsiteDataInWebProcessesForOrigin): * Source/WebKit/UIProcess/PageLoadState.cpp: (WebKit::PageLoadState::reset): (WebKit::PageLoadState::activeURL): (WebKit::PageLoadState::hasOnlySecureContent): (WebKit::PageLoadState::didExplicitOpen): (WebKit::PageLoadState::didStartProvisionalLoad): (WebKit::PageLoadState::didReceiveServerRedirectForProvisionalLoad): (WebKit::PageLoadState::didFailProvisionalLoad): (WebKit::PageLoadState::didCommitLoad): (WebKit::PageLoadState::didSameDocumentNavigation): (WebKit::PageLoadState::setUnreachableURL): * Source/WebKit/UIProcess/PageLoadState.h: (WebKit::PageLoadState::activeURL const): * Source/WebKit/UIProcess/RemotePageProxy.cpp: (WebKit::RemotePageProxy::injectPageIntoNewProcess): (WebKit::RemotePageProxy::setDrawingArea): * Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp: (WebKit::UserMediaPermissionRequestManagerProxy::decidePolicyForUserMediaPermissionRequest): (WebKit::UserMediaPermissionRequestManagerProxy::checkUserMediaPermissionForSpeechRecognition): (WebKit::UserMediaPermissionRequestManagerProxy::shouldChangeDeniedToPromptForCamera const): (WebKit::UserMediaPermissionRequestManagerProxy::shouldChangeDeniedToPromptForMicrophone const): (WebKit::UserMediaPermissionRequestManagerProxy::getUserMediaPermissionInfo): * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::launchProcessForReload): (WebKit::WebPageProxy::loadRequestWithNavigationShared): (WebKit::WebPageProxy::loadFile): (WebKit::WebPageProxy::loadDataWithNavigationShared): (WebKit::WebPageProxy::loadSimulatedRequest): (WebKit::WebPageProxy::loadAlternateHTML): (WebKit::WebPageProxy::reload): (WebKit::WebPageProxy::goToBackForwardItem): (WebKit::WebPageProxy::receivedNavigationActionPolicyDecision): (WebKit::WebPageProxy::continueNavigationInNewProcess): (WebKit::WebPageProxy::sessionState const): (WebKit::WebPageProxy::didStartProvisionalLoadForFrameShared): (WebKit::WebPageProxy::didExplicitOpenForFrame): (WebKit::WebPageProxy::didReceiveServerRedirectForProvisionalLoadForFrameShared): (WebKit::WebPageProxy::didChangeProvisionalURLForFrameShared): (WebKit::WebPageProxy::didSameDocumentNavigationForFrame): (WebKit::WebPageProxy::didSameDocumentNavigationForFrameViaJS): (WebKit::WebPageProxy::decidePolicyForNavigationAction): (WebKit::WebPageProxy::decidePolicyForResponseShared): (WebKit::WebPageProxy::currentURL const): (WebKit::WebPageProxy::tryReloadAfterProcessTermination): * Source/WebKit/UIProcess/WebProcessPool.cpp: (WebKit::WebProcessPool::processForNavigationInternal): * Source/WebKit/UIProcess/WebsiteData/WebDeviceOrientationAndMotionAccessController.cpp: (WebKit::WebDeviceOrientationAndMotionAccessController::shouldAllowAccess): * Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp: (WebKit::WebsiteDataStore::download): * Tools/TestWebKitAPI/Tests/WebKit/WKWebView/WKBackForwardListTests.mm: (TEST(WKBackForwardList, InteractionStateRestoration)): (TEST(WKBackForwardList, InteractionStateRestorationNil)): (TEST(WKBackForwardList, InteractionStateRestorationInvalid)): (TEST(WKBackForwardList, InteractionStateRestorationMultipleItems)): (TEST(WKBackForwardList, BackSwipeNavigationSkipsItemsWithoutUserGesture)): (TEST(WKBackForwardList, BackSwipeNavigationDoesNotSkipItemsWithUserGesture)): (runBackForwardNavigationSkipsItemsWithoutUserGestureTest): (runBackForwardNavigationDoesNotSkipItemsWithUserGestureTest): (TEST(WKBackForwardList, BackForwardNavigationDoesNotSkipUpdatedItemWithRecentUserGesture)): (TEST(WKBackForwardList, BackNavigationHijacking)): * Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp: (WebViewTest::loadURI): (WebViewTest::loadHtml): (WebViewTest::loadBytes): (WebViewTest::loadRequest): (WebViewTest::loadAlternateHTML): Canonical link: https://commits.webkit.org/311033@main
c17138e to
56d2552
Compare
|
Committed 311033@main (56d2552): https://commits.webkit.org/311033@main Reviewed commits have been landed. Closing PR #62522 and removing active labels. |
|
|
||
| RetainPtr<WKScrollGeometry> _currentScrollGeometry; | ||
|
|
||
| std::pair<URL, RetainPtr<NSURL>> _cachedActiveNSURL; |
There was a problem hiding this comment.
We don’t need to store the URL. Since all we do is compare the URL with a new URL can just store the URL’s string and compare the URL’s string with the new URL’s string.
There was a problem hiding this comment.
Ok. I was trying to move away from storing URLs as Strings but it doesn't hurt here since we don't need to ever re-parse it and would save some memory. I'll follow-up.
🛠 vision
56d2552
c17138e
🛠 win🧪 win-tests