[native] Add encrypted source support in <Multimedia>
ClosedPublic
Actions

Authored by bartek on Mar 28 2023, 11:28 AM.

Details

Reviewers

ashoat
atul
marcin

Commits

rCOMM7b2032895388: [native] Add encrypted source support in <Multimedia>

Summary

Until now, the <Miltimedia> component operated on URIs but now encrypted media don't operate on URIs (although for keyserver uploads, the holder is a URI on which we rely here).
Replaced URIs with a Source objects which can be either a URI or encrypted media source.

The component uses the departing URI concept to indicate whether a temporary local file can be disposed. In theory, encrypted media don't have to be marked as departing, but I did it anyway for consistency (this should have no negative impact).

Added support for rendering encrypted source by using the <EncryptedImage> component added in D7225 is used.

Depends on D7225
Depends on D7168

Test Plan

Added a fake mediaInfo prop containing encrypted image URL and ensured that the component displays it.
Dynamically replaced a source with and ensured state updates. Departing URI changes can be checked in input-state-container (inspecting the activeURIs variable).

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bartek created this revision.Mar 28 2023, 11:28 AM

bartek held this revision as a draft.

Herald added a subscriber: tomek. · View Herald TranscriptMar 28 2023, 11:28 AM

bartek added a child revision: D7228: [native] Fixes for handling encrypted media messages.Mar 28 2023, 11:32 AM

bartek edited the summary of this revision. (Show Details)Mar 28 2023, 11:34 AM

bartek added a parent revision: D7168: [lib] Introduce encrypted media types.

Harbormaster completed remote builds in B17761: Diff 24305.Mar 28 2023, 11:44 AM

bartek published this revision for review.Mar 28 2023, 12:07 PM

Minor comments

native/media/multimedia.react.js
13–22 ↗	(On Diff #24305)	Given this is React state, it should definitely be read-only. It should not be possible to do eg. `this.state.source.holder = 'blah'` In general it's better to "default" to read-only types, and only make it writeable if you need to / you're sure it makes sense (eg. in the case of returning a new object from some function, it's fine to make it writeable)
69 ↗	(On Diff #24305)	This could probably just be a deep equality comparison (`_isEqual`). I know technically you are considering eg. a video with thumbnail X to be the same as the image X, but I don't think it's possible for X to be the same in those cases anyways If we do want to keep this for whatever reason, I don't think it makes sense to redefine the function on every execution of `componentDidUpdate`, eg. it can be defined at the top-level scope or as a static method
159 ↗	(On Diff #24305)	Are you sure we need to call `reportURIDisplayed` for holders? This function is used here and here to dispose of temporary files after they're no longer being displayed in the app, but I'm not sure if those codepaths are relevant for encrypted media

This revision now requires changes to proceed.Mar 28 2023, 12:59 PM

Made type read-only
Replaced custom compare fn with _isEqual
Stopped reporting holders as URIs

Harbormaster completed remote builds in B17772: Diff 24319.Mar 29 2023, 2:41 AM

bartek added inline comments.Mar 29 2023, 5:14 AM

native/media/multimedia.react.js
13–22 ↗	(On Diff #24305)	Yeah, thanks for the tip. I always keep forgetting about `$ReadOnly` (I came from the TypeScript world where there's no such practice)
69 ↗	(On Diff #24305)	Lodash `_isEqual` is what I was looking for
159 ↗	(On Diff #24305)	I mentioned it in the diff description. Basically, these holders will be only stored in that `activeURIs` map but will never be accessed from the places you linked (somebody would have to put holder in place of uri here). Anyway, I can skip reporting holders to avoid further confusion.