Last Comment Bug 709247 - Fix to sync feeds database on folderpane moves
: Fix to sync feeds database on folderpane moves
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Feed Reader (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 12.0
Assigned To: alta88
:
:
Mentors:
: 257700 311697 (view as bug list)
Depends on: 705504
Blocks: 711173
  Show dependency treegraph
 
Reported: 2011-12-09 12:16 PST by alta88
Modified: 2012-01-09 01:05 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
fix. (1.94 KB, patch)
2011-12-09 12:16 PST, alta88
iann_bugzilla: review-
Details | Diff | Splinter Review
fix2. (552 bytes, patch)
2011-12-11 20:22 PST, alta88
mozilla: review+
Details | Diff | Splinter Review
address comments. (549 bytes, patch)
2011-12-29 07:22 PST, alta88
alta88: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description alta88 2011-12-09 12:16:30 PST
Created attachment 580499 [details] [diff] [review]
fix.

review is requested from iann and neil since you were both involved in Bug 471932.  please advise who, if not you, would be the right reviewer(s) to get this and Bug 705504 moving.
Comment 1 Ian Neal 2011-12-11 12:15:12 PST
Comment on attachment 580499 [details] [diff] [review]
fix.

>diff --git a/mailnews/extensions/newsblog/content/utils.js b/mailnews/extensions/newsblog/content/utils.js
>--- a/mailnews/extensions/newsblog/content/utils.js
>+++ b/mailnews/extensions/newsblog/content/utils.js
>@@ -211,11 +211,28 @@
>   }
>   if (msgDb && msgDb.dBFolderInfo) {
>     feedurls = msgDb.dBFolderInfo.getCharProperty("feedUrl");
>-    // Clean up the feedUrl string.
>+    // Clean up the feedUrl string.  Also sync the feeds database with the
>+    // feed url's real current folder.
>     feedurls.split(kFeedUrlDelimiter).forEach(
>       function(url) {
>-        if (url && feedUrlArray.indexOf(url) == -1)
>+        if (url && feedUrlArray.indexOf(url) == -1) {
>           feedUrlArray.push(url);
>+
>+          try {
Which part of the code is going to throw?

>+            var rdf = Components.classes["@mozilla.org/rdf/rdf-service;1"]
>+                                .getService(Components.interfaces.nsIRDFService);
Isn't rdf already global in this file? (line 49 and 50)

>+            var ds = getSubscriptionsDS(aFolder.server);
Do we really need to keep getting ds, can it not be declared outside of the function and then used?

>+            var id = rdf.GetResource(url);
>+            var resource = rdf.GetResource(aFolder.URI);
>+            // Get the node for the current folder URI.
>+            var node = ds.GetTarget(id, FZ_DESTFOLDER, true);
>+            if (node)
>+              ds.Change(id, FZ_DESTFOLDER, node, resource);
>+            else
>+              addFeed(url, resource.name, resource);
We seem to be duplicating code between here and newsblog.js (see updateSubscriptionsDS - http://mxr.mozilla.org/comm-central/source/mailnews/extensions/newsblog/js/newsblog.js#236), is there not a way we could share it?
r- for the moment
Comment 2 alta88 2011-12-11 20:18:51 PST
(In reply to Ian Neal from comment #1)
> Comment on attachment 580499 [details] [diff] [review]
> fix.
> 
> >diff --git a/mailnews/extensions/newsblog/content/utils.js b/mailnews/extensions/newsblog/content/utils.js
> >--- a/mailnews/extensions/newsblog/content/utils.js
> >+++ b/mailnews/extensions/newsblog/content/utils.js
> >@@ -211,11 +211,28 @@
> >   }
> >   if (msgDb && msgDb.dBFolderInfo) {
> >     feedurls = msgDb.dBFolderInfo.getCharProperty("feedUrl");
> >-    // Clean up the feedUrl string.
> >+    // Clean up the feedUrl string.  Also sync the feeds database with the
> >+    // feed url's real current folder.
> >     feedurls.split(kFeedUrlDelimiter).forEach(
> >       function(url) {
> >-        if (url && feedUrlArray.indexOf(url) == -1)
> >+        if (url && feedUrlArray.indexOf(url) == -1) {
> >           feedUrlArray.push(url);
> >+
> >+          try {
> Which part of the code is going to throw?
>

the intent is to guarantee it doesn't.  

> >+            var rdf = Components.classes["@mozilla.org/rdf/rdf-service;1"]
> >+                                .getService(Components.interfaces.nsIRDFService);
> Isn't rdf already global in this file? (line 49 and 50)
> 

true.

> >+            var ds = getSubscriptionsDS(aFolder.server);
> Do we really need to keep getting ds, can it not be declared outside of the
> function and then used?
> 

this is done well over a dozen times already in the codebase.  imo a real fix with a proper utils object with getters is out of scope for this bug.

> >+            var id = rdf.GetResource(url);
> >+            var resource = rdf.GetResource(aFolder.URI);
> >+            // Get the node for the current folder URI.
> >+            var node = ds.GetTarget(id, FZ_DESTFOLDER, true);
> >+            if (node)
> >+              ds.Change(id, FZ_DESTFOLDER, node, resource);
> >+            else
> >+              addFeed(url, resource.name, resource);
> We seem to be duplicating code between here and newsblog.js (see
> updateSubscriptionsDS -
> http://mxr.mozilla.org/comm-central/source/mailnews/extensions/newsblog/js/
> newsblog.js#236), is there not a way we could share it?
> r- for the moment

yes, that's where i got it.  this code block is meant to run only once per folder and not again, and could be removed after a few versions.  imo, a handful of lines isn't worth it.

anyway, this section isn't important to fix the real problem.
Comment 3 alta88 2011-12-11 20:22:49 PST
Created attachment 580820 [details] [diff] [review]
fix2.
Comment 4 neil@parkwaycc.co.uk 2011-12-14 16:36:27 PST
Comment on attachment 580499 [details] [diff] [review]
fix.

> NS_IMETHODIMP nsRssIncomingServer::FolderMoveCopyCompleted(bool aMove, nsIMsgFolder *aSrcFolder, nsIMsgFolder *aDestFolder)
> {
>-  return FolderChanged(aSrcFolder, PR_FALSE);
>+  return FolderChanged(aDestFolder, PR_FALSE);
So, the point of this is to run through all the feeds in the destination, updating the data source with their new folder, in much the same way as renaming a folder does. I wonder what the right thing to do in the case of a copy (aMove is false) is though.

I see your other patch only has this change and not the JS change, does that exist only to clean up after existing moved folders?

Note that you can probably invoke UpdateSubscriptionsDS from utils.js since it seems to exist on the nsNewsBlogFeedDownloader global variable.
Comment 5 alta88 2011-12-15 07:32:20 PST
(In reply to neil@parkwaycc.co.uk from comment #4)
> Comment on attachment 580499 [details] [diff] [review]
> fix.
> 
> > NS_IMETHODIMP nsRssIncomingServer::FolderMoveCopyCompleted(bool aMove, nsIMsgFolder *aSrcFolder, nsIMsgFolder *aDestFolder)
> > {
> >-  return FolderChanged(aSrcFolder, PR_FALSE);
> >+  return FolderChanged(aDestFolder, PR_FALSE);
> So, the point of this is to run through all the feeds in the destination,
> updating the data source with their new folder, in much the same way as
> renaming a folder does. I wonder what the right thing to do in the case of a
> copy (aMove is false) is though.
> 

yes.  in fact, a move followed by a rename would have been ok.  i thought about copy, but don't think it matters, as you can't copy anywhere in the same feed account, so the account's db isn't changed.  if you copy to another feed account, it will be like a move for that account's db.  a folder copy to a non rss account won't run.

> I see your other patch only has this change and not the JS change, does that
> exist only to clean up after existing moved folders?
> 

yes, for those poor unfortunates who finally got themselves organized, with an out of sync db, but nonetheless working due to the feedUrl business, which is very fragile (and contrary to a db design that might consider the concept of transactional integrity).

it can be added here or else i also have it in another patch that fixes the UI freeze on biffs.  as mentioned it will only run once per folder where the feedUrl hasn't yet been set by setStringProperty.

> Note that you can probably invoke UpdateSubscriptionsDS from utils.js since
> it seems to exist on the nsNewsBlogFeedDownloader global variable.

unfortunately that would cause recursion, and only part of it is needed.
Comment 6 alta88 2011-12-16 07:21:38 PST
*** Bug 311697 has been marked as a duplicate of this bug. ***
Comment 7 alta88 2011-12-16 07:22:55 PST
*** Bug 257700 has been marked as a duplicate of this bug. ***
Comment 8 neil@parkwaycc.co.uk 2011-12-16 15:36:38 PST
(In reply to alta88 from comment #5)
> (In reply to neil@parkwaycc.co.uk from comment #4)
> > Note that you can probably invoke UpdateSubscriptionsDS from utils.js since
> > it seems to exist on the nsNewsBlogFeedDownloader global variable.
> unfortunately that would cause recursion, and only part of it is needed.
How do you mean? updateSubsciptionsDS looks just like the code you're adding.
(Sorry for the typo and wrong source file name.)
Comment 9 alta88 2011-12-17 07:33:58 PST
per Bug 705504 checkin, updateSubsciptionsDS calls getFeedUrlsInFolder..

i propose this bug be solely about the 1 line fix patch.
Comment 10 David :Bienvenu 2011-12-28 16:06:04 PST
Comment on attachment 580820 [details] [diff] [review]
fix2.

can you use false instead of PR_FALSE?
Comment 11 alta88 2011-12-29 07:22:32 PST
Created attachment 584744 [details] [diff] [review]
address comments.


this bug should be in the same branch as bug 705504, so requesting aurora.
Comment 12 Mark Banner (:standard8, afk until Dec) 2011-12-31 10:48:54 PST
Checked in: http://hg.mozilla.org/comm-central/rev/a3b6a095274e
Comment 13 alta88 2012-01-06 11:49:41 PST
i guess this needs a checkin for aurora post approval..?
Comment 14 Mark Banner (:standard8, afk until Dec) 2012-01-06 13:38:46 PST
Yes, but please keep the milestone as the trunk version it was fixed in - a status flag will indicate fixed on branches.
Comment 15 Mark Banner (:standard8, afk until Dec) 2012-01-09 01:05:31 PST
Checked in: http://hg.mozilla.org/releases/comm-aurora/rev/e39e86f75d02

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