Bug 231407 - [GPU Process] Unique RenderingResourceIdentifiers Part 7: Migrate RemoteResourceCache to QualifiedRenderingResourceIdentifier
Summary: [GPU Process] Unique RenderingResourceIdentifiers Part 7: Migrate RemoteResou...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 231546
Blocks: 217638
  Show dependency treegraph
 
Reported: 2021-10-07 19:35 PDT by Myles C. Maxfield
Modified: 2021-10-11 21:20 PDT (History)
6 users (show)

See Also:


Attachments
Patch (25.09 KB, patch)
2021-10-07 19:54 PDT, Myles C. Maxfield
sabouhallawa: review+
Details | Formatted Diff | Diff
Patch for committing (32.03 KB, patch)
2021-10-11 21:12 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2021-10-07 19:35:05 PDT
Migrate RemoteResourceCache to QualifiedRenderingResourceIdentifier
Comment 1 Radar WebKit Bug Importer 2021-10-07 19:35:42 PDT
<rdar://problem/84010479>
Comment 2 Myles C. Maxfield 2021-10-07 19:54:52 PDT
Created attachment 440567 [details]
Patch
Comment 3 Said Abou-Hallawa 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.
Comment 4 Wenson Hsieh 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.
Comment 5 Myles C. Maxfield 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.
Comment 6 Myles C. Maxfield 2021-10-11 17:48:32 PDT
Committed r283955 (242806@main): <https://commits.webkit.org/242806@main>
Comment 7 Myles C. Maxfield 2021-10-11 18:33:15 PDT
This broke the build.
Comment 8 Myles C. Maxfield 2021-10-11 18:41:03 PDT
Broke the build
Comment 9 Myles C. Maxfield 2021-10-11 21:12:59 PDT
Created attachment 440882 [details]
Patch for committing
Comment 10 Myles C. Maxfield 2021-10-11 21:20:14 PDT
Committed r283968 (242815@main): <https://commits.webkit.org/242815@main>