[webkit-reviews] review canceled: [Bug 188245] [Cocoa] More tweaks and refactoring to prepare for ARC : [Attachment 346605] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has canceled Darin Adler <darin at apple.com>'s
request for review:
Bug 188245: [Cocoa] More tweaks and refactoring to prepare for ARC
https://bugs.webkit.org/show_bug.cgi?id=188245

Attachment 346605: Patch

https://bugs.webkit.org/attachment.cgi?id=346605&action=review




--- Comment #31 from Darin Adler <darin at apple.com> ---
Comment on attachment 346605
  --> https://bugs.webkit.org/attachment.cgi?id=346605
Patch

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

Since the Tools part is completely separable from the non-Tools part, it’s OK
if I just get a review on one or the other. I can land that half and open
another bug for the other half.

>> Source/WTF/wtf/spi/cocoa/FoundationSPI.h:47
>> +WTF_EXTERN_C_END
> 
> Not new to this patch, but I’d have named this header objcSPI.h (like the one
in WebKit).

OK, I’ll rename and upload a new version of the patch.

>> Source/WebKitLegacy/mac/History/WebHistory.mm:647
>> +	    static NSData *emptyHistoryData = [[NSData alloc]
initWithBytes:nullptr length:0];
> 
> Can also use -init.

Will do.

>> Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:85
>> +		result =
CFBridgingRelease(CFStringCreateWithCharactersNoCopy(nullptr, uniCharPtr, len,
nullptr));
> 
> I’d replace the first nullptr with kCFAllocatorDefault like in some of the
above code, but I don’t think there’s a style guideline either way.

Sure, OK, why not?

>> Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:91
>> +	return result;
> 
> This introduces a slight change in behavior under manual retain/release. Not
a problem since the only caller is in this file and doesn’t deallocate the
receiver right after calling this method.

I think I’ll leave it like this.

Otherwise, the alternative I would probably choose would be a copy/autorelease
in the "result = self" line above. Even under ARC I could imagine returning a
copy rather than self. But I don’t want to risk a small performance regression
without a compelling reason.

>> Source/WebKitLegacy/mac/WebView/WebImmediateActionController.mm:148
>> +	Frame* coreFrame = [_webView _mainCoreFrame];
> 
> Seems like this won’t do the right thing if the selection or focus is in a
subframe. Unless WebCore hit-testing now traverses frames? Even then, it seems
wrong.

I see. I was under the incorrect impression that the frame was only to work
with the immediate action stage, like the other call sites. But I see now that
it’s also used for hit testing.

I will restore the old code for the hit testing purposes.