Closed Bug 400061 Opened 17 years ago Closed 16 years ago

Add "Podcast" and "Video Podcast" entries to the applications prefpane

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: faaborg, Assigned: will.guaraldi)

References

Details

Attachments

(6 files, 3 obsolete files)

The applications prefpane should contain two additional RSS-based content types, Podcast and Video Podcast.  Since there is currently no way to get applications that handle these content types from the OS, we are going to have to set the list ourselves, similar to Web Feed.

I propose that we include an entry called "Get Miro" that takes the user to a landing page hosted on getmiro.com that explains what podcasting is, what Miro is, and a link to download the application.  Once Miro is installed this option needs to be removed, and Miro should set itself as the default for audio and video podcasts, but only if the user has not yet specified another default.
I should note that we will need to get new icons designed for these two new entries, I'm adding that to the icon inventory.
I looked at the code and this looks non-trivial for a couple of reasons.  First it looks like "feeds" are associated with the MIME-type application/vnd.mozilla.maybe.feed .  Second, it looks like there's one application and one webservice associated with this MIME-type and it's done in the preferences.

So...  is it a good idea to create two more MIME-types, application/vnd.mozilla.maybe.video.feed and application/vnd.mozilla.maybe.audio.feed, and mirror what's done with application/vnd.mozilla.maybe.feed ?
Myk or dmose: can you give Will some direction on the best way to implement this?
Folks have said that application/vnd.mozilla.maybe.feed should die, but I doubt that's in scope for Firefox 3, so perhaps related MIME types are the best option for fixing this bug, and the web content converter service should return those types when it processes a feed containing the appropriate content.

But we should try to store configuration information for these types in the handlers datastore rather than in preferences (as with maybe.feed).  The only potential difficulty there will be getting handleInternally working, since we don't currently expect any types in the handlers datastore to be handled internally, but feed types can be handled internally (i.e. we can show a preview and ask the user what else to do with it).

Nevertheless, I'm relatively new to the feed handling code, so cc:ing mano, who knows the code better than me and may have some better ideas for what to do.
I'm having problem with the applications pane in the preferences panel.  I'm running off of the latest in trunk and there's no content types or applications listed in the pane.  I checked the nightly (1/14) and beta 2, but the applications pane was empty in both of those as well.

Any idea what populates the data in the applications pane?
>I'm having problem with the applications pane in the preferences panel.  I'm
>running off of the latest in trunk and there's no content types or applications
>listed in the pane.  I checked the nightly (1/14) and beta 2, but the
>applications pane was empty in both of those as well.


We had bug 410900, which was fixed.  I'm cc'ing Florian to see if he can help.
Will: do any messages appear in the Error Console when you open the prefpane?
Flags: in-litmus?
Sorry it took so long.  Turns out I am getting an error with the latest in trunk:

JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: chrome://browser/content/preferences/applications.js :: <TOP_LEVEL> :: line 444"  data: no]

I'll ping #developers and see if I can figure it out or find someone else with the same problem.
I found myk on #developers and after some poking around and rebuilding and changing of socks, I'm pretty sure it's a build issue on my side.  I'm looking into the possibility that my .mozconfig isn't set up right.

If worse comes to worse, the applications.js error goes away if I uncomment a few lines in a Makefile.in that remove the HAVE_SHELL_SERVICE option.
I curried the existing feedHandlerInfo so that I could create a videoFeedHandlerInfo and audioFeedHandlerInfo while adjusting as little of the code as possible.  Trying to make the modifications while keeping the risk low.

I don't know if I have to add the two lines I added to browser/base/content/browser.js.

There are FIXME items regarding icons for video feeds and audio feeds.  I wasn't sure what to do about that since the icons don't exist.
Attachment #297932 - Flags: review?(myk)
>There are FIXME items regarding icons for video feeds and audio feeds.  I
>wasn't sure what to do about that since the icons don't exist.

We can drop in final icons later when we have them produced, so don't worry about that.
The previous patch was missing the changes to preferences.properties.  I added them in this patch.
Attachment #297932 - Attachment is obsolete: true
Attachment #298293 - Flags: review?(myk)
Attachment #297932 - Flags: review?(myk)
Attachment #297936 - Flags: ui-review?(beltzner)
Comment on attachment 298293 [details] [diff] [review]
Previous patch plus changes to preferences.properties.

>Index: browser/app/profile/firefox.js

>+pref("browser.video.feeds.handler", "ask");
>+pref("browser.audio.feeds.handler", "ask");

These should be called browser.videoFeeds.handler and browser.audioFeeds.handler.


Index: browser/locales/en-US/chrome/browser/preferences/preferences.properties

> webFeed=Web Feed
>+podcastFeed=Podcast
>+videoPodcastFeed=Video Podcast
> alwaysAsk=Always ask

It seems like the podcastFeed property should be called audioPodcastFeed (even if the value of the property is simply "Podcast") for consistency with naming convention elsewhere in your patch.

Also, nit: put the video one first for consistency with their order elsewhere in your patch.


>Index: browser/base/content/browser.js

> const TYPE_MAYBE_FEED = "application/vnd.mozilla.maybe.feed";
>+const TYPE_MAYBE_AUDIO_FEED = "application/vnd.mozilla.maybe.audio.feed";
>+const TYPE_MAYBE_VIDEO_FEED = "application/vnd.mozilla.maybe.video.feed";
> const TYPE_XUL = "application/vnd.mozilla.xul+xml";

As you correctly surmised, browser.js doesn't need these definitions.  As far as I can tell, it doesn't need the TYPE_MAYBE_FEED definition either, since it doesn't use it anywhere, but let's reduce risk and not muck with it in this patch.


>Index: browser/components/preferences/applications.js

>- * This object implements nsIHandlerInfo for the feed type.  It's a separate
>- * object because we currently store handling information for the feed type
>- * in a set of preferences rather than the nsIHandlerService-managed datastore.
>+ * This returns an object that implements nsIHandlerInfo for the feed type.  
>+ * It's a separate * object because we currently store handling information 
>+ * for the feed types * in a set of preferences rather than the 
>+ * nsIHandlerService-managed datastore.

Nit: for the feed type -> for the feed types
Nit: remove the asterisks from the middles of lines.


>+function buildFeedHandlerInfo(mimetype, selected_app, selected_web, selected_action, 
>+                              selected_reader, small_icon, large_icon, app_pref_label) {
>+  return {
>+
>+  // not pushing the indentation out two more spaces so that it diffs better.
>+  __proto__: new HandlerInfoWrapper(mimetype, null),
>+
>+  _mimetype: mimetype,
>+
>+  _PREF_SELECTED_APP: selected_app,
>+  _PREF_SELECTED_WEB: selected_web,
>+  _PREF_SELECTED_ACTION: selected_action,
>+  _PREF_SELECTED_READER: selected_reader,
>+  _small_icon: small_icon,
>+  _large_icon: large_icon,
>+  _app_pref_label: app_pref_label,

The parameter names should be prefixed with "a" and both the parameter and the property names should be formatted in CamelCase for consistency with the rest of the code and the JavaScript style guide <http://developer.mozilla.org/en/docs/JavaScript_style_guide>, i.e. aMIMEType, aSelectedApp, etc. for arguments and _prefSelectedApp, _prefSelectedWeb, etc. for properties.

Also, this would be better as a FeedHandlerInfo constructor that subclasses HandlerInfoWrapper along with three instances, one per feed type, that define the instance-specific properties, i.e. something like:

function FeedHandlerInfo(aMIMEType) {
  HandlerInfoWrapper.call(this, aMIMEType, null);
}

FeedHandlerInfo.prototype = {
  __proto__: HandlerInfoWrapper.prototype,

  // the rest of the generic feed handler properties...
}

var feedHandlerInfo = {
  __proto__: new FeedHandlerInfo(TYPE_MAYBE_FEED),
  _prefSelectedApp: PREF_FEED_SELECTED_APP,

  // the rest of the regular feed handler properties...
}

var videoFeedHandlerInfo = {
  __proto__: new FeedHandlerInfo(TYPE_MAYBE_VIDEO_FEED),
  _prefSelectedApp: PREF_VIDEO_FEED_SELECTED_APP,

  // the rest of the video feed handler properties...
}

var audioFeedHandlerInfo = {
  __proto__: new FeedHandlerInfo(TYPE_MAYBE_AUDIO_FEED),
  _prefSelectedApp: PREF_AUDIO_FEED_SELECTED_APP,

  // the rest of the audio feed handler properties...
}

Note: you don't need a _mimeType property, as the instances inherit (and populate) HandlerInfoWrapper::type.

More info on the inheritance model being used here is available in the JavaScript Guide <http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Guide:Inheritance>.


>     if ((handlerInfo.wrappedHandlerInfo instanceof Ci.nsIMIMEInfo) &&
>-        handlerInfo.type != TYPE_MAYBE_FEED) {
>+        !(isFeedType(handlerInfo.type))) {

Nit: no need for parentheses around the function call, I believe.

Otherwise this looks great and is reassuringly simple.  I guess the hard part is in the other patch. :-)
Attachment #298293 - Flags: review?(myk) → review-
Attachment #297936 - Flags: ui-review?(beltzner) → ui-review+
This patch has the string changes need.  I'm marking it as ui+ per Beltzner's previous review.  I'm marking it as approval1.9? so it can get approved and land tonight.
Attachment #299338 - Flags: ui-review+
Attachment #299338 - Flags: approval1.9?
This patch doesn't include the strings changes anymore--that's in a separate patch.
Attachment #298293 - Attachment is obsolete: true
Attachment #299339 - Flags: review?(myk)
Comment on attachment 299338 [details] [diff] [review]
Strings-only patch

a=beltzner for 1.9
Attachment #299338 - Flags: approval1.9? → approval1.9+
>There are FIXME items regarding icons for video feeds and audio feeds.  I
>wasn't sure what to do about that since the icons don't exist.

Let's put in separate icons for podcast and video podcast, and just copy over the web feed icon until we have the new artwork ready.
I currently refer to icons that don't exist:

   chrome://browser/skin/feeds/videoFeedIcon16.png
   chrome://browser/skin/feeds/videoFeedIcon.png

   chrome://browser/skin/feeds/audioFeedIcon16.png
   chrome://browser/skin/feeds/audioFeedIcon.png

I can definitely create filler icons on my machine, but I'm not sure how to add them to the patch.  How does that work?
Assignee: nobody → will.guaraldi
Strings-only patch checked in:

Checking in browser/locales/en-US/chrome/browser/preferences/preferences.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/preferences.properties,v  <--  preferences.properties
new revision: 1.25; previous revision: 1.24
done
Comment on attachment 299339 [details] [diff] [review]
Previous patch 298293 with myk's suggested changes.

> I can definitely create filler icons on my machine, but I'm not sure how to add
> them to the patch.  How does that work?

You can't add them to the patch, but you can attach them to this bug, and the person who checks in your patch can check them in.  Alternately, just identify the icon file(s) to copy and the name(s) to copy it/them to, and the in-checker will do the needful.


>Index: browser/components/preferences/applications.xul
>     </preferences>
> 
>+
>     <script type="application/x-javascript" src="chrome://browser/content/preferences/applications.js"/>

Any particular reason to add this extra newline?


>Index: browser/components/preferences/applications.js

> /**
>- * This object implements nsIHandlerInfo for the feed type.  It's a separate
>- * object because we currently store handling information for the feed type
>- * in a set of preferences rather than the nsIHandlerService-managed datastore.
>+ * This returns an object that implements nsIHandlerInfo for the feed types.  
>+ * It's a separate object because we currently store handling information 
>+ * for the feed types in a set of preferences rather than the 
>+ * nsIHandlerService-managed datastore.
>  * 
>- * This object inherits from HandlerInfoWrapper in order to get functionality
>- * that isn't special to the feed type.
>+ * The returned object inherits from HandlerInfoWrapper in order to get 
>+ * functionality that isn't special to the feed type.

Since you've switched to a constructor, this comment should be reverted, except for the change from "feed type" to "feed types", which should remain (and be applied to that last sentence as well).

Otherwise, this looks great. r=myk with the comment reverted (and the newline removed if it isn't there for a reason).
Attachment #299339 - Flags: review?(myk) → review+
myk already landed the string changes.
Keywords: checkin-needed
This has the two tweaks myk asked for in the previous review.  I'm marking this one as r+, too.

When this gets checked in, it will need these 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 #299339 - Attachment is obsolete: true
Attachment #299513 - Flags: review+
Attachment #299513 - Flags: review?(sayrer)
Attachment #299513 - Flags: review?(sayrer) → review+
Comment on attachment 299513 [details] [diff] [review]
Previous patch 299339 with two minor tweaks per myk's suggestions.

Skip the file copying--it'll get done when the patch for 303645 lands and that needs to land first anyhow.
Now that 303645 is landed, this is ready for checkin.

No need to copy the images per the instructions in comment 24--those went with 303645.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment on attachment 299513 [details] [diff] [review]
Previous patch 299339 with two minor tweaks per myk's suggestions.

a=beltzner
Attachment #299513 - Flags: approval1.9+
This needs an unbitrotten patch, as it completely fails to apply. :(
Keywords: checkin-needed
This patch is identical to attachment 299513 [details] [diff] [review] but updated to resolve trivial conflicts with recent checkins.  This is the version of the patch to check in.
Checked in, thanks for the fix!

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.268; previous revision: 1.267
done
Checking in browser/components/preferences/applications.xul;
/cvsroot/mozilla/browser/components/preferences/applications.xul,v  <--  applications.xul
new revision: 1.8; previous revision: 1.7
done
Checking in browser/components/preferences/applications.js;
/cvsroot/mozilla/browser/components/preferences/applications.js,v  <--  applications.js
new revision: 1.40; previous revision: 1.39
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
I backed this back out because of a test failure:

Checking in browser/components/preferences/applications.js;
/cvsroot/mozilla/browser/components/preferences/applications.js,v  <--  applications.js
new revision: 1.41; previous revision: 1.40
done
Checking in browser/components/preferences/applications.xul;
/cvsroot/mozilla/browser/components/preferences/applications.xul,v  <--  applications.xul
new revision: 1.9; previous revision: 1.8
done
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.271; previous revision: 1.270
done

I'm looking into the problem now...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The failure is:

FAIL - App handler list
populated -
chrome://mochikit/content/browser/browser/components/preferences/tests/browser_bug410900.js
Looks like Mano has a fix for some problems in bug 414735 that may help?
My patch that resolved conflicts contained a typo, and the original patch contained a problem when building on Mac.  Mano found both problems and included fixes in his patch on bug 414735, but I had already backed this out, so he didn't check those in.  Thus here's the version of this patch with his fixes.
Relanded:

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.272; previous revision: 1.271
done
Checking in browser/components/preferences/applications.xul;
/cvsroot/mozilla/browser/components/preferences/applications.xul,v  <--  applications.xul
new revision: 1.10; previous revision: 1.9
done
Checking in browser/components/preferences/applications.js;
/cvsroot/mozilla/browser/components/preferences/applications.js,v  <--  applications.js
new revision: 1.42; previous revision: 1.41
done
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Litmus test case https://litmus.mozilla.org/show_test.cgi?id=5166 added for Video podcast feeds and https://litmus.mozilla.org/show_test.cgi?id=4300 modified to include podcasts.
Flags: in-litmus? → in-litmus+
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008020404 Minefield/3.0b3pre. Also verified on Win XP as well. Both podcast and video podcast entries show in the applications prefpane.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.