Bug 159731

Summary: REGRESSION (r191907): Maxthon Browser -After exit full screen video playback the browser window is blank with audio running
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jeremyj-wk, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
thorton: review+
Patch for landing.
none
Patch for landing. none

Description Jer Noble 2016-07-13 11:59:26 PDT 
REGRESSION (r191907): Maxthon Browser -After exit full screen video playback the browser window is blank with audio running
Comment 1 Jer Noble 2016-07-13 12:00:23 PDT 
<rdar://problem/26674003>
Comment 2 Jer Noble 2016-07-13 12:06:46 PDT 
Created attachment 283555 [details]
Patch
Comment 3 Tim Horton 2016-07-13 12:09:12 PDT 
Comment on attachment 283555 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283555&action=review

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:68
> +@interface NSView (WebNSViewDetails)
> +- (NSArray *)_referencingConstraints;
> +@end

This should be in an SPI header. Actually... do you ever use it?

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:254
> +    self.savedConstraints = _webView.superview.constraints;

Did you mean to use _referencingConstraints here? I'm not sure that saving all constraints on a view we don't own is right.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:317
> +        [NSLayoutConstraint activateConstraints:self.savedConstraints];

Should we be more careful if this throws?
Comment 4 Jer Noble 2016-07-13 12:09:39 PDT 
Created attachment 283556 [details]
Patch
Comment 5 Jer Noble 2016-07-13 12:25:30 PDT 
Comment on attachment 283555 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283555&action=review

>> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:254
>> +    self.savedConstraints = _webView.superview.constraints;
> 
> Did you mean to use _referencingConstraints here? I'm not sure that saving all constraints on a view we don't own is right.

Nope. I removed the SPI header in the most recent patch.

>> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:317
>> +        [NSLayoutConstraint activateConstraints:self.savedConstraints];
> 
> Should we be more careful if this throws?

Will this throw? that seems.... Weird.
Comment 6 Jer Noble 2016-07-13 14:41:57 PDT 
Created attachment 283567 [details]
Patch
Comment 7 Tim Horton 2016-07-13 15:01:31 PDT 
Comment on attachment 283567 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283567&action=review

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:313
> +        @try {

I mean, if you're going to do it, do it to both :) And one would expect some {BEGIN,END}_BLOCK_OBJC_EXCEPTIONS, not this.
Comment 8 Sam Weinig 2016-07-13 15:56:02 PDT 
How are going to test this so it doesn't regress again?
Comment 9 Jeremy Jones 2016-07-15 17:11:02 PDT 
Created attachment 283823 [details]
Patch for landing.
Comment 10 Jeremy Jones 2016-07-15 19:09:18 PDT 
Created attachment 283831 [details]
Patch for landing.
Comment 11 Jeremy Jones 2016-07-17 13:15:21 PDT 
(In reply to comment #8)
> How are going to test this so it doesn't regress again?

I'm working on an API test for this.
Comment 12 WebKit Commit Bot 2016-07-17 13:36:12 PDT 
Comment on attachment 283831 [details]
Patch for landing.

Clearing flags on attachment: 283831

Committed r203335: <http://trac.webkit.org/changeset/203335>