Details

Reviewers

atul
marcin
ginsu
rohan

Commits

rCOMM6a00d81adcd8: [web][native] Support fetching Blob service URIs

Summary

This diff adds support for fetching Blob service URIs for the native and web.

Depends on D7616

Test Plan

Uploaded both encrypted and non-encrypted blob to local blob service instance.
Amended the keyserver's upload-fetchers to return these Blob URIs.
Verified media is fetched and displayed correctly both on web and native.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bartek created this revision.Apr 25 2023, 11:10 AM

bartek held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptApr 25 2023, 11:10 AM

bartek added a child revision: D7618: [keyserver] Support external blobs in uploads table.Apr 25 2023, 11:18 AM

Harbormaster completed remote builds in B18751: Diff 25691.Apr 25 2023, 11:29 AM

Fix missing diff after rebasing

bartek published this revision for review.Apr 25 2023, 11:44 AM

bartek added inline comments.

web/media/media-utils.js
206–207 ↗	(On Diff #25695)
web/media/multimedia-modal.react.js
46–52 ↗	(On Diff #25695)	This adds support for hosting non-encrypted media on Blob service. I plan to use Blob service for uploading encrypted media only, but it won't hurt to support this here just in case. This is also compatible with my changes from D7618 where, in theory, blob URI can be returned regardless of media type.

Harbormaster completed remote builds in B18755: Diff 25695.Apr 25 2023, 11:58 AM

bartek mentioned this in D7616: [lib] Shim Blob service hosted multimedia.Apr 25 2023, 9:25 PM

Rebase

Harbormaster completed remote builds in B18837: Diff 25806.Apr 27 2023, 4:04 AM

Add missing error check

Harbormaster completed remote builds in B18934: Diff 25933.Apr 29 2023, 9:05 AM

Uploaded both encrypted and non-encrypted blob to local blob service instance. Amended the keyserver's upload-fetchers to return these Blob URIs. Verified media is fetched and displayed correctly both on web and native.

Would there be a good way for a reviewer to test this in their local environment?

native/media/encryption-utils.js
272 ↗	(On Diff #25933)	We often use snake_cased error messages EG and can handle them conditionally in any outer catch blocks EG would it make sense to do something similar here? Maybe throw different errors based on `status`/`statusText`? Will leave it up to you since you have more context on what type of errors we might encounter, etc.
web/media/media-utils.js
202–205 ↗	(On Diff #25933)	Looks like we have similar logic in `identifier-utils`. Up to you, but might make sense to consolidate at some point in the future if we re-use further?
web/media/multimedia-modal.react.js
49 ↗	(On Diff #25933)	Could we move this above `media.type === 'photo'` check?

This revision is now accepted and ready to land.May 3 2023, 9:26 AM

atul attached a referenced file: F520940: db863b.png. (Show Details)May 3 2023, 9:26 AM

atul attached a referenced file: F520938: 980e6b.png. (Show Details)

atul mentioned this in D7649: [native] Add function to upload media to blob service.May 3 2023, 10:57 AM

bartek added inline comments.May 7 2023, 10:10 PM

native/media/encryption-utils.js
272 ↗	(On Diff #25933)	This is a good idea, worth thinking of more thoroughly. I'll make a Linear task for this
web/media/media-utils.js
202–205 ↗	(On Diff #25933)	Yeah, good idea, not sure why I didn't do it in the first place
web/media/multimedia-modal.react.js
49 ↗	(On Diff #25933)	Already tried that, Flow complains