| Summary: | Consider making m_page in WebVideoFullscreenManagerProxy a reference | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jon Lee <jonlee> | ||||||
| Component: | Media | Assignee: | Jeremy Jones <jeremyj-wk> | ||||||
| Status: | RESOLVED INVALID | ||||||||
| Severity: | Normal | CC: | buildbot, jer.noble, mjs, simon.fraser, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | iPhone / iPad | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
(In reply to comment #0) > Follow-up from Simon's review for 128143: > > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:60 > > + WebPageProxy* m_page; > > Can this be a reference? This follows the same pattern in WebFullScreenManagerProxy. WebPageProxy has a RefPointer to the WebVideoFullscreenManagerProxy, so I think using a RefPtr here would create a retain cycle. (In reply to comment #1) > (In reply to comment #0) > > Follow-up from Simon's review for 128143: > > > > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:60 > > > + WebPageProxy* m_page; > > > > Can this be a reference? > > This follows the same pattern in WebFullScreenManagerProxy. > > WebPageProxy has a RefPointer to the WebVideoFullscreenManagerProxy, so I think using a RefPtr here would create a retain cycle. Not RefPtr. WebPageProxy&. Created attachment 325742 [details]
Patch
Comment on attachment 325742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325742&action=review > Source/WebKit/ChangeLog:9 > + Cleanup. Use a WebPageProxy& instead of a pointer. Are we sure that we can do this, given than a WKWebView can outlive the WebPageProxy (see -[WKWebView _isValid])? Comment on attachment 325742 [details] Patch Attachment 325742 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5079739 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting.https.html Created attachment 325770 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Behaves correctly. Comment on attachment 325742 [details]
Patch
Unflagging and obsoleting, since this bug was resolved as invalid.
|
Follow-up from Simon's review for 128143: > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:60 > + WebPageProxy* m_page; Can this be a reference?