Bug 214659 – [Cocoa] Adopt -[AVContentKeyRequest willOutputBeObscuredDueToInsufficientExternalProtectionForDisplays:]

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[Cocoa] Adopt -[AVContentKeyRequest willOutputBeObscuredDueToInsufficientExternalProtectionForDisplays:]

[Jer Noble]

<rdar://problem/63555006>

[Jer Noble]

Created attachment 404986 [details]
Patch

[Jer Noble]

Created attachment 405014 [details]
Patch

[Darin Adler]

Comment on attachment 405014 [details]
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/CDMInstanceFairPlayStreamingAVFObjC.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/CDMInstanceFairPlayStreamingAVFObjC.mm:1375
> +    UNUSED_PARAM(request);

I suggest omitting the argument name instead of using UNUSED_PARAM.

[Jer Noble]

(In reply to Darin Adler from comment #4)
> Comment on attachment 405014 [details]
> 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?

Totally fine. Will change.

> > 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.

The current style is both the shortest possible with the least indentation. Being consistent in both if() statements would either make the function longer or deeper, which is why it is the way it currently is.  But sure, I'll switch the second statement to an early `return 0`.

> > Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:115
> > +    // DisplayChangedObserver
> 
> Doesn’t seem valuable to add this comment.

Well, it's gone now anyway. :)

> > 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.

Yeah, I wanted to avoid pulling in Document.h just to get this typedef.  I'll just use Observer<> directly.

> > 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.

Currently, "using PlatformDisplayID = uint32_t;" exists in 12 separate locations inside WebKit, including ANGLE, WebCore, and WebKit, so this was just extending that pattern. But sure, we could just return a uint32_t.

> > 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.

I think if we just use uint32_t here, this function should be named "displayIDChanged()" to make it perfectly clear what the parameter is.

> > Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.h:69
> > +    virtual void externalProtectionStatusDidChangeForContentKeyRequest(AVContentKeyRequest*) = 0;
> 
> Can this be a reference rather than a pointer? Or is this an Objective-C
> class?

This is an Objective-C class.

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

I have been told in the past that our rule for type names is "C++ style in C++ files", and I've internalized it. It's probably wrong, but it's also ambiguously worded in our Style Guidelines. We should probably clarify it there. Meanwhile, I'll fix this.

> > Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:1375
> > +    UNUSED_PARAM(request);
> 
> I suggest omitting the argument name instead of using UNUSED_PARAM.

Will do.

[Darin Adler]

(In reply to Jer Noble from comment #5)
> I have been told in the past that our rule for type names is "C++ style in
> C++ files"

Wow, it’s definitely not.

If we wanted to *change* our style rule, I would personally advocate a rule of "our C++ style in all files except for Cocoa public headers".

But right now, the rule we have agreed on is that we use the style to distinguish the types; has nothing to do with which source file it’s in.

[Jer Noble]

Created attachment 405199 [details]
Patch for landing

[Jer Noble]

(In reply to Darin Adler from comment #6)
> (In reply to Jer Noble from comment #5)
> > I have been told in the past that our rule for type names is "C++ style in
> > C++ files"
> 
> Wow, it’s definitely not.
> 
> If we wanted to *change* our style rule, I would personally advocate a rule
> of "our C++ style in all files except for Cocoa public headers".
> 
> But right now, the rule we have agreed on is that we use the style to
> distinguish the types; has nothing to do with which source file it’s in.

Got it. I'll work on un-internalize my incorrect understanding. :)

[Jer Noble]

Created attachment 405618 [details]
Patch for Landing

[EWS]

Committed r265115: <https://trac.webkit.org/changeset/265115>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405618 [details].