Bug 217884 – REGRESSION (r25430): Fix -Wstring-concatenation warning from open source clang

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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>

[David Kilzer (:ddkilzer)]

Created attachment 411685 [details]
Patch v1

[Alexey Proskuryakov]

Comment on attachment 411685 [details]
Patch v1

r=me as a mechanical change, but is any of this still necessary?

[David Kilzer (:ddkilzer)]

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

[Radar WebKit Bug Importer]

<rdar://problem/70414026>

[David Kilzer (:ddkilzer)]

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

[EWS]

Committed r268652: <https://trac.webkit.org/changeset/268652>

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

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

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

[Darin Adler]

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

[David Kilzer (:ddkilzer)]

(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