| Summary: | Add a WebViewDidMoveToWindowObserver for WKWebView | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||
| Component: | WebKit Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bfulgham, commit-queue, darin, jiewen_tan, thorton, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Jiewen Tan
2019-03-01 13:02:27 PST
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> |