Bug 110161 – Upstream SharedTimerIOS.mm

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[David Kilzer (:ddkilzer)]

Upstream SharedTimerIOS.mm

[David Kilzer (:ddkilzer)]

Created attachment 188953 [details]
Patch v1

[Mark Rowe (bdash)]

Comment on attachment 188953 [details]
Patch v1

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

> Source/WebCore/platform/ios/SharedTimerIOS.mm:27
> +#import "SharedTimer.h"
> +#import "WebCoreThread.h"

Should be an empty line between these two #includes.

> Source/WebCore/platform/ios/SharedTimerIOS.mm:30
> +#import "WebCoreThreadRun.h"
> +
> +#import <Foundation/Foundation.h>

And none here.

> Source/WebCore/platform/ios/SharedTimerIOS.mm:54
> +    self = [super init];
> +    if (!self) {

if (!(self = [super init])) {

> Source/WebCore/platform/ios/SharedTimerIOS.mm:90
> +    NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
> +    sharedTimerFiredFunction();
> +    [pool drain];

@autoreleasepool { … }?

> Source/WebCore/platform/ios/SharedTimerIOS.mm:112
> +    if (sharedTimer) {

Early-return rather than indenting the entire body of the function?

[David Kilzer (:ddkilzer)]

Created attachment 189113 [details]
Patch v2

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> (From update of attachment 188953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188953&action=review

Thanks Mark!  Addressed all of these comments.

[Benjamin Poulain]

Comment on attachment 189113 [details]
Patch v2

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

> Source/WebCore/platform/ios/SharedTimerIOS.mm:2
> + * Copyright (C) 2006, 2010, 2011 Apple Inc.  All rights reserved.

not +2013?

> Source/WebCore/platform/ios/SharedTimerIOS.mm:36
> +@interface WebCorePowerNotifierIOS : NSObject
> +- (void)didWake;
> +@end

The name PowerNotifier is misleading if we only respond to "resume" notification.

> Source/WebCore/platform/ios/SharedTimerIOS.mm:46
> +
> +static WebCorePowerNotifierIOS *powerNotifier;
> +static CFRunLoopTimerRef sharedTimer;
> +static void (*sharedTimerFiredFunction)();
> +static void timerFired(CFRunLoopTimerRef, void*);
> +

I would:
-remove the space surrounding the scope.
-use a typedef for the function pointer.
-Split the declaration in 2: 1) static variables, WebCorePowerNotifierIOS, CFRunLoopTimerRef, typedef-ed function pointer. Then Space. Then 2) declaration of timerFired.

> Source/WebCore/platform/ios/SharedTimerIOS.mm:58
> +    [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(didWake) name:@"UIApplicationDidBecomeActiveNotification" object:nil];

There is no corresponding removeObserver in dealloc.

If WebCorePowerNotifierIOS exist for the sole purpose of registering for the notification, I think it would be much cleaner to use the CoreFoundation C API for notifications.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> (From update of attachment 189113 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189113&action=review
> 
> > Source/WebCore/platform/ios/SharedTimerIOS.mm:2
> > + * Copyright (C) 2006, 2010, 2011 Apple Inc.  All rights reserved.
> 
> not +2013?

Simply forgot to update it after the last round of changes.

> > Source/WebCore/platform/ios/SharedTimerIOS.mm:36
> > +@interface WebCorePowerNotifierIOS : NSObject
> > +- (void)didWake;
> > +@end
> 
> The name PowerNotifier is misleading if we only respond to "resume" notification.

Changed to WebCoreResumeNotifierIOS.

> > Source/WebCore/platform/ios/SharedTimerIOS.mm:46
> > +
> > +static WebCorePowerNotifierIOS *powerNotifier;
> > +static CFRunLoopTimerRef sharedTimer;
> > +static void (*sharedTimerFiredFunction)();
> > +static void timerFired(CFRunLoopTimerRef, void*);
> > +
> 
> I would:
> -remove the space surrounding the scope.
> -use a typedef for the function pointer.

Done.

> -Split the declaration in 2: 1) static variables, WebCorePowerNotifierIOS, CFRunLoopTimerRef, typedef-ed function pointer. Then Space. Then 2) declaration of timerFired.

This didn't quite match what the code did, so I did my best to follow your directions.

> > Source/WebCore/platform/ios/SharedTimerIOS.mm:58
> > +    [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(didWake) name:@"UIApplicationDidBecomeActiveNotification" object:nil];
> 
> There is no corresponding removeObserver in dealloc.

The object is never dealloc-ed because it's a static variable that's only set once.

However, I like to do this in case that ever changes in the future (and someone forgets to add it), so I added a -dealloc method.

> If WebCorePowerNotifierIOS exist for the sole purpose of registering for the notification, I think it would be much cleaner to use the CoreFoundation C API for notifications.

I think it makes more sense to use NSNotificationCenter because this is a notification sent from UIKit.

I recall something about subtle differences between how CFNotifications and NSNotifications work, but I don't remember the details.  I would rather use the same type of notification to receive them as the sender is using to send them (if possible).

[David Kilzer (:ddkilzer)]

Created attachment 189422 [details]
Patch v3

[David Kilzer (:ddkilzer)]

Created attachment 189424 [details]
Patch v4

Removed unnecessary/unused headers.

[Benjamin Poulain]

> > If WebCorePowerNotifierIOS exist for the sole purpose of registering for the notification, I think it would be much cleaner to use the CoreFoundation C API for notifications.
> 
> I think it makes more sense to use NSNotificationCenter because this is a notification sent from UIKit.
> 
> I recall something about subtle differences between how CFNotifications and NSNotifications work, but I don't remember the details.  I would rather use the same type of notification to receive them as the sender is using to send them (if possible).

If you don't mind, I will look into using CFNotifications in the future.

I don't like static objects because they can create fixed memory pages you can never give back to the system.

[WebKit Review Bot]

Comment on attachment 189424 [details]
Patch v4

Clearing flags on attachment: 189424

Committed r143550: <http://trac.webkit.org/changeset/143550>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> > > If WebCorePowerNotifierIOS exist for the sole purpose of registering for the notification, I think it would be much cleaner to use the CoreFoundation C API for notifications.
> > 
> > I think it makes more sense to use NSNotificationCenter because this is a notification sent from UIKit.
> > 
> > I recall something about subtle differences between how CFNotifications and NSNotifications work, but I don't remember the details.  I would rather use the same type of notification to receive them as the sender is using to send them (if possible).
> 
> If you don't mind, I will look into using CFNotifications in the future.
> 
> I don't like static objects because they can create fixed memory pages you can never give back to the system.

Not at all!