159912 2016-07-18 17:49:29 -0700 webbookmarksd needs to use the same AppCache directory as MobileSafari 2016-07-19 16:08:19 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 achristensen achristensen beidson commit-queue oldest_to_newest 1212221 0 achristensen 2016-07-18 17:49:29 -0700 webbookmarksd needs to use the same AppCache directory as MobileSafari 1212222 1 283967 achristensen 2016-07-18 17:50:31 -0700 Created attachment 283967 Patch 1212223 2 commit-queue 2016-07-18 17:51:57 -0700 Attachment 283967 did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style. 1212240 3 283967 mitz 2016-07-18 18:52:38 -0700 Comment on attachment 283967 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283967&action=review > Source/WebKit2/UIProcess/API/Cocoa/APIWebsiteDataStoreCocoa.mm:43 > // FIXME: Ideally we should just have Safari and WebApp create a data store with You should add webbookmarksd to the comment. It would be good to also have a reference to the Apple bug tracking fixing this. 1212241 4 beidson 2016-07-18 18:56:17 -0700 (In reply to comment #2) > Attachment 283967 [details] did not pass style-queue: > > > ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and > either add and list tests, or explain why no new tests were possible. > [changelog/nonewtests] [5] > Total errors found: 1 in 5 files This is actually testable - We've recently started adding API tests where we manually put SQLite and data files in place, then do something with WK API to remove them, then manually check that they were removed. 1212242 5 achristensen 2016-07-18 19:03:46 -0700 https://trac.webkit.org/changeset/203392 1212243 6 achristensen 2016-07-18 19:04:36 -0700 (In reply to comment #4) > (In reply to comment #2) > > Attachment 283967 [details] did not pass style-queue: > > > > > > ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and > > either add and list tests, or explain why no new tests were possible. > > [changelog/nonewtests] [5] > > Total errors found: 1 in 5 files > > This is actually testable - We've recently started adding API tests where we > manually put SQLite and data files in place, then do something with WK API > to remove them, then manually check that they were removed. Can we change the main bundle identifier to com.apple.webbookmarksd in the tests? 1212255 7 beidson 2016-07-18 20:28:40 -0700 (In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #2) > > > Attachment 283967 [details] did not pass style-queue: > > > > > > > > > ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and > > > either add and list tests, or explain why no new tests were possible. > > > [changelog/nonewtests] [5] > > > Total errors found: 1 in 5 files > > > > This is actually testable - We've recently started adding API tests where we > > manually put SQLite and data files in place, then do something with WK API > > to remove them, then manually check that they were removed. > Can we change the main bundle identifier to com.apple.webbookmarksd in the > tests? Alexey - Is there a way too spoof bundle IDs in API tests? Or do you have an alternate idea here? 1212257 8 ap 2016-07-18 20:35:40 -0700 Swizzling should work I guess. We need to do something to make API tests scalable, I'm skeptical of adding more when they run sequentially and each one is so slow. 1212259 9 beidson 2016-07-18 20:49:12 -0700 (In reply to comment #8) > Swizzling should work I guess. > > We need to do something to make API tests scalable, I'm skeptical of adding > more when they run sequentially and each one is so slow. This *almost* sounds like you're messaging "We shouldn't add any more API tests until we find a way to make them run in parallel", but I'm sure that's not your actual point ;) I don't see why run-api-tests couldn't run multiple parallel instances of TestWebKitAPI just like run-webkit-tests does, but that's clearly out of scope for this bug. 1212267 10 achristensen 2016-07-18 21:30:16 -0700 cmake ports make multiple api test executables. We could even make one per test. I would like to add a test for this, but I wasn't aware of this "Swizzling". If someone could point me in the right direction, I'll add a test. 1212274 11 beidson 2016-07-18 22:22:04 -0700 (In reply to comment #10) > cmake ports make multiple api test executables. We could even make one per > test. > > I would like to add a test for this, but I wasn't aware of this "Swizzling". > If someone could point me in the right direction, I'll add a test. http://darkdust.net/index.php/writings/objective-c/method-swizzling So, what we want to change is this: static String applicationBundleIdentifier() { // The override only gets set in WebKit2's WebProcess and NetworkProcess. If unset, we use the main bundle identifier. const auto& identifier = applicationBundleIdentifierOverride(); ASSERT(identifier.isNull() || RunLoop::isMain()); return identifier.isNull() ? String([[NSBundle mainBundle] bundleIdentifier]) : identifier; } So you'd swizzle the -[NSBundle bundleIdentifier] method to be the fake one, run the test, then swizzle it back. 1212275 12 beidson 2016-07-18 22:22:43 -0700 Also https://blog.newrelic.com/2014/04/16/right-way-to-swizzle/ 1212288 13 achristensen 2016-07-18 23:57:04 -0700 Oh, THAT swizzling. I verified this fix manually. I'm not sure how much value it would add to have a test that uses swizzling to verify the existence of an iOS-specific hack that we want to get rid of. My complete lack of testing isn't the best, either... 1212398 14 beidson 2016-07-19 09:47:36 -0700 (In reply to comment #13) > Oh, THAT swizzling. I verified this fix manually. I'm not sure how much > value it would add to have a test that uses swizzling to verify the > existence of an iOS-specific hack that we want to get rid of. The value is that we don't regress again in the meantime. "Regression tests" <--- It's right there in the name. 1212582 15 achristensen 2016-07-19 16:08:19 -0700 Regression tests added in https://bugs.webkit.org/show_bug.cgi?id=159949 283967 2016-07-18 17:50:31 -0700 2016-07-18 18:29:25 -0700 Patch bug-159912-20160718174936.patch text/plain 3889 achristensen SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIwMzM3NCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDE2LTA3LTE4ICBBbGV4IENo cmlzdGVuc2VuICA8YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+CisKKyAgICAgICAgd2ViYm9va21h cmtzZCBuZWVkcyB0byB1c2UgdGhlIHNhbWUgQXBwQ2FjaGUgZGlyZWN0b3J5IGFzIE1vYmlsZVNh ZmFyaQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU5 OTEyCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8g bmV3IHRlc3RzIChPT1BTISkuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9SdW50aW1lQXBwbGljYXRp b25DaGVja3MuaDoKKyAgICAgICAgKiBwbGF0Zm9ybS9SdW50aW1lQXBwbGljYXRpb25DaGVja3Mu bW06CisgICAgICAgIChXZWJDb3JlOjpJT1NBcHBsaWNhdGlvbjo6aXNNb2JpbGVTYWZhcmkpOgor ICAgICAgICAoV2ViQ29yZTo6SU9TQXBwbGljYXRpb246OmlzV2ViQm9va21hcmtzRCk6CisgICAg ICAgIChXZWJDb3JlOjpJT1NBcHBsaWNhdGlvbjo6aXNEdW1wUmVuZGVyVHJlZSk6CisKIDIwMTYt MDctMTggIEJyZW50IEZ1bGdoYW0gIDxiZnVsZ2hhbUBhcHBsZS5jb20+CiAKICAgICAgICAgVW5y ZXZpZXdlZCwgcm9sbGluZyBvdXQgcjIwMzM3My4KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL1J1bnRpbWVBcHBsaWNhdGlvbkNoZWNrcy5oCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL1J1bnRpbWVBcHBsaWNhdGlvbkNoZWNrcy5oCShyZXZpc2lvbiAyMDMzNzQp CisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9SdW50aW1lQXBwbGljYXRpb25DaGVja3MuaAko d29ya2luZyBjb3B5KQpAQCAtNjIsNiArNjIsNyBAQCBuYW1lc3BhY2UgSU9TQXBwbGljYXRpb24g ewogCiBXRUJDT1JFX0VYUE9SVCBib29sIGlzTW9iaWxlTWFpbCgpOwogV0VCQ09SRV9FWFBPUlQg Ym9vbCBpc01vYmlsZVNhZmFyaSgpOworV0VCQ09SRV9FWFBPUlQgYm9vbCBpc1dlYkJvb2ttYXJr c0QoKTsKIGJvb2wgaXNEdW1wUmVuZGVyVHJlZSgpOwogYm9vbCBpc01vYmlsZVN0b3JlKCk7CiBX RUJDT1JFX0VYUE9SVCBib29sIGlzV2ViQXBwKCk7CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9SdW50aW1lQXBwbGljYXRpb25DaGVja3MubW0KPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dl YkNvcmUvcGxhdGZvcm0vUnVudGltZUFwcGxpY2F0aW9uQ2hlY2tzLm1tCShyZXZpc2lvbiAyMDMz NzQpCisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9SdW50aW1lQXBwbGljYXRpb25DaGVja3Mu bW0JKHdvcmtpbmcgY29weSkKQEAgLTE3MCw2ICsxNzAsMTIgQEAgYm9vbCBJT1NBcHBsaWNhdGlv bjo6aXNNb2JpbGVTYWZhcmkoKQogICAgIHJldHVybiBpc01vYmlsZVNhZmFyaTsKIH0KIAorYm9v bCBJT1NBcHBsaWNhdGlvbjo6aXNXZWJCb29rbWFya3NEKCkKK3sKKyAgICBzdGF0aWMgYm9vbCBp c1dlYkJvb2ttYXJrc0QgPSBhcHBsaWNhdGlvbkJ1bmRsZUlzRXF1YWxUbygiY29tLmFwcGxlLndl YmJvb2ttYXJrc2QiKTsKKyAgICByZXR1cm4gaXNXZWJCb29rbWFya3NEOworfQorCiBib29sIElP U0FwcGxpY2F0aW9uOjppc0R1bXBSZW5kZXJUcmVlKCkKIHsKICAgICAvLyBXZSB1c2UgYSBwcmVm aXggbWF0Y2ggaW5zdGVhZCBvZiBzdHJpY3QgZXF1YWxpdHkgc2luY2UgTGF5b3V0VGVzdFJlbGF5 IG1heSBsYXVuY2ggbXVsdGlwbGUgaW5zdGFuY2VzIG9mCkluZGV4OiBTb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCShyZXZpc2lv biAyMDMzODgpCisrKyBTb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAg LTEsMyArMSwxNSBAQAorMjAxNi0wNy0xOCAgQWxleCBDaHJpc3RlbnNlbiAgPGFjaHJpc3RlbnNl bkB3ZWJraXQub3JnPgorCisgICAgICAgIHdlYmJvb2ttYXJrc2QgbmVlZHMgdG8gdXNlIHRoZSBz YW1lIEFwcENhY2hlIGRpcmVjdG9yeSBhcyBNb2JpbGVTYWZhcmkKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1OTkxMgorICAgICAgICA8cmRhcjovL3By b2JsZW0vMjcwNTY4NDQ+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgKiBVSVByb2Nlc3MvQVBJL0NvY29hL0FQSVdlYnNpdGVEYXRhU3RvcmVDb2NvYS5t bToKKyAgICAgICAgKEFQSTo6V2Vic2l0ZURhdGFTdG9yZTo6ZGVmYXVsdEFwcGxpY2F0aW9uQ2Fj aGVEaXJlY3RvcnkpOgorICAgICAgICBNYWtlIHdlYmJvb2ttYXJrc2QgbWF0Y2ggTW9iaWxlU2Fm YXJpIGJ5IGFkZGluZyBhIG1hdGNoaW5nIHJ1bnRpbWUgZXhjZXB0aW9uLgorCiAyMDE2LTA3LTE4 ICBBbmRlcnMgQ2FybHNzb24gIDxhbmRlcnNjYUBhcHBsZS5jb20+CiAKICAgICAgICAgRG9uJ3Qg bnVsbCBvdXQgdGhlIElQQzo6Q29ubmVjdGlvbidzIFhQQyBjb25uZWN0aW9uCkluZGV4OiBTb3Vy Y2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL0NvY29hL0FQSVdlYnNpdGVEYXRhU3RvcmVDb2NvYS5t bQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL0NvY29hL0FQSVdl YnNpdGVEYXRhU3RvcmVDb2NvYS5tbQkocmV2aXNpb24gMjAzMzc0KQorKysgU291cmNlL1dlYktp dDIvVUlQcm9jZXNzL0FQSS9Db2NvYS9BUElXZWJzaXRlRGF0YVN0b3JlQ29jb2EubW0JKHdvcmtp bmcgY29weSkKQEAgLTQyLDcgKzQyLDcgQEAgU3RyaW5nIFdlYnNpdGVEYXRhU3RvcmU6OmRlZmF1 bHRBcHBsaWNhdAogICAgIC8vIFByZXNlcnZpbmcgaXQgYXZvaWRzIHRoZSBuZWVkIHRvIG1pZ3Jh dGUgZGF0YSB3aGVuIHVwZ3JhZGluZy4KICAgICAvLyBGSVhNRTogSWRlYWxseSB3ZSBzaG91bGQg anVzdCBoYXZlIFNhZmFyaSBhbmQgV2ViQXBwIGNyZWF0ZSBhIGRhdGEgc3RvcmUgd2l0aAogICAg IC8vIHRoaXMgYXBwbGljYXRpb24gY2FjaGUgcGF0aCwgYnV0IHRoYXQncyBub3Qgc3VwcG9ydGVk IGFzIG9mIHJpZ2h0IG5vdy4KLSAgICBpZiAoV2ViQ29yZTo6SU9TQXBwbGljYXRpb246OmlzTW9i aWxlU2FmYXJpKCkgfHwgV2ViQ29yZTo6SU9TQXBwbGljYXRpb246OmlzV2ViQXBwKCkpIHsKKyAg ICBpZiAoV2ViQ29yZTo6SU9TQXBwbGljYXRpb246OmlzTW9iaWxlU2FmYXJpKCkgfHwgV2ViQ29y ZTo6SU9TQXBwbGljYXRpb246OmlzV2ViQXBwKCkgfHwgV2ViQ29yZTo6SU9TQXBwbGljYXRpb246 OmlzV2ViQm9va21hcmtzRCgpKSB7CiAgICAgICAgIE5TU3RyaW5nICpjYWNoZVBhdGggPSBbTlNI b21lRGlyZWN0b3J5KCkgc3RyaW5nQnlBcHBlbmRpbmdQYXRoQ29tcG9uZW50OkAiTGlicmFyeS9D YWNoZXMvY29tLmFwcGxlLldlYkFwcENhY2hlIl07CiAKICAgICAgICAgcmV0dXJuIFdlYktpdDo6 c3RyaW5nQnlSZXNvbHZpbmdTeW1saW5rc0luUGF0aChjYWNoZVBhdGguc3RyaW5nQnlTdGFuZGFy ZGl6aW5nUGF0aCk7Cg==