Bug 159731 - REGRESSION (r191907): Maxthon Browser -After exit full screen video playback the browser window is blank with audio running
Summary: REGRESSION (r191907): Maxthon Browser -After exit full screen video playback ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-07-13 11:59 PDT by Jer Noble
Modified: 2016-10-31 09:15 PDT (History)
4 users (show)

See Also:

Attachments
Patch (4.91 KB, patch)
2016-07-13 12:06 PDT, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (4.55 KB, patch)
2016-07-13 12:09 PDT, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (4.61 KB, patch)
2016-07-13 14:41 PDT, Jer Noble 		thorton: review+
Details | Formatted Diff | Diff
Patch for landing. (4.70 KB, patch)
2016-07-15 17:11 PDT, Jeremy Jones 		no flags Details | Formatted Diff | Diff
Patch for landing. (4.94 KB, patch)
2016-07-15 19:09 PDT, Jeremy Jones 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) 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 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>