This is useful for testing layout and api cases where a bundle identifier is needed, so we don't have to manually set it each time.
<rdar://problem/59820107>
Created attachment 391779 [details] Patch
Comment on attachment 391779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391779&action=review > Tools/TestWebKitAPI/Configurations/TestWebKitAPI.xcconfig:27 > +PRODUCT_BUNDLE_IDENTIFIER = com.apple.TestWebKitAPI; I wonder if these should be com.apple.WebKit.* like the subprocesses are > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:5333 > + CREATE_INFOPLIST_SECTION_IN_BINARY = YES; > + INFOPLIST_FILE = "$(SRCROOT)/Info.plist"; You should move these two lines to the xcconfig, not here. > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:5342 > + CREATE_INFOPLIST_SECTION_IN_BINARY = YES; > + INFOPLIST_FILE = "$(SRCROOT)/Info.plist"; Ditto > Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:1291 > + CREATE_INFOPLIST_SECTION_IN_BINARY = YES; > + INFOPLIST_FILE = "$(SRCROOT)/Info.plist"; Ditto etc etc etc
Created attachment 391784 [details] Patch for landing
(In reply to Tim Horton from comment #3) > Comment on attachment 391779 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391779&action=review > > > Tools/TestWebKitAPI/Configurations/TestWebKitAPI.xcconfig:27 > > +PRODUCT_BUNDLE_IDENTIFIER = com.apple.TestWebKitAPI; > > I wonder if these should be com.apple.WebKit.* like the subprocesses are > Good call. > > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:5333 > > + CREATE_INFOPLIST_SECTION_IN_BINARY = YES; > > + INFOPLIST_FILE = "$(SRCROOT)/Info.plist"; > > You should move these two lines to the xcconfig, not here. > > > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:5342 > > + CREATE_INFOPLIST_SECTION_IN_BINARY = YES; > > + INFOPLIST_FILE = "$(SRCROOT)/Info.plist"; > > Ditto > > > Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:1291 > > + CREATE_INFOPLIST_SECTION_IN_BINARY = YES; > > + INFOPLIST_FILE = "$(SRCROOT)/Info.plist"; > > Ditto etc etc etc Done! Thanks Tim.
Comment on attachment 391784 [details] Patch for landing Clearing flags on attachment: 391784 Committed r257522: <https://trac.webkit.org/changeset/257522>
All reviewed patches have been landed. Closing bug.
This change caused 16 API test failures on the bots: https://build.webkit.org/builders/Apple-Catalina-Release-WK2-Tests/builds/3268 Please let API test EWS bots complete before re-landing.
Reverted r257522 for reason: Caused 16 API test failures on the bots Committed r257540: <https://trac.webkit.org/changeset/257540>
Created attachment 391872 [details] Patch
Letting EWS run. Adding a bundle ID changes the file path for TestWebKitAPI website data, so this patch updates the expectations for tests which need to know it. There should probably be a central place where tests can construct the default path the same way the default WebsiteDataStore does instead of hardcoding it each time. That would avoid test failures like these if the default path changes in the future.
(In reply to katherine_cheney from comment #11) > Letting EWS run. > > Adding a bundle ID changes the file path for TestWebKitAPI website data, so > this patch updates the expectations for tests which need to know it. There > should probably be a central place where tests can construct the default > path the same way the default WebsiteDataStore does instead of hardcoding it > each time. That would avoid test failures like these if the default path > changes in the future. We should be careful about this. That 'central place' needs to be in testing code, not WebKit, because we have had bugs where we used the wrong default data store, and we want API tests to catch that sort of thing.
Created attachment 391911 [details] Patch
Created attachment 391918 [details] Patch
Comment on attachment 391918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391918&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:456 > + NSURL *defaultResourceLoadStatisticsFilePath = [NSURL fileURLWithPath:[@"~/Library/WebKit/com.apple.WebKit.TestWebKitAPI/WebsiteData/ResourceLoadStatistics/full_browsing_session_resourceLog.plist" stringByExpandingTildeInPath] isDirectory:NO]; I wonder if we should have a helper for "give me a path inside TWKAPI's library dir" > Tools/TestWebKitAPI/Tests/mac/SetAndUpdateCacheModel.mm:49 > +static void resetTestState() > +{ > + WebPreferences *standardPreferences = [WebPreferences standardPreferences]; > + [standardPreferences setCacheModel:WebCacheModelDocumentViewer]; > +} > + > TEST(WebKitLegacy, SetAndUpdateCacheModelInitialModel) > { > + resetTestState(); > + > EXPECT_EQ((int)WebCacheModelDocumentViewer, (int)[WebView _cacheModel]); > I think the point of this test is to test the default value, so it's wrong to set it to something beforehand. Can you instead /delete/ the default, so we fall back to the default-default?
(In reply to Tim Horton from comment #15) > Comment on attachment 391918 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391918&action=review > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:456 > > + NSURL *defaultResourceLoadStatisticsFilePath = [NSURL fileURLWithPath:[@"~/Library/WebKit/com.apple.WebKit.TestWebKitAPI/WebsiteData/ResourceLoadStatistics/full_browsing_session_resourceLog.plist" stringByExpandingTildeInPath] isDirectory:NO]; > > I wonder if we should have a helper for "give me a path inside TWKAPI's > library dir" ... oh, you already said that
(In reply to Tim Horton from comment #15) > Comment on attachment 391918 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391918&action=review > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:456 > > + NSURL *defaultResourceLoadStatisticsFilePath = [NSURL fileURLWithPath:[@"~/Library/WebKit/com.apple.WebKit.TestWebKitAPI/WebsiteData/ResourceLoadStatistics/full_browsing_session_resourceLog.plist" stringByExpandingTildeInPath] isDirectory:NO]; > > I wonder if we should have a helper for "give me a path inside TWKAPI's > library dir" > > > Tools/TestWebKitAPI/Tests/mac/SetAndUpdateCacheModel.mm:49 > > +static void resetTestState() > > +{ > > + WebPreferences *standardPreferences = [WebPreferences standardPreferences]; > > + [standardPreferences setCacheModel:WebCacheModelDocumentViewer]; > > +} > > + > > TEST(WebKitLegacy, SetAndUpdateCacheModelInitialModel) > > { > > + resetTestState(); > > + > > EXPECT_EQ((int)WebCacheModelDocumentViewer, (int)[WebView _cacheModel]); > > > > I think the point of this test is to test the default value, so it's wrong > to set it to something beforehand. > > Can you instead /delete/ the default, so we fall back to the default-default? Yes. Very good idea.
Created attachment 391933 [details] Patch
Comment on attachment 391933 [details] Patch Clearing flags on attachment: 391933 Committed r257614: <https://trac.webkit.org/changeset/257614>