Page MenuHomePhabricator

[lib] Stop shimming Blob-hosted multimedia messages
ClosedPublic

Authored by bartek on Thu, Nov 7, 3:37 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 21, 9:30 AM
Unknown Object (File)
Thu, Nov 21, 8:11 AM
Unknown Object (File)
Thu, Nov 21, 4:47 AM
Unknown Object (File)
Sat, Nov 16, 11:24 PM
Unknown Object (File)
Sat, Nov 16, 2:44 PM
Unknown Object (File)
Fri, Nov 15, 6:23 PM
Unknown Object (File)
Fri, Nov 15, 10:57 AM
Unknown Object (File)
Fri, Nov 15, 6:41 AM
Subscribers

Details

Summary

Points 3a and 3b from ENG-6459.

Test Plan

Replaced next code version constant with a past version and verified that robotext no longer appears and blob-hosted media messages are displayed.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.
bartek edited the test plan for this revision. (Show Details)
lib/shared/messages/multimedia-message-spec.js
256

We've never been shimming these for web. This code was written before we started versioning web

This revision is now accepted and ready to land.Thu, Nov 7, 5:29 AM

First, give this comment a read.

Basically my thinking is that if we want to have clients immediately start sending blob-hosted multimedia in thin threads, we should also stop shimming blob-hosted multimedia on the keyserver for recent codeVersions.

Rather than only unshimming for the most recent codeVersions, we should instead unshim for any codeVersion that can support blob-hosted multimedia.

This revision is now accepted and ready to land.Thu, Nov 7, 7:22 PM
lib/shared/messages/multimedia-message-spec.js
256 ↗(On Diff #45693)

Please replace NEXT_CODE_VERSION here with the earliest codeVersion that supports blob-hosted multimedia. (Would be helpful to explain your reasoning)

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

I think this would be 313: https://github.com/CommE2E/comm/compare/mobile-v1.0.311...mobile-v1.0.313

  • This release introduced passing CSAT when fetching media in the EncryptedImage component - stack of D10910 is included there
  • This release is older than 401 for which we bumped minimum supported version in D13790
lib/shared/messages/multimedia-message-spec.js
256 ↗(On Diff #45693)

Actually it should be 389 because since this release we enabled CSAT for everyone. Previous releases can have it missing: https://github.com/CommE2E/comm/tree/mobile-v1.0.389

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

Makes sense to me!

Use version 389 as minimal

Hmmmm this doesn't look right... the file on the left doesn't seem to be master

Sorry, you're absolutely right... that was my mistake, I mixed up FUTURE_CODE_VERSION and NEXT_CODE_VERSION