Closed Bug 303645 Opened 19 years ago Closed 16 years ago

Add support for media feeds and objects (enclosures)

Categories

(Firefox Graveyard :: RSS Discovery and Preview, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mozilla, Assigned: will.guaraldi)

References

()

Details

Attachments

(3 files, 18 obsolete files)

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
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.
Component: General → RSS Discovery and Preview
No longer blocks: feedview
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. ***
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
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
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
Summary: Add support for media feeds and objects → Add support for media feeds and objects (enclosures)
Any chance of getting a copy of FeedProcessor.js that populates the enclosures attribute?
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.
Blocks: 400059
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.
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.
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?
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.
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.
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.
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
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 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.
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.
(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.
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)
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
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)
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)
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 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]
I made the changes requested.
Attachment #296979 - Attachment is obsolete: true
Attachment #297246 - Flags: review?(sayrer)
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.
Can we get a quick risk/reward assessment for this? 
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?
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
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?
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).
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 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+
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.
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.
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.
Attached image Screenshot of enclosures in FeedWriter (obsolete) —
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?
Comment on attachment 297810 [details]
Screenshot of enclosures in FeedWriter

Tagging this for review.
Attachment #297810 - Flags: ui-review?(beltzner)
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)
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
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)
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)
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)
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)
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.
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)
Attachment #298759 - Flags: review?(sayrer) → review+
Attachment #298759 - Flags: approval1.9+
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-
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
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+
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)
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)
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+
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.
Can I get attachment 299281 [details] [diff] [review] (the second strings patch) checked in?
Keywords: uiwantedcheckin-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 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.
Attachment #299302 - Attachment is obsolete: true
Attachment #299912 - Flags: review?(sayrer)
Attachment #299302 - Flags: review?(sayrer)
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 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+
Carrying over sayrer's r+.  This is ready to land.
Attachment #300035 - Attachment is obsolete: true
Attachment #300053 - Flags: review+
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+
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Sorry for the bugspam, but after seeing FF3b5, I'm really impressed.
Great work!
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.