[webkit-reviews] review granted: [Bug 208090] [GPUP] Implement Modern EME API in the GPU Process : [Attachment 391622] Part 1: WebCore

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 208090: [GPUP] Implement Modern EME API in the GPU Process
https://bugs.webkit.org/show_bug.cgi?id=208090

Attachment 391622: Part 1: WebCore

https://bugs.webkit.org/attachment.cgi?id=391622&action=review




--- Comment #27 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 391622
  --> https://bugs.webkit.org/attachment.cgi?id=391622
Part 1: WebCore

View in context: https://bugs.webkit.org/attachment.cgi?id=391622&action=review

> Source/WebCore/Modules/encryptedmedia/MediaKeySystemAccess.cpp:89
> +	   instance->initializeWithConfiguration(*m_configuration,
useDistinctiveIdentifier ? AllowDistinctiveIdentifiers::Yes :
AllowDistinctiveIdentifiers::No, persistentStateAllowed ?
AllowPersistentState::Yes : AllowPersistentState::No, [this, protectedThis =
makeRef(*this), useDistinctiveIdentifier, persistentStateAllowed, instance =
WTFMove(instance), promise = WTFMove(promise)] (auto successValue) mutable {

Wouldn't a weakPtr be better than a refPtr when using the GPU process?

> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:110
> +    m_instance->setServerCertificate(WTFMove(certificate), [promise =
WTFMove(promise)] (auto success) {

Doesn't this need one or the other?

> Source/WebCore/platform/encryptedmedia/CDMPrivate.cpp:214
> +	   persistentStateRequirement =  CDMRequirement::NotAllowed;

Nit: two spaces.

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreaming
AVFObjC.mm:275
> +    auto initialize = [&] () {
> +	   if (configuration.distinctiveIdentifier == CDMRequirement::Required)

Doesn't this need a protectedThis or weakThis?