Bug 128353 - Consider making m_page in WebVideoFullscreenManagerProxy a reference
Summary: Consider making m_page in WebVideoFullscreenManagerProxy a reference
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-06 18:11 PST by Jon Lee
Modified: 2020-06-02 01:10 PDT (History)
5 users (show)

See Also:

Attachments
Patch (11.37 KB, patch)
2017-11-02 12:10 PDT, Jeremy Jones 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (4.18 MB, application/zip)
2017-11-02 14:37 PDT, Build Bot 		no flags Details
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2014-02-06 18:11:09 PST 
Follow-up from Simon's review for 128143:

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:60
> +    WebPageProxy* m_page;

Can this be a reference?
Comment 1 Jeremy Jones 2014-02-07 10:09:13 PST 
(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.
Comment 2 Simon Fraser (smfr) 2014-02-07 10:50:04 PST 
(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&.
Comment 3 Jeremy Jones 2017-11-02 12:05:09 PDT 
rdar://problem/35319903
Comment 4 Jeremy Jones 2017-11-02 12:10:38 PDT 
Created attachment 325742 [details]
Patch
Comment 5 Simon Fraser (smfr) 2017-11-02 13:00:44 PDT 
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 6 Build Bot 2017-11-02 14:37:08 PDT 
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
Comment 7 Build Bot 2017-11-02 14:37:09 PDT 
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
Comment 8 Jeremy Jones 2017-11-03 10:48:14 PDT 
Behaves correctly.
Comment 9 Maciej Stachowiak 2020-06-02 01:10:24 PDT 
Comment on attachment 325742 [details]
Patch

Unflagging and obsoleting, since this bug was resolved as invalid.