RESOLVED FIXED 231407
[GPU Process] Unique RenderingResourceIdentifiers Part 7: Migrate RemoteResourceCache to QualifiedRenderingResourceIdentifier 
https://bugs.webkit.org/show_bug.cgi?id=231407
Summary [GPU Process] Unique RenderingResourceIdentifiers Part 7: Migrate RemoteResou...
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.