Summary: | Upstream SharedTimerIOS.mm | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||||
Component: | Platform | Assignee: | 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
David Kilzer (:ddkilzer)
2013-02-18 15:32:39 PST
Created attachment 188953 [details]
Patch v1
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? Created attachment 189113 [details]
Patch v2
(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 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. (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). Created attachment 189422 [details]
Patch v3
Created attachment 189424 [details]
Patch v4
Removed unnecessary/unused headers.
> > 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 on attachment 189424 [details] Patch v4 Clearing flags on attachment: 189424 Committed r143550: <http://trac.webkit.org/changeset/143550> All reviewed patches have been landed. Closing bug. (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! |