Adapt Web Feed preview page for Podcasts and Video Podcasts

RESOLVED FIXED in Firefox 3 beta3

Status

()

Firefox
RSS Discovery and Preview
--
enhancement
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: faaborg, Assigned: Will Guaraldi)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 3 beta3
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 10 obsolete attachments)

77.50 KB, image/png
Details
1.90 KB, patch
Details | Diff | Splinter Review
23.74 KB, patch
Will Guaraldi
: review+
beltzner
: approval1.9+
Details | Diff | Splinter Review
22.78 KB, patch
Will Guaraldi
: review+
beltzner
: approval1.9b3+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
If an RSS feed is determined to be a podcast or video podcast (bug #400062), we need to adapt the feed preview page:

-change the icon
-change the descriptive text
-change the set of available applications (needs to be the same set as what is listed in the applications prefpane)
(Assignee)

Comment 1

10 years ago
What's the best way to do this in Firefox-land?  

Is it better to create a subscribe_video.xhtml (with dtd and properties files) or is it better to adjust subscribe.* so that it handles all three types of feeds?
(Reporter)

Comment 2

10 years ago
dmose: do you know the answer to comment #1, or can you cc the person who might?

Updated

10 years ago
Flags: blocking-firefox3?
(Assignee)

Comment 3

10 years ago
I can work through this using this._getFormattedString if I switch the location of some of the strings from subscribe.dtd to subscribe.properties.  For example, I'll need to move subscribeUsing from subscribe.dtd to subscribe.properties.

I don't know what the effect on localization is when strings get moved from one file to another.  Will that be ok?
Moving the strings should be fine. It will make a bit more work for localizers, but not more than creating an entirely new page would.
(Assignee)

Comment 5

10 years ago
Created attachment 297934 [details] [diff] [review]
First attempt

I really apologize, but there's some overlap between the patch for this bug and the patch for bug #303645 in FeedWriter.js.  This patch depends on that patch going in, but I removed a lot of the stuff in the other patch since it'll be easier for reviewers to review the stuff relevant to 400064.  Kick me if that's a bad assumption and if you need me to do something different.

I had problems getting subscribe.xhtml to work nicely.  The problem is that we've got three kinds of feeds and the text needs to reflect that.

There aren't a whole lot of changes here.  I think the bigger issue might be what I missed rather than what I caught.
Attachment #297934 - Flags: review?(myk)
(Assignee)

Comment 6

10 years ago
Created attachment 297937 [details]
Screenshot
(Assignee)

Comment 7

10 years ago
Created attachment 298294 [details] [diff] [review]
Previous patch plus some fixes.

In the previous patch I tried to remove changes that are in the patch for #303645, but ...  that makes the patch kind of funky in terms of line numbers and context.  I didn't do that this time around--I don't know which way is easier for you to review.  For the most part it's a few constants at the top and the section on writing the enclosure data to the page.  Ping me if you want me to remove that from future patches.

This patch also has some fixes for things I missed in the first one.
Attachment #297934 - Attachment is obsolete: true
Attachment #298294 - Flags: review?(myk)
Attachment #297934 - Flags: review?(myk)
(Assignee)

Updated

10 years ago
Attachment #297937 - Flags: ui-review?(beltzner)
Comment on attachment 298294 [details] [diff] [review]
Previous patch plus some fixes.

I'm neither the owner of this code nor a module owner/peer for Firefox, so I can't officially review it, but here's an unofficial review to get it in better shape for the official reviewer.  beltzner mentioned that he would find someone else to review this part.  I'll ping him about that.

>Index: browser/components/feeds/content/subscribe.xhtml

>+          <xul:description id="feedSubscriptionInfo1" />
>+          <xul:description id="feedSubscriptionInfo2" />

I think Mano's comment below this about not allowing whitespace between XUL and XHTML applies here and should be taken into account.


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

> const PREF_SELECTED_APP = "browser.feeds.handlers.application";
> const PREF_SELECTED_WEB = "browser.feeds.handlers.webservice";
> const PREF_SELECTED_ACTION = "browser.feeds.handler";
> const PREF_SELECTED_READER = "browser.feeds.handler.default";
> 
>+const PREF_VIDEO_SELECTED_APP = "browser.video.feeds.handlers.application";
>+const PREF_VIDEO_SELECTED_WEB = "browser.video.feeds.handlers.webservice";
>+const PREF_VIDEO_SELECTED_ACTION = "browser.video.feeds.handler";
>+const PREF_VIDEO_SELECTED_READER = "browser.video.feeds.handler.default";
>+
>+const PREF_AUDIO_SELECTED_APP = "browser.audio.feeds.handlers.application";
>+const PREF_AUDIO_SELECTED_WEB = "browser.audio.feeds.handlers.webservice";
>+const PREF_AUDIO_SELECTED_ACTION = "browser.audio.feeds.handler";
>+const PREF_AUDIO_SELECTED_READER = "browser.audio.feeds.handler.default";

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


>+function getPrefAppForType(t) {
>+function getPrefWebForType(t) {
>+function getPrefActionForType(t) {
>+function getPrefReaderForType(t) {

In these functions, don't use brackets for one-line conditional blocks, and don't use "else" after "return" per the JavaScript Style Guide.

Also, nit: these might be a bit simpler as switches.  They might be even simpler as lookup tables, provided they accounted for all types defined by nsIFeed.  Not sure that's worth it.


>-              var feed = result.doc.QueryInterface(Ci.nsIFeed);
>-              if (feed.type == Ci.nsIFeed.TYPE_FEED &&
>-                  wccr.getAutoHandler(TYPE_MAYBE_FEED)) {
>+              if ((feed.type == Ci.nsIFeed.TYPE_FEED &&
>+                      wccr.getAutoHandler(TYPE_MAYBE_FEED)) ||
>+                  (feed.type == Ci.nsIFeed.TYPE_VIDEO &&
>+                      wccr.getAutoHandler(TYPE_MAYBE_VIDEO_FEED)) ||
>+                  (feed.type == Ci.nsIFeed.TYPE_AUDIO &&
>+                      wccr.getAutoHandler(TYPE_MAYBE_AUDIO_FEED))) {

Nit: line up multiple conditionals, i.e.:

              if ((feed.type == Ci.nsIFeed.TYPE_FEED &&
                   wccr.getAutoHandler(TYPE_MAYBE_FEED)) ||
                  (feed.type == Ci.nsIFeed.TYPE_VIDEO &&
                   wccr.getAutoHandler(TYPE_MAYBE_VIDEO_FEED)) ||
                  (feed.type == Ci.nsIFeed.TYPE_AUDIO &&
                   wccr.getAutoHandler(TYPE_MAYBE_AUDIO_FEED))) {


>@@ -340,28 +396,28 @@ var FeedResultService = {
>   /**
>    * See nsIFeedService.idl
>    */
>   forcePreviewPage: false,
>   
>   /**
>    * See nsIFeedService.idl
>    */
>-  addToClientReader: function FRS_addToClientReader(spec, title, subtitle) {
>+  addToClientReader: function FRS_addToClientReader(spec, feedtype, title, subtitle) {

addToClientReader is an XPCOM method, so you need to update the interface method it implements in nsIFeedResultService.idl when changing its behavior.  Thus you should probably put the type parameter at the end of the parameter list to make it easier for consumers to update their code and make sure you update the UUID of the interface when you change it.

Also, while you're in here, could you update those two comments (and the three others that are incorrect in the same way) to read "See nsIFeedResultService.idl"?


>+// FIXME - is this right?  we should register two more conversions--one
>+// for each of the new maybe-feed mime types?

Yes, I think it's both right and necessary; what happens if you leave it out?


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

>+  /**
>+   * Returns the label in subscribe.properties for the name of this thing 
>+   * we're previewing.
>+   */
>+  _getPropertyName: function FW__getPropertyName() {
>+    var feedtype = this._getFeedType();
>+    if (feedtype == Ci.nsIFeed.TYPE_VIDEO) {
>+      return "videofeed";
>+    } else if (feedtype == Ci.nsIFeed.TYPE_AUDIO) {
>+      return "audiofeed";
>+    }
>+    return "webfeed";
>+  },

Hmm, seems like these property names should be consistent with the ones you're adding to preferences.properties in the patch on bug 400061.


>   /**
>+   * Takes a FeedEntry with enclosures, generates the XUL code to represent
>+   * them, and returns that.

Nit: it looks like it's actually generating HTML code.


>+    for (var i_enc = 0; i_enc < entry.enclosures.length; ++i_enc) {
>+      var enc = entry.enclosures.queryElementAt(i_enc, Ci.nsIWritablePropertyBag2);
>+      var enclosureDiv = this._document.createElementNS(HTML_NS, "div");
>+      enclosureDiv.setAttribute("class", "enclosure");
>+
>+      if (!(enc.hasKey("url"))) 
>+        continue;

Nit: it seems like this conditional should be before the statement that defines the enclosureDiv variable, since the variable doesn't get used if the conditional is true.


>+      if (enc.hasKey("length") && /^[0-9]+$/.test(enc.get("length"))) {
>+        var enc_size = 0 + enc.get("length");

I'm not sure if it makes any difference, but folks tend to use parseInt for this.

Also, it looks like we have some code that does this unit conversion for the downloads dialog <http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/downloads/content/downloads.js#980>.  We should use the same code here (copying it over or factoring it out into a JS module).


>-  _setAlwaysUseCheckedState: function FW__setAlwaysUseCheckedState() {
>+  _setAlwaysUseCheckedState: function FW__setAlwaysUseCheckedState(feedtype) {

Nit: here and elsewhere, I would call this "feedType" for consistency with other variable names.


>-      if (feedHeader)
>+      if (feedHeader) {
>+// FIXME - is this icky?  it's changing the text at the top of the
>+// subscribe preview page.  is there a better way to do this?
>         feedHeader.setAttribute("firstrun", "true");
>+        var feedinfo1 = this._document.getElementById("feedSubscriptionInfo1")
>+        feedinfo1.setAttribute("value", this._getFormattedString("feedsubscription1",
>+                                             [this._getString(this._getPropertyName())]));
>+
>+        var feedinfo2 = this._document.getElementById("feedSubscriptionInfo2")
>+        feedinfo2.setAttribute("value", this._getFormattedString("feedsubscription2",
>+                                             [this._getString(this._getPropertyName())]));
>+      }

I don't see a better way, but you should set the "firstrun" attribute after changing the text so that the entire thing appears all at once (setting the "firstrun" attribute unhides the text per CSS rules in subscribe.css).


>Index: browser/locales/en-US/chrome/browser/feeds/subscribe.properties

>-alwaysUse=Always use %S to subscribe to feeds
>+alwaysUse=Always use %S to subscribe to %S feeds

Now that this string contains two replacement tokens, it should use a localization note (like the one you wrote for enclosureSizeText) to explain what each token represents.
Attachment #298294 - Flags: review?(myk) → review-
(In reply to comment #8)
> (From update of attachment 298294 [details] [diff] [review])
> I'm neither the owner of this code nor a module owner/peer for Firefox, so I
> can't officially review it, but here's an unofficial review to get it in 
> better shape for the official reviewer.  beltzner mentioned that he would find
> someone else to review this part.  I'll ping him about that.

I chatted with mconnor on IRC, and I'm going to review iterations of this patch until I'm satisfied, after which mconnor or sayrer will do a final review.
Comment on attachment 297937 [details]
Screenshot

uir+ for the use of "podcast" and "video podcast" in the top section of the feed preview.

nit: "[ ] Always use {whatever} to subscribe to podcasts" would be better, if it isn't too tough ...
Attachment #297937 - Flags: ui-review?(beltzner) → ui-review+
(Assignee)

Comment 11

10 years ago
Created attachment 299342 [details] [diff] [review]
Patch for strings.

Flagging Beltzner for ui-review for this since he hasn't posted a + yet.

I was able to modify the code such that all the strings are additive--there aren't any changed strings.  Landing the strings patch by itself should be safe.
Attachment #299342 - Flags: ui-review?(beltzner)
(Assignee)

Comment 12

10 years ago
Created attachment 299343 [details] [diff] [review]
Previous patch 298294 with changes based on myk's suggestions.

This has changes for all the suggestions myk made.  The required strings are separated into a different patch, but this patch includes changes to subscribe.dtd to remove things that won't be used anymore once this patch has landed.
Attachment #298294 - Attachment is obsolete: true
Attachment #299343 - Flags: review?(myk)
(Assignee)

Comment 13

10 years ago
Created attachment 299360 [details]
Screenshot of the revised look with Beltzner's comments factored in.
Attachment #297937 - Attachment is obsolete: true
(Assignee)

Comment 14

10 years ago
Created attachment 299362 [details] [diff] [review]
Previous patch (299343) with a bugfix.
Attachment #299343 - Attachment is obsolete: true
Attachment #299362 - Flags: review?(myk)
Attachment #299343 - Flags: review?(myk)
(Reporter)

Comment 15

10 years ago
Will: do we have support for different icons for podcast and video podcast in the preview page?  Even if they currently use the same files, we can drop in the new icons once we have them produced.
(Assignee)

Comment 16

10 years ago
Alex: I'm not sure what you mean.  Are you saying that the 64x64 feed icon to the left of the "Subscribe to this ...  with ..." text should change depending on whether the feed is a feed, video podcast or audio podcast?  If that's what you're asking, then no, this current patch doesn't change that.

I don't think that's a big change, though.  Should be pretty straight-forward if you want me to do it.
(Reporter)

Comment 17

10 years ago
yeah, 32x32 icon directly to the left of "Subscribe to this" we are working on coming up with standard icons for podcasts and video podcasts, so we would like theme placed on the preview page instead of the traditional Web feed icon.  The plan is for these to also match the 16x16 icons used in the application prefpane for podcast and video podcast (I'll follow up on that in bug 400061).
(Assignee)

Comment 18

10 years ago
Created attachment 299365 [details] [diff] [review]
Previous patch (299362) plus this one changes the feed icon based on the feed type per Alex's request (comments 15 and 17). 

The code switches the class for the feedHeader element in order to switch the icon which is set up as a background.  It works and I think it's a small change, but I'm not sure if that's klugy or not (not sure how to spell klugy either).

It uses the same icons as the changes in the applications preferences pane in 400061.
Attachment #299362 - Attachment is obsolete: true
Attachment #299365 - Flags: review?(myk)
Attachment #299362 - Flags: review?(myk)
Comment on attachment 299342 [details] [diff] [review]
Patch for strings.

r+a=beltzner for 1.9
Attachment #299342 - Flags: ui-review?(beltzner)
Attachment #299342 - Flags: ui-review+
Attachment #299342 - Flags: approval1.9+
Keywords: checkin-needed
(Assignee)

Updated

10 years ago
Assignee: nobody → will.guaraldi
Created attachment 299381 [details] [diff] [review]
strings changes as checked in

CVS complained "malformed patch at line 42:" about the strings patch in this bug, with line 42 being the last line of the file and looking perfectly normal (I thought perhaps there was an extra newline, but deleting it resulted in "unexpected end of file in patch".

In any case, the patch conflicted with the patch for bug 303645, which I had previously checked in, so I spun a new version of the patch which is identical in what it changes but applied cleanly to the tree.  I have checked it 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.9; previous revision: 1.8
done
Attachment #299342 - Attachment is obsolete: true
Sorry to bother, but how can a comment like this be useful for localizers?

# "This is a "xyz" of frequently changing content on this site."
feedsubscription1=This is a "%S" of frequently changing content on this site.

What is "XYZ"?

Many languages have female and male words, and "this" and "a" must match this gender. So we have two possible situations: 
* %S is a fixed word, so we can choose one gender and it's ok
* %S changes, so we have a problem and we need to change the sentence's structure to avoid errors (not sure if this it's possible, I need to know what "xyz" could be)
(note: not negating your l10n note issue...)

To answer your question though..

that "xyz" (and the %S in "feedsubscription2" as well) is one of:
 	
webFeed=feed
videoPodcastFeed=video podcast
audioPodcastFeed=podcast

Comment 23

10 years ago
I'd like to double Francesco's Comment #21.

For Lithuanian, such dynamic building of a sentences isn't suitable, because, for example in these lines:

feedsubscription1=This is a "%S" of frequently changing content on this site.
feedsubscription2=You can subscribe to this %S to receive updates when this content changes.

The cases of %S must be different.

My suggestion is to create 6 similar, but full strings instead of 5 partial. For example:
feedSubscription=This is a feed of frequently changing content on this site.
feedSubscriptionAudioPodcast=This is a podcast of frequently changing content on this site.
feedSubscriptionVideoPodcast=This is a video podcast of frequently changing content on this site.
feedSubscription2=You can subscribe to this feed to receive updates when this content changes.
feedsubscriptionAudioPodcast2=You can subscribe to this podcast to receive updates when this content changes.
feedsubscriptionVideoPodcast2=You can subscribe to this video podcast to receive updates when this content changes.

Comment 24

10 years ago
(continuing Comment #23)
Btw, my suggested scheme is already in use just a few lines above:
alwaysUseForFeeds=Always use %S to subscribe to feeds.
alwaysUseForPodcasts=Always use %S to subscribe to podcasts.
alwaysUseForVideoPodcasts=Always use %S to subscribe to video podcasts.

Please do not assume that English grammar applies to all languages. ;)

Also, you may probably want to decide upon whether you call podcasts AudioPodcasts or simply Podcasts in entities (I'm talking about "alwaysUseForPodcasts" and "alwaysUseForVideoPodcasts" vs. "audioPodcastFeed" and "videoPodcastFeed").

Comment 25

10 years ago
I agree with Rimas about using full strings here. The %S in feedsubscription1 is an indefinite noun, while it is definite in feedsubscription2, requiring a different ending in some languages. Luckily for sv-SE, all three feed types have the same gender so we can add the right ending directly to %S in feedsubscription2. Other locales may not be so lucky.
(Assignee)

Comment 26

10 years ago
I really apologize--I should have caught the feedsubscription1/2 issues when I fixed the alwaysUse lines after Pike said composition in those cases was a terrible idea.

I'll roll up a new set of patches.
(Assignee)

Comment 27

10 years ago
Created attachment 299457 [details] [diff] [review]
New strings patch against subscribe.properties 1.9 fixing localization issues

This fixes the localization issues discussed in the last 10 comments or so.  Instead of using composition for those sentences, the sentences are there in full just like subscribeUsing* .  Additionally, I fixed some of the property names so that they're consistent.
Attachment #299457 - Flags: ui-review?(beltzner)

Comment 28

10 years ago
(In reply to comment #27)

Probably not my business, but why would you use quotes in feedSubscription*1 and not use them elsewhere (including feedSubscription*2)?

(Assignee)

Comment 29

10 years ago
I'm just maintaining the structure of what was previously there.  You can see it in subscribe.dtd.

If the quotes look silly, I can change it.

Comment 30

10 years ago
Well, my suggestion is to remove them, but well, I don't know the motivation behind adding them (and if there was any), so it's not a decision I should dictate.

I haven't added used them in the translation I maintain however.
Comment on attachment 299365 [details] [diff] [review]
Previous patch (299362) plus this one changes the feed icon based on the feed type per Alex's request (comments 15 and 17). 

Almost there!  Most of my review comments are nits.


>Index: browser/components/feeds/content/subscribe.xhtml

>         <div id="feedIntroText">
>-          <p id="feedSubscriptionInfo1">
>-            &feedSubscriptionInfo1a;<strong>&feedName;</strong>&feedSubscriptionInfo1b;
>-          </p>
>-          <p id="feedSubscriptionInfo2">&feedSubscriptionInfo2;</p>
>+          <xul:description id="feedSubscriptionInfo1" /><xul:description id="feedSubscriptionInfo2" />
>         </div>

There also needs to be no whitespace between the opening and closing div tags and the XUL tags they enclose, i.e.:

        <div id="feedIntroText"
          ><xul:description id="feedSubscriptionInfo1" 
          /><xul:description id="feedSubscriptionInfo2" 
        /></div>


>-              ><xul:description id="subscribeUsingDescription">&subscribeUsing;</xul:description
>+              ><xul:description id="subscribeUsingDescription"></xul:description
>               ><xul:menulist id="handlersMenuList" aaa:labelledby="subscribeUsingDescription"

Since the tag is now empty, don't use a closing tag, just do:

              ><xul:description id="subscribeUsingDescription"
              /><xul:menulist id="handlersMenuList" aaa:labelledby="subscribeUsingDescription"


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

>+        var feed = result.doc.QueryInterface(Ci.nsIFeed);
...
>         if (handler != "ask") {
...
>           switch (handler) {
...
>             case "client":
>               try {
>                 var feed = result.doc.QueryInterface(Ci.nsIFeed);
>                 var title = feed.title ? feed.title.plainText() : "";
>                 var desc = feed.subtitle ? feed.subtitle.plainText() : "";
>-                feedService.addToClientReader(result.uri.spec, title, desc);
>+                feedService.addToClientReader(result.uri.spec, title, desc, feed.type);

Remove the declaration of the "feed" variable here as well now that you're declaring it earlier in an enclosing block.


>-  addToClientReader: function FRS_addToClientReader(spec, title, subtitle) {
>+  addToClientReader: function FRS_addToClientReader(spec, title, subtitle, feedtype) {

Nit: make this new parameter CamelCase (feedType) here and in the interface.


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

>+function getPrefAppFromType(t) {
...
>+function getPrefWebFromType(t) {
...
>+function getPrefReaderFromType(t) {
...
>+function getPrefActionFromType(t) {
>+  if (t == Ci.nsIFeed.TYPE_VIDEO) {
>+    return PREF_VIDEO_SELECTED_ACTION;
>+  } else if (t == Ci.nsIFeed.TYPE_AUDIO) {
>+    return PREF_AUDIO_SELECTED_ACTION;
>+  }
>+  return PREF_SELECTED_ACTION;
>+}

Nit: since you've switched from if-else to switch-case in FeedConverter.js, switch to switch-case here as well.

Also, order these getPrefAppFromType, getPrefWebFromType, getPrefActionFromType, getPrefReaderFromType for consistency with their order in FeedConverter.js and the order in which the preference constants are defined in both files.


>@@ -158,16 +236,17 @@ FeedWriter.prototype = {
>                       createBundle(URI_BUNDLE);
>     }
>     return this.__bundle;
>   },
> 
>   _getFormattedString: function FW__getFormattedString(key, params) {
>     return this._bundle.formatStringFromName(key, params, params.length);
>   },
>+
>   
>   _getString: function FW__getString(key) {
>     return this._bundle.GetStringFromName(key);
>   },

Any particular reason to add this newline here?


>   /**
>+   * Returns the feed type.
>+   */
>+  __feedtype: null,
>+  _getFeedType: function FW__getFeedType() {
>+    if (this.__feedtype != null) {
>+      return this.__feedtype;
>+    }

Nit: no need for parentheses around this one-line block.


>+    try {
>+      // grab the feed because it's got the feed.type in it.
>+      var container = this._getContainer();
>+      var feed = container.QueryInterface(Ci.nsIFeed);
>+      this.__feedtype = feed.type;
>+      return feed.type;
>+    } catch (ex) { }
>+    return Ci.nsIFeed.TYPE_FEED;
>+  },

Nit: put a blank line before the final return for consistency with the space after the initial one and to distinguish it from the try block.


>+  /**
>+   * Maps a feed type to a maybe-feed mimetype.
>+   */
>+  _getMimeTypeForFeedType: function FW__getMimeTypeForFeedType() {
>+    switch (this._getFeedType()) {
>+      case Ci.nsIFeed.TYPE_VIDEO:
>+        return TYPE_MAYBE_VIDEO_FEED;
>+
>+      case Ci.nsIFeed.TYPE_AUDIO:
>+        return TYPE_MAYBE_AUDIO_FEED;
>+
>+      default:
>+        return TYPE_MAYBE_FEED;
>+     }
>+   },

Nit: move both closing parentheses left one space so they line up with their opening block statements.


>   _writeFeedContent: function FW__writeFeedContent(container) {
>     // Build the actual feed content
>     var feedContent = this._document.getElementById("feedContent");
>     var feed = container.QueryInterface(Ci.nsIFeed);
>+
>     
>     for (var i = 0; i < feed.items.length; ++i) {
>       var entry = feed.items.queryElementAt(i, Ci.nsIFeedEntry);
>       entry.QueryInterface(Ci.nsIFeedContainer);

Any particular reason to add this newline here?


>+  _setSubscribeUsingLabel: function FW__setSubscribeUsingLabel() {
>+    var subscribeusing = this._document.getElementById("subscribeUsingDescription");
>+    var stringlabel = null;

Nit: name these using CamelCase, i.e. subscribeUsing and stringLabel.


>   // nsIDomEventListener
>   handleEvent: function(event) {
...
>+    var feedType = this._getFeedType();
>+
...
>-            this._setSelectedHandler();
>+            this._setSelectedHandler(feedType);

Nit: it seems unnecessary to define a variable here when it's only used once; it would be simpler to just inline the call to _getFeedType.


>+    prefs.addObserver(PREF_AUDIO_SELECTED_ACTION, this, false);
>+    prefs.addObserver(PREF_AUDIO_SELECTED_READER, this, false);
>+    prefs.addObserver(PREF_AUDIO_SELECTED_WEB, this, false);
>+    prefs.addObserver(PREF_AUDIO_SELECTED_APP, this, false);
>+
>+    prefs.addObserver(PREF_VIDEO_SELECTED_ACTION, this, false);
>+    prefs.addObserver(PREF_VIDEO_SELECTED_READER, this, false);
>+    prefs.addObserver(PREF_VIDEO_SELECTED_WEB, this, false);
>+    prefs.addObserver(PREF_VIDEO_SELECTED_APP, this, false);
...
>+    prefs.removeObserver(PREF_AUDIO_SELECTED_ACTION, this);
>+    prefs.removeObserver(PREF_AUDIO_SELECTED_READER, this);
>+    prefs.removeObserver(PREF_AUDIO_SELECTED_WEB, this);
>+    prefs.removeObserver(PREF_AUDIO_SELECTED_APP, this);
>+
>+    prefs.removeObserver(PREF_VIDEO_SELECTED_ACTION, this);
>+    prefs.removeObserver(PREF_VIDEO_SELECTED_READER, this);
>+    prefs.removeObserver(PREF_VIDEO_SELECTED_WEB, this);
>+    prefs.removeObserver(PREF_VIDEO_SELECTED_APP, this);
...
>+        case PREF_AUDIO_SELECTED_ACTION:
>+        case PREF_VIDEO_SELECTED_ACTION:
>+          this._setAlwaysUseCheckedState(feedType);

Nit: order these with the video item(s) first for consistency with code elsewhere.


>Index: browser/themes/gnomestripe/browser/feeds/subscribe.css
> #feedHeader[dir="rtl"] {
>   background-position: 100% 10%;
> }
> 
>+.feedBackground {
>+  background: url("chrome://browser/skin/feeds/feedIcon.png") 0% 10% no-repeat InfoBackground;
>+}

I'm not sure which selector has higher precedence here.  It may well still be the #feedHeader[dir="rtl"] selector, but make sure you test in right-to-left mode that the icon changes position.
Attachment #299365 - Flags: review?(myk) → review-
I don't see anything ready to land here. Please re-add the checkin-needed keyword if that is incorrect.
Keywords: checkin-needed
Comment on attachment 299457 [details] [diff] [review]
New strings patch against subscribe.properties 1.9 fixing localization issues

uir+ on the string portion, but there's no point in landing this now as we're already past string freeze; might as well wait for the entire patch to go in at once.
Attachment #299457 - Flags: ui-review?(beltzner) → approval1.9-
(Assignee)

Comment 34

10 years ago
Created attachment 299518 [details] [diff] [review]
Patch 299365 with myk's suggestions and the strings changes in 299457.

Myk--I adjusted the selectors so that it's a little more specific and checked the ltr vs. rtl using Philor's suggested attachment patch--it looks ok.

I folded the new strings changes into the patch per Beltzner's suggestion.
Attachment #299365 - Attachment is obsolete: true
Attachment #299457 - Attachment is obsolete: true
Attachment #299518 - Flags: review?(myk)
Comment on attachment 299518 [details] [diff] [review]
Patch 299365 with myk's suggestions and the strings changes in 299457.

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

>+const TYPE_MAYBE_AUDIO_FEED = "application/vnd.mozilla.maybe.audio.feed";
>+const TYPE_MAYBE_VIDEO_FEED = "application/vnd.mozilla.maybe.video.feed";
...
>+const PREF_AUDIO_SELECTED_APP = "browser.audioFeeds.handlers.application";
>+const PREF_AUDIO_SELECTED_WEB = "browser.audioFeeds.handlers.webservice";
>+const PREF_AUDIO_SELECTED_ACTION = "browser.audioFeeds.handler";
>+const PREF_AUDIO_SELECTED_READER = "browser.audioFeeds.handler.default";
>+
>+const PREF_VIDEO_SELECTED_APP = "browser.videoFeeds.handlers.application";
>+const PREF_VIDEO_SELECTED_WEB = "browser.videoFeeds.handlers.webservice";
>+const PREF_VIDEO_SELECTED_ACTION = "browser.videoFeeds.handler";
>+const PREF_VIDEO_SELECTED_READER = "browser.videoFeeds.handler.default";

Nit: put the video entries before the audio entries for consistency with placement elsewhere.


>Index: browser/themes/gnomestripe/browser/feeds/subscribe.css

>+#feedHeader[class="feedBackground"] {
>+  background: url("chrome://browser/skin/feeds/feedIcon.png") 0% 10% no-repeat InfoBackground;
>+}
>+
>+#feedHeader[class="videoPodcastBackground"] {
>+  background: url("chrome://browser/skin/feeds/videoFeedIcon.png") 0% 10% no-repeat InfoBackground;
>+}
>+
>+#feedHeader[class="audioPodcastBackground"] {
>+  background: url("chrome://browser/skin/feeds/audioFeedIcon.png") 0% 10% no-repeat InfoBackground;
>+}

The worry was that these rules would be more specific than the rtl rule, and here you've made them even more specific.  Better to revert to the old rules, I think, if they worked fine in rtl mode.


> }
> 
>+
> #feedIntroText {

Nit: potential unnecessary newline.


Otherwise this looks great.  Note that I did not review the parts of this version of the patch that are from the patch in bug 303465, I just reviewed the parts that are native to this patch.

-> sayrer for final review
Attachment #299518 - Flags: review?(sayrer)
Attachment #299518 - Flags: review?(myk)
Attachment #299518 - Flags: review+
(Assignee)

Comment 36

10 years ago
Comment on attachment 299518 [details] [diff] [review]
Patch 299365 with myk's suggestions and the strings changes in 299457.

This patch has some overlap with the patch in 303645 in FeedWriter.js.

I can remove that if you need.

Regardless 303645 has to land first.  After that I can roll a new version of this patch that's easier to apply.
(Assignee)

Comment 37

10 years ago
Created attachment 299595 [details] [diff] [review]
Patch 299518 with tweaks suggested by myk.

Sorry about that.  I forgot where I was last night just before I went to bed.

This patch has the two minor tweaks myk suggested in his last review.

And for review purposes, it has the strings changes that need to land.

It also has some overlap with the patch from bug 303645 in FeedWriter.js (303645 includes other changes).  303645 needs to land before this patch lands.  When 303645 lands, I can re-diff this patch and make a more accurate version, but it wouldn't be hard for someone to figure out the two or three sections that need to be fixed.
Attachment #299518 - Attachment is obsolete: true
Attachment #299595 - Flags: review?(myk)
Attachment #299518 - Flags: review?(sayrer)
Comment on attachment 299595 [details] [diff] [review]
Patch 299518 with tweaks suggested by myk.

Looks great, r=myk
Attachment #299595 - Flags: review?(myk) → review+
Comment on attachment 299595 [details] [diff] [review]
Patch 299518 with tweaks suggested by myk.

Looks great, r=myk
Comment on attachment 299595 [details] [diff] [review]
Patch 299518 with tweaks suggested by myk.

Looks great, r=myk
(Assignee)

Updated

10 years ago
Attachment #299595 - Flags: review?(sayrer)

Comment 41

10 years ago
Comment on attachment 299595 [details] [diff] [review]
Patch 299518 with tweaks suggested by myk.

r=sayrer. Please land bug 303645 first, and then produce a new diff here for checkin.
Attachment #299595 - Flags: review?(sayrer) → review+
(Assignee)

Comment 42

10 years ago
Created attachment 300241 [details] [diff] [review]
Patch ready for checkin

Carrying over the r+ from myk and sayrer.

This patch is ready for checkin.
Attachment #299595 - Attachment is obsolete: true
Attachment #300241 - Flags: review+
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment on attachment 300241 [details] [diff] [review]
Patch ready for checkin

a=beltzner
Attachment #300241 - Flags: approval1.9+
Checking in browser/locales/en-US/chrome/browser/feeds/subscribe.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/feeds/subscribe.dtd,v  <--  subscribe.dtd
new revision: 1.6; previous revision: 1.5
done
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.10; previous revision: 1.9
done
Checking in browser/components/feeds/content/subscribe.xhtml;
/cvsroot/mozilla/browser/components/feeds/content/subscribe.xhtml,v  <--  subscribe.xhtml
new revision: 1.19; previous revision: 1.18
done
Checking in browser/components/feeds/public/nsIFeedResultService.idl;
/cvsroot/mozilla/browser/components/feeds/public/nsIFeedResultService.idl,v  <--  nsIFeedResultService.idl
new revision: 1.10; previous revision: 1.9
done
Checking in browser/components/feeds/src/FeedConverter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedConverter.js,v  <--  FeedConverter.js
new revision: 1.31; previous revision: 1.30
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.19; previous revision: 1.18
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.20; previous revision: 1.19
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.19; previous revision: 1.18
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta3
(Assignee)

Comment 45

10 years ago
Created attachment 300442 [details] [diff] [review]
File missing from patch 3000241 that was in 299595.

I missed a file in the patch that was checked in that was in the previous version of the patch.

I'm carrying over the r+ from 299595.

This needs to be checked in for the functionality in 400064 to work correctly.
Attachment #300442 - Flags: review+
Attachment #300442 - Flags: approval1.9?
Attachment #300442 - Flags: approval1.9b3?
Comment on attachment 300442 [details] [diff] [review]
File missing from patch 3000241 that was in 299595.

a=beltzner for missing bit of code that was supposed to land last night
Attachment #300442 - Flags: approval1.9b3? → approval1.9b3+
Comment on attachment 300442 [details] [diff] [review]
File missing from patch 3000241 that was in 299595.

Checking in browser/components/feeds/src/FeedWriter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedWriter.js,v  <--  FeedWriter.js
new revision: 1.50; previous revision: 1.49
done
Attachment #300442 - Flags: approval1.9?
A l10n note should be added to the feedLiveBookmarks entity, as it is not clear from the DTD file anymore, that this entity is a part of a sentence starting with "Subscribe using". This note should clearly state what other entities/properties are used to make the sentence.

This is important, as in languages other than English noun declension may alternate this text, i.e. "Live bookmarks" are "Dynamiczne zakładki" in Polish, but "Subscribe using live bookmarks" is "Subskrybuj za pomocą *dynamicznych zakładek*". 
Depends on: 415245

Updated

10 years ago
Flags: blocking-firefox3?

Updated

10 years ago
Depends on: 417913
You need to log in before you can comment on or make changes to this bug.