Bug 169065 – Move stringMatchesWildcardString from WebKit2 to WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Aakash Jain]

stringMatchesWildcardString from WebKit2/Platform/mac/StringUtilities.h should be moved to WebCore.

[Aakash Jain]

Created attachment 303151 [details]
Proposed patch

Still need to fix TestWebKitAPI, but please review meanwhile.

[Alexey Proskuryakov]

Comment on attachment 303151 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:3
> +        Move stringMatchesWildcardString from WebKit2 to WebCore

Looks good. I'm interested in Andy's take too!

> Source/WebCore/platform/mac/StringUtilities.mm:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

Please keep the old copyright, this is not new code.

> Source/WebKit2/ChangeLog:8
> +        * Platform/mac/StringUtilities.h: Moved stringMatchesWildcardString to WebCore.

This file can be made a project one now.

Can the rest of its content be moved too, so that we could delete it altogether?

> Source/WebKit2/WebProcess/Plugins/WebPluginInfoProvider.cpp:220
> +        if (key.contains('*') && key != "*" && WebCore::stringMatchesWildcardString(host, key)) {

We usually add "using namespace WebCore" to any files that do, and omit the prefixes.

[Sam Weinig]

The ChangeLog does not seem to indicate why this change is necessary. Could you add that?

[Alexey Proskuryakov]

Comment on attachment 303151 [details]
Proposed patch

Marking r- since this breaks the build.

Sam, we now need to have WebKit API/SPI headers strictly match framework exports. Another way to fix this would be to mark the other functions in WebKit2/StringUtilities.h as visibility("hidden"), but moving internal functions to WebCore is cleaner.

[Andy Estes]

Comment on attachment 303151 [details]
Proposed patch

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

You'll need to update the StringUtilities API test to include the right header file. You'll also need to update WebCore/PlatformMac.cmake.

Looks good to me once you get it building properly.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:18276
> +				ECA680C61E67724500731D20 /* StringUtilities.h */,
> +				ECA680C81E67730B00731D20 /* StringUtilities.mm */,

Please keep the WebCore/platform/mac/ group sorted. You can reorder these in the Xcode UI or use sort-Xcode-project-file (although that'll likely change more than just this).

[Aakash Jain]

Created attachment 303240 [details]
Updated patch

Updated patch with review comments incorporated.

[Alexey Proskuryakov]

Comment on attachment 303240 [details]
Updated patch

Looking at it some more, I don't think that there is enough similarity to move them together.

Also, looks like nsStringFromWebCoreString is badly over-used in WebKit2, given that its purpose is to be used on secondary threads.

[WebKit Commit Bot]

Comment on attachment 303240 [details]
Updated patch

Clearing flags on attachment: 303240

Committed r213319: <http://trac.webkit.org/changeset/213319>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.