[webkit-reviews] review granted: [Bug 176280] [Cocoa] Tidy a few things in legacy WebHTMLView : [Attachment 319795] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted Darin Adler <darin at apple.com>'s request for review:
Bug 176280: [Cocoa] Tidy a few things in legacy WebHTMLView
https://bugs.webkit.org/show_bug.cgi?id=176280

Attachment 319795: Patch

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




--- Comment #7 from mitz at webkit.org ---
Comment on attachment 319795
  --> https://bugs.webkit.org/attachment.cgi?id=319795
Patch

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

> Source/WebKitLegacy/mac/ChangeLog:36
> +	   (-[WebHTMLView _shouldDeleteRange:]): Deleted.

Why? Presumably because it’s unused.

> Source/WebKitLegacy/mac/ChangeLog:40
> +	   (-[WebHTMLView _web_setPrintingModeRecursive:adjustViewSize:]):
Added. Helper method so

I’m not sure you need “web_” in these internal methods in a private class.
Maybe worth doing because it’s an NSView subclass.

> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:909
> +#if !PLATFORM(MAC)

Should this say #if PLATFORM(IOS)? Or perhaps start with the #if PLATFORM(MAC)
case?

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:777
> +#define AUTOSCROLL_INTERVAL 0.1

Could turn this into a static const NSTimeInterval, or just inline it in the
one place it’s used.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1008
> +static NSString *createUUIDString()
> +{
> +    return (NSString *)adoptCF(CFUUIDCreateString(kCFAllocatorDefault,
adoptCF(CFUUIDCreate(kCFAllocatorDefault)).get())).autorelease();
> +}

Simpler to use all-Objective-C API these days: [NSUUID UUID].UUIDString, and
probably not worth wrapping in a static function.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1406
> +	   const unichar nonBreakingSpaceCharacter = 0xA0;
> +	   NSString *nonBreakingSpaceString = [NSString
stringWithCharacters:&nonBreakingSpaceCharacter length:1];

This is so awkward, and using the Objective-C string literal @"\u00A0" would
also streamline the code.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:2050
> +    } else if ([type isEqual:NSTIFFPboardType] &&
_private->promisedDragTIFFDataSource) {

Can use -isEqualToString:

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:3926
> +	   static bool version3OrLaterClient =
WebKitLinkedOnOrAfter(WEBKIT_FIRST_VERSION_WITHOUT_QUICKBOOKS_QUIRK);

The variable type perfectly matched the return type in the declaration of
WebKitLinkedOnOrAfter!

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:6427
> +    if (!_private->autoscrollTimer) {

Can reverse the condition and return early.