Page MenuHomePhabricator
Differential D13888

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

Authored by bartek on Nov 7 2024, 3:37 AM.
Tags
None
Referenced Files
F3522883: D13888.id45727.diff
Mon, Dec 23, 7:50 AM
F3522882: D13888.id45716.diff
Mon, Dec 23, 7:50 AM
F3522880: D13888.id45693.diff
Mon, Dec 23, 7:50 AM
F3522879: D13888.id45655.diff
Mon, Dec 23, 7:50 AM
F3522873: D13888.id.diff
Mon, Dec 23, 7:50 AM
F3522872: D13888.diff
Mon, Dec 23, 7:50 AM
Unknown Object (File)
Fri, Dec 20, 10:13 PM
Unknown Object (File)
Mon, Dec 16, 1:31 PM
View All 49 Files
Subscribers
tomek

Details

Reviewers
ashoat
Commits
rCOMM6ff0c6e70b0b: [lib] Stop shimming Blob-hosted multimedia messages
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.Nov 7 2024, 3:37 AM
bartek held this revision as a draft.
Herald added a subscriber: tomek. · View Herald TranscriptNov 7 2024, 3:37 AM
bartek added a child revision: D13889: [native][web] Add migration to unshim Blob-hosted multimedia.Nov 7 2024, 3:37 AM
bartek edited the summary of this revision. (Show Details)Nov 7 2024, 3:48 AM
bartek edited the test plan for this revision. (Show Details)
bartek added inline comments.Nov 7 2024, 3:50 AM
lib/shared/messages/multimedia-message-spec.js
256 ↗(On Diff #45655)

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.Nov 7 2024, 3:55 AM
bartek published this revision for review.Nov 7 2024, 4:50 AM
bartek mentioned this in D13889: [native][web] Add migration to unshim Blob-hosted multimedia.
ashoat accepted this revision.Nov 7 2024, 5:29 AM
This revision is now accepted and ready to land.Nov 7 2024, 5:29 AM
Closed by commit rCOMM0b058dcfae34: [lib] Stop shimming Blob-hosted multimedia messages (authored by bartek). · Explain WhyNov 7 2024, 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".Nov 7 2024, 7:13 PM
ashoat mentioned this in D13891: [native][web] Flip the switch for blob-hosted media/avatar uploads.Nov 7 2024, 7:19 PM
ashoat reopened this revision.EditedNov 7 2024, 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.Nov 7 2024, 7:22 PM
ashoat added inline comments.Nov 7 2024, 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.Nov 8 2024, 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.Nov 8 2024, 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

ashoat added inline comments.Nov 8 2024, 4:56 AM
lib/shared/messages/multimedia-message-spec.js
256 ↗(On Diff #45693)

Makes sense to me!

bartek updated this revision to Diff 45716.Nov 8 2024, 5:23 AM

Use version 389 as minimal

ashoat added a comment.Nov 8 2024, 5:26 AM

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

Harbormaster completed remote builds in B32590: Diff 45716.Nov 8 2024, 5:44 AM
bartek added a comment.Nov 8 2024, 6:20 AM
In D13888#388166, @ashoat wrote:

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

I rebased on latest master which is this commit: d8ed20cf4b9c1488f208825b7a636b3e5d882f6d

https://github.com/CommE2E/comm/blob/d8ed20cf4b9c1488f208825b7a636b3e5d882f6d/lib/shared/messages/multimedia-message-spec.js

ashoat added a comment.Nov 8 2024, 6:24 AM

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

Closed by commit rCOMM6ff0c6e70b0b: [lib] Stop shimming Blob-hosted multimedia messages (authored by bartek). · Explain WhyNov 8 2024, 10:23 AM
This revision was automatically updated to reflect the committed changes.
bartek added a commit: rCOMM6ff0c6e70b0b: [lib] Stop shimming Blob-hosted multimedia messages.

Revision Contents
Changeset List

PathSize
lib/
shared/
messages/
7 lines

Diff 45716

View Options

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

Loading...