Closed Bug 1343409 Opened 3 years ago Closed 3 years ago

HTMLMediaElement::MozRequestDebugInfo should also return EMEInfo

Categories

(Core :: Audio/Video: Playback, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: kkoorts, Unassigned)

Details

Attachments

(1 file)

It would be useful, for debugging purposes, to also return EMEInfo with MediaKeys' ids and their session information.
Comment on attachment 8842252 [details]
Bug 1343409 - HTMLMediaElement::MozRequestDebugInfo returns EMEInfo.

https://reviewboard.mozilla.org/r/116128/#review117694

::: dom/media/eme/MediaKeys.cpp:589
(Diff revision 1)
> +       !it.Done();
> +       it.Next()) {
> +    MediaKeySession* keySession = it.Data();
> +    nsString sessionID;
> +    keySession->GetSessionId(sessionID);
> +    sessionsInfo.AppendPrintf("(sid:%s ", sessionID);

Here you AppendPrintf an nsString with a %s and without calling .get() on the nsString, and elsewhere you do use %S and use .get().

You should at least be consistent.

I expect you're supposed to use %S here (and call get()), and I expect you won't see a problem with your code as-is unless you try to compile this with clang or gcc, or run it on MacOS or Linux.

You could instead use:

sessionsInfo.AppendLiteral("sid:");
sessionsInfo.Append(sessionID);

And that would be slightly safer than a printf (as the string's length would be known in advance), and unambiguous.
Attachment #8842252 - Flags: review?(cpearce) → review-
Comment on attachment 8842252 [details]
Bug 1343409 - HTMLMediaElement::MozRequestDebugInfo returns EMEInfo.

https://reviewboard.mozilla.org/r/116128/#review117704
Comment on attachment 8842252 [details]
Bug 1343409 - HTMLMediaElement::MozRequestDebugInfo returns EMEInfo.

https://reviewboard.mozilla.org/r/116128/#review117706

::: dom/media/eme/MediaKeys.cpp:589
(Diff revisions 1 - 2)
>         !it.Done();
>         it.Next()) {
>      MediaKeySession* keySession = it.Data();
>      nsString sessionID;
>      keySession->GetSessionId(sessionID);
> -    sessionsInfo.AppendPrintf("(sid:%s ", sessionID);
> +    sessionsInfo.AppendLiteral("(sid:");

So now it's not consistent; you're using Append(nsString) in one place and AppendPrintf("%S", nsString.get()) in others.

I'd be happy for you to use one or the other, consistently.
Attachment #8842252 - Flags: review?(cpearce) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cd7f919c7583
HTMLMediaElement::MozRequestDebugInfo returns EMEInfo. r=cpearce
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cd7f919c7583
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.