Bug 217884

Summary: REGRESSION (r25430): Fix -Wstring-concatenation warning from open source clang
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebKit Misc.Assignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ggaren, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 217960    
Attachments:
Description Flags
Patch v1 none

Description David Kilzer (:ddkilzer) 2020-10-17 16:13:47 PDT
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>
Comment 1 David Kilzer (:ddkilzer) 2020-10-17 16:19:07 PDT
Created attachment 411685 [details]
Patch v1
Comment 2 Alexey Proskuryakov 2020-10-17 19:35:11 PDT
Comment on attachment 411685 [details]
Patch v1

r=me as a mechanical change, but is any of this still necessary?
Comment 3 David Kilzer (:ddkilzer) 2020-10-17 22:13:59 PDT
(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.
Comment 4 Radar WebKit Bug Importer 2020-10-17 22:14:12 PDT
<rdar://problem/70414026>
Comment 5 David Kilzer (:ddkilzer) 2020-10-17 22:19:26 PDT
(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.
Comment 6 EWS 2020-10-17 22:28:43 PDT
Committed r268652: <https://trac.webkit.org/changeset/268652>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411685 [details].
Comment 7 Darin Adler 2020-10-18 09:54:19 PDT
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.
Comment 8 David Kilzer (:ddkilzer) 2020-10-18 16:25:34 PDT
(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.
Comment 9 Darin Adler 2020-10-18 16:27:29 PDT
(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.
Comment 10 David Kilzer (:ddkilzer) 2020-10-20 09:08:43 PDT
(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