Bug 193884 - REGRESSION (r236481): Move soft-linking of LocalAuthentication.framework out of LocalAuthenticationSoftLink.h
Summary: REGRESSION (r236481): Move soft-linking of LocalAuthentication.framework out ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-27 11:02 PST by David Kilzer (:ddkilzer)
Modified: 2019-01-28 18:10 PST (History)
6 users (show)

See Also:


Attachments
Patch v1 (11.89 KB, patch)
2019-01-27 11:18 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2019-01-27 11:02:28 PST
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.
Comment 1 David Kilzer (:ddkilzer) 2019-01-27 11:18:56 PST
Created attachment 360300 [details]
Patch v1
Comment 2 Jiewen Tan 2019-01-27 15:27:07 PST
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 3 David Kilzer (:ddkilzer) 2019-01-28 11:31:48 PST
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 4 Jiewen Tan 2019-01-28 17:15:21 PST
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 5 David Kilzer (:ddkilzer) 2019-01-28 17:39:30 PST
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 6 David Kilzer (:ddkilzer) 2019-01-28 17:40:47 PST
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 7 WebKit Commit Bot 2019-01-28 17:58:39 PST
Comment on attachment 360300 [details]
Patch v1

Clearing flags on attachment: 360300

Committed r240631: <https://trac.webkit.org/changeset/240631>
Comment 8 WebKit Commit Bot 2019-01-28 17:58:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-01-28 17:59:28 PST
<rdar://problem/47618985>
Comment 10 Jiewen Tan 2019-01-28 18:10:54 PST
(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.