Last Comment Bug 873872 - Feed messages with image attachments and Display Inline set should show the remote image
: Feed messages with image attachments and Display Inline set should show the r...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Message Reader UI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 24.0
Assigned To: alta88
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-19 13:23 PDT by alta88
Modified: 2013-06-25 05:18 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (1.69 KB, patch)
2013-05-19 13:27 PDT, alta88
no flags Details | Diff | Review
updated (2.22 KB, patch)
2013-05-20 09:20 PDT, alta88
no flags Details | Diff | Review
shrinktofit update (2.46 KB, patch)
2013-05-21 08:53 PDT, alta88
mkmelin+mozilla: review+
Details | Diff | Review
for checkin (2.42 KB, patch)
2013-05-23 07:35 PDT, alta88
alta88: review+
Details | Diff | Review

Description alta88 2013-05-19 13:23:03 PDT
The mime parser creates an <img> element with a src of a mailbox:// url, which doesn't apply as feed attachments are always remote links and there's nothing valid in that there part to stream.  The src should be set with the link url.
Comment 1 alta88 2013-05-19 13:27:21 PDT
Created attachment 751503 [details] [diff] [review]
patch
Comment 2 alta88 2013-05-19 13:31:25 PDT
Also, I had expected Filelink image attachments to also display inline.  Is there a reason they don't (unless it's a business agreement reason).  It would be easy enough to extract a X-Mozilla-Cloud-Part url and do the same..
Comment 3 alta88 2013-05-20 09:20:28 PDT
Created attachment 751722 [details] [diff] [review]
updated


robustness tweak for GlodaUtils.PART_RE regex failing to take into account a ? in a url.
Comment 4 Magnus Melin 2013-05-20 12:24:56 PDT
Do you have a sample feed url?
Comment 5 alta88 2013-05-20 13:46:15 PDT
Since file:// urls can't be loaded in a messagepane doc, I use these; a recent guardian item had 19 media (.jpg) attachments:

http://feeds.guardian.co.uk/theguardian/sport/rss
http://news.nationalgeographic.com/index.rss
Comment 6 alta88 2013-05-21 08:53:11 PDT
Created attachment 752220 [details] [diff] [review]
shrinktofit update


tweak to ensure shrink to fit works nicely.
Comment 7 Magnus Melin 2013-05-23 03:21:21 PDT
Comment on attachment 752220 [details] [diff] [review]
shrinktofit update

Review of attachment 752220 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me. r=mkmelin

::: mail/base/content/msgHdrViewOverlay.js
@@ +684,5 @@
>        if (!currentAttachments.length && this.mSaveHdr)
>          this.mSaveHdr.markHasAttachments(false);
> +
> +      let browser = getBrowser();
> +      let inlineAttachments = Services.prefs.getBoolPref("mail.inline_attachments");

you could just inline inlineAttachments since it's used only once

@@ +698,5 @@
> +              break;
> +            }
> +            else {
> +              // GlodaUtils.PART_RE fails to extract a partID for urls with
> +              // an embedded ?, at least.  So, check the filename too.  Frontend

please don't double space for new sentences
Comment 8 alta88 2013-05-23 07:35:24 PDT
Created attachment 753283 [details] [diff] [review]
for checkin


updated for comments.
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-05-28 10:17:21 PDT
https://hg.mozilla.org/comm-central/rev/5b57f54e2cb0

Note You need to log in before you can comment on or make changes to this bug.