879452 - [MMS] Render multimedia as Attachments

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

Description

[Mike Pennisi [:jugglinmike]]

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.)

Comment 1

[Mike Pennisi [:jugglinmike]]

Attachment: Render multimedia as Attachments

Comment 2

[Corey Frang [:gnarf]]

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.

Comment 3

[Mike Pennisi [:jugglinmike]]

Attachment: Render multimedia as Attachments v2

> ::: 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.

Comment 4

[Corey Frang [:gnarf]]

Comment on attachment 759368 [details] [diff] [review]
Render multimedia as Attachments v2

r=me

Comment 5

[Mike Pennisi [:jugglinmike]]

Landed with commit 23eed04e7ce361887f1c219728a4854155c4aa60

Thanks, Corey!

Comment 6

[John Ford [:jhford] CET/CEST Berlin Time]

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

Comment 7

[Corey Frang [:gnarf]]

uplifted master: 23eed04e7ce361887f1c219728a4854155c4aa60
to v1-train: c8856f7ce6b1b64229d1b1932e2ff6282b8ba9c6

Comment 8

[Mike Pennisi [:jugglinmike]]

Thanks for resolving the conflict, Corey!