Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Address post review comments for <https://commits.webkit.org/272703@main> #22543

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Address post review comments for <https://commits.webkit.org/272703@main> #22543

merged 1 commit into from
Jan 17, 2024

Conversation

pvollan
Copy link
Contributor

@pvollan pvollan commented Jan 9, 2024 •
edited by webkit-early-warning-system
Loading

90be7bc

Address post review comments for <https://commits.webkit.org/272703@main>
https://bugs.webkit.org/show_bug.cgi?id=267275
rdar://120867428

Reviewed by Jer Noble and Brent Fulgham.

This patch is addressing post review comments for <https://commits.webkit.org/272703@main>. It also makes
sure that commit is called on the update coordinator in the UI process right after the message is sent
to the WebContent process to update the layer size.

Additionally, this patch addresses an issue where we did not update the hosting handle in the UI process
when the context ID changed. This issue would stop video rendering in certain cases.

* Source/WebCore/platform/cocoa/VideoPresentationModelVideoElement.mm:
(WebCore::VideoPresentationModelVideoElement::setVideoSizeFenced):
* Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:
(WebKit::RemoteMediaPlayerProxy::setVideoLayerSizeFenced):
* Source/WebKit/Platform/cocoa/LayerHostingContext.h:
(WebKit::LayerHostingContext::hostable const):
* Source/WebKit/Platform/cocoa/LayerHostingContext.mm:
(WebKit::LayerHostingContext::createForExternalHostingProcess):
(WebKit::LayerHostingContext::~LayerHostingContext):
(WebKit::LayerHostingContext::contextID const):
(WebKit::LayerHostingContext::setFencePort):
(WebKit::LayerHostingContext::createHostingUpdateCoordinator):
(WebKit::LayerHostingContext::createHostingHandle):
(WebKit::LayerHostingContext::commit): Deleted.
* Source/WebKit/Platform/spi/Cocoa/ExtensionKitSPI.h:
* Source/WebKit/UIProcess/Cocoa/VideoPresentationManagerProxy.mm:
(WebKit::VideoPresentationManagerProxy::createLayerHostViewWithID):
(WebKit::VideoPresentationManagerProxy::setVideoLayerFrame):
(-[WKLayerHostView dealloc]): Deleted.

Canonical link: https://commits.webkit.org/273143@main

ebe5c3d

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2   🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@pvollan pvollan self-assigned this Jan 9, 2024
@pvollan pvollan added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Jan 9, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 9, 2024 •
edited
Loading

EWS run on previous version of this PR (hash c583e68)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ❌ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 9, 2024
brentfulgham
brentfulgham reviewed Jan 10, 2024
View reviewed changes
Copy link
Contributor

@brentfulgham brentfulgham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a few questions about the patch, so I'm not comfortable approving quite yet (see below).

Source/WebCore/platform/cocoa/VideoPresentationModelVideoElement.mm Show resolved Hide resolved
Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm Outdated

m_configuration.videoLayerSize = size;
setVideoLayerSizeIfPossible(size);

m_player->setVideoLayerSizeFenced(size, WTFMove(machSendRight));
m_player->setVideoLayerSizeFenced(size, WTFMove(machSendRight), [] { });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to pass a no-op completion handler here? Does our completion handler not need to run after the player sets its layer size?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the below code for committing the hosting update coordinator should be the completion handler for m_player->setVideoLayerSizeFenced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are completely right, this has been addressed in the latest version of the patch.

Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm Outdated
if (m_inlineLayerHostingContext)
m_inlineLayerHostingContext->commit();
[hostingUpdateCoordinator commit];
completionHandler();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only use completionHandler if USE(EXTENSIONKIT) is true. I think this might create a compile error for platforms that don't use EXTENSIONKIT; you might need to add a UNUSED_PARAM(completionHandler) as an #else here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively (and probably better style-wise) would be to call the completionHandler outside of the #if USE(EXTENSIONKIT) block.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking below at MediaPlayerPrivateRemote::setVideoLayerSizeFenced, it seems even more wrong that we pass a no-op completion handler, then run code that invokes the completion handler, since setVideoLayerSizeFenced clearly triggers async remote calls in some cases. I think that means we will sometimes call our completion handler before the async work completes, which doesn't seem right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe you are right. I have addressed this and the compile issue in the latest version.

Source/WebKit/Platform/cocoa/LayerHostingContext.mm Show resolved Hide resolved
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Jan 15, 2024
@pvollan pvollan force-pushed the eng/Address-post-review-comments-for-httpscommits-webkit-org272703main branch from c583e68 to cdaebc9 Compare January 15, 2024 17:40
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for cdaebc9. Live statuses available at the PR page, #22543

@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 15, 2024 •
edited
Loading

EWS run on previous version of this PR (hash cdaebc9)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ⏳ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@pvollan pvollan force-pushed the eng/Address-post-review-comments-for-httpscommits-webkit-org272703main branch from cdaebc9 to d565792 Compare January 16, 2024 02:45
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 16, 2024 •
edited
Loading

EWS run on previous version of this PR (hash d565792)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ⏳ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2   🧪 api-mac   🧪 api-wpe
✅ 🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 🧪 jsc ✅ 🧪 api-ios   🧪 mac-wk2   🧪 gtk-wk2
✅ 🛠 tv ⏳ 🧪 mac-AS-debug-wk2   🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@pvollan pvollan force-pushed the eng/Address-post-review-comments-for-httpscommits-webkit-org272703main branch from d565792 to c6be3db Compare January 16, 2024 03:12
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for c6be3db. Live statuses available at the PR page, #22543

@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 16, 2024 •
edited
Loading

EWS run on previous version of this PR (hash c6be3db)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac   🛠 wpe ⏳ 🛠 wincairo
  🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
  🧪 webkitperl   🧪 ios-wk2   🧪 api-mac   🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1   🛠 gtk
⏳ 🛠 🧪 jsc   🧪 api-ios   🧪 mac-wk2   🧪 gtk-wk2
⏳ 🛠 tv ⏳ 🧪 mac-AS-debug-wk2   🧪 api-gtk
⏳ 🛠 tv-sim
⏳ 🛠 watch
✅ 🛠 watch-sim

@pvollan pvollan force-pushed the eng/Address-post-review-comments-for-httpscommits-webkit-org272703main branch from c6be3db to f662f25 Compare January 16, 2024 03:14
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 16, 2024 •
edited
Loading

EWS run on previous version of this PR (hash f662f25)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ⏳ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2   🧪 gtk-wk2
✅ 🛠 tv ⏳ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@pvollan pvollan force-pushed the eng/Address-post-review-comments-for-httpscommits-webkit-org272703main branch from f662f25 to c1f0fdc Compare January 16, 2024 03:54
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for c1f0fdc. Live statuses available at the PR page, #22543

@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 16, 2024 •
edited
Loading

EWS run on previous version of this PR (hash c1f0fdc)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@pvollan pvollan force-pushed the eng/Address-post-review-comments-for-httpscommits-webkit-org272703main branch from c1f0fdc to dcce7ae Compare January 16, 2024 15:43
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 16, 2024 •
edited
Loading

EWS run on previous version of this PR (hash dcce7ae)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ⏳ 🛠 wincairo
  🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
  🧪 webkitperl   🧪 ios-wk2   🧪 api-mac   🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1   🛠 gtk
⏳ 🛠 🧪 jsc   🧪 api-ios   🧪 mac-wk2   🧪 gtk-wk2
✅ 🛠 tv   🧪 mac-AS-debug-wk2   🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@pvollan pvollan force-pushed the eng/Address-post-review-comments-for-httpscommits-webkit-org272703main branch from dcce7ae to d9859aa Compare January 16, 2024 15:45
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 16, 2024 •
edited
Loading

EWS run on previous version of this PR (hash d9859aa)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ⏳ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

brentfulgham
brentfulgham approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@brentfulgham brentfulgham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making those changes. I think this looks correct, though it would be best for an expert like Tim or Simon to review.

smfr
smfr reviewed Jan 16, 2024
View reviewed changes
Source/WebKit/UIProcess/Cocoa/VideoPresentationManagerProxy.mm
}

#if USE(EXTENSIONKIT)
auto pid = m_page->process().processPool().gpuProcess()->processID();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not great to have to two ends of this communication (here and RemoteMediaPlayerProxyCocoa) be in two separate classes. It would be much nicer to factor all the SEHosting/xpc goop into its own class that both ends use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a very good point! Will fix.

Thanks for reviewing!

@pvollan pvollan force-pushed the eng/Address-post-review-comments-for-httpscommits-webkit-org272703main branch from d9859aa to d27d4a5 Compare January 16, 2024 22:18
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 16, 2024 •
edited
Loading

EWS run on previous version of this PR (hash d27d4a5)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ❌ 🛠 ios ❌ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ❌ 🛠 ios-sim ❌ 🛠 mac-AS-debug   🧪 wpe-wk2
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ❌ 🧪 api-mac ✅ 🧪 api-wpe
❌ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ✅ 🛠 gtk
❌ 🧪 api-ios ❌ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
❌ 🛠 tv ❌ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
❌ 🛠 tv-sim ❌ 🧪 mac-wk2-stress
  🛠 watch
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 16, 2024
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Jan 17, 2024
@pvollan pvollan force-pushed the eng/Address-post-review-comments-for-httpscommits-webkit-org272703main branch from d27d4a5 to 022ae01 Compare January 17, 2024 00:42
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 17, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 022ae01)

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ⏳ 🛠 wincairo
✅ 🧪 bindings ⏳ 🛠 ios-sim ⏳ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl ⏳ 🧪 ios-wk2   🧪 api-mac ✅ 🧪 api-wpe
⏳ 🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 gtk
⏳ 🛠 🧪 jsc ⏳ 🧪 api-ios   🧪 mac-wk2 ⏳ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc-arm64 ⏳ 🛠 tv ⏳ 🧪 mac-AS-debug-wk2   🧪 api-gtk
⏳ 🛠 tv-sim
⏳ 🛠 watch
✅ 🛠 watch-sim

@pvollan pvollan force-pushed the eng/Address-post-review-comments-for-httpscommits-webkit-org272703main branch from 022ae01 to fcd62a3 Compare January 17, 2024 00:53
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 17, 2024 •
edited
Loading

EWS run on previous version of this PR (hash fcd62a3)

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
  🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

brentfulgham
brentfulgham approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@brentfulgham brentfulgham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good -- thanks for taking the time to address all of these comments! r+

@pvollan pvollan added merge-queue Applied to send a pull request to merge-queue and removed merge-queue Applied to send a pull request to merge-queue labels Jan 17, 2024
jernoble
jernoble approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@jernoble jernoble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me, but the commit message should be updated to reflect the most recent implementation choices.

@pvollan
Copy link
Contributor Author

pvollan commented Jan 17, 2024

r=me, but the commit message should be updated to reflect the most recent implementation choices.

Will do!

Thanks for reviewing!

@pvollan pvollan force-pushed the eng/Address-post-review-comments-for-httpscommits-webkit-org272703main branch from fcd62a3 to ebe5c3d Compare January 17, 2024 20:05
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 17, 2024 •
edited
Loading

EWS run on current version of this PR (hash ebe5c3d)

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2   🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@pvollan pvollan added the merge-queue Applied to send a pull request to merge-queue label Jan 17, 2024
@pvollan
Address post review comments for <https://commits.webkit.org/272703@main
90be7bc 
>

https://bugs.webkit.org/show_bug.cgi?id=267275
rdar://120867428

Reviewed by Jer Noble and Brent Fulgham.

This patch is addressing post review comments for <https://commits.webkit.org/272703@main>. It also makes
sure that commit is called on the update coordinator in the UI process right after the message is sent
to the WebContent process to update the layer size.

Additionally, this patch addresses an issue where we did not update the hosting handle in the UI process
when the context ID changed. This issue would stop video rendering in certain cases.

* Source/WebCore/platform/cocoa/VideoPresentationModelVideoElement.mm:
(WebCore::VideoPresentationModelVideoElement::setVideoSizeFenced):
* Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:
(WebKit::RemoteMediaPlayerProxy::setVideoLayerSizeFenced):
* Source/WebKit/Platform/cocoa/LayerHostingContext.h:
(WebKit::LayerHostingContext::hostable const):
* Source/WebKit/Platform/cocoa/LayerHostingContext.mm:
(WebKit::LayerHostingContext::createForExternalHostingProcess):
(WebKit::LayerHostingContext::~LayerHostingContext):
(WebKit::LayerHostingContext::contextID const):
(WebKit::LayerHostingContext::setFencePort):
(WebKit::LayerHostingContext::createHostingUpdateCoordinator):
(WebKit::LayerHostingContext::createHostingHandle):
(WebKit::LayerHostingContext::commit): Deleted.
* Source/WebKit/Platform/spi/Cocoa/ExtensionKitSPI.h:
* Source/WebKit/UIProcess/Cocoa/VideoPresentationManagerProxy.mm:
(WebKit::VideoPresentationManagerProxy::createLayerHostViewWithID):
(WebKit::VideoPresentationManagerProxy::setVideoLayerFrame):
(-[WKLayerHostView dealloc]): Deleted.

Canonical link: https://commits.webkit.org/273143@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Address-post-review-comments-for-httpscommits-webkit-org272703main branch from ebe5c3d to 90be7bc Compare January 17, 2024 21:54
@webkit-commit-queue
Copy link
Collaborator

Committed 273143@main (90be7bc): https://commits.webkit.org/273143@main

Reviewed commits have been landed. Closing PR #22543 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 90be7bc into WebKit:main Jan 17, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smfr smfr smfr left review comments

@jernoble jernoble jernoble approved these changes

@brentfulgham brentfulgham brentfulgham approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@rniwa rniwa Awaiting requested review from rniwa

@aestes aestes Awaiting requested review from aestes

@hortont424 hortont424 Awaiting requested review from hortont424

Assignees

@pvollan pvollan

Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
Milestone
No milestone
7 participants
@pvollan @webkit-early-warning-system @webkit-commit-queue @smfr @jernoble @brentfulgham @webkit-ews-buildbot