Move soft-linking of LocalAuthentication.framework out of LocalAuthenticationSoftLink.h. This regressed in <https://trac.webkit.org/r236481> when LocalAuthenticationSoftLink.{h,mm} were moved out of Source/WebCore. See Bug 193750 for why we don't want to put the soft-linking macros in headers.
Created attachment 360300 [details] Patch v1
Comment on attachment 360300 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=360300&action=review Thanks Dave for making the correction. LGTM. r=me. > Source/WebKit/SourcesCocoa.txt:473 > +UIProcess/WebAuthentication/Cocoa/LocalAuthenticationSoftLink.mm @no-unify Do we really need this?
Comment on attachment 360300 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=360300&action=review >> Source/WebKit/SourcesCocoa.txt:473 >> +UIProcess/WebAuthentication/Cocoa/LocalAuthenticationSoftLink.mm @no-unify > > Do we really need this? Are you asking about the "@no-unify" keyword, or the entry in SourcesCocoa.txt? I think it's safe to include LocalAuthenticationSoftLink.mm in the Unified Sources as it doesn't do anything like including a #define that redefines a constant variable, but the general policy is not to include *SoftLink.{cpp,mm} in Unified Sources, so I would just leave it as-is.
Comment on attachment 360300 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=360300&action=review >>> Source/WebKit/SourcesCocoa.txt:473 >>> +UIProcess/WebAuthentication/Cocoa/LocalAuthenticationSoftLink.mm @no-unify >> >> Do we really need this? > > Are you asking about the "@no-unify" keyword, or the entry in SourcesCocoa.txt? > > I think it's safe to include LocalAuthenticationSoftLink.mm in the Unified Sources as it doesn't do anything like including a #define that redefines a constant variable, but the general policy is not to include *SoftLink.{cpp,mm} in Unified Sources, so I would just leave it as-is. The line really confuses me: you add this line to indicate the file to be included in Unified Sources, but then you add @no-unify to indicate it should not be included. It just confuses me.
Comment on attachment 360300 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=360300&action=review >>>> Source/WebKit/SourcesCocoa.txt:473 >>>> +UIProcess/WebAuthentication/Cocoa/LocalAuthenticationSoftLink.mm @no-unify >>> >>> Do we really need this? >> >> Are you asking about the "@no-unify" keyword, or the entry in SourcesCocoa.txt? >> >> I think it's safe to include LocalAuthenticationSoftLink.mm in the Unified Sources as it doesn't do anything like including a #define that redefines a constant variable, but the general policy is not to include *SoftLink.{cpp,mm} in Unified Sources, so I would just leave it as-is. > > The line really confuses me: you add this line to indicate the file to be included in Unified Sources, but then you add @no-unify to indicate it should not be included. It just confuses me. Okay. I was following the convention used in Sources/WebCore/Sources[Cocoa].txt: $ grep @no-unify Source/WebCore/Sources*.txt | wc -l 77 $ grep @no-unify Source/WebKit/Sources*.txt | wc -l 282
Comment on attachment 360300 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=360300&action=review >>>>> Source/WebKit/SourcesCocoa.txt:473 >>>>> +UIProcess/WebAuthentication/Cocoa/LocalAuthenticationSoftLink.mm @no-unify >>>> >>>> Do we really need this? >>> >>> Are you asking about the "@no-unify" keyword, or the entry in SourcesCocoa.txt? >>> >>> I think it's safe to include LocalAuthenticationSoftLink.mm in the Unified Sources as it doesn't do anything like including a #define that redefines a constant variable, but the general policy is not to include *SoftLink.{cpp,mm} in Unified Sources, so I would just leave it as-is. >> >> The line really confuses me: you add this line to indicate the file to be included in Unified Sources, but then you add @no-unify to indicate it should not be included. It just confuses me. > > Okay. I was following the convention used in Sources/WebCore/Sources[Cocoa].txt: > > $ grep @no-unify Source/WebCore/Sources*.txt | wc -l > 77 > > $ grep @no-unify Source/WebKit/Sources*.txt | wc -l > 282 I suspect @no-unify sources are included so someone doesn't try to add it later (thinking it's missing), then it gets mistakenly included in unified sources when it shouldn't.
Comment on attachment 360300 [details] Patch v1 Clearing flags on attachment: 360300 Committed r240631: <https://trac.webkit.org/changeset/240631>
All reviewed patches have been landed. Closing bug.
<rdar://problem/47618985>
(In reply to David Kilzer (:ddkilzer) from comment #6) > Comment on attachment 360300 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=360300&action=review > > >>>>> Source/WebKit/SourcesCocoa.txt:473 > >>>>> +UIProcess/WebAuthentication/Cocoa/LocalAuthenticationSoftLink.mm @no-unify > >>>> > >>>> Do we really need this? > >>> > >>> Are you asking about the "@no-unify" keyword, or the entry in SourcesCocoa.txt? > >>> > >>> I think it's safe to include LocalAuthenticationSoftLink.mm in the Unified Sources as it doesn't do anything like including a #define that redefines a constant variable, but the general policy is not to include *SoftLink.{cpp,mm} in Unified Sources, so I would just leave it as-is. > >> > >> The line really confuses me: you add this line to indicate the file to be included in Unified Sources, but then you add @no-unify to indicate it should not be included. It just confuses me. > > > > Okay. I was following the convention used in Sources/WebCore/Sources[Cocoa].txt: > > > > $ grep @no-unify Source/WebCore/Sources*.txt | wc -l > > 77 > > > > $ grep @no-unify Source/WebKit/Sources*.txt | wc -l > > 282 > > I suspect @no-unify sources are included so someone doesn't try to add it > later (thinking it's missing), then it gets mistakenly included in unified > sources when it shouldn't. That makes sense now.