217884 2020-10-17 16:13:47 -0700 REGRESSION (r25430): Fix -Wstring-concatenation warning from open source clang 2020-10-20 09:08:43 -0700 1 1 1 Unclassified WebKit WebKit Misc. Other Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 217960 1 ddkilzer ddkilzer darin ggaren webkit-bug-importer oldest_to_newest 1699019 0 ddkilzer 2020-10-17 16:13:47 -0700 Fix -Wstring-concatenation warning from open source clang: Source/WebKitLegacy/mac/WebView/WebPreferences.mm:135:13: warning: suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma? [-Wstring-concatenation] "com.app4mac.wKiosk", ^ Source/WebKitLegacy/mac/WebView/WebPreferences.mm:134:13: note: place parentheses around the string literal to silence warning "com.app4mac.KidsBrowser" ^ This regressed way back in r25430. <https://trac.webkit.org/r25430> 1699020 1 411685 ddkilzer 2020-10-17 16:19:07 -0700 Created attachment 411685 Patch v1 1699049 2 411685 ap 2020-10-17 19:35:11 -0700 Comment on attachment 411685 Patch v1 r=me as a mechanical change, but is any of this still necessary? 1699056 3 ddkilzer 2020-10-17 22:13:59 -0700 (In reply to Alexey Proskuryakov from comment #2) > Comment on attachment 411685 [details] > Patch v1 > > r=me as a mechanical change, but is any of this still necessary? Thanks. Removing this seems like a separate concern. 1699057 4 webkit-bug-importer 2020-10-17 22:14:12 -0700 <rdar://problem/70414026> 1699058 5 ddkilzer 2020-10-17 22:19:26 -0700 (In reply to David Kilzer (:ddkilzer) from comment #3) > (In reply to Alexey Proskuryakov from comment #2) > > Comment on attachment 411685 [details] > > Patch v1 > > > > r=me as a mechanical change, but is any of this still necessary? > > Thanks. Removing this seems like a separate concern. Oops, hit the Save button too soon. I’m not sure what the time limit is for removing older quirks. We should have a way to document when (or what releases) they were added for so we can make a judgment call to remove them without resorting to reviewing commit history. 1699059 6 ews-feeder 2020-10-17 22:28:43 -0700 Committed r268652: <https://trac.webkit.org/changeset/268652> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411685. 1699098 7 411685 darin 2020-10-18 09:54:19 -0700 Comment on attachment 411685 Patch v1 Before this change, both com.app4mac.KidsBrowser and com.app4mac.wKiosk were *not* getting treated as primary web browsers! Because string pasting turned the first string in the array into this: "com.app4mac.KidsBrowsercom.app4mac.wKiosk" So while this fix is great to have, fixing it without a regression test kind of violates our WebKit policy and we should explain why we are unable to effectively test. 1699162 8 ddkilzer 2020-10-18 16:25:34 -0700 (In reply to Darin Adler from comment #7) > Comment on attachment 411685 [details] > Patch v1 > > Before this change, both com.app4mac.KidsBrowser and com.app4mac.wKiosk were > *not* getting treated as primary web browsers! > > Because string pasting turned the first string in the array into this: > > "com.app4mac.KidsBrowsercom.app4mac.wKiosk" > > So while this fix is great to have, fixing it without a regression test kind > of violates our WebKit policy and we should explain why we are unable to > effectively test. Because WebKit compiles with -Werror, this warning (which is on by default in open source clang) will generate an error in future versions of Apple clang, causing the compile to fail. I will look at adding a test next week if it's not sufficient for the compiler to catch the issue. 1699164 9 darin 2020-10-18 16:27:29 -0700 (In reply to David Kilzer (:ddkilzer) from comment #8) > Because WebKit compiles with -Werror, this warning (which is on by default > in open source clang) will generate an error in future versions of Apple > clang, causing the compile to fail. My point is that the code didn’t work because it was untested. Normally we’d test that the code did what was intended. > I will look at adding a test next week if it's not sufficient for the > compiler to catch the issue. I’m not worried about catching the exact same issue. I’m worried about this being broken and untested, and now fixed and still untested. 1699755 10 ddkilzer 2020-10-20 09:08:43 -0700 (In reply to Darin Adler from comment #9) > (In reply to David Kilzer (:ddkilzer) from comment #8) > > Because WebKit compiles with -Werror, this warning (which is on by default > > in open source clang) will generate an error in future versions of Apple > > clang, causing the compile to fail. > > My point is that the code didn’t work because it was untested. Normally we’d > test that the code did what was intended. > > > I will look at adding a test next week if it's not sufficient for the > > compiler to catch the issue. > > I’m not worried about catching the exact same issue. I’m worried about this > being broken and untested, and now fixed and still untested. Bug 217960: Add test for cacheModelForMainBundle() in WebKitLegacy 411685 2020-10-17 16:19:07 -0700 2020-10-17 22:28:43 -0700 Patch v1 bug-217884-20201017162259.patch text/plain 1505 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMjY4NjQ1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0TGVn YWN5L21hYy9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0TGVnYWN5L21hYy9DaGFuZ2VMb2cKaW5k ZXggMWZlMmJmZDIxMjA5OTgxM2JkNjgxNjgyNjVkNDk1YWZjOWI5ZDY0MS4uOWVkZDJlZDBiNWMw YWU2NDE1MWRkOTQwOWFkYmU1ODUyYmU1MjkwZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdExl Z2FjeS9tYWMvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXRMZWdhY3kvbWFjL0NoYW5nZUxv ZwpAQCAtMSwzICsxLDE0IEBACisyMDIwLTEwLTE3ICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHplckBh cHBsZS5jb20+CisKKyAgICAgICAgUkVHUkVTU0lPTiAocjI1NDMwKTogRml4IC1Xc3RyaW5nLWNv bmNhdGVuYXRpb24gd2FybmluZyBmcm9tIG9wZW4gc291cmNlIGNsYW5nCisgICAgICAgIDxodHRw czovL3dlYmtpdC5vcmcvYi8yMTc4ODQ+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgKiBXZWJWaWV3L1dlYlByZWZlcmVuY2VzLm1tOgorICAgICAgICAo Y2FjaGVNb2RlbEZvck1haW5CdW5kbGUpOgorICAgICAgICAtIEFkZCBtaXNzaW5nIGNvbW1hIHRv IGZpeCB1bndhbnRlZCBzdHJpbmcgY29uY2F0ZW5hdGlvbi4KKwogMjAyMC0xMC0xNiAgRGV2aW4g Um91c3NvICA8ZHJvdXNzb0BhcHBsZS5jb20+CiAKICAgICAgICAgV2ViIEluc3BlY3RvcjogcmVu YW1lIGBIaWdobGlnaHRgIHNvIGl0IGRvZXNuJ3QgY29uZmxpY3Qgd2l0aCBDU1MgYEhpZ2hsaWdo dGAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXRMZWdhY3kvbWFjL1dlYlZpZXcvV2ViUHJlZmVy ZW5jZXMubW0gYi9Tb3VyY2UvV2ViS2l0TGVnYWN5L21hYy9XZWJWaWV3L1dlYlByZWZlcmVuY2Vz Lm1tCmluZGV4IDA4NmZlZGY1N2RmMWRkZjc5OTAwZjNlZDFmNTBkYmJlMDFjN2RkNWEuLjJhN2U4 ZTRiN2U0NTQyMWM2MGU2NTY2NDdiYzMzYWY4YjY3MmU3ZTggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9X ZWJLaXRMZWdhY3kvbWFjL1dlYlZpZXcvV2ViUHJlZmVyZW5jZXMubW0KKysrIGIvU291cmNlL1dl YktpdExlZ2FjeS9tYWMvV2ViVmlldy9XZWJQcmVmZXJlbmNlcy5tbQpAQCAtMTMxLDcgKzEzMSw3 IEBAIHN0YXRpYyBXZWJDYWNoZU1vZGVsIGNhY2hlTW9kZWxGb3JNYWluQnVuZGxlKHZvaWQpCiAK ICAgICAgICAgLy8gQXBwcyB0aGF0IHByb2JhYmx5IG5lZWQgdGhlIGxhcmdlIHNldHRpbmcKICAg ICAgICAgc3RhdGljIGNvbnN0IGNoYXIqIGNvbnN0IHByaW1hcnlXZWJCcm93c2VySURzW10gPSB7 Ci0gICAgICAgICAgICAiY29tLmFwcDRtYWMuS2lkc0Jyb3dzZXIiCisgICAgICAgICAgICAiY29t LmFwcDRtYWMuS2lkc0Jyb3dzZXIiLAogICAgICAgICAgICAgImNvbS5hcHA0bWFjLndLaW9zayIs CiAgICAgICAgICAgICAiY29tLmZyZWV2ZXJzZS5idW1wZXJjYXIiLAogICAgICAgICAgICAgImNv bS5vbW5pZ3JvdXAuT21uaVdlYjUiLAo=