Closed
Bug 303645
Opened 19 years ago
Closed 17 years ago
Add support for media feeds and objects (enclosures)
Categories
(Firefox Graveyard :: RSS Discovery and Preview, enhancement)
Firefox Graveyard
RSS Discovery and Preview
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mozilla, Assigned: will.guaraldi)
References
()
Details
Attachments
(3 files, 18 obsolete files)
(deleted),
image/png
|
beltzner
:
ui-review+
|
Details |
(deleted),
patch
|
will.guaraldi
:
ui-review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
(deleted),
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.
Comment 1•19 years ago
|
||
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•19 years ago
|
Component: General → RSS Discovery and Preview
Updated•18 years ago
|
Whiteboard: [wontfix?]
Updated•18 years ago
|
Summary: Feedview should show enclosures (podcasts) → Feed preview should show enclosures (podcasts)
Whiteboard: [wontfix?]
Comment 2•18 years ago
|
||
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
Comment 3•18 years ago
|
||
*** Bug 360143 has been marked as a duplicate of this bug. ***
Comment 4•18 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
Comment 6•18 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•18 years ago
|
Flags: blocking-firefox3?
Target Milestone: Firefox 3 alpha1 → ---
Comment 7•17 years ago
|
||
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]
Comment 8•17 years ago
|
||
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
Comment 9•17 years ago
|
||
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•17 years ago
|
Summary: Add support for media feeds and objects → Add support for media feeds and objects (enclosures)
Comment 11•17 years ago
|
||
Any chance of getting a copy of FeedProcessor.js that populates the enclosures attribute?
Comment 12•17 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.
Assignee | ||
Comment 14•17 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•17 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.
Comment 16•17 years ago
|
||
dmose: are you the best person to ask for the question in comment #14?
Comment 17•17 years ago
|
||
faaborg: sayrer wrote all the code, so his advice is likely to be much more useful.
Assignee | ||
Comment 18•17 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•17 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•17 years ago
|
||
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.
Comment 21•17 years ago
|
||
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•17 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•17 years ago
|
||
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•17 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 25•17 years ago
|
||
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•17 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•17 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•17 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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
Any status on this? That last attachment/patch completes this issue.
Comment 32•17 years ago
|
||
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•17 years ago
|
||
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•17 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•17 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+
Updated•17 years ago
|
Assignee: nobody → will.guaraldi
Updated•17 years ago
|
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
Assignee | ||
Comment 36•17 years ago
|
||
I made the changes requested.
Attachment #296979 -
Attachment is obsolete: true
Attachment #297246 -
Flags: review?(sayrer)
Updated•17 years ago
|
Attachment #297246 -
Flags: review?(sayrer)
Attachment #297246 -
Flags: review+
Attachment #297246 -
Flags: approval1.9?
Comment 37•17 years ago
|
||
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•17 years ago
|
||
Can we get a quick risk/reward assessment for this?
Comment 39•17 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•17 years ago
|
||
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•17 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 42•17 years ago
|
||
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.
Comment 43•17 years ago
|
||
(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•17 years ago
|
||
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)
Comment 45•17 years ago
|
||
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.
Comment 46•17 years ago
|
||
>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•17 years ago
|
||
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•17 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 49•17 years ago
|
||
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-
Comment 50•17 years ago
|
||
>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•17 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?
Comment 52•17 years ago
|
||
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•17 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.
Comment 54•17 years ago
|
||
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.
Comment 55•17 years ago
|
||
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•17 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•17 years ago
|
||
Assignee | ||
Comment 58•17 years ago
|
||
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•17 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•17 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•17 years ago
|
||
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•17 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•17 years ago
|
||
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•17 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•17 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•17 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•17 years ago
|
||
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•17 years ago
|
Attachment #298759 -
Flags: review?(sayrer) → review+
Updated•17 years ago
|
Attachment #298759 -
Flags: approval1.9+
Comment 68•17 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 69•17 years ago
|
||
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 70•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
Attachment #299123 -
Flags: ui-review?(beltzner)
Comment 72•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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•17 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 76•17 years ago
|
||
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•17 years ago
|
||
Attachment #299125 -
Attachment is obsolete: true
Attachment #299302 -
Flags: review?(sayrer)
Attachment #299125 -
Flags: review?(sayrer)
Comment 78•17 years ago
|
||
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.
Comment 79•17 years ago
|
||
Never mind, just saw bug 412002, I'll follow up there.
Comment 80•17 years ago
|
||
er, bug 400064
Assignee | ||
Comment 81•17 years ago
|
||
Can I get attachment 299281 [details] [diff] [review] (the second strings patch) checked in?
Keywords: uiwanted → checkin-needed
Comment 82•17 years ago
|
||
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
Comment 84•17 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•17 years ago
|
||
Attachment #299302 -
Attachment is obsolete: true
Attachment #299912 -
Flags: review?(sayrer)
Attachment #299302 -
Flags: review?(sayrer)
Assignee | ||
Comment 86•17 years ago
|
||
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•17 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•17 years ago
|
||
Carrying over sayrer's r+. This is ready to land.
Attachment #300035 -
Attachment is obsolete: true
Attachment #300053 -
Flags: review+
Assignee | ||
Comment 89•17 years ago
|
||
Marking this as checkin-needed.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment 90•17 years ago
|
||
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•17 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•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Keywords: checkin-needed
Comment 92•17 years ago
|
||
Sorry for the bugspam, but after seeing FF3b5, I'm really impressed.
Great work!
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•