Closed
Bug 879452
Opened 11 years ago
Closed 11 years ago
[MMS] Render multimedia as Attachments
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect, P1)
Tracking
(blocking-b2g:leo+, b2g18 fixed)
Tracking | Status | |
---|---|---|
b2g18 | --- | fixed |
People
(Reporter: jugglinmike, Assigned: jugglinmike)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
gnarf
:
review+
|
Details | Diff | Splinter Review |
Currently, received MMS messages are rendered with logic that is unique to the Thread view. Re-factor this code into the Attachment object so that it may be used during message composition. (This is the first step in enabling the sending of video and audio content.)
Updated•11 years ago
|
Severity: normal → blocker
blocking-b2g: --- → leo?
Priority: -- → P1
Target Milestone: --- → 1.1 QE3
Updated•11 years ago
|
blocking-b2g: leo? → leo+
Comment 2•11 years ago
|
||
Comment on attachment 759293 [details] [diff] [review] Render multimedia as Attachments Review of attachment 759293 [details] [diff] [review]: ----------------------------------------------------------------- Also, I'm not really sure why, but there is a scrollbar in the attachment placeholder for audio and video. Looking pretty good otherwise - appreciate the work! ::: apps/sms/index.html @@ +274,5 @@ > > + <!-- The image in this template ensures that the iFrame's 'load' event is > + not triggered until after the attachment media is loaded. This, in turn, > + prevents the Object URL from being revoked before the image has been drawn. > + --> I feel like as per our discussion about the setTimeout, this isn't needed? ::: apps/sms/js/activity_handler.js @@ +74,5 @@ > window.removeEventListener('hashchange', insertAttachments); > > blobs.forEach(function(blob, idx) { > + var attachment = new Attachment(blob, { > + name: names[idx] This should be isDraft: true yeah? it's inserting as an attachment into compose? ::: apps/sms/js/attachment.js @@ +59,5 @@ > event.target.click.bind(event.target)); > }, > render: function() { > var el = document.createElement('iframe'); > + var baseURL = location.protocol + "//" + location.host; lint: double quoted string here @@ +60,5 @@ > }, > render: function() { > var el = document.createElement('iframe'); > + var baseURL = location.protocol + "//" + location.host; > + var objectURL, inlineStyle; Seems possible for `inlineStyle` to still be undefined by the time we pass it to interpolate, perhaps we should set it to = '' to start with? ::: apps/sms/js/thread_ui.js @@ +835,3 @@ > var mediaElement, textElement; > > + if (messageData.name && messageData.blob) { As long as you are in here, I think we should get rid of the .name check here - I wrote this line and I think it is still causing some problems with iPhone attachments... We should have a default name of like "unknown" maybe? In case .name happens to be undefined? This isn't really necessary if you don't feel comfortable making that change here, but I think it should happen anyway.
Attachment #759293 -
Flags: review?(gnarf37) → review-
Assignee | ||
Comment 3•11 years ago
|
||
> ::: apps/sms/index.html > @@ +274,5 @@ > > > > + <!-- The image in this template ensures that the iFrame's 'load' event is > > + not triggered until after the attachment media is loaded. This, in turn, > > + prevents the Object URL from being revoked before the image has been drawn. > > + --> > > I feel like as per our discussion about the setTimeout, this isn't needed? Well, this pushes the uncertainty back to *just* the rendering of the `background-image`. This means the temporary hack doesn't have to abstract over the variability in image loading, which makes it somewhat safer. > @@ +60,5 @@ > > }, > > render: function() { > > var el = document.createElement('iframe'); > > + var baseURL = location.protocol + "//" + location.host; > > + var objectURL, inlineStyle; > > Seems possible for `inlineStyle` to still be undefined by the time we pass > it to interpolate, perhaps we should set it to = '' to start with? It is possible. I was leveraging the behavior of the `Utils.escapeHTML` method: https://github.com/mozilla-b2g/gaia/blob/6d90ff43a72bcc6563aa5578de5a37e219543c0f/apps/sms/js/utils.js#L79-L86 , but I can see how being explicit would be more clear here, so I'll update this. > This isn't really necessary if you don't feel comfortable making that change > here, but I think it should happen anyway. In my work-in-progress for Bug 877727, I have already been playing with the `name` parameter, so this change would probably fit better there.
Attachment #759293 -
Attachment is obsolete: true
Attachment #759368 -
Flags: review?(gnarf37)
Comment 4•11 years ago
|
||
Comment on attachment 759368 [details] [diff] [review] Render multimedia as Attachments v2 r=me
Attachment #759368 -
Flags: review?(gnarf37) → review+
Assignee | ||
Comment 5•11 years ago
|
||
Landed with commit 23eed04e7ce361887f1c219728a4854155c4aa60 Thanks, Corey!
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 6•11 years ago
|
||
I was not able to uplift this bug to v1-train. If this bug has dependencies which are not marked in this bug, please comment on this bug. If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval. Otherwise, if this is just a merge conflict, you might be able to resolve it with: git checkout v1-train git cherry-pick -x 23eed04e7ce361887f1c219728a4854155c4aa60 <RESOLVE MERGE CONFLICTS> git commit
Flags: needinfo?(mike)
Comment 7•11 years ago
|
||
uplifted master: 23eed04e7ce361887f1c219728a4854155c4aa60 to v1-train: c8856f7ce6b1b64229d1b1932e2ff6282b8ba9c6
You need to log in
before you can comment on or make changes to this bug.
Description
•