Last Comment Bug 562227 - When using the URL referenced the feed gets cut into two pieces making the feed invalid
: When using the URL referenced the feed gets cut into two pieces making the fe...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Feed Reader (show other bugs)
: unspecified
: All All
: -- major (vote)
: Thunderbird 24.0
Assigned To: alta88
:
Mentors:
http://houston.craigslist.org/search/...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-27 17:09 PDT by M Prindle
Modified: 2013-06-25 05:20 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.98 KB, patch)
2013-05-18 09:54 PDT, alta88
mkmelin+mozilla: review+
Details | Diff | Splinter Review
updated (3.70 KB, patch)
2013-05-23 07:29 PDT, alta88
alta88: review+
Details | Diff | Splinter Review

Description M Prindle 2010-04-27 17:09:56 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.9) Gecko/20100317 Lightning/1.0b1 Thunderbird/3.0.4

When I add this url "http://houston.craigslist.org/search/sss?query=google+nexus+(N1|one)+-(broken)+-(NOT+CHINA+KNOCK+OFF)+-(832-531-5184)+-(Dr+Cell+Phone)&format=rss" into my RSS feed TB will check the feed once, but when I close and reopen TB the feed is cut into two entries.  The first entry is "http://houston.craigslist.org/search/sss?query=google+nexus+(N1" and the 2nd entry is "one)+-(broken)+-(NOT+CHINA+KNOCK+OFF)+-(832-531-5184)+-(Dr+Cell+Phone)&format=rss".  It appears the | in the feed is breaking the url in TB.  On craigslist the | is use to OR a statement together.

The feeds are structured like this
Craigslist (Folder)
 - Feed 1
 - Feed 2
 - Feed 3

Also, as a part of this bug once the feed is broken in to two pieces I can no longer delete the feed.  I have to go to "C:\Users\username\AppData\Roaming\Thunderbird\Profiles\ve5l8jty.user\Mail\News & Blogs" and delete everything in the folder.  I'm sure there's a way to figure out what file to delete, but I'm unable to figure it out.

Reproducible: Always

Steps to Reproduce:
1.Subscribe to the feed referenced above
2.Close TB
3.Open TB and the feed is broke
Actual Results:  
1.Feed subscribes and updates once
2. After open & close the feed is broke

Expected Results:  
1.Subscribe the feed
2.Open TB and the feed updates
Comment 1 alta88 2012-04-21 09:45:05 PDT
yes, Tb uses | as a delimiter for multiple feeds subscribed to a folder.
Comment 2 alta88 2013-05-18 09:54:40 PDT
Created attachment 751366 [details] [diff] [review]
patch
Comment 3 Magnus Melin 2013-05-20 12:03:59 PDT
Comment on attachment 751366 [details] [diff] [review]
patch

Review of attachment 751366 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/extensions/newsblog/content/utils.js
@@ +270,5 @@
>      let feedurls = aFolder.getStringProperty("feedUrl");
> +    if (feedurls) {
> +      feedurls = feedurls.replace(this.kFeedUrlDelimiter + "http",
> +                                  "\x01http", "g");
> +      feedurls = feedurls.replace(this.kFeedUrlDelimiter + "file://",

we don't allow file:// feeds, at least yet. 
so you should be able to simply split on "|http", no? that should also be forward-compatible (for downgrade scenarios)

@@ +395,5 @@
> +                                        "\x01file://", "g");
> +      curFeedUrls = curFeedUrls.split("\x01");
> +    }
> +    else
> +      curFeedUrls = [];

if one if-else has braces, they all should is the style rule.

(though if yo split on "|http" you don't need an if-else at all)
Comment 4 alta88 2013-05-20 13:42:02 PDT
(In reply to Magnus Melin from comment #3)
> Comment on attachment 751366 [details] [diff] [review]
> patch
> 
> Review of attachment 751366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/extensions/newsblog/content/utils.js
> @@ +270,5 @@
> >      let feedurls = aFolder.getStringProperty("feedUrl");
> > +    if (feedurls) {
> > +      feedurls = feedurls.replace(this.kFeedUrlDelimiter + "http",
> > +                                  "\x01http", "g");
> > +      feedurls = feedurls.replace(this.kFeedUrlDelimiter + "file://",
> 
> we don't allow file:// feeds, at least yet. 
> so you should be able to simply split on "|http", no? that should also be
> forward-compatible (for downgrade scenarios)
> 

yes, but it is possible to override the isValidScheme function, which i do locally and it's pretty important for testing, so i'd like to have this handled up front.


> @@ +395,5 @@
> > +                                        "\x01file://", "g");
> > +      curFeedUrls = curFeedUrls.split("\x01");
> > +    }
> > +    else
> > +      curFeedUrls = [];
> 
> if one if-else has braces, they all should is the style rule.
> 
> (though if yo split on "|http" you don't need an if-else at all)

ok.  (that's how i had it initially but needed to include file://.)
Comment 5 Magnus Melin 2013-05-23 03:07:43 PDT
Comment on attachment 751366 [details] [diff] [review]
patch

Review of attachment 751366 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/extensions/newsblog/content/utils.js
@@ +389,5 @@
>  
>      let curFeedUrls = aFolder.getStringProperty("feedUrl");
> +    if (curFeedUrls) {
> +      curFeedUrls = curFeedUrls.replace(this.kFeedUrlDelimiter + "http",
> +                                        "\x01http", "g");

Maybe do this for http:// and https:// cases separately? For edge cases.
Comment 6 alta88 2013-05-23 07:29:07 PDT
Created attachment 753281 [details] [diff] [review]
updated


yes, better to be stricter.  converted to a function as well.
Comment 7 Ryan VanderMeulen [:RyanVM] 2013-05-28 10:18:08 PDT
https://hg.mozilla.org/comm-central/rev/de4183a854a0
Comment 8 M Prindle 2013-05-28 11:58:04 PDT
Thanks for getting this bug fixed.  I can now go back to using the RSS reader in TB.

Note You need to log in before you can comment on or make changes to this bug.