Bug 185887

<rdar://problem/40463929>

Created attachment 341026 [details] Patch v1

Comment on attachment 341026 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=341026&action=review > Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.mm:69 > +@property (nonatomic, assign) UIVisualEffectView *secondaryMaterialOverlayView; Everything looks good except this seems totally sketchy. Should we just make it a (retain) computed member variable, or a method?

Comment on attachment 341026 [details] Patch v1 Attachment 341026 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7770535 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html http/tests/preload/onload_event.html http/tests/security/video-poster-cross-origin-crash2.html

Created attachment 341048 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment on attachment 341026 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=341026&action=review >> Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.mm:69 >> +@property (nonatomic, assign) UIVisualEffectView *secondaryMaterialOverlayView; > > Everything looks good except this seems totally sketchy. Should we just make it a (retain) computed member variable, or a method? I almost made it `retain` but wasn’t sure if it would create a retain cycle since this view gets added as a subview of this object in -updateConstraints. Maybe make it `weak`? They would require a little code restructuring in -updateConstraints the way it assigns the object to the property now.

Wouldn’t it be better to use RetainPtr where possible instead of using explicit calls to retain and release?

(In reply to Anders Carlsson from comment #7) > Wouldn’t it be better to use RetainPtr where possible instead of using > explicit calls to retain and release? Won’t we just remove RetainPtr use for Objective-C objects once we enable ARC? Seems like more editing is required to switch from RetainPtr to ARC as well, especially for instance variables.

Created attachment 341109 [details] Patch v2

Comment on attachment 341109 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=341109&action=review > Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.mm:69 > +@property (nonatomic, retain) UIVisualEffectView *secondaryMaterialOverlayView; Err, now this is weird because nothing assigns to _secondaryMaterialOverlayView. The getter always returns a new instance, and hopefully nobody uses the setter because the setter/getter would return different values. The existing implementation doesn't seem suitable to a property since it creates a new instance each time, nor does it appear to want a setter.

Comment on attachment 341109 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=341109&action=review >> Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.mm:69 >> +@property (nonatomic, retain) UIVisualEffectView *secondaryMaterialOverlayView; > > Err, now this is weird because nothing assigns to _secondaryMaterialOverlayView. The getter always returns a new instance, and hopefully nobody uses the setter because the setter/getter would return different values. > > The existing implementation doesn't seem suitable to a property since it creates a new instance each time, nor does it appear to want a setter. The assignment to _secondaryMaterialOverlayView happens in -updateConstraints via olde-fashioned setter invocation: [self setSecondaryMaterialOverlayView:[[self class] secondaryMaterialOverlayView]]; The "getter" you're looking at is a class method, not an instance method: + (UIVisualEffectView *)secondaryMaterialOverlayView The -updateConstraints method also uses this value later: [_secondaryMaterialOverlayView removeFromSuperview]; So this seems like a valid use of an instance variable (with a questionable design pattern). (I'm going to send email to Jer Noble to find out if the `else` clause that calls -removeFromSuperview should also clear out _secondaryMaterialOverlayView, but that seems like a separate concern. Maybe.) If it's easier, I can just drop the changes to the secondaryMaterialOverlayView & secondaryMaterialOverlayViewConstraints properties so we can fix the other leaks.

I spoke with Joe in person today, and he said it was okay to land this without the changes to the secondaryMaterialOverlayView & secondaryMaterialOverlayViewConstraints properties, and then follow-up with a separate bug about fixing those.

(In reply to David Kilzer (:ddkilzer) from comment #12) > I spoke with Joe in person today, and he said it was okay to land this > without the changes to the secondaryMaterialOverlayView & > secondaryMaterialOverlayViewConstraints properties, and then follow-up with > a separate bug about fixing those. Yep. If you want to re-open Patch v1 I'll r+ it.

(In reply to Joseph Pecoraro from comment #13) > (In reply to David Kilzer (:ddkilzer) from comment #12) > > I spoke with Joe in person today, and he said it was okay to land this > > without the changes to the secondaryMaterialOverlayView & > > secondaryMaterialOverlayViewConstraints properties, and then follow-up with > > a separate bug about fixing those. > > Yep. If you want to re-open Patch v1 I'll r+ it. Done.

Comment on attachment 341026 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=341026&action=review > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:152 > + [_location release]; I notice that `location` is `@property (retain, nonatomic) NSString *location;` normally NSStrings are (copy) not (retain). Seems we could be doing some cleanup / modernization around this code.

Committed r232187: <https://trac.webkit.org/changeset/232187>

(In reply to Joseph Pecoraro from comment #15) > Comment on attachment 341026 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=341026&action=review > > > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:152 > > + [_location release]; > > I notice that `location` is `@property (retain, nonatomic) NSString > *location;` normally NSStrings are (copy) not (retain). Seems we could be > doing some cleanup / modernization around this code. Made that change and also fixed _WKPreviewControllerDataSource.mimeType to be `copy`.