Page MenuHomePhabricator

[native] "Flip the switch" to enable video messages
AbandonedPublic

Authored by atul on Sep 27 2022, 10:15 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 21, 11:57 AM
Unknown Object (File)
Sun, Apr 21, 11:57 AM
Unknown Object (File)
Sun, Apr 21, 11:57 AM
Unknown Object (File)
Sun, Apr 21, 11:57 AM
Unknown Object (File)
Sun, Apr 21, 11:57 AM
Unknown Object (File)
Sun, Apr 21, 11:53 AM
Unknown Object (File)
Fri, Apr 5, 10:12 AM
Unknown Object (File)
Mon, Mar 25, 5:30 PM
Subscribers
None

Details

Summary

Now that we have video messages working end-to-end, we can "flip the switch" to enable video messages.

Specifically:

  1. Set serverCanHandle to true for video types in lib/media/file-utils
  2. Set versionCode for hasMinCodeVersion check in multimedia-message-spec to 148
  3. Add MediaLibrary.MediaType.video to mediaType array in multimedia-gallery-keyboard.react so users can select videos from their media gallery

Depends on D5240

Test Plan

All the diffs that came before this one in the stack tested various part of the video message system.

Outside of that I've done a lot of testing on iOS/Android and different codeVersions to make sure that things work as expected... and I'm going to do a lot more before I land this diff.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

remove some extraneous comments

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 27 2022, 10:25 AM
Harbormaster failed remote builds in B12469: Diff 17116!
Harbormaster failed remote builds in B12470: Diff 17117!
atul requested review of this revision.Sep 27 2022, 12:55 PM
tomek added inline comments.
lib/shared/messages/multimedia-message-spec.js
207 ↗(On Diff #17117)

I'm not sure if this is a good idea. We should probably keep it high and then set the proper version just before the release. The reason is that we might want some more releases (e.g. with bugfixes) before we decide to make this feature live. So at this point it is hard to say if 148 will be the release that introduces this feature, or maybe some later would do that.

This revision is now accepted and ready to land.Sep 28 2022, 10:17 AM
lib/shared/messages/multimedia-message-spec.js
207 ↗(On Diff #17117)

Yeah you're right, I was going to change it before landing (was planning on landing right when I planned on making a release)... but you're right that I might forget update and other releases may be made.

Added a big bold notice at the top of the diff description as a reminder.

atul edited the summary of this revision. (Show Details)
lib/shared/messages/multimedia-message-spec.js
207 ↗(On Diff #17117)

I think that updating it now to something higher and then decreasing just before the release is less risky

lib/shared/messages/multimedia-message-spec.js
207 ↗(On Diff #17117)

I agree with @tomek, that's our standard approach in these cases

VERY exciting to see this diff, by the way!! This has been multiple years in the works

increase codeversion check to 999 for now

will change at time of video release

This revision was landed with ongoing or failed builds.Sep 29 2022, 9:44 AM
This revision was automatically updated to reflect the committed changes.

Unfortunately not quite ready... going to jot down some notes and put followups on Linear

This revision is now accepted and ready to land.Sep 29 2022, 1:41 PM

Have a diff in my current local video stack that handles all of this, so abandoning this out of convenience