[webkit-reviews] review granted: [Bug 179504] [PAL] Remove FileSystem's dependency on WebCoreNSStringExtras : [Attachment 327374] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Christopher Reid
<christopher.reid at am.sony.com>'s request for review:
Bug 179504: [PAL] Remove FileSystem's dependency on WebCoreNSStringExtras
https://bugs.webkit.org/show_bug.cgi?id=179504

Attachment 327374: Patch

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




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

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

Great job. I have a few comments about how to do this even better.

> Source/WebCore/loader/mac/LoaderNSURLExtras.h:29
> +#pragma once

If every include of a header uses #import rather than #include, then there is
no need for #pragma once. Objective-C compilers all have #import, this is an
Objective-C header, and we should always use #import for it. Thus there is no
need to add #pragma once.

> Source/WebCore/loader/mac/LoaderNSURLExtras.h:35
>  namespace WTF {
>  class String;
>  }

It would be more normal to import <wtf/Forward.h> instead of doing this, but
it’s not new so I guess we can leave it like this.

> Source/WebCore/loader/mac/LoaderNSURLExtras.h:38
> +WEBCORE_EXPORT NSString *filenameByFixingIllegalCharacters(NSString *);

This doesn’t really fit into the category "NSURL extras", but it’s better than
adding another source file, I think.

> Source/WebCore/loader/mac/LoaderNSURLExtras.mm:69
>      if ((mimeType == "application/tar" || mimeType == "application/x-tar")
> -	   && (hasCaseInsensitiveSubstring(filename, @".tar")
> -	   || hasCaseInsensitiveSuffix(filename, @".tgz"))) {
> +	   && (String(filename).containsIgnoringASCIICase(".tar")
> +	   || String(filename).endsWithIgnoringASCIICase(".tgz"))) {
>	   return filename;
>      }

This is inefficient; converts the filename from an NSString to a String twice
and throws away the result each time. But given how this function is used, I
guess that’s fine.

> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:723
> +    if (String(string).startsWithIgnoringASCIICase("mailto:")) {

Here is way to write this that is even nicer:

    if (protocolIs(string, "mailto")) {

Since the protocolIs function from the URL.h header takes a String, we don’t
need to explicitly convert to String.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:3803
> +    String extensionAsSuffix(".");
> +    extensionAsSuffix.append(extension);

There is no need for this local variable. Can just write this below:

    filename.endsWithIgnoringASCIICase("." + extension)

We should almost never use String::append, so it’s good we don’t need it.
Should use makeString or "operator+" instead.

Might need to add an include of StringConcatenate.h if it’s not already
included, though.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:3805
> +    return filename.endsWithIgnoringASCIICase(extensionAsSuffix) ||
(equalIgnoringASCIICase(extension, "jpeg")
> +	   && filename.endsWithIgnoringASCIICase(".jpg"));

Can use equalLettersIgnoringASCIICase instead of equalIgnoringASCIICase; it is
more efficient for cases where it can be used.

Breaking the expression across two lines like this would make the logic
clearer:

    return filename.endsWithIgnoringASCIICase("." + extension)
	|| (equalLettersIgnoringASCIICase(extension, "jpeg") &&
filename.endsWithIgnoringASCIICase(".jpg"));

> Source/WebKitLegacy/mac/Misc/WebKitNSStringExtras.mm:182
> +    return [self rangeOfString:prefix options:(NSCaseInsensitiveSearch |
NSAnchoredSearch)].location != NSNotFound;

Strange that we did not include NSLiteralSearch for this, even though we did
include it above. I think this was an existing mistake; not asking you to fix
this.

> Source/WebKitLegacy/mac/Misc/WebKitNSStringExtras.mm:187
> +    return [self rangeOfString:suffix options:(NSCaseInsensitiveSearch |
NSBackwardsSearch | NSAnchoredSearch)].location != NSNotFound;

Strange that we did not include NSLiteralSearch for this, even though we did
include it above. I think this was an existing mistake; not asking you to fix
this.

> Source/WebKitLegacy/mac/Misc/WebKitNSStringExtras.mm:192
> +    return [self rangeOfString:substring
options:NSCaseInsensitiveSearch].location != NSNotFound;

Strange that we did not include NSLiteralSearch for this, even though we did
include it above. I think this was an existing mistake; not asking you to fix
this.

> Source/WebKitLegacy/mac/Misc/WebNSBundleExtras.h:2
> + * Copyright (C) 2017 Apple Inc.  All rights reserved.

Should have only one space between the period and "All".

> Source/WebKitLegacy/mac/Misc/WebNSBundleExtras.h:29
> +#pragma once

No need for this #pragma once, for the same reason we don’t need it above.

> Source/WebKitLegacy/mac/Misc/WebNSBundleExtras.h:38
> +#include <objc/objc.h>
> +
> +#ifdef __OBJC__
> +#include <Foundation/Foundation.h>
> + at class NSString;
> +#else
> +typedef struct NSString NSString;
> +#endif

None of this is needed.

We could do just #import <Foundation/Foundation.h>. Or we could do just @class
NSString. Or we could do nothing at all. There is no file that is going to
#import this that doesn’t have the Foundation types already from the prefix.
But this is all a moot point; I don’t think we should create this header at all
(see comment below).

> Source/WebKitLegacy/mac/Misc/WebNSBundleExtras.mm:34
> +NSString *preferredBundleLocalizationName()

I think we should just put this function into NetscapePluginHostManager.mm with
"static" in front of it instead of adding a new source file. We don’t want any
new code to call this; at this point it’s effectively part of the plug-in code.
So then we don’t have to worry about any of my comments on the header, because
we don’t need a header. And no need for the project file modifications.

> Source/WebKitLegacy/mac/Misc/WebNSBundleExtras.mm:54
> +    RetainPtr<CFStringRef> code =
adoptCF(CFLocaleCreateCanonicalLocaleIdentifierFromScriptManagerCodes(0,
languageCode, regionCode));

Can just use auto here instead of writing RetainPtr<CFStringRef>.