Bug 68421

Summary: Stop calling UpdateSystemActivity in places where we hold power assertions that achieve the same effect
Product: WebKit Reporter: Mark Rowe (bdash) <mrowe>
Component: MediaAssignee: Mark Rowe (bdash) <mrowe>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, jer.noble, mitz, mrowe, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch v1 mitz: review+

Description Mark Rowe (bdash) 2011-09-19 20:17:24 PDT
In a couple of locations related to fullscreen media playback we periodically call UpdateSystemActivity.  There’s no need to do this on SnowLeopard and newer as the power assertion that we hold to prevent display sleep also prevents the screensaver kicking in.  It doesn’t prevent the screensaver on Leopard though, so we’ll still need to call UpdateSystemActivity in that case.
Comment 1 Mark Rowe (bdash) 2011-09-19 20:34:21 PDT
Created attachment 107961 [details]
Patch v1
Comment 2 WebKit Review Bot 2011-09-19 20:37:59 PDT
Attachment 107961 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/mac/WebVideoFullscreenController.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/platform/mac/DisplaySleepDisabler.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebKit/mac/WebView/WebFullScreenController.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Rowe (bdash) 2011-09-19 20:38:12 PDT
Comment on attachment 107961 [details]
Patch v1

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

> Source/WebCore/platform/mac/DisplaySleepDisabler.cpp:56
> +void DisplaySleepDisabler::systemActivityTimerFired(Timer<DisplaySleepDisabler>* timer)

I nuked the unused variable name from here.
Comment 4 mitz 2011-09-19 20:46:46 PDT
Comment on attachment 107961 [details]
Patch v1

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

> Source/WebCore/ChangeLog:27
> +        (-[WebVideoFullscreenController setupVideoOverlay:]): Remove the now-unnecessary explicit use of the WebCore namespace..

..

> Source/WebCore/platform/mac/DisplaySleepDisabler.cpp:27
> +#include "DisplaySleepDisabler.h"

Missing blank line after this.

> Source/WebCore/platform/mac/DisplaySleepDisabler.cpp:42
> +    RetainPtr<CFStringRef> reasonCF(AdoptCF, CFStringCreateWithCString(0, reason, kCFStringEncodingUTF8));

I prefer kCFAllocatorDefault

> Source/WebCore/platform/mac/DisplaySleepDisabler.cpp:44
> +#else

Does this branch need an UNUSED_PARAM(reason)?

> Source/WebCore/platform/mac/DisplaySleepDisabler.cpp:56
> +void DisplaySleepDisabler::systemActivityTimerFired(Timer<DisplaySleepDisabler>* timer)

No need to name the unused parameter here.
Comment 5 Mark Rowe (bdash) 2011-09-19 21:14:05 PDT
Landed in r95513.