| Summary: | Move stringMatchesWildcardString from WebKit2 to WebCore | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Aakash Jain <aakash_jain> | ||||||
| Component: | WebKit2 | Assignee: | Aakash Jain <aakash_jain> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | aakash_jain, aestes, ap, commit-queue, sam | ||||||
| Priority: | P2 | ||||||||
| Version: | Other | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Aakash Jain
2017-03-01 16:06:02 PST
Created attachment 303151 [details]
Proposed patch
Still need to fix TestWebKitAPI, but please review meanwhile.
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. The ChangeLog does not seem to indicate why this change is necessary. Could you add that? 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.
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). Created attachment 303240 [details]
Updated patch
Updated patch with review comments incorporated.
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.
Comment on attachment 303240 [details] Updated patch Clearing flags on attachment: 303240 Committed r213319: <http://trac.webkit.org/changeset/213319> All reviewed patches have been landed. Closing bug. |