Add a WebViewDidMoveToWindowObserver for WKWebView.
<rdar://problem/48520161>
Created attachment 363365 [details] Patch
Comment on attachment 363365 [details] Patch What is the "load optimizer"?
Comment on attachment 363365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363365&action=review > Source/WebKit/UIProcess/WebPageProxy.h:1467 > + void addWebViewDidMoveToWindowObserver(WebViewDidMoveToWindowObserver&); > + void removeWebViewDidMoveToWindowObserver(WebViewDidMoveToWindowObserver*); I think these functions could have shorter names. C++ overloading means they could have names as short as "add/remove", but I was thinking "addObserver/removeObserver". Can the remove function take a reference instead of a pointer? Is there a caller that needs to call it with null? > Source/WebKit/UIProcess/WebPageProxy.h:2389 > + HashMap<WebViewDidMoveToWindowObserver*, WeakPtr<WebViewDidMoveToWindowObserver>> m_webViewDidMoveToWindowObservers; Is a WeakPtr the optimal pattern here? Where else are we using that for observers? Since this entire observer function is just a "call this function" thing, maybe we could do something with functions instead? Unless this is a common pattern.
Comment on attachment 363365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363365&action=review Thanks Darin for r+ the patch. >> Source/WebKit/UIProcess/WebPageProxy.h:1467 >> + void removeWebViewDidMoveToWindowObserver(WebViewDidMoveToWindowObserver*); > > I think these functions could have shorter names. C++ overloading means they could have names as short as "add/remove", but I was thinking "addObserver/removeObserver". > > Can the remove function take a reference instead of a pointer? Is there a caller that needs to call it with null? Fixed. >> Source/WebKit/UIProcess/WebPageProxy.h:2389 >> + HashMap<WebViewDidMoveToWindowObserver*, WeakPtr<WebViewDidMoveToWindowObserver>> m_webViewDidMoveToWindowObservers; > > Is a WeakPtr the optimal pattern here? Where else are we using that for observers? > > Since this entire observer function is just a "call this function" thing, maybe we could do something with functions instead? Unless this is a common pattern. A more common pattern is to use HashSet<WebViewDidMoveToWindowObserver*> without weak ptrs. However, that relies on the observers to remove themselves from the set when they are about to destroy. Otherwise, UAF could happen. I could see pros and cons of using Functions here. Pros: More clean code; Leave UAF from the WebPageProxy to Functions suppliers. Cons: More memory. I don't really have opinions on which is significantly better than which. It looks like that it is a bigger topic to discuss than this patch. If you don't have strong opinions with my current approach, I will go ahead to land this patch. Maybe we could open a discussion on webkit-dev@lists.webkit.org?
Created attachment 363478 [details] Patch for landing
Comment on attachment 363478 [details] Patch for landing Clearing flags on attachment: 363478 Committed r242336: <https://trac.webkit.org/changeset/242336>
A follow up from Tim's comment is committed: Committed r242398: <https://trac.webkit.org/changeset/242398>