Page MenuHomePhabricator
Differential D13888

[lib] Stop shimming Blob-hosted multimedia messages
AcceptedPublic
Actions

Authored by bartek on Thu, Nov 7, 3:37 AM.
Tags
None
Referenced Files
F3178023: D13888.diff
Fri, Nov 8, 12:43 AM
F3177994: D13888.id45693.diff
Fri, Nov 8, 12:37 AM
F3169458: D13888.id45655.diff
Thu, Nov 7, 11:30 AM
F3168660: D13888.id.diff
Thu, Nov 7, 7:22 AM
F3167973: D13888.diff
Thu, Nov 7, 4:53 AM
F3167964: D13888.diff
Thu, Nov 7, 4:51 AM
F3167332: D13888.id45655.diff
Thu, Nov 7, 3:51 AM
F3167326: D13888.id.diff
Thu, Nov 7, 3:51 AM
View All 9 Files
Subscribers
tomek

Details

Reviewers
ashoat
Commits
rCOMM0b058dcfae34: [lib] Stop shimming Blob-hosted multimedia messages
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 created this revision.Thu, Nov 7, 3:37 AM
bartek held this revision as a draft.
Herald added a subscriber: tomek. · View Herald TranscriptThu, Nov 7, 3:37 AM
bartek added a child revision: D13889: [native][web] Add migration to unshim Blob-hosted multimedia.Thu, Nov 7, 3:37 AM
bartek edited the summary of this revision. (Show Details)Thu, Nov 7, 3:48 AM
bartek edited the test plan for this revision. (Show Details)
bartek added inline comments.Thu, Nov 7, 3:50 AM
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

Harbormaster completed remote builds in B32568: Diff 45655.Thu, Nov 7, 3:55 AM
bartek published this revision for review.Thu, Nov 7, 4:50 AM
bartek mentioned this in D13889: [native][web] Add migration to unshim Blob-hosted multimedia.
ashoat accepted this revision.Thu, Nov 7, 5:29 AM
This revision is now accepted and ready to land.Thu, Nov 7, 5:29 AM
Closed by commit rCOMM0b058dcfae34: [lib] Stop shimming Blob-hosted multimedia messages (authored by bartek). · Explain WhyThu, Nov 7, 8:25 AM
This revision was automatically updated to reflect the committed changes.
bartek added a commit: rCOMM0b058dcfae34: [lib] Stop shimming Blob-hosted multimedia messages.
ashoat added a reverting change: rCOMMf73497ae9535: Revert "[lib] Stop shimming Blob-hosted multimedia messages".Thu, Nov 7, 7:13 PM
ashoat mentioned this in D13891: [native][web] Flip the switch for blob-hosted media/avatar uploads.Thu, Nov 7, 7:19 PM
ashoat reopened this revision.EditedThu, Nov 7, 7:22 PM

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
ashoat added inline comments.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)

bartek added inline comments.Fri, Nov 8, 1:09 AM
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
bartek added inline comments.Fri, Nov 8, 1:42 AM
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

Revision Contents
Changeset List

PathSize
lib/
shared/
messages/
7 lines

Diff 45655

View Options

lib/shared/messages/multimedia-message-spec.js

Loading...