Closed Bug 1341452 Opened 3 years ago Closed 3 years ago

Make MediaResult::Description() more readable

Categories

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

53 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

Details

Attachments

(2 files, 1 obsolete file)

At the moment, MediaResult::Description() only outputs the error code in hex and the optional message for failures.

I'm thinking of the following improvements:
- Write the name associated with the error code, e.g.: "NS_ERROR_DOM_MEDIA_DEMUXER_ERR" instead of just "0x806e000c").
- Where possible, write a more precise function name, e.g.: "virtual RefPtr<MP4Demuxer::InitPromise> mozilla::MP4Demuxer::Init()" instead of just "Init()"
- Output description for non-failures as well. It should be up to the caller to decide whether they want successfull MediaResult descriptions as well.
- A bit of tidying up, e.g.: No need to show ': ' if there is no message that follows.

Jean-Yves, I'll just upload the patches for your review, you may then decide if any of these are not actually appropriate...
Comment on attachment 8839709 [details]
Bug 1341452 - Use MSC's __FUNCSIG__ or gcc/clang's __PRETTY_FUNCTION__ in MediaResult's RESULT_DETAIL -

https://reviewboard.mozilla.org/r/114282/#review115852

Needs to be validated by privacy folks
Attachment #8839709 - Flags: review?(jyavenard) → review+
Comment on attachment 8839711 [details]
Bug 1341452 - MediaResult::Description() also describes successes -

https://reviewboard.mozilla.org/r/114286/#review115854

This will break the new HTML5 message spec that message has to be empty when the MediaError object isnt set.

plus how could media result at this stsge ever have a message when mCode is NS_OK?
Attachment #8839711 - Flags: review?(jyavenard)
Comment on attachment 8839711 [details]
Bug 1341452 - MediaResult::Description() also describes successes -

https://reviewboard.mozilla.org/r/114286/#review115854

Not sure what this MediaError is, but shouldn't *it* (or the code constructing it) decide not to call MediaResult::Description() in this case?

NS_OK with message: `MediaResult(NS_OK, RESULT_DETAIL("Nicely done"));`
Probably useless, but not impossible! Once again, I would think it's up to the caller to decide what they want to be described or not!?

For my own usage, I was hoping to have success codes (apart from NS_OK) with messages, that I could use as warnings.

Anyway, in the end I can deal with all this myself elsewhere, if you're sure you don't want it exposed in MediaResult.
Comment on attachment 8839711 [details]
Bug 1341452 - MediaResult::Description() also describes successes -

https://reviewboard.mozilla.org/r/114286/#review115854

Alternatively, we could have two functions: `ErrorDescription()` which only describes errors like the current one, and `Description()` which describes everything as I suggested.
Comment on attachment 8839710 [details]
Bug 1341452 - Write nsresult name in MediaResult::Description() -

https://reviewboard.mozilla.org/r/114284/#review115922
Attachment #8839710 - Flags: review?(jyavenard) → review+
Comment on attachment 8839711 [details]
Bug 1341452 - MediaResult::Description() also describes successes -

https://reviewboard.mozilla.org/r/114286/#review115924
Comment on attachment 8839709 [details]
Bug 1341452 - Use MSC's __FUNCSIG__ or gcc/clang's __PRETTY_FUNCTION__ in MediaResult's RESULT_DETAIL -

https://reviewboard.mozilla.org/r/114282/#review115852

Thank you for the tip. I've discussed with pauljt and mt, and they're both fine with it as there is no fingerprinting data.
Attachment #8839711 - Attachment is obsolete: true
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e14df5457369
Use MSC's __FUNCSIG__ or gcc/clang's __PRETTY_FUNCTION__ in MediaResult's RESULT_DETAIL - r=jya
https://hg.mozilla.org/integration/autoland/rev/76b0a067ca51
Write nsresult name in MediaResult::Description() - r=jya
https://hg.mozilla.org/mozilla-central/rev/e14df5457369
https://hg.mozilla.org/mozilla-central/rev/76b0a067ca51
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.