Summary: | Create CA layer contexts with +remoteContextWithOptions. | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||
Status: | NEW --- | ||||||||||||||||||||||||
Severity: | Normal | CC: | ap, bfulgham, commit-queue, simon.fraser, thorton, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 182855, 183048 | ||||||||||||||||||||||||
Bug Blocks: | 183663 | ||||||||||||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2018-02-13 14:47:28 PST
Created attachment 333729 [details]
Patch
Comment on attachment 333729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333729&action=review > Source/WebKit/Platform/mac/LayerHostingContext.mm:68 > + layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior : @"ignore"}]; I think this new behavior needs to be gated on a version check. This might need to be added to the SPI header declarations. Or is this code path 101400 and beyond? Also, why are we using different options than iOS? Should they be more similar? > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:591 > + (global-name "com.apple.CARenderServer") I've started adding comments to explain why these are needed. I'd add " ; Needed for [CAContext remoteContextWithOptions]" at the end of the line. (In reply to Brent Fulgham from comment #2) > Comment on attachment 333729 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333729&action=review > > > Source/WebKit/Platform/mac/LayerHostingContext.mm:68 > > + layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior : @"ignore"}]; > > I think this new behavior needs to be gated on a version check. This might > need to be added to the SPI header declarations. Or is this code path 101400 > and beyond? Also, why are we using different options than iOS? Should they > be more similar? > > > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:591 > > + (global-name "com.apple.CARenderServer") > > I've started adding comments to explain why these are needed. I'd add " ; > Needed for [CAContext remoteContextWithOptions]" at the end of the line. Thanks for reviewing! I'll update the patch. Created attachment 333738 [details]
Patch
Comment on attachment 333738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333738&action=review > Source/WebKit/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=182747 Needs a radar number. Created attachment 333740 [details]
Patch
(In reply to Simon Fraser (smfr) from comment #5) > Comment on attachment 333738 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333738&action=review > > > Source/WebKit/ChangeLog:4 > > + https://bugs.webkit.org/show_bug.cgi?id=182747 > > Needs a radar number. Thanks! I also expanded the changelog entry a little. Comment on attachment 333740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333740&action=review Looks good! > Source/WebKit/Platform/mac/LayerHostingContext.mm:68 > + layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior : @"ignore"}]; Do we need to include 'kCAContextIgnoresHitTest" or "kCAContextDisplayId"? Maybe smfr knows. Comment on attachment 333740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333740&action=review > Source/WebKit/Platform/mac/LayerHostingContext.mm:67 > + [CAContext setAllowsCGSConnections: false]; No space after the colon, and this is probably a BOOL so it should be NO not false, right? >> Source/WebKit/Platform/mac/LayerHostingContext.mm:68 >> + layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior : @"ignore"}]; > > Do we need to include 'kCAContextIgnoresHitTest" or "kCAContextDisplayId"? Maybe smfr knows. No, those are for iOS. This is more like the one below it. Created attachment 333756 [details]
Patch
(In reply to Tim Horton from comment #9) > Comment on attachment 333740 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333740&action=review > > > Source/WebKit/Platform/mac/LayerHostingContext.mm:67 > > + [CAContext setAllowsCGSConnections: false]; > > No space after the colon, and this is probably a BOOL so it should be NO not > false, right? > > >> Source/WebKit/Platform/mac/LayerHostingContext.mm:68 > >> + layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior : @"ignore"}]; > > > > Do we need to include 'kCAContextIgnoresHitTest" or "kCAContextDisplayId"? Maybe smfr knows. > > No, those are for iOS. This is more like the one below it. Thanks! I have updated the patch. (In reply to Per Arne Vollan from comment #11) > (In reply to Tim Horton from comment #9) > > Comment on attachment 333740 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=333740&action=review > > > > > Source/WebKit/Platform/mac/LayerHostingContext.mm:67 > > > + [CAContext setAllowsCGSConnections: false]; > > > > No space after the colon, and this is probably a BOOL so it should be NO not > > false, right? > > > > >> Source/WebKit/Platform/mac/LayerHostingContext.mm:68 > > >> + layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior : @"ignore"}]; > > > > > > Do we need to include 'kCAContextIgnoresHitTest" or "kCAContextDisplayId"? Maybe smfr knows. > > > > No, those are for iOS. This is more like the one below it. > > Thanks! I have updated the patch. Were you planning on more changes? Or is this up for review now? (In reply to Brent Fulgham from comment #12) > (In reply to Per Arne Vollan from comment #11) > > (In reply to Tim Horton from comment #9) > > > Comment on attachment 333740 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=333740&action=review > > > > > > > Source/WebKit/Platform/mac/LayerHostingContext.mm:67 > > > > + [CAContext setAllowsCGSConnections: false]; > > > > > > No space after the colon, and this is probably a BOOL so it should be NO not > > > false, right? > > > > > > >> Source/WebKit/Platform/mac/LayerHostingContext.mm:68 > > > >> + layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior : @"ignore"}]; > > > > > > > > Do we need to include 'kCAContextIgnoresHitTest" or "kCAContextDisplayId"? Maybe smfr knows. > > > > > > No, those are for iOS. This is more like the one below it. > > > > Thanks! I have updated the patch. > > Were you planning on more changes? Or is this up for review now? I have one more small change, I'll upload it soon. Created attachment 333808 [details]
Patch
Comment on attachment 333808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333808&action=review > Source/WebKit/WebProcess/WebProcess.cpp:226 > + CGSShutdownServerConnections(); Are there connections to shut down? Why would we have made any before this? Should we just assert that there are none? (In reply to Per Arne Vollan from comment #14) > Created attachment 333808 [details] > Patch The patch seems to introduce some layout tests failures on 10.14, e.g. compositing/animation/animation-backing.html. I will look into it. (In reply to Simon Fraser (smfr) from comment #15) > Comment on attachment 333808 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333808&action=review > > > Source/WebKit/WebProcess/WebProcess.cpp:226 > > + CGSShutdownServerConnections(); > > Are there connections to shut down? Why would we have made any before this? > Should we just assert that there are none? Thanks, good point! Debugging shows that there are no connections at this point, I will add an assert. Comment on attachment 333808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333808&action=review > Source/WebKit/Shared/mac/HangDetectionDisablerMac.mm:51 > + // In this case, there will be no valid WindowServer main connection. If that's the case, should we ASSERT(!CGSMainConnectionID()) and just return, and have the CGSSetConnectionProperty be in an #else clause? > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:590 > (global-name "com.apple.windowserver.active") Do we need "windowserver.active"? I can't remember if you found that this endpoint was needed when using the CARenderServer. (In reply to Brent Fulgham from comment #18) > Comment on attachment 333808 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333808&action=review > > > Source/WebKit/Shared/mac/HangDetectionDisablerMac.mm:51 > > + // In this case, there will be no valid WindowServer main connection. > > If that's the case, should we ASSERT(!CGSMainConnectionID()) and just > return, and have the CGSSetConnectionProperty be in an #else clause? > I think we still need CGSSetConnectionProperty, since this is shared code that might be executed by processes other than the WebContent process. Or am I mistaken? > > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:590 > > (global-name "com.apple.windowserver.active") > > Do we need "windowserver.active"? I can't remember if you found that this > endpoint was needed when using the CARenderServer. Thanks! I believe you are correct, it is probably not needed. I'll update the patch. Comment on attachment 333808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333808&action=review >>> Source/WebKit/Shared/mac/HangDetectionDisablerMac.mm:51 >>> + // In this case, there will be no valid WindowServer main connection. >> >> If that's the case, should we ASSERT(!CGSMainConnectionID()) and just return, and have the CGSSetConnectionProperty be in an #else clause? > > I think we still need CGSSetConnectionProperty, since this is shared code that might be executed by processes other than the WebContent process. Or am I mistaken? Hmm. I'll bet you are right. This is probably needed for PluginProcess. Created attachment 333818 [details]
Patch
Created attachment 333822 [details]
Patch
This might deserve at least a cursory glance at some graphics benchmark numbers to make sure the architecture change doesn’t have any impact. Can you do that? Maybe MotionMark? (In reply to Tim Horton from comment #24) > This might deserve at least a cursory glance at some graphics benchmark > numbers to make sure the architecture change doesn’t have any impact. Can > you do that? Maybe MotionMark? Absolutely, I'll look into that. Created attachment 335238 [details]
Patch
Created attachment 335341 [details]
Patch
Comment on attachment 335341 [details]
Patch
Great -- this looks ready to go!
Created attachment 335457 [details]
Patch
Comment on attachment 335457 [details]
Patch
Thanks for reviewing!
Comment on attachment 335457 [details] Patch Clearing flags on attachment: 335457 Committed r229484: <https://trac.webkit.org/changeset/229484> This is: <rdar://problem/37517466> |