Bug 179504 – [PAL] Remove FileSystem's dependency on WebCoreNSStringExtras

• Component: Platform
• Status: RESOLVED FIXED

[Christopher Reid]

These are dependencies for FileSystem. They do not have any dependencies of their own.

[Christopher Reid]

Created attachment 326600 [details]
Patch

[Build Bot]

Attachment 326600 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/Plugins/Hosted/NetscapePluginHostManager.mm:123:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 62 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Christopher Reid]

Created attachment 326604 [details]
Patch

Fixing style issues

[Alex Christensen]

Comment on attachment 326604 [details]
Patch

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

> Source/WebCore/PAL/pal/mac/PlatformNSStringExtras.hSource/WebCore/platform/mac/WebCoreNSStringExtras.h:44
> +namespace PAL {
> +
>  #ifdef __cplusplus
>  extern "C" {
>  #endif

If we have a namespace, then this is c++-only.  We shouldn't need extern "C" then.

[Alexey Proskuryakov]

Comment on attachment 326604 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [PAL] Move WebCoreNSStringExtras and WebCoreObjCExtras into PAL

Why is this needed? This doesn’t abstract away any platform differences, the code is Cocoa only.

[Christopher Reid]

(In reply to Alexey Proskuryakov from comment #5)
> Comment on attachment 326604 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326604&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [PAL] Move WebCoreNSStringExtras and WebCoreObjCExtras into PAL
> 
> Why is this needed? This doesn’t abstract away any platform differences, the
> code is Cocoa only.

We're working on moving FileSystem to PAL but first we need to sort out its dependencies on WebCore. FileSystemMac has a dependency on WebCoreNSStringExtras. It looked like WebCoreObjCExtras was a dependency for filesystem too but it was just including the file and not using it. WebCoreObjCExtras is however a dependency for platform/SharedBuffer.

[Alexey Proskuryakov]

Comment on attachment 326604 [details]
Patch

> FileSystemMac has a dependency on WebCoreNSStringExtras.

Looking at WebCoreNSStringExtras.h, I don't see anything worth moving as is. These files seem like remnants of some past code move, in an effort to share code between WebCore and WebKitLegacy that didn't quite work.

There are several groups of unrelated functions in this file:

WEBCORE_EXPORT BOOL stringIsCaseInsensitiveEqualToString(NSString *first, NSString *second);
WEBCORE_EXPORT BOOL hasCaseInsensitiveSuffix(NSString *, NSString *suffix);
WEBCORE_EXPORT BOOL hasCaseInsensitivePrefix(NSString *, NSString *prefix);
WEBCORE_EXPORT BOOL hasCaseInsensitiveSubstring(NSString *, NSString *substring);

These are barely used one-liners. It would likely be beneficial to rewrite the only call site in WebCore to use String instead.

WEBCORE_EXPORT NSString *filenameByFixingIllegalCharacters(NSString *);

This is a file system specific function, not a legitimate "NSString extra". This one is used in WebCore, WebKit and WebKitLegacy, so it may be harder to eliminate, but it should live elsewhere.

WEBCORE_EXPORT NSString *preferredBundleLocalizationName();

Even less of an "NSString extra", only used in one place in WebKitLegacy. So it should go there.

NSString *canonicalLocaleName(NSString *);

Only used in preferredBundleLocalizationName, so it should be merged into that function. The concept of canonical locale name is not clear enough to have the function safely reusable, and the implementation is super obsolete.

[Christopher Reid]

Created attachment 327372 [details]
Patch

Thanks for the feedback Alexey, here's an updated patch.

I have moved filenameByFixingIllegalCharacters into LoaderNSURLExtras but I'm not sure if this is the best place for it. Let me know if there's a better place for it.

I'll keep this bug focused on WebCoreNSStringExtras. I can make a new bug for removing all the unnecessary includes of WebCoreObjCExtras.h to remove FileSystem's dependency on that.

[EWS Watchlist]

Attachment 327372 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/Misc/WebNSBundleExtras.mm:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/loader/mac/LoaderNSURLExtras.mm:105:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WebCore/loader/mac/LoaderNSURLExtras.mm:106:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKitLegacy/mac/Misc/WebKitNSStringExtras.mm:177:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/loader/mac/LoaderNSURLExtras.h:38:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Christopher Reid]

Created attachment 327374 [details]
Patch

Fixing some style issues

[EWS Watchlist]

Attachment 327374 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/Misc/WebNSBundleExtras.mm:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 327374 [details]
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>
> +@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>.

[Christopher Reid]

(In reply to Darin Adler from comment #12)
> Comment on attachment 327374 [details]
> 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.

Ah, I am new to Objective-C but that's interesting to see that this is handled automatically by its #import directive.

Thanks for the feedback, I'll make your suggested changes.

[Christopher Reid]

Created attachment 327430 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 327430 [details]
Patch for landing

Clearing flags on attachment: 327430

Committed r225087: <https://trac.webkit.org/changeset/225087>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35660909>