Bug 231407

Summary: [GPU Process] Unique RenderingResourceIdentifiers Part 7: Migrate RemoteResourceCache to QualifiedRenderingResourceIdentifier
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: Layout and RenderingAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, sabouhallawa, simon.fraser, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 231546    
Bug Blocks: 217638    
Attachments:
Description Flags
Patch
sabouhallawa: review+
Patch for committing none

Myles C. Maxfield
Reported 2021-10-07 19:35:05 PDT
Migrate RemoteResourceCache to QualifiedRenderingResourceIdentifier
Attachments
Patch (25.09 KB, patch)
2021-10-07 19:54 PDT, Myles C. Maxfield 		sabouhallawa: review+
DetailsFormatted DiffDiff
Patch for committing (32.03 KB, patch)
2021-10-11 21:12 PDT, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2021-10-07 19:35:42 PDT
<rdar://problem/84010479>
Myles C. Maxfield
Comment 2 2021-10-07 19:54:52 PDT
Created attachment 440567 [details] Patch
Said Abou-Hallawa
Comment 3 2021-10-11 11:59:19 PDT
Comment on attachment 440567 [details] Patch This patch looks okay. But I am surprised that RemoteImageBuffer currently has two identifiers. One is from the base class: ConcreteImageBuffer::m_renderingResourceIdentifier and the other is RemoteImageBuffer::m_renderingResourceIdentifier. The later was added in r269682. Luckily they hold the same value. But I think this needs to be cleaned-up.
Wenson Hsieh
Comment 4 2021-10-11 12:03:34 PDT
(In reply to Said Abou-Hallawa from comment #3) > Comment on attachment 440567 [details] > Patch > > This patch looks okay. But I am surprised that RemoteImageBuffer currently > has two identifiers. One is from the base class: > ConcreteImageBuffer::m_renderingResourceIdentifier and the other is > RemoteImageBuffer::m_renderingResourceIdentifier. The later was added in > r269682. Luckily they hold the same value. But I think this needs to be > cleaned-up. On trunk, RemoteImageBuffer's `m_renderingResourceIdentifier` is a WebKit::QualifiedRenderingResourceIdentifier while the base class just has a regular RenderingResourceIdentifier.
Myles C. Maxfield
Comment 5 2021-10-11 17:46:13 PDT
(In reply to Said Abou-Hallawa from comment #3) > Comment on attachment 440567 [details] > Patch > > This patch looks okay. But I am surprised that RemoteImageBuffer currently > has two identifiers. One is from the base class: > ConcreteImageBuffer::m_renderingResourceIdentifier and the other is > RemoteImageBuffer::m_renderingResourceIdentifier. The later was added in > r269682. Luckily they hold the same value. But I think this needs to be > cleaned-up. Yeah. Personally I think it's a mistake to put the identifiers inside the objects themselves, but I do understand why it's expedient. Philosophically, I think the objects themselves should have no idea that they're being shared across processes, and the process sharing code should be out-of-band. I think that 1) not everyone on the team agrees with this, though, and 2) even if they did, doing so is out of scope for this patch.
Myles C. Maxfield
Comment 6 2021-10-11 17:48:32 PDT
Committed r283955 (242806@main): <https://commits.webkit.org/242806@main>
Myles C. Maxfield
Comment 7 2021-10-11 18:33:15 PDT
This broke the build.
Myles C. Maxfield
Comment 8 2021-10-11 18:41:03 PDT
Broke the build
Myles C. Maxfield
Comment 9 2021-10-11 21:12:59 PDT
Created attachment 440882 [details] Patch for committing
Myles C. Maxfield
Comment 10 2021-10-11 21:20:14 PDT
Committed r283968 (242815@main): <https://commits.webkit.org/242815@main>
Note You need to log in before you can comment on or make changes to this bug.