.
<rdar://problem/89434696>
Created attachment 454894 [details] [fast-cq] Patch
Comment on attachment 454894 [details] [fast-cq] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454894&action=review > Source/WebCore/page/FrameView.cpp:349 > +void FrameView::willReplaceMainFrameView(FrameView& oldMainFrameView) This seems reminiscent of the propagation that usually happens in e.g. WebFrameLoaderClient::transitionToCommittedForNewPage; is there are reason you're doing it a different way than everything else?
Created attachment 455257 [details] [fast-cq] Patch
Created attachment 455305 [details] [fast-cq] Patch
Comment on attachment 455305 [details] [fast-cq] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455305&action=review > Source/WebCore/page/FrameView.h:245 > + WEBCORE_EXPORT void copyCSSViewportUnits(FrameView&); Not sure about "copy" (would expect a method named "copy" to return a copy of something, but it does not). Also "copy CSS Viewport units" isn't quite right either, it's copying the viewport sizes for the units. IDK, I feel like the name could use a re-think. > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:626 > + @discussion For apps with size-changing UI around the @link WKWebView @/link, specify minimumViewportInset and "size-changing UI" reads a little weird, but I assume you'll review this text elsewhere > Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:-3271 > - _lastSentMinimumUnobscuredSize = newMinimumUnobscuredSize; > - _lastSentMaximumUnobscuredSize = newMaximumUnobscuredSize; I have some vague and non-concrete fears about some of the deletions here re: rotation in apps that use animated resize. We can chat elsewhere, but should make sure we are careful. > Source/WebKit/WebProcess/WebPage/WebPage.cpp:2444 > + maximumUnobscuredSize.scale(1 / m_viewportConfiguration.initialScaleIgnoringContentSize()); I don't remember the deal with initialScaleIgnoringContentSize vs. initialScale; was this choice well-considered? Does it make sense in all the weird viewport modes?
Comment on attachment 455305 [details] [fast-cq] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455305&action=review > Source/WebCore/page/FrameView.cpp:5636 > + if (m_defaultViewportSizeOverride && *m_defaultViewportSizeOverride == size) Nit - I think just `m_defaultViewportSizeOverride == size` should work here. > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1869 > +#if PLATFORM(IOS_FAMILY) > + Should this go in `WKWebViewIOS.mm`? >> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2444 >> + maximumUnobscuredSize.scale(1 / m_viewportConfiguration.initialScaleIgnoringContentSize()); > > I don't remember the deal with initialScaleIgnoringContentSize vs. initialScale; was this choice well-considered? Does it make sense in all the weird viewport modes? I _believe_ the rationale for using `initialScaleIgnoringContentSize()` here is that it's what (regular) viewport units use, in order to avoid an ever-changing initial scale when there's content that's sized using viewport units.
Comment on attachment 455305 [details] [fast-cq] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455305&action=review >> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:-3271 >> - _lastSentMaximumUnobscuredSize = newMaximumUnobscuredSize; > > I have some vague and non-concrete fears about some of the deletions here re: rotation in apps that use animated resize. We can chat elsewhere, but should make sure we are careful. Specifically and more concretely, my worry is about https://trac.webkit.org/changeset/221153/webkit and whether you have lost the fix for it...
Comment on attachment 455305 [details] [fast-cq] Patch style bot seems unhappy
Comment on attachment 455305 [details] [fast-cq] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455305&action=review >> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:626 >> + @discussion For apps with size-changing UI around the @link WKWebView @/link, specify minimumViewportInset and > > "size-changing UI" reads a little weird, but I assume you'll review this text elsewhere I did have this reviewed, and nobody else mentioned this 😅 Do you have a suggestion for an alternative? >> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1869 >> + > > Should this go in `WKWebViewIOS.mm`? I wanted to leave this here because I have a followup bug to add this API to macOS <https://webkit.org/b/238173> and figured it'd make things a bit easier/cleaner. >>> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:-3271 >>> - _lastSentMaximumUnobscuredSize = newMaximumUnobscuredSize; >> >> I have some vague and non-concrete fears about some of the deletions here re: rotation in apps that use animated resize. We can chat elsewhere, but should make sure we are careful. > > Specifically and more concretely, my worry is about https://trac.webkit.org/changeset/221153/webkit and whether you have lost the fix for it... I just moved these checks to `WebPageProxy::setDefaultUnobscuredSize`/`WebPageProxy::setMinimumUnobscuredSize`/`WebPageProxy::setMaximumUnobscuredSize` instead of having them be iOS only. This is because it's also used by the followup <https://webkit.org/b/238173>. I will test again just in case :) >>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2444 >>> + maximumUnobscuredSize.scale(1 / m_viewportConfiguration.initialScaleIgnoringContentSize()); >> >> I don't remember the deal with initialScaleIgnoringContentSize vs. initialScale; was this choice well-considered? Does it make sense in all the weird viewport modes? > > I _believe_ the rationale for using `initialScaleIgnoringContentSize()` here is that it's what (regular) viewport units use, in order to avoid an ever-changing initial scale when there's content that's sized using viewport units. FWIW this is existing code that was moved from `Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm` to support the followup <https://webkit.org/b/238173>.
Created attachment 455926 [details] [fast-cq] Patch
Committed r291980 (248939@main): <https://commits.webkit.org/248939@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 455926 [details].
Comment on attachment 455926 [details] [fast-cq] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455926&action=review > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1527 > + auto oldView = m_frame->coreFrame()->view(); This probably needs to be a RefPtr, right?
Comment on attachment 455926 [details] [fast-cq] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455926&action=review >> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1527 >> + auto oldView = m_frame->coreFrame()->view(); > > This probably needs to be a RefPtr, right? yyyuuuppp oops i'll fix this asap
Created attachment 456037 [details] [fast-cq] [Patch] fix UAF
Comment on attachment 455926 [details] [fast-cq] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455926&action=review >>> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1527 >>> + auto oldView = m_frame->coreFrame()->view(); >> >> This probably needs to be a RefPtr, right? > > yyyuuuppp oops > > i'll fix this asap r292042