Summary: | REGRESSION (r236481): Move soft-linking of LocalAuthentication.framework out of LocalAuthenticationSoftLink.h | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Component: | WebKit Misc. | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cdumez, commit-queue, ews-watchlist, glenn, jiewen_tan, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=193750 | ||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2019-01-27 11:02:28 PST
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. (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. |