[webkit-reviews] review granted: [Bug 217960] Add test for cacheModelForMainBundle() in WebKitLegacy : [Attachment 411955] Patch v3

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 21 09:24:08 PDT 2020


Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 217960: Add test for cacheModelForMainBundle() in WebKitLegacy
https://bugs.webkit.org/show_bug.cgi?id=217960

Attachment 411955: Patch v3

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




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

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

>>> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:81
>>> +#ifdef __cplusplus
>> 
>> Don’t we have macros that we use for this in API and SPI headers? I’m
surprised to see it written out like this.
> 
> We do, but I was concerned that using the macros (which are in
<wtf/Compiler.h> would cause issues if other projects at Apple were to include
WebPreferencesPrivate.h.
> 
> Also, there are currently zero uses of WTF_EXTERN_C_{BEGIN,END} in
WebKitLegacy, so I wasn't sure if it would be a good idea to add it:
> 
> $ grep -l -r 'WTF_EXTERN' Source/WebKitLegacy | wc -l
>	 0
> 
> There are other uses of 'extern "C" { }', though:
> 
> $ grep -l -r 'extern "C"' Source/WebKitLegacy | grep -v ChangeLog | wc -l
>	27
> 
> So I went with the more common solution for this particular project.

No, not WTF_EXTERN_C_BEGIN, that can’t be used in API/SPI headers. That’s
internal to the project. You are correct on that!

But something like CF_EXTERN_C_BEGIN *could* be used. But, apparently, we use
extern "C" directly in WebKit. OK, I had remembered it differently.

>>> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:84
>>> +WebCacheModel WebPreferencesCacheModelForMainBundle(NSString
*bundleIdentifier);
>> 
>> Since this is just for testing. do we need to add it to an SPI header? I
know I have written TestWebKitAPI tests that test things in WebCore directly
for example.
> 
> Yes, it's required due to TAPI/InstallAPI on iOS.  See this failure from
"Patch v1" on the iOS bot:
> 
> <https://ews-build.webkit.org/#/builders/53/builds/196/steps/7/logs/errors>
> 
> It also can't be in the WebPreferencesInternal.h header because that's not
included in the TAPI processing (since those symbols don't have to be
exported).
> 
> It would be possible to make a "TAPI extras for testing" header that just
declares this function, as I've done something similar for the libxml2 project
to declare exported functions that aren't in headers, but WebKitLegacy doesn't
have one of those yet.	(It's not hard to add--I'm just noting the current
state of things.)  Adding such a header does become "one more thing" to track
down when trying to fix TAPI/InstallAPI issues, though.

Big picture, I don’t want us to expose things for testing and then have others
at Apple or elsewhere accidentally depend on them as SPI. Whatever strategy we
have for that should be followed consistently. You don’t have to invent a new
strategy for this patch, but I don’t want us in a bind where everything we want
to test we also have to expose in a way that tempts people to use it. Maybe the
least we can do is put "ForTesting" in function names?


More information about the webkit-reviews mailing list