Bug 193884 – REGRESSION (r236481): Move soft-linking of LocalAuthentication.framework out of LocalAuthenticationSoftLink.h

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 360300 [details]
Patch v1

[Jiewen Tan]

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?

[David Kilzer (:ddkilzer)]

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.

[Jiewen Tan]

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.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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.

[WebKit Commit Bot]

Comment on attachment 360300 [details]
Patch v1

Clearing flags on attachment: 360300

Committed r240631: <https://trac.webkit.org/changeset/240631>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47618985>

[Jiewen Tan]

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