Bug 110161

Summary: Upstream SharedTimerIOS.mm
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: PlatformAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, joepeck, koivisto, webkit.review.bot, yongjun_zhang
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch v1
none
Patch v2
none
Patch v3
none
Patch v4 none

Description David Kilzer (:ddkilzer) 2013-02-18 15:32:39 PST
Upstream SharedTimerIOS.mm
Comment 1 David Kilzer (:ddkilzer) 2013-02-18 15:35:24 PST
Created attachment 188953 [details]
Patch v1
Comment 2 Mark Rowe (bdash) 2013-02-18 23:34:25 PST
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?
Comment 3 David Kilzer (:ddkilzer) 2013-02-19 10:44:14 PST
Created attachment 189113 [details]
Patch v2
Comment 4 David Kilzer (:ddkilzer) 2013-02-19 10:44:49 PST
(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.
Comment 5 Benjamin Poulain 2013-02-19 13:13:12 PST
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.
Comment 6 David Kilzer (:ddkilzer) 2013-02-20 17:24:28 PST
(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).
Comment 7 David Kilzer (:ddkilzer) 2013-02-20 17:26:35 PST
Created attachment 189422 [details]
Patch v3
Comment 8 David Kilzer (:ddkilzer) 2013-02-20 17:31:46 PST
Created attachment 189424 [details]
Patch v4

Removed unnecessary/unused headers.
Comment 9 Benjamin Poulain 2013-02-20 18:26:28 PST
> > 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.
Comment 10 WebKit Review Bot 2013-02-20 18:27:37 PST
Comment on attachment 189424 [details]
Patch v4

Clearing flags on attachment: 189424

Committed r143550: <http://trac.webkit.org/changeset/143550>
Comment 11 WebKit Review Bot 2013-02-20 18:27:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 David Kilzer (:ddkilzer) 2013-02-21 17:06:10 PST
(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!