Bug 217960 – Add test for cacheModelForMainBundle() in WebKitLegacy

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Add test for cacheModelForMainBundle() in WebKitLegacy.

See Bug 217884 which regressed in r25430 and was fixed in r268652.

[David Kilzer (:ddkilzer)]

Created attachment 411950 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Comment on attachment 411950 [details]
Patch v1

Hrm.  Doesn't compile on iOS.

[David Kilzer (:ddkilzer)]

Created attachment 411954 [details]
Patch v2

[David Kilzer (:ddkilzer)]

Comment on attachment 411954 [details]
Patch v2

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

> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:79
>  extern NSString *WebPreferencesChangedInternalNotification WEBKIT_DEPRECATED_MAC(10_3, 10_14);
>  extern NSString *WebPreferencesCacheModelChangedInternalNotification WEBKIT_DEPRECATED_MAC(10_5, 10_14);

I really wonder if these declarations should also use `extern "C"` as well, but I'm not sure if it's too late to change that.

This is completely separate from this patch.

[David Kilzer (:ddkilzer)]

Comment on attachment 411954 [details]
Patch v2

Can only use `exterm "C"` when compiling with C++.

[David Kilzer (:ddkilzer)]

Comment on attachment 411954 [details]
Patch v2

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

>> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:79
>>  extern NSString *WebPreferencesCacheModelChangedInternalNotification WEBKIT_DEPRECATED_MAC(10_5, 10_14);
> 
> I really wonder if these declarations should also use `extern "C"` as well, but I'm not sure if it's too late to change that.
> 
> This is completely separate from this patch.

Oops, I keep thinking these are functions, but they're constants.

[David Kilzer (:ddkilzer)]

Created attachment 411955 [details]
Patch v3

[Darin Adler]

Comment on attachment 411955 [details]
Patch v3

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

Thank you for adding a test!

> 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.

> 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.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #8)
> Comment on attachment 411955 [details]
> Patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411955&action=review
> 
> Thank you for adding a test!
> 
> > 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.

> > 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.

[Darin Adler]

Comment on attachment 411955 [details]
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?

[David Kilzer (:ddkilzer)]

Created attachment 412025 [details]
Patch v4

[David Kilzer (:ddkilzer)]

Created attachment 412027 [details]
Patch v5

[David Kilzer (:ddkilzer)]

Comment on attachment 411955 [details]
Patch v3

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

>>>> 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?

Patch v4/v5 implements the new strategy.  Not that difficult, but it's a bit different.

[Darin Adler]

Comment on attachment 412027 [details]
Patch v5

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

> Source/WebKitLegacy/mac/InstallAPIExportsForTesting.h:31
> +WebCacheModel WebPreferencesCacheModelForMainBundle(NSString *bundleIdentifier);

Not sure I’d put the name "InstallAPIExports" into this header’s name. Maybe just TestingFunctions.h?

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:95
> +WTF_EXTERN_C_BEGIN
> +// See also mac/InstallAPIExportsForTesting.h.
> +WebCacheModel WebPreferencesCacheModelForMainBundle(NSString *bundleIdentifier);
> +WTF_EXTERN_C_END

Seems like we’d just include the InstallAPIExportsForTesting.h header instead of doing this.

[David Kilzer (:ddkilzer)]

Created attachment 412029 [details]
Patch v6

[David Kilzer (:ddkilzer)]

Comment on attachment 412025 [details]
Patch v4

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

> Source/WebKitLegacy/mac/InstallAPIExportsForTesting.h:26
> + */
> +
> +#include <Foundation/Foundation.h>
> +#include <WebKitLegacy/WebPreferences.h>

Forgot to add the `#pragma once` here, then uploaded Patch v5 without amending the local change.  Patch v6 fixes it.

[David Kilzer (:ddkilzer)]

Comment on attachment 412027 [details]
Patch v5

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

>> Source/WebKitLegacy/mac/InstallAPIExportsForTesting.h:31
>> +WebCacheModel WebPreferencesCacheModelForMainBundle(NSString *bundleIdentifier);
> 
> Not sure I’d put the name "InstallAPIExports" into this header’s name. Maybe just TestingFunctions.h?

The only reason this exists is for InstallAPI to run successfully without declaring the function in a Private.h or Internal.h header.

(Unless we actually install the header in PrivateHeaders, which seems even weirder to me.)

>> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:95
>> +WTF_EXTERN_C_END
> 
> Seems like we’d just include the InstallAPIExportsForTesting.h header instead of doing this.

That would mean we'd have to install InstallAPIExportsForTesting.h as a Private Header for the WebKitLegacy.framework, and you wanted to avoid doing that so other Apple clients couldn't try to use it.

If we're going to install InstallAPIExportsForTesting.h as a Private Header, we might as well just move it back to WebPreferencesPrivate.h.

I guess I'm not clear what you would prefer to do now.

[Darin Adler]

Comment on attachment 412025 [details]
Patch v4

This all looks fine to me. Please consider my naming suggestion and my suggestion of including the header rather than doing a second extern "C" inside the .mm file.

[Darin Adler]

Comment on attachment 412027 [details]
Patch v5

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

>>> Source/WebKitLegacy/mac/InstallAPIExportsForTesting.h:31
>>> +WebCacheModel WebPreferencesCacheModelForMainBundle(NSString *bundleIdentifier);
>> 
>> Not sure I’d put the name "InstallAPIExports" into this header’s name. Maybe just TestingFunctions.h?
> 
> The only reason this exists is for InstallAPI to run successfully without declaring the function in a Private.h or Internal.h header.
> 
> (Unless we actually install the header in PrivateHeaders, which seems even weirder to me.)

We want to put it somewhere our test code can find it, but not where others can find it. To what ever extent it needs to be "installed" that would be configurations where we run our test code.

>>> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:95
>>> +WTF_EXTERN_C_END
>> 
>> Seems like we’d just include the InstallAPIExportsForTesting.h header instead of doing this.
> 
> That would mean we'd have to install InstallAPIExportsForTesting.h as a Private Header for the WebKitLegacy.framework, and you wanted to avoid doing that so other Apple clients couldn't try to use it.
> 
> If we're going to install InstallAPIExportsForTesting.h as a Private Header, we might as well just move it back to WebPreferencesPrivate.h.
> 
> I guess I'm not clear what you would prefer to do now.

We would not have to "install" a header to include it from *inside* WebKitLegacy, would we?

[Darin Adler]

You can land it like this. I am no trying to keep you spinning in a loop forever. But here are some of my goals:

1) Don’t make it easy to accidentally call test functions outside WebKit. Use naming, headers, linking restrictions, whatever, to minimize the chance of that mistake.

2) Signal that test functions are for testing, not other purposes. Use naming, headers, etc. to do that. They can be moved if we find non-testing purposes for them.

3) Expose the test functions so they can be used in the unit tests (the ones based on Google Test at the moment). Use a header so the unit test code is checked against the function in case the function is changed and someone overlooks the effect it has on the test. I believe that there is no constraint that the unit test has to be able to run on "the real system WebKit"; but I may be mistaken.

4) Name everything involved in a way that encourages people to use it in the future, and doesn’t require them to know the names of the tricky technology that might have made this hard to get working. Headers would thus be named "put tests in here", rather than "this header is for the install API technology that you have never heard of". There might well still be comments mentioning InstallAPI, but it’s unlikely to be in the header name.

[David Kilzer (:ddkilzer)]

Comment on attachment 412027 [details]
Patch v5

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

>>>> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:95
>>>> +WTF_EXTERN_C_END
>>> 
>>> Seems like we’d just include the InstallAPIExportsForTesting.h header instead of doing this.
>> 
>> That would mean we'd have to install InstallAPIExportsForTesting.h as a Private Header for the WebKitLegacy.framework, and you wanted to avoid doing that so other Apple clients couldn't try to use it.
>> 
>> If we're going to install InstallAPIExportsForTesting.h as a Private Header, we might as well just move it back to WebPreferencesPrivate.h.
>> 
>> I guess I'm not clear what you would prefer to do now.
> 
> We would not have to "install" a header to include it from *inside* WebKitLegacy, would we?

I see, you just want to use InstallAPIExportsForTesting.h header for WebPreferences.mm, not for WebPreferencesTest.mm (which is in a completely separate project directory).

If we wanted to use InstallAPIExportsForTesting.h in WebPreferencesTest.mm in the TestWebKitAPI project, we _would_ have to install InstallAPIExportsForTesting.h somewhere in the WebKitLegacy.framework project.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #20)
> You can land it like this. I am no trying to keep you spinning in a loop
> forever. But here are some of my goals:
> 
> 1) Don’t make it easy to accidentally call test functions outside WebKit.
> Use naming, headers, linking restrictions, whatever, to minimize the chance
> of that mistake.
> 
> 2) Signal that test functions are for testing, not other purposes. Use
> naming, headers, etc. to do that. They can be moved if we find non-testing
> purposes for them.
> 
> 3) Expose the test functions so they can be used in the unit tests (the ones
> based on Google Test at the moment). Use a header so the unit test code is
> checked against the function in case the function is changed and someone
> overlooks the effect it has on the test. I believe that there is no
> constraint that the unit test has to be able to run on "the real system
> WebKit"; but I may be mistaken.
> 
> 4) Name everything involved in a way that encourages people to use it in the
> future, and doesn’t require them to know the names of the tricky technology
> that might have made this hard to get working. Headers would thus be named
> "put tests in here", rather than "this header is for the install API
> technology that you have never heard of". There might well still be comments
> mentioning InstallAPI, but it’s unlikely to be in the header name.

Okay, where I'm confused is that WebPreferencesCacheModelForMainBundle() is both "the test function" and it is being used in production code.

Maybe you're asking for a slightly different design where WebPreferencesCacheModelForMainBundle() stays static and not exported, but there _another_ test function that calls WebPreferencesCacheModelForMainBundle() which _is_ exported, but is only declared in a special "testing functions" header, and that testing functions header is not installed with the project, and maybe we even turn off the testing function (and its export) in Production builds?

I can do that, but I don't know what macro to use for "disable this code in Production builds, but not anywhere else".

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #22)
> Maybe you're asking for a slightly different design where
> WebPreferencesCacheModelForMainBundle() stays static and not exported, but
> there _another_ test function that calls
> WebPreferencesCacheModelForMainBundle() which _is_ exported, but is only
> declared in a special "testing functions" header, and that testing functions
> header is not installed with the project, and maybe we even turn off the
> testing function (and its export) in Production builds?

Sure, that’s slightly better if it helps us communicate that we don’t want to export the function.

> I can do that, but I don't know what macro to use for "disable this code in
> Production builds, but not anywhere else".

I don’t know either, but it should be pretty easy to figure out. Let me know if you want help researching it. I’d start by looking at the DebugRelease.xcconfig files.

I’m mostly focused on the goals here, not the specific way we achieve them. Hoping for a pattern we can keep using in the future.

[Darin Adler]

(In reply to Darin Adler from comment #23)
> I’d start by looking at the DebugRelease.xcconfig files.

Or maybe at how WebKitInternals is built.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #24)
> (In reply to Darin Adler from comment #23)
> > I’d start by looking at the DebugRelease.xcconfig files.
> 
> Or maybe at how WebKitInternals is built.

That's a different mechanism.

I added a WK_BUILD_FOR_TESTING Xcode variable that lives in DebugRelease.xcconfig that simply sets a macro in OTHER_CFLAGS and OTHER_TAPI_FLAGS to enable test code when building in those configurations.

The nice thing is that it could be overwritten in Production builds by passing it on the command-line (if that's ever needed), which it might be for internal bots if they build and test using Production builds.

[David Kilzer (:ddkilzer)]

Created attachment 412111 [details]
Patch v7

[Darin Adler]

Comment on attachment 412111 [details]
Patch v7

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

> Source/WebKitLegacy/mac/Configurations/WebKitLegacy.xcconfig:45
> +OTHER_CFLAGS = $(inherited) $(OTHER_CFLAGS_COCOA_TOUCH) $(WK_CFLAGS_BUILD_FOR_TESTING_$(WK_BUILD_FOR_TESTING));

Could add a WK_CFLAGS_BUILD_FOR_TESTING without a suffix to make this and the one other use simpler.

> Source/WebKitLegacy/mac/Configurations/WebKitLegacy.xcconfig:50
> +WK_CFLAGS_BUILD_FOR_TESTING_YES = -DENABLE_BUILD_FOR_TESTING=1;

No need for the "=1" here.

[Radar WebKit Bug Importer]

<rdar://problem/70724139>

[David Kilzer (:ddkilzer)]

Comment on attachment 412111 [details]
Patch v7

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

>> Source/WebKitLegacy/mac/Configurations/WebKitLegacy.xcconfig:45
>> +OTHER_CFLAGS = $(inherited) $(OTHER_CFLAGS_COCOA_TOUCH) $(WK_CFLAGS_BUILD_FOR_TESTING_$(WK_BUILD_FOR_TESTING));
> 
> Could add a WK_CFLAGS_BUILD_FOR_TESTING without a suffix to make this and the one other use simpler.

I'm going to keep this as-is for now.  If (for some reason) someone wanted to enable testing code in Production builds, I want them to pass WK_BUILD_FOR_TESTING=YES on the command-line instead of having to pass WK_CFLAGS_BUILD_FOR_TESTING="-DENABLE_BUILD_FOR_TESTING".

>> Source/WebKitLegacy/mac/Configurations/WebKitLegacy.xcconfig:50
>> +WK_CFLAGS_BUILD_FOR_TESTING_YES = -DENABLE_BUILD_FOR_TESTING=1;
> 
> No need for the "=1" here.

Will fix.

[David Kilzer (:ddkilzer)]

Comment on attachment 412111 [details]
Patch v7

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

>>> Source/WebKitLegacy/mac/Configurations/WebKitLegacy.xcconfig:50
>>> +WK_CFLAGS_BUILD_FOR_TESTING_YES = -DENABLE_BUILD_FOR_TESTING=1;
>> 
>> No need for the "=1" here.
> 
> Will fix.

Actually, the way the ENABLE() macro is written, I think it should be =1.

[David Kilzer (:ddkilzer)]

Created attachment 413087 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #31)
> Created attachment 413087 [details]
> Patch for landing

The only changes from "Patch v7" are the updated date in the ChangeLogs and adding Darin as the reviewer.

[Darin Adler]

Comment on attachment 412111 [details]
Patch v7

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

>>>> Source/WebKitLegacy/mac/Configurations/WebKitLegacy.xcconfig:50
>>>> +WK_CFLAGS_BUILD_FOR_TESTING_YES = -DENABLE_BUILD_FOR_TESTING=1;
>>> 
>>> No need for the "=1" here.
>> 
>> Will fix.
> 
> Actually, the way the ENABLE() macro is written, I think it should be =1.

That’s not correct. The way the -D command line option works is that -DENABLE_BUILD_FOR_TESTING sets ENABLE_BUILD_FOR_TESTING to 1. The =1 is a no-op.

[David Kilzer (:ddkilzer)]

Comment on attachment 412111 [details]
Patch v7

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

>>>>> Source/WebKitLegacy/mac/Configurations/WebKitLegacy.xcconfig:50
>>>>> +WK_CFLAGS_BUILD_FOR_TESTING_YES = -DENABLE_BUILD_FOR_TESTING=1;
>>>> 
>>>> No need for the "=1" here.
>>> 
>>> Will fix.
>> 
>> Actually, the way the ENABLE() macro is written, I think it should be =1.
> 
> That’s not correct. The way the -D command line option works is that -DENABLE_BUILD_FOR_TESTING sets ENABLE_BUILD_FOR_TESTING to 1. The =1 is a no-op.

Thanks!  Will fix.

[David Kilzer (:ddkilzer)]

Created attachment 413094 [details]
Patch for landing v2

[David Kilzer (:ddkilzer)]

Comment on attachment 413094 [details]
Patch for landing v2

Marking cq+ since the only change from "Patch v7" is to remove the "=1" from the macro definition, and mac-wk1 tests bot already passed.

[EWS]

Committed r269332: <https://trac.webkit.org/changeset/269332>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413094 [details].