Closed Bug 998063 Opened 6 years ago Closed 6 years ago

Feed parser should support media:thumbnail

Categories

(Toolkit :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: wesj, Assigned: wesj)

References

Details

Attachments

(1 file, 1 obsolete file)

The new Firefox Hub addons for Android are using the Feed Parser for RSS feeds. None of the feeds I've seen show thumbnails, but a little digging showed (some) feeds using the media:thumbnail markup. We should support it!
Attached patch Patch (obsolete) — Splinter Review
Tests coming...
Attachment #8408589 - Flags: review?(mak77)
Attached patch PatchSplinter Review
Didn't realize the tests here basically just made sure we could parse some files. Added a media:thumbnail bit to them. Removed some sneaky logging as well :)
Attachment #8408589 - Attachment is obsolete: true
Attachment #8408589 - Flags: review?(mak77)
Attachment #8408630 - Flags: review?(mak77)
Comment on attachment 8408630 [details] [diff] [review]
Patch

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

this code hurts my eyes every time :)

::: toolkit/components/feeds/FeedProcessor.js
@@ +564,5 @@
>        if (bagHasKey(contentElement, "type")) {
>          enc.setPropertyAsAString("type", contentElement.getPropertyAsAString("type"));
> +      } else if (mediaType == "mediathumbnail") {
> +        // thumbnails won't have a type, but default to image types
> +        enc.setPropertyAsAString("type", "image/*");

How do we distinguish this thumbnail from a media:content having type="image/*"? Maybe there should be an additional "thumbnail" boolean property in case the consumer wants to distinguish them?

::: toolkit/components/feeds/test/xml/rss2/mrss_content.xml
@@ +9,5 @@
>  <channel>
>  <item>
>  
> +<media:content fileSize="24986239" type="video/mpeg" url="http://dallas.example.com/joebob_050689.mpeg" />
> +<media:thumbnail url="http://dallas.example.com/joebob_050689.mpeg"  width="75" height="50" time="12:05:01.123"/>

from what I see, thumbnail should always be an image. Doesn't make a difference here, but for coherence should probably be .jpg

Also, there's 2 spaces before width.
Attachment #8408630 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/253acf385229
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1048140
Depends on: 1046500
You need to log in before you can comment on or make changes to this bug.