[webkit-reviews] review granted: [Bug 21810] Remove use of static C++ objects that are destroyed at exit time (destructors) : [Attachment 24956] Incremental patch addressing Retain & GC and PassRefPtr<T>

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Greg Bolsinga <bolsinga at apple.com>'s
request for review:
Bug 21810: Remove use of static C++ objects that are destroyed at exit time
(destructors)
https://bugs.webkit.org/show_bug.cgi?id=21810

Attachment 24956: Incremental patch addressing Retain & GC and PassRefPtr<T>
https://bugs.webkit.org/attachment.cgi?id=24956&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
> -    static RefPtr<Image> brokenImage;
> +    static Image* brokenImage = 0;
>      if (!brokenImage)
> -	   brokenImage = Image::loadPlatformResource("missingImage");
> -    return brokenImage.get();
> +	   brokenImage =
Image::loadPlatformResource("missingImage").releaseRef();

There's no need for the if statement here; you didn't add it, but it's not
needed.

    static Image* brokenImage =
Image::loadPlatformResource("missingImage").releaseRef();

The above will do the same thing a bit more efficiently.

> +	       static NSColor *clearColor = (NSColor*)CFRetain([NSColor
clearColor]);

For these it would be slightly better to use the HardRetain function from
FoundationExtras.h. I think that using HardRetain would eliminate the need for
the typecast.

If it doesn't remove the need for the typecast, I'll mention that we'd normally
use static_cast or reinterpret_cast rather than a C-style cast for these. Also,
we use "NSColor *" with a space for Objective-C types. It's a confusing rule,
but we've been following it consistently elsewhere.

> -	   spellingPatternColor = color;
> +	   spellingPatternColor = (NSColor*)CFRetain(color);

In these cases where it's just assignment and not a definition, I think it's
better for type safety to just do the HardRetain on a separate line.

> -    static RetainPtr<NSString> webFallbackFontFamily = nil;
> +    static NSString* webFallbackFontFamily = nil;
>      if (!webFallbackFontFamily)
> -	   webFallbackFontFamily = [[NSFont systemFontOfSize:16.0f]
familyName];
> -    return webFallbackFontFamily.get();
> +	   webFallbackFontFamily = (NSString*)CFRetain([[NSFont
systemFontOfSize:16.0f] familyName]);
> +    return webFallbackFontFamily;

We use spaces in Objective-C types like "NSString *". This can be done in a
single line. Please try this:

    static NSString *webFallbackFontFamily = HardRetain([[NSFont
systemFontOfSize:16.0f] familyName]);

By the way, this single-line approach can be used even for more complicated
cases by putting the code to compute the initial value into a separate
function, perhaps an inline function. But I'm not asking you to change that
everywhere -- just where it's simple and easy.

> -    static RetainPtr<NSMutableArray> types = nil;
> +    static NSMutableArray *types = nil;
>      if (!types) {
>	   types = [[NSMutableArray alloc] initWithObjects:NSTIFFPboardType,
nil];
> -	   [types.get() addObjectsFromArray:writableTypesForURL()];
> -	   [types.get() addObject:NSRTFDPboardType];
> +	   [types addObjectsFromArray:writableTypesForURL()];
> +	   [types addObject:NSRTFDPboardType];
> +	   CFRetain(types);

This one could use HardRetainWithNSRelease; there's no need for the double
retain in a case like this.

> -    static RefPtr<Image> resizeCornerImage;
> +    static Image* resizeCornerImage = 0;
>      if (!resizeCornerImage)
> -	   resizeCornerImage =
Image::loadPlatformResource("textAreaResizeCorner");
> +	   resizeCornerImage =
Image::loadPlatformResource("textAreaResizeCorner").releaseRef();

Another single-line candidate.

>  + (NSArray *)_web_writableTypesForURL
>  {
> -    static RetainPtr<NSArray> types;
> +    static NSArray *types = nil;
>      if (!types) {
>	   types = [[NSArray alloc] initWithObjects:
>	       WebURLsWithTitlesPboardType,
> @@ -67,29 +67,32 @@ NSString *WebURLNamePboardType = @"publi
>	       WebURLNamePboardType,
>	       NSStringPboardType,
>	       nil];
> +	   CFRetain(types);
>      }
> -    return types.get();
> +    return types;
>  }

Another HardRetainWithNSRelease candidate.

>  static NSArray *_writableTypesForImageWithoutArchive (void)
>  {
> -    static RetainPtr<NSMutableArray> types;
> +    static NSMutableArray *types = nil;
>      if (types == nil) {
>	   types = [[NSMutableArray alloc] initWithObjects:NSTIFFPboardType,
nil];
> -	   [types.get() addObjectsFromArray:[NSPasteboard
_web_writableTypesForURL]];
> +	   [types addObjectsFromArray:[NSPasteboard _web_writableTypesForURL]];

> +	   CFRetain(types);
>      }
> -    return types.get();
> +    return types;
>  }

Ditto.

>  static NSArray *_writableTypesForImageWithArchive (void)
>  {
> -    static RetainPtr<NSMutableArray> types;
> +    static NSMutableArray *types = nil;
>      if (types == nil) {
>	   types = [_writableTypesForImageWithoutArchive() mutableCopy];
> -	   [types.get() addObject:NSRTFDPboardType];
> -	   [types.get() addObject:WebArchivePboardType];
> +	   [types addObject:NSRTFDPboardType];
> +	   [types addObject:WebArchivePboardType];
> +	   CFRetain(types);
>      }
> -    return types.get();
> +    return types;
>  }

Ditto.

> -    static RetainPtr<NSArray> types = [[NSArray alloc]
initWithObjects:WebArchivePboardType, NSHTMLPboardType,
> +    static NSArray *types = (NSArray*)CFRetain([[NSArray alloc]
initWithObjects:WebArchivePboardType, NSHTMLPboardType,
>	      NSFilenamesPboardType, NSTIFFPboardType, NSPICTPboardType,
NSURLPboardType, 
> -	      NSRTFDPboardType, NSRTFPboardType, NSStringPboardType,
NSColorPboardType, nil];
> +	      NSRTFDPboardType, NSRTFPboardType, NSStringPboardType,
NSColorPboardType, nil]);

HardRetainWithNSRelease is exactly what we want here.

>	   if (!$result) {
> -	       print "$shortName has exit time destructors in it! ($path)\n";
> +	       print "$shortName has exit time destructors in it!\n 
($path)\n";
>	       $result = 1;
>	   }

I don't like this change. Usually you want to ignore the full path of the file,
which is why it's off to the right.

And if you are convinced that the new format is better, then I'd like to change
the other two check-for scripts that use this same format for the error
message.

I'm going to say review+ because none of my comments above seem like
showstoppers, but I think this can be improved.