Add support for media feeds and objects (enclosures)

RESOLVED FIXED

Status

()

Firefox
RSS Discovery and Preview
--
enhancement
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: gkn, Assigned: Will Guaraldi)

Tracking

Trunk
Points:
---
Bug Flags:
blocking-firefox3 -
wanted-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 18 obsolete attachments)

73.50 KB, image/png
beltzner
: ui-review+
Details
1.54 KB, patch
Will Guaraldi
: ui-review+
beltzner
: approval1.9+
Details | Diff | Splinter Review
48.06 KB, patch
Will Guaraldi
: review+
beltzner
: approval1.9+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
If a feed includes enclosures, Feedview should provide a link to download the
enclosed file.
Undeniably useful for Marketing Buzzword Bingo, but are there significant
numbers of reasonable use-cases, where it would somehow be better to go to
http://www.mozilla.org/reorganization/podcast.xml and click on an XML attribute
transformed into an HTML link, rather than going to
http://www.mozilla.org/reorganization/ and clicking a link directly?

Easy enough to do, but since the whole point of podcasting is to automatically
download stuff to a portable player while you aren't paying attention, it's hard
to see it fitting in to feedview.

Updated

12 years ago
Component: General → RSS Discovery and Preview
No longer blocks: 302121
QA Contact: general → rss.preview
Whiteboard: [wontfix?]
Summary: Feedview should show enclosures (podcasts) → Feed preview should show enclosures (podcasts)
Whiteboard: [wontfix?]
mconnor-via-irc has a point, that if the purpose of feed preview is to show you what's in the feed, then enclosures are one of those things, and if you want to know whether they are in a format you can handle, you need a link that will let you find out, so I'm taking back my "[wontfix?]" and upping it from enhancement to bug.
Severity: enhancement → normal
*** Bug 360143 has been marked as a duplicate of this bug. ***

Comment 4

11 years ago
This should cover all the various enclosure-like things, like Media RSS and the like.
Severity: normal → enhancement
Summary: Feed preview should show enclosures (podcasts) → Add support for media feeds and objects
Target Milestone: --- → Firefox 3 alpha1
Duplicate of this bug: 366140

Comment 6

11 years ago
A lightly edited transcript of an IRC discussion with Robert about what we need to do here:

[2:25pm] dmose: sayrer: at the most basic level, we just need to be able to allow the user to choose an app to display with
[2:26pm] sayrer: right, but we want to let them go automatic for "normal" feeds
[2:26pm] dmose: sayrer: ideally we might figure out a good preview story
[2:27pm] sayrer: for the preview story, just showing a link with some indication of what it is does the trick for lots of readers
[2:28pm] sayrer: for normal vs. media feed detection, you're looking for title+link+description vs. title+link+description+enclosure
[2:29pm] sayrer: enclosure being the thing with the type/url/etc of the media file
[2:29pm] sayrer: for podcasts, that's the mp3/wmv/etc
[2:30pm] sayrer: but this being syndication, there are a brazillion synonyms for that element, no clear indication of which mime types mean what, etc
[2:31pm] sayrer: the synonyms I can take care of
[2:31pm] dmose: so what's your opinion on the best strategy here?
[2:31pm] sayrer: best strategy is to make a list of types that correspond to "audio" "video" "photo", etc
[2:31pm] sayrer: allow some way to override our detection
[2:32pm] sayrer: pay close attention to IE7 / Safari
[2:32pm] sayrer: and prepare for arguments that we're "violating standards"
[2:33pm] sayrer: oh, Apple made up a new top level mime type for their photocasts

Updated

11 years ago
Keywords: uiwanted

Updated

10 years ago
Flags: blocking-firefox3?
Target Milestone: Firefox 3 alpha1 → ---
This doesn't feel like a blocker until we have a really compelling solution presented, but adding it to the wanted list.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
I don't have a lot of mockups up just yet, but I am doing the UI design for this as part of my work on content handling:

http://wiki.mozilla.org/ContentHandling:User_Interface

For instance, apps like Democracy and iTunes get promoted in the list of feed readers when the feed contains videos:

http://wiki.mozilla.org/ContentHandling:User_Interface/Proposed_UI2
Here is a proposal for changes to preferences to handle feeds with different types of content:

http://wiki.mozilla.org/ContentHandling:User_Interface/Preferences_Feeds

Updated

10 years ago
Duplicate of this bug: 385331
Summary: Add support for media feeds and objects → Add support for media feeds and objects (enclosures)

Comment 11

10 years ago
Any chance of getting a copy of FeedProcessor.js that populates the enclosures attribute?

Comment 12

10 years ago
I have to say that many websites do an incredibly poor job of indicating which RSS links are podcasts, and which are not. It is sure getting old having to click on the feed link every time, view source, and search for "<enclosure" every time I am uncertain if this is the podcast or just the news feed.
Duplicate of this bug: 400062
Blocks: 400059
(Assignee)

Comment 14

10 years ago
I'm interested in doing the work for fixing FeedProcessor.js to populate the enclosures attribute with an array of enclosures.

Any thoughts on what should be in the enclosure array?  Should I create a new idl for enclosures, an Enclosure object and populate the array with enclosure instances.
(Assignee)

Comment 15

10 years ago
I'm still interested in doing the work, but I want to make sure that what I'm thinking of doing aligns with how you guys think it should be done.
dmose: are you the best person to ask for the question in comment #14?
faaborg: sayrer wrote all the code, so his advice is likely to be much more useful.
(Assignee)

Comment 18

10 years ago
It look like each item has an nsIArray of nsIWritablePropertyBag2 things each one representing an enclosure already.  I'm going to take that and do a pass in normalize to build the enclosures array based on the enclosures in the items.

nsIFeedEntry has "enclosures" as well as "mediaContent".  Is "mediaContent" supposed to be a subset of "enclosures"--things that can be displayed in the feed preview?

Comment 19

10 years ago
Will, the nsIArray of enclosures never gets populated. Plus there are other bugs related to using parseAsync. Robert Sayre knows about the parseAsync bugs.

Also, I *suspect* there is another bug. i.e. Feed items without a pubDate. I'm not 100% sure about this, but items without a pubDate seem to lock up the entire parsing process for quite a long time.
(Assignee)

Comment 20

10 years ago
Created attachment 291237 [details] [diff] [review]
two-line patch that fixes the code to populate the nsIFeedEntry enclosures array

Here's the two line patch that populates the enclosures array with nsIWritablePropertyBag2 objects representing the enclosures.  This picks up RSS2 enclosures--I haven't looked at Atom, MRSS, or other enclosure support yet.
Will, if you can create a unified diff of this (I usually use "cvs diff -up8N"), and request review from sayrer, we may be able to get the above patch landed after the tree reopens for beta3.
(Assignee)

Comment 22

10 years ago
I want to add support for Atom and MRSS before anything gets committed since they're less trivial.  When I get that working, I'll obsolete the first patch with the new one.

I wanted to submit the trivial patch to get RSS2 enclosures working so that other people could tag along if they wanted to.  I'll use the -up8N flags next time I do a diff.  Thanks for the heads-up.
(Assignee)

Comment 23

10 years ago
Created attachment 292544 [details] [diff] [review]
Populates enclosures array and shows enclosures in subscribe feed preview

In FeedProcessor.js, this adds RSS 2, Atom 1 and Yahoo MRSS enclosure support.  It populates the enclosures array in the FeedEntry items with nsIWritablePropertyBag2 items.

In FeedWriter.js, this shows the enclosures in the feed subscription preview page.  It's not pretty and I wasn't wholly sure what kinds of information would be interesting to show, but the information is in the right place now.

It's my first Mozilla patch--suggestions are more than welcome.
Attachment #291237 - Attachment is obsolete: true

Comment 24

10 years ago
Hope this feature makes it into Firefox 3 at some point.  I guess I'm one of those people that don't subscribe to podcasts... I just like to go to the site and find a way to download it.  Having the links in the rendered view of RSS is handy, especially for sites that are badly designed. IE does this now.

Thanks,
Bruno

Comment on attachment 292544 [details] [diff] [review]
Populates enclosures array and shows enclosures in subscribe feed preview

Mano: can you take a look at this? The feature's a really-really nice to have for Firefox 3.
Attachment #292544 - Flags: review?(mano)

Comment 26

10 years ago
Comment on attachment 292544 [details] [diff] [review]
Populates enclosures array and shows enclosures in subscribe feed preview

This patch looks OK in principle, but we don't take FeedProcessor.js changes without unit tests.
(Assignee)

Comment 27

10 years ago
I asked a couple of times on the #developers channel about how to run the tests in the tests/ directory and didn't get any answers, so I didn't do anything with that.  If you can point me to how to run those tests, then I'll run the existing ones and add tests for enclosures, too.

Comment 28

10 years ago
(In reply to comment #27)
> I asked a couple of times on the #developers channel about how to run the tests
> in the tests/ directory and didn't get any answers, so I didn't do anything
> with that.  If you can point me to how to run those tests, then I'll run the
> existing ones and add tests for enclosures, too.

Will found me on IRC and we sorted this out.
(Assignee)

Comment 29

10 years ago
Created attachment 292913 [details] [diff] [review]
populates enclosures, shows enclosures in feed preview page, adds new tests

I adjusted the test for item_enclosure.xml because I changed the RSS 2.0 enclosure item in IN_ITEMS so it's not an array (RSS 2.0 can only have one enclosure to an item).  I added a bunch of new tests for the additional tags supported with tests to make sure we're populating the enclosures array on the FeedEntry with the correct data.

I also rewrote bits of the patch so that it's more straight-forward and futzes less with the data from the parser.

I had to do some CVS voodoo to get the new files in the patch easily....  If that doesn't work, let me know what I should do instead.
Attachment #292544 - Attachment is obsolete: true
Attachment #292544 - Flags: review?(mano)
(Assignee)

Comment 30

10 years ago
Created attachment 293566 [details] [diff] [review]
similar to enclosures_in_preview2 except with some minor adjustments and css

This is the same patch as enclosures_in_previe2, except that it has some minor fixes (file sizes are rounded more cleanly) and it adds a .enclosures class to the subscribe.css files in themes.

This is ready for review.

Any suggestions/comments/feedback are more than welcome.
Attachment #292913 - Attachment is obsolete: true
(Assignee)

Comment 31

10 years ago
Any status on this?  That last attachment/patch completes this issue.
Comment on attachment 293566 [details] [diff] [review]
similar to enclosures_in_preview2 except with some minor adjustments and css

Need to request from a specific person; sayrer is probably the most appropriate.
Attachment #293566 - Flags: review?(sayrer)
(Assignee)

Comment 33

10 years ago
Created attachment 296979 [details] [diff] [review]
Previous patch plus it figures out Feed.enclosureCount and Feed.type.

I added code to figure out Feed.enclosureCount and also Feed.type.  The intuition behind Feed.type is this:

1. feed has no enclosures -> TYPE_FEED
2. feed has enclosures exclusively video -> TYPE_VIDEO
3. feed has enclosures exclusively audio -> TYPE_AUDIO
4. feed has enclosures exclusively image -> TYPE_IMAGE
5. everything else (including combinations of types) -> TYPE_FEED

I also added a few more tests.
Attachment #293566 - Attachment is obsolete: true
Attachment #296979 - Flags: review+
Attachment #293566 - Flags: review?(sayrer)
(Assignee)

Comment 34

10 years ago
Comment on attachment 296979 [details] [diff] [review]
Previous patch plus it figures out Feed.enclosureCount and Feed.type.

Fixed the review flag--I'm slowly getting the hang of how you guyst do things.
Attachment #296979 - Flags: review+ → review?(sayrer)

Comment 35

10 years ago
Comment on attachment 296979 [details] [diff] [review]
Previous patch plus it figures out Feed.enclosureCount and Feed.type.

Please break out the enclosure code in FeedWriter to a separate function, and make things like "onemb" consts. Sorry for the delay on this, and nice tests.
Attachment #296979 - Flags: review?(sayrer) → review+
Assignee: nobody → will.guaraldi
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
(Assignee)

Comment 36

10 years ago
Created attachment 297246 [details] [diff] [review]
Everything in the last patch with fixes suggested by sayrer.

I made the changes requested.
Attachment #296979 - Attachment is obsolete: true
Attachment #297246 - Flags: review?(sayrer)

Updated

10 years ago
Attachment #297246 - Flags: review?(sayrer)
Attachment #297246 - Flags: review+
Attachment #297246 - Flags: approval1.9?
Comment on attachment 297246 [details] [diff] [review]
Everything in the last patch with fixes suggested by sayrer.

>Index: browser/components/feeds/src/FeedWriter.js

>+const ONE_GB = ONE_GB * 1024;

huh. I guess you wanted ONE_MB here.

>+    var roundme = function(n) {
>+      return Math.round(n * 100) / 100;
>+    }
>+

I think you want to call .toLocaleString() here.

>+        enclosureDiv.appendChild(this._document.createTextNode(" type: " + enc.get("type")));

>+          enc_size = "" + enc_size;
>+        } else if (enc_size < ONE_MB) {
>+          enc_size = "" + roundme(enc_size / ONE_KB) + " KB";
>+        } else if (enc_size < ONE_GB) {
>+          enc_size = "" + roundme(enc_size / ONE_MB) + " MB";
>+        } else {
>+          enc_size = "" + roundme(enc_size / ONE_GB) + " GB";
>+        }
>+        enclosureDiv.appendChild(this._document.createTextNode(" size: " + enc_size));
>+      }

If this is visible in the UI, you probably want localized strings. You can use getFormattedString.
What's the point of the empty strings here? Is it to make sure enc_size will be considered as a string?

>Index: toolkit/components/feeds/src/FeedProcessor.js

>+    for (var i=0; i < this.items.length; i++) {

Code style nit: a space before and after '=', like in the other for loops of the file.
You may also want to replace i++ by ++i.

>+              audio_count += 1;

I think ++audio_count would be more readable.

Also, as this touches the UI, you probably need ui-review too.

Comment 38

10 years ago
Can we get a quick risk/reward assessment for this? 

Comment 39

10 years ago
Comment on attachment 297246 [details] [diff] [review]
Everything in the last patch with fixes suggested by sayrer.

>
>+
>+      var enc_href = this._document.createElementNS(HTML_NS, "a");
>+      enc_href.appendChild(this._document.createTextNode(enc.get("url")));
>+      enc_href.setAttribute("href", enc.get("url"));

philor points out that this needs to be checked for safety like the other URLs in this file.
Attachment #297246 - Flags: review-
Attachment #297246 - Flags: review+
Attachment #297246 - Flags: approval1.9?
(Assignee)

Comment 40

10 years ago
Created attachment 297306 [details] [diff] [review]
Previous patch with fixes for comments 37 and 39.

Sorry about the ONE_GB thing--I should have caught that.

This patch factors in all the comments so far.

Many thanks for the work you all are putting into getting this moving along.
Attachment #297246 - Attachment is obsolete: true
(Assignee)

Comment 41

10 years ago
Comment on attachment 297306 [details] [diff] [review]
Previous patch with fixes for comments 37 and 39.

I'm flagging this for sayrer to review again, but I'm not sure if that's what I need to do to move it along.
Attachment #297306 - Flags: review?(sayrer)
Comment on attachment 297306 [details] [diff] [review]
Previous patch with fixes for comments 37 and 39.

>Index: browser/components/feeds/src/FeedWriter.js

>+          enc_size = enc_size + " B";
>+        } else if (enc_size < ONE_MB) {
>+          enc_size = roundme(enc_size / ONE_KB) + " KB";
>+        } else if (enc_size < ONE_GB) {
>+          enc_size = roundme(enc_size / ONE_MB) + " MB";
>+        } else {
>+          enc_size = roundme(enc_size / ONE_GB) + " GB";
>+        }

These are formatted strings too: B, KB, MB and GB should be localizable.

>Index: toolkit/components/feeds/src/FeedProcessor.js

>+        for (var e=0; e < entry.enclosures.length; e++) {

>+            other_count += 1;

While doing the code style change, you missed one of the for loops and one of the += 1.
(In reply to comment #42)
> (From update of attachment 297306 [details] [diff] [review])
> >Index: browser/components/feeds/src/FeedWriter.js
...
> These are formatted strings too: B, KB, MB and GB should be localizable.

Is it possible to reuse the size translations in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties for this purpose?
(Assignee)

Comment 44

10 years ago
Created attachment 297488 [details] [diff] [review]
Previous patch plus fixes for comment 42 using ideas in comment 43.

I tried to be really vigilant about localization, coding style and typos this time around.

I did use bytes, kilobytes, megabytes, and gigabytes strings from downloads.properties--I created a new _getStringFromDownloads function for that.
Attachment #297306 - Attachment is obsolete: true
Attachment #297488 - Flags: review?(sayrer)
Attachment #297306 - Flags: review?(sayrer)
Sorry, I meant to comment about string reuse last night, and got distracted. That's pretty much never the right thing to do. Not only do you want to give localizers a fighting chance to see where the strings they are translating live, but in this case the width of the download manager is localizable, so a particular language without a common abbreviation for GB might use a whole long word, make their default width much wider for the download manager, and have no idea that by doing so they are completely ruining the layout of feed previews with enclosures until after it's too late to do anything about it.
>Can we get a quick risk/reward assessment for this? 

Someone else can cover risk, but I'll address rewards.  This patch aims to bring audio and video podcasting to main stream audiences in the same way that our initial RSS support helped to drive adoption of feed readers for textual news.

This post covers how this patch is part of a larger effort to have improved content handling: http://blog.mozilla.com/faaborg/2007/10/08/firefox-and-miro/

Creating an easy to use handoff between Firefox and applications like Miro is becoming increasingly important as video and audio delivered through RSS starts to supplant traditional broadcasting. The goals of the Participatory Culture Foundation for the future of media (http://participatoryculture.org/) are very closely aligned with the principles outlined in the Mozilla manifesto (in particular manifesto points 2, 5 and 6).
(Assignee)

Comment 47

10 years ago
Created attachment 297662 [details] [diff] [review]
Everything in the previous patch minus usage of downloads.properties.

I removed usage of downloads.properties items and copied the items I needed (bytes, kilobytes, megabytes and gigabytes) over to subscribe.properties.
Attachment #297488 - Attachment is obsolete: true
Attachment #297662 - Flags: review?(sayrer)
Attachment #297488 - Flags: review?(sayrer)

Comment 48

10 years ago
Comment on attachment 297662 [details] [diff] [review]
Everything in the previous patch minus usage of downloads.properties.

hmm, UI localization is not my strong suit. Gavin should know what's up. Sorry for all the bureaucracy.
Attachment #297662 - Flags: review?(sayrer) → review?(gavin.sharp)
Comment on attachment 297662 [details] [diff] [review]
Everything in the previous patch minus usage of downloads.properties.

>Index: browser/components/feeds/src/FeedWriter.js

>+      if (enc.hasKey("length")) {
>+        var enc_size = 0 + enc.get("length");
>+        if (enc_size < ONE_KB) {
>+          enc_size = enc_size + " " + this._getString("bytes");
>+        } else if (enc_size < ONE_MB) {
>+          enc_size = roundme(enc_size / ONE_KB) + " " + this._getString("kilobyte");
>+        } else if (enc_size < ONE_GB) {
>+          enc_size = roundme(enc_size / ONE_MB) + " " + this._getString("megabyte");
>+        } else {
>+          enc_size = roundme(enc_size / ONE_GB) + " " + this._getString("gigabyte");
>+        }
>+        var size_text = " " + this._getFormattedString("enclosureSizeText", [enc_size]);
>+        enclosureDiv.appendChild(this._document.createTextNode(size_text));

Unfortunately you can't rely on ordering such that just doing string concatenation is OK. In some languages localizers will want to use the equivalent of "MB 3", or use a separator other than " ", for example. You should use localized strings like "%S MB" and getFormattedString... I guess that means merging it into enclosureSizeText. (The download manager code does this in a more complicated way using custom code for some reason, but the net effect is the same.)

The rest of this looks OK from a l10n point of view, I think (r+ once that problem is fixed). Since you're adding UI you should be getting UI-review on this as well, though - pinging beltzner or faaborg's directly, via IRC or email, would probably work best (and some screenshots of what the feed preview patch looks like with this patch in effect might help).
Attachment #297662 - Flags: review?(gavin.sharp) → review-
>pinging beltzner or faaborg's directly, via IRC or email

I'm closely monitoring this bug :)  If you attach some screen shots I can do an initial ui-review, and then give beltzner a recommendation for the formal ui-r+
(Assignee)

Comment 51

10 years ago
Gavin: Does this look right?

in subscribe.properties:

>+ enclosureSizeText=size: %S
>+ bytes=%S bytes
>+ kilobyte=%S KB
>+ megabyte=%S MB
>+ gigabyte=%S GB

in FeedWriter.js:

>+        var enc_size = 0 + enc.get("length");
>+        var enc_size_unit = null;
>+        if (enc_size < ONE_KB) {
>+          enc_size_unit = "bytes";
>+        } else if (enc_size < ONE_MB) {
>+          enc_size_unit = "kilobyte";
>+        } else if (enc_size < ONE_GB) {
>+          enc_size_unit = "megabyte";
>+        } else {
>+          enc_size_unit = "gigabyte";
>+        }
>+        var size_text = this._getFormattedString("enclosureSize", 
>+                            [this._getFormattedString(enc_size_unit,                      [enc_size]) ]);
>+        enclosureDiv.appendChild(this._document.createTextNode(size_text));

Is that what you're thinking of in terms of localization string composition?
Yeah, that would work, though I think it might be clearer as:

enclosureSizeText=size: %S %S

var unit = getString(enc_size_unit);
var size_text = getFormattedString(enclosureSizeText, [enc_size, unit]);

That way the ordering is more explicit in the final string (and you only use one getFormattedString instead of two). Would also be good to add an l10n note (http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/locales/en-US/crashreporter/crashreporter.ini&rev=1.2&mark=21,22 is an example) that describes the ordering.
(Assignee)

Comment 53

10 years ago
Gavin: I'm a little puzzled--I thought you were suggesting in comment 49 that localizers might want to change the order of the size bits.  So the code I was suggesting allows for:

   size: 5 MB

as well as:

   size: MB 5

(as well as other variations).

But then in comment 52, I think you're suggesting it's better not to allow for different orderings.

I can adjust the code either way, but which one should I do?

I'll add comments in the next patch--that'd definitely make it clearer as to what the %S bits are replaced with.
The code in comment 52 allows different orderings. Localizers just adjust enclosureSizeText to use positional markers like "%1$s" and "%2$s" if they want to reverse the position of the two strings.
Though it might be better to explicitly use %1$s and %2$s instead of two "%s", as in http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/locales/en-US/chrome/browser/preferences/preferences.properties&rev=1.23&mark=32-38#30 - Axel?

Comment 56

10 years ago
Nit 1, you probably want %1$S instead of %1$s ;-)

Using explict ordering by using %1$S etc makes it much easier to write the localization note, as you can then say something like

%2$S will be replaced by the unit (one of the values of the ... entries in this file), %1$S will be replaced by the amount.

Of course, this opens up the plural can of worms again. Something that I need to look at again in bug 394516.
(Assignee)

Comment 57

10 years ago
Created attachment 297810 [details]
Screenshot of enclosures in FeedWriter
(Assignee)

Comment 58

10 years ago
Created attachment 297813 [details]
Second screenshot of enclosures in FeedWriter with possible issue

I just discovered that if a feed has Yahoo MRSS media:content items in it and RSS enclosures, then it's possible for the resulting set of enclosures in nsIFeedEntry to have duplicate items which FeedWriter currently shows.  The minor complication to this is that it's possible for the different enclosure items to have the same url, but different data otherwise.

Should FeedProcessor or FeedWriter remove duplicate enclosures in an entry?
(Assignee)

Comment 59

10 years ago
Comment on attachment 297810 [details]
Screenshot of enclosures in FeedWriter

Tagging this for review.
Attachment #297810 - Flags: ui-review?(beltzner)
(Assignee)

Comment 60

10 years ago
Comment on attachment 297813 [details]
Second screenshot of enclosures in FeedWriter with possible issue

Tagging this for review, too.
Attachment #297813 - Flags: ui-review?(beltzner)
(Assignee)

Comment 61

10 years ago
Created attachment 297907 [details] [diff] [review]
Previous patch plus testing for enclosure size and localization fixes for enclosure size.

This uses the %1$S/%2$S method of localizing the enclosure size bits from comments 48 through 56.

This also makes sure that the size is a real number.  Previously if the size was some weird text like "foobar", then FeedWriter would print something like "size: NaN GB".  Now if size isn't a number, it won't print size information at all.
Attachment #297662 - Attachment is obsolete: true
(Assignee)

Comment 62

10 years ago
Comment on attachment 297907 [details] [diff] [review]
Previous patch plus testing for enclosure size and localization fixes for enclosure size.

I'm setting sayrer on this review, but if it should be someone else, let me know.  I thought I had flagged it the first time around, but didn't somehow.
Attachment #297907 - Flags: review?(sayrer)
(Assignee)

Comment 63

10 years ago
Created attachment 298635 [details] [diff] [review]
Previous patch plus some fixes suggested by myk.

This should be all set to go.  It's a little cleaner than the previous patch.  There are tests for the changes in FeedProcessor.js.
Attachment #297907 - Attachment is obsolete: true
Attachment #298635 - Flags: review?(sayrer)
Attachment #297907 - Flags: review?(sayrer)
(Assignee)

Comment 64

10 years ago
Comment on attachment 298635 [details] [diff] [review]
Previous patch plus some fixes suggested by myk.

Switching the review request to gavin per comment 48.
Attachment #298635 - Flags: review?(sayrer) → review?(gavin.sharp)
(Assignee)

Comment 65

10 years ago
Comment on attachment 298635 [details] [diff] [review]
Previous patch plus some fixes suggested by myk.

Nixing the review request--I'm going to tweak the patch again.
Attachment #298635 - Flags: review?(gavin.sharp)
(Assignee)

Comment 66

10 years ago
After talking with beltzner and sayrer, I'm going to change the feed type detection logic to something like this:

if there is at least one enclosure per entry in the feed:
   if all enclosures are of type video/*:
      mark as TYPE_VIDEO

   else if all enclosures are of type audio/*:
      mark as TYPE_AUDIO

everything else is TYPE_FEED.

The thinking here is that video podcasts and audio podcasts should be treated in a special way and that's what this patch is adding handling for.  Everything else should use the existing preferences and behavior for TYPE_FEED just like in Firefox 2.
(Assignee)

Comment 67

10 years ago
Created attachment 298759 [details] [diff] [review]
Everything in previous patch with minor adjustments to logic determining feed type.

sayrer--I'm marking you as the reviewer.  There aren't many changes in the last few patches.  Should be quick to review (in the grand scheme of things).
Attachment #298635 - Attachment is obsolete: true
Attachment #298759 - Flags: review?(sayrer)

Updated

10 years ago
Attachment #298759 - Flags: review?(sayrer) → review+

Updated

10 years ago
Attachment #298759 - Flags: approval1.9+

Comment 68

10 years ago
Comment on attachment 298759 [details] [diff] [review]
Everything in previous patch with minor adjustments to logic determining feed type.

Will approve once ui-review is done
Attachment #298759 - Flags: approval1.9+
Comment on attachment 297813 [details]
Second screenshot of enclosures in FeedWriter with possible issue

Should definitely remove the duplicates.
Attachment #297813 - Flags: ui-review?(beltzner) → ui-review-
Comment on attachment 297810 [details]
Screenshot of enclosures in FeedWriter

This is close, just some string changes and a layout change. I like the use of the border and background colour - looks nice.

Maybe instead, though like this:

ENTRY TITLE

Entry text blah blah here's our new video, kids!

 .---------------------------------------------------------------.
 | Media files                                                   |
 |  [ ] Doghouse_-_As_If_By_Magic.mp3 (audio, 7.4 MB)            |
 |  [ ] Our new video.avi (video, 18.4 MB)                       |
 '---------------------------------------------------------------'

-----

where [ ] is the file icon. The name should be pretty printed (ie: %20s as spaces) and shown as an active link. The metadata should contain the media type ("audio" or "video") and the file size to a single decimal point, both in smaller text.
Attachment #297810 - Flags: ui-review?(beltzner) → ui-review-
(Assignee)

Comment 71

10 years ago
Created attachment 299123 [details]
Screenshot of revised look

Here's what it looks like after look'n'feel adjustments from Beltzner's comments.
Attachment #297810 - Attachment is obsolete: true
Attachment #297813 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #299123 - Flags: ui-review?(beltzner)
Comment on attachment 299123 [details]
Screenshot of revised look

Thanks, Will. Great (and fast!) work.
Attachment #299123 - Flags: ui-review?(beltzner) → ui-review+
(Assignee)

Comment 73

10 years ago
Created attachment 299125 [details] [diff] [review]
Previous patch plus ui-changes in FeedWriter and duplicate removing in FeedProcessor

I added a few more tests to make sure the duplicate removing works.  One of the things it does when removing duplicates is attempt to increase the quality of the data.

The code changes in FeedWriter are a bit more involved.  I'm trying to pick up icons and friendly descriptions for mimetypes.  I think I have that code right, but ...  it's new to me.
Attachment #298759 - Attachment is obsolete: true
Attachment #299125 - Flags: review?(sayrer)
(Assignee)

Comment 74

10 years ago
Created attachment 299281 [details] [diff] [review]
This has the string changes from the previous patch.

Splitting the string changes out from the rest of the patch so that Beltzner can review them, approve them, and set them on fire.
Attachment #299281 - Flags: ui-review?(beltzner)
(Assignee)

Comment 75

10 years ago
Comment on attachment 299281 [details] [diff] [review]
This has the string changes from the previous patch.

Carrying over the ui-review from Beltzner per his instructions.  Asking for approval1.9 so it can land.
Attachment #299281 - Flags: ui-review?(beltzner)
Attachment #299281 - Flags: ui-review+
Attachment #299281 - Flags: approval1.9?
Comment on attachment 299281 [details] [diff] [review]
This has the string changes from the previous patch.

a=beltzner for 1.9
Attachment #299281 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 77

10 years ago
Created attachment 299302 [details] [diff] [review]
Patch 299125 minus the strings changes.
Attachment #299125 - Attachment is obsolete: true
Attachment #299302 - Flags: review?(sayrer)
Attachment #299125 - Flags: review?(sayrer)
Will: for the feed preview, are we changing text and icons yet based on the type (feed, podcast, video podcast).  I think this will be important for cases where the user selects the "always use this app" checkbox for one type but not another.  For instance, you check it for a feed, and then still get the preview for a podcast which it refers to as a feed.

Also, we want to try to evangelize icons for podcasts and video podcasts so site stop using buttons that say "POD" and "XML" right next to "iTunes" and "Zune."  So it would be good to integrate these icons into the preview as well.
Never mind, just saw bug 412002, I'll follow up there.
er, bug 400064
(Assignee)

Comment 81

10 years ago
Can I get attachment 299281 [details] [diff] [review] (the second strings patch) checked in?
Keywords: uiwanted → checkin-needed
The strings patch has been checked in:

Checking in browser/locales/en-US/chrome/browser/feeds/subscribe.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/feeds/subscribe.properties,v  <--  subscribe.properties
new revision: 1.8; previous revision: 1.7
done
myk already landed the string changes.
Keywords: checkin-needed

Comment 84

10 years ago
Comment on attachment 299302 [details] [diff] [review]
Patch 299125 minus the strings changes.

>Index: browser/components/feeds/src/FeedWriter.js
>===================================================================
>+  _getNameForURL: function FW__getNameForURL(url) {
>+    var bits = url.split("/");
>+    if (bits.length > 0) {
>+      for (var i = bits.length - 1; i >= 0; --i) {
>+        if (bits[i].length > 0)
>+          try {
>+            return decodeURI(bits[i]);
>+          } catch(ex) { }
>+          return bits[i];
>+      }
>+    }
>+    return url;
>+  },
>+

This should be using nsIURI methods to inspect path segments.
(Assignee)

Comment 85

10 years ago
Created attachment 299912 [details] [diff] [review]
Patch 299302 with _getNameForURL re-written, re-named and commented.
Attachment #299302 - Attachment is obsolete: true
Attachment #299912 - Flags: review?(sayrer)
Attachment #299302 - Flags: review?(sayrer)
(Assignee)

Comment 86

10 years ago
Created attachment 300035 [details] [diff] [review]
Patch 299912 plus the forgotten jar.mn file changes

Bah--in the previous patch I forgot the jar.mn changes.  This adds those.

When this patch is applied, I need the following files copied:

in browser/themes/gnomestripe/browser/feeds:
feedIcon.png -> videoFeedIcon.png
feedIcon.png -> audioFeedIcon.png
feedIcon16.png -> videoFeedIcon16.png
feedIcon16.png -> audioFeedIcon16.png

in browser/themes/pinstripe/browser/feeds:
feedIcon.png -> videoFeedIcon.png
feedIcon.png -> audioFeedIcon.png
feedIcon16.png -> videoFeedIcon16.png
feedIcon16.png -> audioFeedIcon16.png

in browser/themes/winstripe/browser/feeds:
feedIcon.png -> videoFeedIcon.png
feedIcon.png -> audioFeedIcon.png
feedIcon16.png -> videoFeedIcon16.png
feedIcon16.png -> audioFeedIcon16.png
Attachment #299912 - Attachment is obsolete: true
Attachment #300035 - Flags: review?(sayrer)
Attachment #299912 - Flags: review?(sayrer)

Comment 87

10 years ago
Comment on attachment 300035 [details] [diff] [review]
Patch 299912 plus the forgotten jar.mn file changes


>+  _getURLDisplayName: function FW__getURLDisplayName(aURL) {
>+    var ios = 
>+        Cc["@mozilla.org/network/io-service;1"].
>+        getService(Ci.nsIIOService);
>+    var url = ios.newURI(aURL, null, null).QueryInterface(Ci.nsIURL);
>+

There's a makeURI function at the top of the file. Let's use that. Carry over the review and mark checkin-needed.
Attachment #300035 - Flags: review?(sayrer) → review+
(Assignee)

Comment 88

10 years ago
Created attachment 300053 [details] [diff] [review]
Previous patch with minor change to _getURLDisplayName to use makeURI.

Carrying over sayrer's r+.  This is ready to land.
Attachment #300035 - Attachment is obsolete: true
Attachment #300053 - Flags: review+
(Assignee)

Comment 89

10 years ago
Marking this as checkin-needed.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment on attachment 300053 [details] [diff] [review]
Previous patch with minor change to _getURLDisplayName to use makeURI.

a=beltzner
Attachment #300053 - Flags: approval1.9+

Comment 91

10 years ago
Checking in browser/components/feeds/src/FeedWriter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedWriter.js,v  <--  FeedWriter.js
new revision: 1.49; previous revision: 1.48
done
Checking in browser/themes/gnomestripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.67; previous revision: 1.66
done
RCS file: /cvsroot/mozilla/browser/themes/gnomestripe/browser/feeds/audioFeedIcon.png,v
done
Checking in browser/themes/gnomestripe/browser/feeds/audioFeedIcon.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/feeds/audioFeedIcon.png,v  <--  audioFeedIcon.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/themes/gnomestripe/browser/feeds/audioFeedIcon16.png,v
done
Checking in browser/themes/gnomestripe/browser/feeds/audioFeedIcon16.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/feeds/audioFeedIcon16.png,v  <--  audioFeedIcon16.png
initial revision: 1.1
done
Checking in browser/themes/gnomestripe/browser/feeds/subscribe.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/feeds/subscribe.css,v  <--  subscribe.css
new revision: 1.18; previous revision: 1.17
done
RCS file: /cvsroot/mozilla/browser/themes/gnomestripe/browser/feeds/videoFeedIcon.png,v
done
Checking in browser/themes/gnomestripe/browser/feeds/videoFeedIcon.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/feeds/videoFeedIcon.png,v  <--  videoFeedIcon.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/themes/gnomestripe/browser/feeds/videoFeedIcon16.png,v
done
Checking in browser/themes/gnomestripe/browser/feeds/videoFeedIcon16.png;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/feeds/videoFeedIcon16.png,v  <--  videoFeedIcon16.png
initial revision: 1.1
done
Checking in browser/themes/pinstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/pinstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.70; previous revision: 1.69
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/feeds/audioFeedIcon.png,v
done
Checking in browser/themes/pinstripe/browser/feeds/audioFeedIcon.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/feeds/audioFeedIcon.png,v  <--  audioFeedIcon.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/feeds/audioFeedIcon16.png,v
done
Checking in browser/themes/pinstripe/browser/feeds/audioFeedIcon16.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/feeds/audioFeedIcon16.png,v  <--  audioFeedIcon16.png
initial revision: 1.1
done
Checking in browser/themes/pinstripe/browser/feeds/subscribe.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/feeds/subscribe.css,v  <--  subscribe.css
new revision: 1.19; previous revision: 1.18
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/feeds/videoFeedIcon.png,v
done
Checking in browser/themes/pinstripe/browser/feeds/videoFeedIcon.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/feeds/videoFeedIcon.png,v  <--  videoFeedIcon.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/feeds/videoFeedIcon16.png,v
done
Checking in browser/themes/pinstripe/browser/feeds/videoFeedIcon16.png;
/cvsroot/mozilla/browser/themes/pinstripe/browser/feeds/videoFeedIcon16.png,v  <--  videoFeedIcon16.png
initial revision: 1.1
done
Checking in browser/themes/winstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/winstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.66; previous revision: 1.65
done
RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/feeds/audioFeedIcon.png,v
done
Checking in browser/themes/winstripe/browser/feeds/audioFeedIcon.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/feeds/audioFeedIcon.png,v  <--  audioFeedIcon.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/feeds/audioFeedIcon16.png,v
done
Checking in browser/themes/winstripe/browser/feeds/audioFeedIcon16.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/feeds/audioFeedIcon16.png,v  <--  audioFeedIcon16.png
initial revision: 1.1
done
Checking in browser/themes/winstripe/browser/feeds/subscribe.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/feeds/subscribe.css,v  <--  subscribe.css
new revision: 1.18; previous revision: 1.17
done
RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/feeds/videoFeedIcon.png,v
done
Checking in browser/themes/winstripe/browser/feeds/videoFeedIcon.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/feeds/videoFeedIcon.png,v  <--  videoFeedIcon.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/feeds/videoFeedIcon16.png,v
done
Checking in browser/themes/winstripe/browser/feeds/videoFeedIcon16.png;
/cvsroot/mozilla/browser/themes/winstripe/browser/feeds/videoFeedIcon16.png,v  <--  videoFeedIcon16.png
initial revision: 1.1
done
Checking in toolkit/components/feeds/src/FeedProcessor.js;
/cvsroot/mozilla/toolkit/components/feeds/src/FeedProcessor.js,v  <--  FeedProcessor.js
new revision: 1.31; previous revision: 1.30
done
Checking in toolkit/components/feeds/test/xml/rss2/item_enclosure.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_enclosure.xml,v  <--  item_enclosure.xml
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rfc4287/entry_link_enclosure.xml,v
done
Checking in toolkit/components/feeds/test/xml/rfc4287/entry_link_enclosure.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rfc4287/entry_link_enclosure.xml,v  <--  entry_link_enclosure.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rfc4287/entry_link_enclosure_populate_enclosures.xml,v
done
Checking in toolkit/components/feeds/test/xml/rfc4287/entry_link_enclosure_populate_enclosures.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rfc4287/entry_link_enclosure_populate_enclosures.xml,v  <--  entry_link_enclosure_populate_enclosures.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_enclosure_duplicates.xml,v
done
Checking in toolkit/components/feeds/test/xml/rss2/item_enclosure_duplicates.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_enclosure_duplicates.xml,v  <--  item_enclosure_duplicates.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_enclosure_duplicates2.xml,v
done
Checking in toolkit/components/feeds/test/xml/rss2/item_enclosure_duplicates2.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_enclosure_duplicates2.xml,v  <--  item_enclosure_duplicates2.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_enclosure_mixed.xml,v
done
Checking in toolkit/components/feeds/test/xml/rss2/item_enclosure_mixed.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_enclosure_mixed.xml,v  <--  item_enclosure_mixed.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_enclosure_mixed2.xml,v
done
Checking in toolkit/components/feeds/test/xml/rss2/item_enclosure_mixed2.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_enclosure_mixed2.xml,v  <--  item_enclosure_mixed2.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_populated_enclosures.xml,v
done
Checking in toolkit/components/feeds/test/xml/rss2/item_populated_enclosures.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_populated_enclosures.xml,v  <--  item_populated_enclosures.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/mrss_content.xml,v
done
Checking in toolkit/components/feeds/test/xml/rss2/mrss_content.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/mrss_content.xml,v  <--  mrss_content.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/mrss_content_populate_enclosure.xml,v
done
Checking in toolkit/components/feeds/test/xml/rss2/mrss_content_populate_enclosure.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/mrss_content_populate_enclosure.xml,v  <--  mrss_content_populate_enclosure.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/mrss_group_content.xml,v
done
Checking in toolkit/components/feeds/test/xml/rss2/mrss_group_content.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/mrss_group_content.xml,v  <--  mrss_group_content.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/mrss_group_content_populate_enclosure.xml,v
done
Checking in toolkit/components/feeds/test/xml/rss2/mrss_group_content_populate_enclosure.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/mrss_group_content_populate_enclosure.xml,v  <--  mrss_group_content_populate_enclosure.xml
initial revision: 1.1
done

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Keywords: checkin-needed

Comment 92

9 years ago
Sorry for the bugspam, but after seeing FF3b5, I'm really impressed.
Great work!
You need to log in before you can comment on or make changes to this bug.