[webkit-reviews] review granted: [Bug 214659] [Cocoa] Adopt -[AVContentKeyRequest willOutputBeObscuredDueToInsufficientExternalProtectionForDisplays:] : [Attachment 405014] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 214659: [Cocoa] Adopt -[AVContentKeyRequest
willOutputBeObscuredDueToInsufficientExternalProtectionForDisplays:]
https://bugs.webkit.org/show_bug.cgi?id=214659

Attachment 405014: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 405014
  --> https://bugs.webkit.org/attachment.cgi?id=405014
Patch

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

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:79
> +    , m_displayChangedObserver([this] (auto displayID) {
displayChanged(displayID); })

Why not write m_instanceSession->displayChanged here and then we don’t need a
separate displayChanged member function?

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:737
> +    auto* document = downcast<Document>(scriptExecutionContext());
> +    if (!document)
> +	   return 0;
> +
> +    if (auto* page = document->page())
> +	   return page->displayID();
> +
> +    return 0;

We switch styles here from early return style to variable-scoped-inside-the-if
style. We shouldn’t switch styles back and forth in the same function without a
good reason.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:115
> +    // DisplayChangedObserver

Doesn’t seem valuable to add this comment.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:147
> +    using DisplayChangedObserver = Observer<void(PlatformDisplayID)>;
> +    DisplayChangedObserver m_displayChangedObserver;

Not sure that defining this typename so we can use it once adds clarity.

If we want a reusable named type, then we should define it in Document.h
outside the Document class. Otherwise, I suggest we just write the Observer<>
part.

> Source/WebCore/platform/encryptedmedia/CDMInstanceSession.h:58
> +    using PlatformDisplayID = uint32_t;
> +    virtual PlatformDisplayID displayID() = 0;

Same thought here about the named type. If it’s important to have a name, then
lets put it where we can reuse it. I don’t think that clarifying that a
function’s result is a display ID by naming the type is important if the
function is named displayID.

> Source/WebCore/platform/encryptedmedia/CDMInstanceSession.h:109
> +    using PlatformDisplayID = uint32_t;
> +    virtual void displayChanged(PlatformDisplayID) { }

Again, seems like we could name the argument rather than naming the type.
Unless the type can be usefully used elsewhere.

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreaming
AVFObjC.h:69
> +    virtual void
externalProtectionStatusDidChangeForContentKeyRequest(AVContentKeyRequest*) =
0;

Can this be a reference rather than a pointer? Or is this an Objective-C class?

Why doesn’t this header follow our rule of using subtly different formatting
for Objective-C object pointer types?

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreaming
AVFObjC.mm:1375
> +    UNUSED_PARAM(request);

I suggest omitting the argument name instead of using UNUSED_PARAM.