Page MenuHomePhabricator
Differential D13888

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

Authored by bartek on Thu, Nov 7, 3:37 AM.
Tags
None
Referenced Files
F3355897: D13888.id45716.diff
Sat, Nov 23, 4:07 PM
F3355857: D13888.id45693.diff
Sat, Nov 23, 3:53 PM
F3355737: D13888.id45716.diff
Sat, Nov 23, 3:16 PM
F3351702: D13888.id45655.diff
Sat, Nov 23, 3:16 AM
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
View All 40 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.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

ashoat added inline comments.Fri, Nov 8, 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.Fri, Nov 8, 5:23 AM

Use version 389 as minimal

ashoat added a comment.Fri, Nov 8, 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.Fri, Nov 8, 5:44 AM
bartek added a comment.Fri, Nov 8, 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.Fri, Nov 8, 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 WhyFri, Nov 8, 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 45655

View Options

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

Loading...