1464572 - Statusbar stays busy after "Get Messages" in "Blog & News Feeds"

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[Frank-Rainer Grahl (:frg)]

Attachment: Capture.PNG

I noticed that the statusbar does not update itself in a locally compiled 56 when there are no new messages in a feed. It just hangs at, what seems to me, 100%. This does occur if you click "Get Messages" on the "Blog & News Feeds" level. If you select one feed and do this the statusbar will be cleared. 

This also happens in a local SeaMonkey 2.53 build and TB 60 locally compiled on 05/09 from comm-esr60.

In TB 52 and SeaMonkey 2.49 the message "<feedname> + There are no new articles for this feed" is shown for the last feed after doing this either on the global level for all feeds or for an individual feed.

Seems to me at least the 304 is no longer correctly handled. Using about:Networking in SeaMonkey I also see that SeaMonkey holds a connection which might have an impact if you have many feeds.

I suspect Bug 1339248 or Bug 1312813 might have caused this.

Comment 1

[Frank-Rainer Grahl (:frg)]

The connection seems to go away so maybe only a visual glitch.

Comment 2

[Frank-Rainer Grahl (:frg)]

Attachment: 1464572-progressnotifier.patch

I think it was bug 1312813. Feed updates can be skipped after the  progressNotifier was initialized. So if all feed urls are skipped it is never reset. The patch fixes it for me but not sure if my conclusion is right or something else is going on.

I am still not seeing the "no new messages" with the patch.

I opted for comparing mNumPendingFeedDownloads to 0. Doing just
> if (FeedUtils.progressNotifier.mNumPendingFeedDownloads)
looks not right to me here. Should work as elsewhere but it should always be not null because intialized to 0 and it is not a boolean.

Comment 3

[:aceman]

(In reply to Frank-Rainer Grahl (:frg) from comment #2)
> I opted for comparing mNumPendingFeedDownloads to 0. Doing just
> > if (FeedUtils.progressNotifier.mNumPendingFeedDownloads)
> looks not right to me here. Should work as elsewhere but it should always be
> not null because intialized to 0 and it is not a boolean.

I also agree this would be semantically more correct.

Comment 4

[alta88]

Comment on attachment 8980862 [details] [diff] [review]
1464572-progressnotifier.patch


thanks, frg.  of course the progress should be initialized once in a download transaction and only if there is at least 1 feed that passes the ready to download tests.  r+ based on works for you and visual inspection.

note that "no new messages" only displays for feeds whose publishers are doing the right thing and using If-Modified-Since. for those that aren't, bandwidth and processing are wasted as we have to compare the file to current downloaded items; even if there really are no new messages, nothing is sent to status in this case. (see the guide also).

if you could also fix the below to compare explicitly to 0, while you're here, that would be really great.

420 if (FeedUtils.progressNotifier.mNumPendingFeedDownloads)
1781 if (!this.mNumPendingFeedDownloads)
1950 if (!--this.mNumPendingFeedDownloads)

Comment 5

[Frank-Rainer Grahl (:frg)]

Attachment: 1464572-progressnotifier.patch

Changed the other compares. I moved the decrement outside of the if. Got burned once here with a more complex statment in another programming language (good old PL/I) and try to avoid this now.
While doing it I noticed that I didn't need the "if" I added. init does it just the same and so can be called every time in the loop.

Tested with SeaMonkey 2.53 based on 56 and also applies clean to esr60.

Just for the record. When I set Feeds.logging.console to All I was also unable to reproduce the problem so not sure if it isn't actually a timing problem. Nevertheless I think the fix is ok to include and fixes it for me.

Comment 6

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8980893 [details] [diff] [review]
1464572-progressnotifier.patch

[Approval Request Comment]
Regression caused by (bug #): Bug 1312813
User impact if declined: Progress bar shows incorrect content.
Testing completed (on c-c, etc.): c-r at TB 56 level
Risk to taking this patch (and alternatives if risky): mostly trivial

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/80c2dba8eca3
Initialize the progress notifier on first download only. r=alta88

Comment 8

[Jorg K (CEST = GMT+2)]

TB 60 beta 7 (BETA_60_CONTINUATION branch):
https://hg.mozilla.org/releases/comm-beta/rev/4bc0b1e72db0380acf98353115f60e15c713bd80

Comment 9

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/94a4e05f51dc