711173 - Fix for UI freeze on feed biff/get new messages; renovate newsblog.js for fun.

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[alta88]

Attachment: patch.

this patch attempts the following:
1) fix UI freeze on new messages using an enumerator/yield method for each feed url in each folder.
2) unsubscribe a deleted folder (deleted *and* moved to trash), depends on Bug 709247.
3) resync feeds.rdf db with feedUrl (one time deal), see Bug 709247.

along the way it:
1) now allows getting new messages to work on a folder and all its subfolders.
2) creates a FeedUtils object and moves some vars/functions there.
3) converts newsblog.js to using Services and MailServices helpers.
4) begin to replace debug with the nice log4moz logger.

useful to do?
1) convert newsblog.js to modern xpcom registration methods.

Comment 1

[Ludovic Hirlimann [:Usul]]

I don't see any test are these changes not worth having some tests ?

Comment 2

[Ludovic Hirlimann [:Usul]]

Alta do you plan to switch to the feed reader of firefox too ?

Comment 3

[alta88]

(In reply to Ludovic Hirlimann [:Usul] from comment #2)
> Alta do you plan to switch to the feed reader of firefox too ?

while it would be worthy, that's a Real Project.  imo it should be done with the idea of taking Feeds in Tb to the next level, and would need buyin and interest from moz.

Comment 4

[[e-mail]]

Comment on attachment 582030 [details] [diff] [review]
patch.

Unfortunately there are too many unrelated changes for me to be able to make sense of the whole patch at once.

>+if (Cc == undefined)
>+  var Cc = Components.classes;
>+if (Ci == undefined)
>+  var Ci = Components.interfaces;
>+if (Cr == undefined)
>+  var Cr = Components.results;
>+if (Cu == undefined)
>+  var Cu = Components.utils;
This is the sort of thing that gives Cx a bad name...

>+Cu.import("resource://gre/modules/mailServices.js");
mailServices doesn't live in the GRE.

>+  get log() {
>+    let logger = Log4Moz.getConfiguredLogger("Feeds");
>+    this.__defineGetter__("log", function() { return logger });
>+    return this.log;
>+  },
XPCOMUtils has some methods to help you do this.

>+    // Return if there are no feedUrls for the base folder in the feeds
>+    // database, the base folder has no subfolders, or the folder is in Trash.
Why if no subfolders?

>+          msgDb = folder.QueryInterface(Ci.nsIMsgLocalMailFolder)
>+                        .getDatabaseWOReparse();
I still think folder.msgDatabase is better.

Comment 5

[alta88]

Attachment: address comments.

Comment 6

[Ian Neal]

Comment on attachment 582532 [details] [diff] [review]
address comments.

Just a quick drive by:
> function loadScripts()
> {
>-  var scriptLoader =  Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
>+  var scriptLoader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
>                      .getService(Components.interfaces.mozIJSSubScriptLoader);
Any reason why you are not using Services.scriptloader or is it that Services.jsm is not available in this context?

Comment 7

[Ian Neal]

Comment on attachment 582532 [details] [diff] [review]
address comments.


>+Cu.import("resource://gre/modules/Services.jsm");
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
Doesn't the loading of Services.jsm pull in XPCOMUtils.jsm already?

Comment 8

[alta88]

(In reply to Ian Neal from comment #6)
> Comment on attachment 582532 [details] [diff] [review]
> address comments.
> 
> Just a quick drive by:
> > function loadScripts()
> > {
> >-  var scriptLoader =  Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
> >+  var scriptLoader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
> >                      .getService(Components.interfaces.mozIJSSubScriptLoader);
> Any reason why you are not using Services.scriptloader or is it that
> Services.jsm is not available in this context?

it wouldn't work using Services.  i tried it a number of way, passing in a context, etc.  no errors just no functions from imported files.  

(In reply to Ian Neal from comment #7)
> Comment on attachment 582532 [details] [diff] [review]
> address comments.
> 
> 
> >+Cu.import("resource://gre/modules/Services.jsm");
> >+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Doesn't the loading of Services.jsm pull in XPCOMUtils.jsm already?

i believe only in its own scope.  anyway, removing it makes the Subscribe dialog blow up with 'not defined'.

Comment 9

[David :Bienvenu]

I will review this over the holidays, but I just wanted to say that I think it's a really useful change.

Comment 10

[David :Bienvenu]

moving the aSubscribeMode ? inside the GetStringFromName call would be slightly cleaner:

+          this.mStatusFeedback.showStatusString(aSubscribeMode ?
+            FeedUtils.strings.GetStringFromName('subscribe-validating-feed') :
+            FeedUtils.strings.GetStringFromName('newsblog-getNewMsgsCheck'));

I'll run with the patch for a while - I subscribed to a feed w/o a problem (though I had to use the subscribe UI, not drag drop, but I've had that problem before)

Comment 11

[alta88]

(In reply to David :Bienvenu from comment #10)
> moving the aSubscribeMode ? inside the GetStringFromName call would be
> slightly cleaner:
> 
> +          this.mStatusFeedback.showStatusString(aSubscribeMode ?
> +           
> FeedUtils.strings.GetStringFromName('subscribe-validating-feed') :
> +           
> FeedUtils.strings.GetStringFromName('newsblog-getNewMsgsCheck'));
> 
GetStringFromName is a function that comes with a Services.strings bundle instance, so i'm not sure how that would work.  this existed before; do you mean breaking it out, like
string = aSubscribeMode ? GetSFN("name1") : GetSFN("name2");

> I'll run with the patch for a while - I subscribed to a feed w/o a problem

also, the one liner in bug 709247 is needed to make sure folderpane moved/deletes are happy.

a note: this bug and bug 709247 should be in the same branch as bug 705504, so the feed db resync can happen.

> (though I had to use the subscribe UI, not drag drop, but I've had that
> problem before)

i think dnd of a url on a folderpane feed folder malforms the url (adds 'Subscribe' to the end).

Comment 12

[David :Bienvenu]

No, I mean, this.mStatusFeedback.showStatusString(FeedUtils.strings.GetStringFromName(aSubscribeMode ? 'subscribe-validating-feed' : 'newsblog-getNewMsgsCheck');

Comment 13

[[e-mail]]

Comment on attachment 582532 [details] [diff] [review]
address comments.

I had another look at this and I saw a few potential issues:
1. gNumPendingFeedDownloads should be renamed mNumPendingFeedDownloads.
2. gNumPendingFeedDownloads should be a property of the progress notifier.
3. I don't see the point of making the progress notifier a sub-object.
4. I don't see the point of the feedStringProperties property.

Comment 14

[alta88]

(In reply to [e-mail] from comment #13)
> Comment on attachment 582532 [details] [diff] [review]
> address comments.
> 
> I had another look at this and I saw a few potential issues:
> 1. gNumPendingFeedDownloads should be renamed mNumPendingFeedDownloads.
ok.
> 2. gNumPendingFeedDownloads should be a property of the progress notifier.
ok.
> 3. I don't see the point of making the progress notifier a sub-object.
there is tremendous namespace pollution in feeds code. this function (duplicated somewhat in subscribe) as well as all the constants and functions in utils.js should go into the FeedUtils object.
> 4. I don't see the point of the feedStringProperties property.
meaning it should be hardcoded in the getter?  that style isn't preferable, imo.  else i don't understand the issue.

Comment 16

[[e-mail]]

(In reply to alta88 from comment #14)
> (In reply to comment #13)
> > 3. I don't see the point of making the progress notifier a sub-object.
> there is tremendous namespace pollution in feeds code. this function
> (duplicated somewhat in subscribe) as well as all the constants and
> functions in utils.js should go into the FeedUtils object.
The progress notifier isn't a function.

> > 4. I don't see the point of the feedStringProperties property.
> meaning it should be hardcoded in the getter?  that style isn't preferable,
> imo.  else i don't understand the issue.
MXR for Services.strings.createBundle within all of comm-central returns 101 hits. Admittedly some of them are split across lines so I can't readily see what their style is, but apart from two that use a function parameter and six that use a global constant, the rest all seem to hardcode it.

Comment 17

[alta88]

david, have you had a chance to look at this?

Comment 18

[David :Bienvenu]

I'll try to look at it in the next couple days - I was waiting for you and Neil to resolve the issues you were discussing...

Comment 19

[alta88]

i was thinking to roll those into an updated patch with whatever comments you also had as they don't affect the underlying core changes..  really the substantive thing is whether there is any tuning to be done to the yield method to stop the UI freeze.

Comment 20

[David :Bienvenu]

(In reply to alta88 from comment #19)
> i was thinking to roll those into an updated patch with whatever comments
> you also had as they don't affect the underlying core changes..  really the
> substantive thing is whether there is any tuning to be done to the yield
> method to stop the UI freeze.

ah, ok, then my https://bugzilla.mozilla.org/show_bug.cgi?id=711173#c10 still applies...other than that, instead of a single yield, I was thinking you could chain the requests, i.e., not start one until the previous one had finished. Otherwise, you could get 50 http servers sending you data all at once, and essentially lock up the UI. In other words, I'm not sure that just yielding while kicking off the downloads will really help that much. I can try running with it later.

Comment 21

[alta88]

without this patch, i find a 5 second total UI freeze (Not responding in the titlebar in addition) on a biff with 50 feeds urls; with it i notice there is no longer any freeze (test profile with 5 accounts, the largest of which is 50).

the async handling of all the nsIXMLHttpRequest returns doesn't seem to be a bottleneck, but this is the sort of thing to identify here.  it would be a bit more complex to single thread them and wait before launching the next.

Comment 22

[Michael Sirolly]

Disclosure: I submitted duplicate bug 521535; and, I'm not a developer.

You probably wouldn't want individual feed requests hinging on previous requests -- when a server for a request is slow to respond, non-responsive or unreachable, then what?

Requests might be dispatched in rolling intervals, though -- every second, every 5 seconds, what have you. Don't know if Thunderbird's code offers a platform-independent check of processor utilization, but that would be a good determinant. Don't you hate waking a computer when you need to do something productive, only to have it immediately brought to its knees by virus update checks, software update checks, email checks, file backups and so on? Apps ought to be good citizens, and avoid piling on and saturating a system that's pinned or nearly pinned.

Perhaps consider what a user wants by specifying "Check for new articles every (2/5/15/30/60...) minutes." Check all 50 feeds at the same instant? Check one feed each "# of minutes divided by # of feeds" interval, setting off the new message indicator/sound each time? Of course, smartest might be to check the most recently active feeds more often (e.g., an active Twitter conversation), and less active feeds less often (e.g. a personal blog), but that's another enhancement request entirely. My two cents, thanks for listening!   :-)

Comment 23

[alta88]

Attachment: updated to address nits to date.

Comment 24

[alta88]

(In reply to Michael Sirolly from comment #22)
> My two cents, thanks for listening!   :-)

yes, the immediate freeze upon unlocking the screen was the last straw..

an intelligent algo for dispatching would undoubtedly bring an incremental improvement, but it might be small.  this bug is meant to really triage the lock condition, which imo is 90% (fuzzy) of the bad UX.  simply having a greater amount of time (20ms eg) returned to the main thread between yields could reduce the rest.

it would be a great enhancement to add a per feed refresh interval rather than per account, but this means adding a per feed field and some rearchitecture, and that leads to a bigger project, imo.  with questions like, shouldn't the feeds dbs really be in sqlite, etc etc.

Comment 25

[David :Bienvenu]

Comment on attachment 595108 [details] [diff] [review]
updated to address nits to date.

OK, thx, yeah, I'm sure that opening the db's for each feed in a tight loop is mostly responsible for the hang, and that yielding between each will give the UI a little bit of a chance to breath, and it should be a worthwhile improvement.

Comment 26

[alta88]

checkin-needed.

this bug should be in the same branch as bug 709247, which currently is Tb11 (due to the feed db resync being here).  so either this should go to beta, or bug 709247 should be backed out of beta for aurora (Tb12) and this bug advanced to aurora from trunk..?

Comment 27

[David :Bienvenu]

Comment on attachment 595108 [details] [diff] [review]
updated to address nits to date.

alta88 - that means we should request approval comm-aurora and comm-beta, right? And if denied, then we have to back out the other back from those branches?

Comment 28

[alta88]

checkin-needed.

this bug should be in the same branch as bug 709247, which currently is Tb11 (due to the feed db resync being here).  so either this should go to beta, or bug 709247 should be backed out of beta for aurora (Tb12) and this bug advanced to aurora from trunk..?(In reply to David :Bienvenu from comment #27)
> Comment on attachment 595108 [details] [diff] [review]
> updated to address nits to date.
> 
> alta88 - that means we should request approval comm-aurora and comm-beta,
> right? And if denied, then we have to back out the other back from those
> branches?

yes.  the goal would be to have both bugs hit release at the same time.

Comment 29

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/42b7c8b861b4

Comment 30

[Mark Banner (:standard8)]

(In reply to alta88 from comment #26)
> checkin-needed.
> 
> this bug should be in the same branch as bug 709247, which currently is Tb11
> (due to the feed db resync being here).  so either this should go to beta,
> or bug 709247 should be backed out of beta for aurora (Tb12) and this bug
> advanced to aurora from trunk..?

If we backed out bug 709247 from beta, would we also have to back out bug 705504?

Comment 31

[alta88]

argh, a copy/paste slip, i meant bug 705504 should be in the same branch/backed out, yes.  bug 709247 should remain as it fixes an error going forward.

thanks for the catch.

Comment 32

[Mark Banner (:standard8)]

Comment on attachment 595108 [details] [diff] [review]
updated to address nits to date.

Ok, so I think due to the level of change here, then I'd rather this have a slightly longer period for testing. Hence not taking for aurora, but will for beta. We'll back out bug 705504 from beta.

Comment 33

[Mark Banner (:standard8)]

Checked in to branch for TB 12: http://hg.mozilla.org/releases/comm-aurora/rev/958575ce81d0