Bug 47299 – Screensaver starts while watching fullscreen playback

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

9/25/10 7:25 PM Les Leposo:
Somehow I thought this used to work well: i.e. screensaver never started while I was watching a fullscreen stream/video.
But if it's not a regression, then it's definitely worth fixing since it is frustratingly microsoftesque.

<rdar://problem/8478956>

[Jer Noble]

Created attachment 69984 [details]
Patch

[WebKit Review Bot]

Attachment 69984 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/mac/WebView/WebVideoFullscreenController.h:52:  _idleSystemSleepAssertion is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebVideoFullscreenController.h:53:  _tickleTimer is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 69984 [details]
Patch

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

> WebKit/mac/WebView/WebVideoFullscreenController.h:53
> +    NSTimer* _tickleTimer;

Our style is to put spaces before the "*" for Objective-C types like this. Maybe we should change that style, but we have not yet done that.

> WebKit/mac/WebView/WebVideoFullscreenController.mm:46
> +static NSTimeInterval kTickleTimerInterval = 1.0;

This should be static const, not just static. Also, we don’t use “k” for such things in WebKit.

> WebKit/mac/WebView/WebVideoFullscreenController.mm:86
> +    [_tickleTimer invalidate];

This needs to release the timer.

> WebKit/mac/WebView/WebVideoFullscreenController.mm:387
> +    [_tickleTimer invalidate];
> +    _tickleTimer = [NSTimer scheduledTimerWithTimeInterval:kTickleTimerInterval target:self selector:@selector(_tickleTimerFired) userInfo:nil repeats:YES];

This needs to release the old timer and retain the new timer.

> WebKit/mac/WebView/WebVideoFullscreenController.mm:393
> +    [_tickleTimer invalidate];
> +    _tickleTimer = nil;

This needs to release the timer.

[Jer Noble]

(In reply to comment #3)
> (From update of attachment 69984 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69984&action=review
> 
> > WebKit/mac/WebView/WebVideoFullscreenController.h:53
> > +    NSTimer* _tickleTimer;
> 
> Our style is to put spaces before the "*" for Objective-C types like this. Maybe we should change that style, but we have not yet done that.

Whoops, right; Changed.

> > WebKit/mac/WebView/WebVideoFullscreenController.mm:46
> > +static NSTimeInterval kTickleTimerInterval = 1.0;
> 
> This should be static const, not just static. Also, we don’t use “k” for such things in WebKit.

Changed.

> > WebKit/mac/WebView/WebVideoFullscreenController.mm:86
> > +    [_tickleTimer invalidate];
> 
> This needs to release the timer.

The NSTimer is retained by the NSRunLoop which it is attached to.  The run loop will release its reference when invalidate is called. 

> > WebKit/mac/WebView/WebVideoFullscreenController.mm:387
> > +    [_tickleTimer invalidate];
> > +    _tickleTimer = [NSTimer scheduledTimerWithTimeInterval:kTickleTimerInterval target:self selector:@selector(_tickleTimerFired) userInfo:nil repeats:YES];
> 
> This needs to release the old timer and retain the new timer.

The scheduledTimerWithTimeInterval: method adds the new timer to the current NSRunLoop, which retains it.  Do we really need to retain and release the timer twice? 

> > WebKit/mac/WebView/WebVideoFullscreenController.mm:393
> > +    [_tickleTimer invalidate];
> > +    _tickleTimer = nil;
> 
> This needs to release the timer.

This should be the same as the -dealloc case. (i.e. the NSRunLoop will release the timer.)

[Jer Noble]

Looking through the WebKit source, it looks like everywhere else explicitly retains the results of +scheduledTimerWithInterval:.  I've changed my patch to match.

[Jer Noble]

Created attachment 70018 [details]
Patch

[WebKit Review Bot]

Attachment 70018 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/mac/WebView/WebVideoFullscreenController.h:52:  _idleSystemSleepAssertion is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Committed r69318: <http://trac.webkit.org/changeset/69318>