1312813 - Implement user configurable per feed update interval, pausing feed account or folder updates in folderpane

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Comment 1

[alta88]

Attachment: feedUpdates-strings.patch

tb and sm strings.

Comment 2

[alta88]

Attachment: feedUpdates-css.patch

css rules.

Comment 3

[alta88]

Attachment: feedUpdates-strings.patch

tb and sm strings, again.

Comment 4

[alta88]

Attachment: feedUpdates.patch

main part.

Comment 5

[alta88]

Attachment: feedUpdates-folderpane.patch

tb folderpane and folderpane context.

Comment 6

[alta88]

This patch implements:
1. configurable update interval (minutes/days) per feed url
2. display publisher recommended interval (syndication update tags)
3. folderpane display, per folder, the error or busy state of feed update state
4. folderpane contextmenu to pause/resume account or folder level updates
5. along with bug 24594, no more update on startup or first biff fail after startup problems

Comment 7

[alta88]

Some notes:

1. When a feed account is paused, the root server folder and all folders with feed subscriptions are set to a lower opacity. Feed folders can be individually paused also.
2. Removing paused state causes polling to resume; if the per feed interval has not expired there is no network update.
3. A manual Get New Messages always overrides the timer and does it now, even in paused state.
4. If at least 1 feed is in error or busy state, the folderpane icon indicates this. This state is also shown in the Subscribe dialog, for the specific url, on an open folder. An Update there will verify the feed and display an error message.
5. The system poll interval defaults to 1 minute, ie it checks all individual url expired times. Account update on startup is supported and forces an update regardless of expired timers. This should be less useful than before as all expired times will be checked in 1 minute after startup anyway.

Comment 8

[Richard Marti (:Paenglab)]

Comment on attachment 8804371 [details] [diff] [review]
feedUpdates-css.patch

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

r=me with the comments addressed.

::: mail/themes/linux/mail/folderPane.css
@@ +134,5 @@
> +}
> +treechildren::-moz-tree-image(folderNameCol, closed, isBusy),
> +treechildren::-moz-tree-image(folderNameCol, leaf, isBusy) {
> +  list-style-image: url("chrome://messenger/skin/icons/status.png");
> +  -moz-image-region: rect(0px 32px 16px 16px);

Please 0 instead of 0px.

::: mail/themes/osx/mail/folderPane.css
@@ +54,5 @@
> +/*
> +treechildren::-moz-tree-image(folderNameCol, isBusy) {
> +  list-style-image: url("chrome://messenger/skin/icons/loading.png");
> +}
> +*/

Are you planning to enable this? If yes, this is missing on Linux and Windows. If not, maybe it's better to remove.

@@ +58,5 @@
> +*/
> +treechildren::-moz-tree-image(folderNameCol, closed, isBusy),
> +treechildren::-moz-tree-image(folderNameCol, leaf, isBusy) {
> +  list-style-image: url("chrome://messenger/skin/icons/status.png");
> +  -moz-image-region: rect(0px 32px 16px 16px);

Please 0 instead of 0px.

@@ +63,5 @@
> +}
> +treechildren::-moz-tree-image(folderNameCol, closed, hasError),
> +treechildren::-moz-tree-image(folderNameCol, leaf, hasError) {
> +  list-style-image: url("chrome://global/skin/icons/error-16.png");
> +  -moz-image-region: rect(0 16px 16px 0);

Please can you add also @2x rules with chrome://messenger/skin/icons/[e-mail] and chrome://global/skin/icons/error.png? And if the commented out rule stays add one with chrome://messenger/skin/icons/[e-mail].

::: mail/themes/windows/mail/folderPane.css
@@ +82,5 @@
> +}
> +treechildren::-moz-tree-image(folderNameCol, closed, isBusy),
> +treechildren::-moz-tree-image(folderNameCol, leaf, isBusy) {
> +  list-style-image: url("chrome://messenger/skin/icons/status.png");
> +  -moz-image-region: rect(0px 32px 16px 16px);

Please 0 instead of 0px.

Comment 9

[alta88]

(In reply to Richard Marti (:Paenglab) from comment #8)
> Comment on attachment 8804371 [details] [diff] [review]
> feedUpdates-css.patch
> 
> Review of attachment 8804371 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: mail/themes/osx/mail/folderPane.css
> @@ +54,5 @@
> > +/*
> > +treechildren::-moz-tree-image(folderNameCol, isBusy) {
> > +  list-style-image: url("chrome://messenger/skin/icons/loading.png");
> > +}
> > +*/
> 
> Are you planning to enable this? If yes, this is missing on Linux and
> Windows. If not, maybe it's better to remove.

removed; sadly animated gifs cause permanent cpu load in nsITreeView even when no longer returned, there's a bug for it somewhere.

the rest will be updated.

Comment 10

[alta88]

Attachment: feedUpdates-css.patch

updated css.

Comment 11

[Ian Neal]

Comment on attachment 8804373 [details] [diff] [review]
feedUpdates-strings.patch

> <!ENTITY feedLocation.accesskey      "U">
This needs fixing as it conflicts with the accesskey for the Update button. (F or L perhaps?)

> <!ENTITY autotagEnable.label         "Automatically create tags from feed &lt;category&gt; names">
>-<!ENTITY autotagEnable.accesskey     "c">
>+<!ENTITY autotagEnable.accesskey     "g">
It is best to try and avoid lowercase characters like "g","p","q" and "y" as the underline can get missed. Perhaps use "o" instead?

r/a=me with those addressed.

Comment 12

[alta88]

Attachment: feedUpdates-strings.patch

updated for comment 11.

Comment 13

[Magnus Melin [:mkmelin]]

Large patch, anyone has review cycles?

Comment 14

[Jorg K (CEST = GMT+2)]

"Large" refers to 'feedUpdates.patch'? I have cycles, but sadly no idea :-(

Comment 15

[Magnus Melin [:mkmelin]]

Yes that's the largest one - though I assume the patches are all quite entangled with each other.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 8804374 [details] [diff] [review]
feedUpdates.patch

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

Ok so I finally got to reviewing this. Some comments, questions, observations etc:

- the account node has opacity, even if nothing is paused. 
- working feed has opacity - even though it's working. dunno why
- check for new articles every 1 min: way to short default. half hour maybe?
- in the options: every [] minutes, day [] - maybe skip the days option for simplicity? Or even better, maybe only hours but allow decimals in the field?

- what's the large white area on top of the subsciption dialog? oh, that's where the tree used to show. I see now, there's an error in the console
JavaScript error: resource:///modules/FeedUtils.jsm, line 1241: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
  --- this is apparently the <server>.check_new_mail pref missing
  
After I add that this part is working. 

- for paused feeds, show a pause mark in the folder pane, like the stop mark for those broken?

::: mailnews/extensions/newsblog/content/FeedUtils.jsm
@@ +41,5 @@
>      return this.rdf.GetResource(this.RSS_CONTENT_NS + "encoded");
>    },
>  
> +  RSS_SY_NS: "http://purl.org/rss/1.0/modules/syndication/",
> +  RSS_SY_UNITS:      ["hourly","daily","weekly","monthly","yearly"],

nit: add space after commas between array elements

@@ +95,5 @@
>    // The delay is currently one day.
>    INVALID_ITEM_PURGE_DELAY: 24 * 60 * 60 * 1000,
>  
> +  // Polling inverval to check individual feed update interval preference.
> +  kBiffPollMinutes: 1,

I minute is way too small. Why do you want to change it?

@@ +224,5 @@
>   * @param   nsIDOMWindow aMsgWindow      - window
>   */
>    downloadFeed: function(aFolder, aUrlListener, aIsBiff, aMsgWindow) {
> +    FeedUtils.log.debug("downloadFeed: account loginAtStartUp:isBiff:isOffline - " +
> +                        aFolder.server.loginAtStartUp + ":"+

nit: add space after "
Even for debugging I'd also make it ": "

@@ +300,5 @@
> +                (now - status.lastUpdateTime < status.updateMinutes * 60000))
> +            {
> +              FeedUtils.log.debug("downloadFeed: SKIP feed, " +
> +                                  "aIsBiff:enabled:minsSinceLastUpdate::url - " +
> +                                  aIsBiff+":"+ status.enabled+":"+

nit:
aIsBiff + ": " + status.enabled + ": " +

similar nit for the next line

@@ +475,5 @@
> +
> +      return;
> +    }
> +
> +    let feedUrls = aFolder ? FeedUtils.getFeedUrlsInFolder(aFolder) : null;

aFolder is always set

@@ +479,5 @@
> +    let feedUrls = aFolder ? FeedUtils.getFeedUrlsInFolder(aFolder) : null;
> +    if (!feedUrls)
> +      return;
> +
> +    let resource, feed, options;

declare these inside the loop, where they are used
(and again, further down. It's confusing having them "global" when it's not the same instances after all.)

@@ +705,5 @@
> +   * folderpane or subscibe dialog tree.
> +   *
> +   * @param   nsIMsgFolder aFolder- folder or a feed url's parent folder
> +   * @param   string aFeedUrl     - feed url for a feed row, null for folder
> +   * @return  [string, int]       - [properties, status (1 if error/busy else 0)]

this doesn't look like the format used

@@ +709,5 @@
> +   * @return  [string, int]       - [properties, status (1 if error/busy else 0)]
> +   */
> +  getFolderProperties: function(aFolder, aFeedUrl) {
> +    let folder = aFolder;
> +    let properties = "";

nit: I'd declare this further down where it's actually used, and return "" for the error case.

::: mailnews/extensions/newsblog/content/am-newsblog.js
@@ +5,5 @@
>  
>  Components.utils.import("resource:///modules/FeedUtils.jsm");
>  
> +var gServer, updateEnabled, updateValue, biffUnits,
> +    autotagEnable, autotagUsePrefix, autotagPrefix;

unfortunate that these all didn't have a g prefix to being with, as they are global. Maybe fix the added ones, and as a followup find-replace the existing.

::: mailnews/extensions/newsblog/content/am-newsblog.xul
@@ +85,5 @@
> +                 onchange="setPrefs(this)"/>
> +        <radiogroup id="biffUnits"
> +                    orient="horizontal"
> +                    oncommand="setPrefs(this)">
> +          <radio id="biffMinutes" value="minutes" label="&biffMinutes.label;"

would use SI unit for the magic values. So "min", ("h" for hour), and "d" for day

@@ +88,5 @@
> +                    oncommand="setPrefs(this)">
> +          <radio id="biffMinutes" value="minutes" label="&biffMinutes.label;"
> +                 accesskey="&biffMinutes.accesskey;">
> +            <observes element="updateValue" attribute="disabled"/>
> +          </radio>  

nit: trailing space, also a few lines down

::: mailnews/extensions/newsblog/content/feed-parser.js
@@ +885,5 @@
>  
>      return null;
>    },
>  
> +  findSyUpdateTags: function(aFeed, aChannel)

what's the sy abbreviation? Please add documentation for the function.
(Ok, so it's the namespace abbreviation for the rss1 syndication spec. Wasn't obvious.)

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +1835,5 @@
> +        // Update lastUpdateTime if successful.
> +        let options = feed.options;
> +        options.updates.lastUpdateTime = Date.now();
> +        feed.options = options;
> +//        FeedUtils.setStatus(feed.folder, feed.url, "lastUpdateTime", now);

remove?

::: mailnews/extensions/newsblog/content/newsblogOverlay.js
@@ +19,5 @@
>    kOpenWebPage:             0,
>    kOpenSummary:             1,
>    kOpenToggleInMessagePane: 2,
>    kOpenLoadInBrowser:       3,
> + 

nit: trailing space

@@ +20,5 @@
>    kOpenSummary:             1,
>    kOpenToggleInMessagePane: 2,
>    kOpenLoadInBrowser:       3,
> + 
> +  FeedAccountType: ["rss"],

so this is to allow other types of feed accounts, or what?
Why isn't it FeedAccountTypes as it's an array?

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 8804376 [details] [diff] [review]
feedUpdates-folderpane.patch

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

::: mail/locales/en-US/chrome/messenger/messenger.dtd
@@ +612,5 @@
>  <!ENTITY folderContextGetMessages.label "Get Messages">
>  <!ENTITY folderContextGetMessages.accesskey "G">
> +<!ENTITY folderContextPauseAllUpdates.label "Pause All Updates">
> +<!ENTITY folderContextPauseUpdates.label "Pause Updates">
> +<!ENTITY folderContextPauseUpdates.accesskey "P">

collides with _P_roperties (on *nix)

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 8806328 [details] [diff] [review]
feedUpdates-strings.patch

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

::: mail/locales/en-US/chrome/messenger-newsblog/am-newsblog.dtd
@@ +1,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<!ENTITY loginAtStartup.label          "Check for new articles at startup"> 

Some inherited trailing spaces in here. 
In general I'm don't like dtd key alignments though... I'd rather do away with them completely.

Comment 20

[alta88]

(In reply to Magnus Melin from comment #17)
> Comment on attachment 8804374 [details] [diff] [review]
> feedUpdates.patch
> 
> Review of attachment 8804374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok so I finally got to reviewing this. Some comments, questions,
> observations etc:
> 

Thanks, it's sort of big but large majority is UI and getting user prefs..

> - the account node has opacity, even if nothing is paused. 
> - working feed has opacity - even though it's working. dunno why
> - check for new articles every 1 min: way to short default. half hour maybe?

These 3 are due to the error below.

> - in the options: every [] minutes, day [] - maybe skip the days option for
> simplicity? Or even better, maybe only hours but allow decimals in the field?
> 

I think fractional hours is very unfriendly, doubly so because time is not even 10 based. Minutes is the right amount, and I did see a request for longer than intraday, so days is appropriate. I'd like to keep as is.

> - what's the large white area on top of the subsciption dialog? oh, that's
> where the tree used to show. I see now, there's an error in the console
> JavaScript error: resource:///modules/FeedUtils.jsm, line 1241:
> NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff
> (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
>   --- this is apparently the <server>.check_new_mail pref missing
>   

This error made initialization of server and feed prefs incomplete when coming from a profile without this patch.  To be fixed.  It will also fix most of the above issues.

> After I add that this part is working. 
> 
> - for paused feeds, show a pause mark in the folder pane, like the stop mark
> for those broken?
> 

So I considered this sort of thing quite a bit.  A "pause" icon, like for media, was a strong contender. Perhaps it's right for the server item.  But for feed folders, it would replace the favicon, which is suboptimal.  I think this can be discussed in a followup.

Part 2 for the rest...

Comment 21

[alta88]

Attachment: feedUpdates-strings.patch

fixed trailing spaces (comment 19).

Comment 22

[alta88]

Attachment: feedUpdates-folderpane.patch

changed to U (comment 18).

Comment 23

[alta88]

Attachment: feedUpdates.patch

With the pref error fixed, there should be a clean upgrade from a feed account created in a fresh profile with a daily, and then used with a build with this patch applied.

> 
> ::: mailnews/extensions/newsblog/content/FeedUtils.jsm
> @@ +41,5 @@
> >      return this.rdf.GetResource(this.RSS_CONTENT_NS + "encoded");
> >    },
> >  
> > +  RSS_SY_NS: "http://purl.org/rss/1.0/modules/syndication/",
> > +  RSS_SY_UNITS:      ["hourly","daily","weekly","monthly","yearly"],
> 
> nit: add space after commas between array elements
> 

ok.

> @@ +95,5 @@
> >    // The delay is currently one day.
> >    INVALID_ITEM_PURGE_DELAY: 24 * 60 * 60 * 1000,
> >  
> > +  // Polling inverval to check individual feed update interval preference.
> > +  kBiffPollMinutes: 1,
> 
> I minute is way too small. Why do you want to change it?
> 

No, this is the internal frequency for in-memory scan of feed state objects to determine if update time has arrived.  The default for network downloads for individual feed urls remains 100min.

> @@ +224,5 @@
> >   * @param   nsIDOMWindow aMsgWindow      - window
> >   */
> >    downloadFeed: function(aFolder, aUrlListener, aIsBiff, aMsgWindow) {
> > +    FeedUtils.log.debug("downloadFeed: account loginAtStartUp:isBiff:isOffline - " +
> > +                        aFolder.server.loginAtStartUp + ":"+
> 
> nit: add space after "
> Even for debugging I'd also make it ": "
> 

ok.

> @@ +300,5 @@
> > +                (now - status.lastUpdateTime < status.updateMinutes * 60000))
> > +            {
> > +              FeedUtils.log.debug("downloadFeed: SKIP feed, " +
> > +                                  "aIsBiff:enabled:minsSinceLastUpdate::url - " +
> > +                                  aIsBiff+":"+ status.enabled+":"+
> 
> nit:
> aIsBiff + ": " + status.enabled + ": " +
> 
> similar nit for the next line
> 

ok.

> @@ +475,5 @@
> > +
> > +      return;
> > +    }
> > +
> > +    let feedUrls = aFolder ? FeedUtils.getFeedUrlsInFolder(aFolder) : null;
> 
> aFolder is always set
> 

fixed.

> @@ +479,5 @@
> > +    let feedUrls = aFolder ? FeedUtils.getFeedUrlsInFolder(aFolder) : null;
> > +    if (!feedUrls)
> > +      return;
> > +
> > +    let resource, feed, options;
> 
> declare these inside the loop, where they are used
> (and again, further down. It's confusing having them "global" when it's not
> the same instances after all.)
> 

fixed.

> @@ +705,5 @@
> > +   * folderpane or subscibe dialog tree.
> > +   *
> > +   * @param   nsIMsgFolder aFolder- folder or a feed url's parent folder
> > +   * @param   string aFeedUrl     - feed url for a feed row, null for folder
> > +   * @return  [string, int]       - [properties, status (1 if error/busy else 0)]
> 
> this doesn't look like the format used
> 

fixed comment from old patch for new behavior.

> @@ +709,5 @@
> > +   * @return  [string, int]       - [properties, status (1 if error/busy else 0)]
> > +   */
> > +  getFolderProperties: function(aFolder, aFeedUrl) {
> > +    let folder = aFolder;
> > +    let properties = "";
> 
> nit: I'd declare this further down where it's actually used, and return ""
> for the error case.
> 

ok.

> ::: mailnews/extensions/newsblog/content/am-newsblog.js
> @@ +5,5 @@
> >  
> >  Components.utils.import("resource:///modules/FeedUtils.jsm");
> >  
> > +var gServer, updateEnabled, updateValue, biffUnits,
> > +    autotagEnable, autotagUsePrefix, autotagPrefix;
> 
> unfortunate that these all didn't have a g prefix to being with, as they are
> global. Maybe fix the added ones, and as a followup find-replace the
> existing.
> 

fixed them all here.

> ::: mailnews/extensions/newsblog/content/am-newsblog.xul
> @@ +85,5 @@
> > +                 onchange="setPrefs(this)"/>
> > +        <radiogroup id="biffUnits"
> > +                    orient="horizontal"
> > +                    oncommand="setPrefs(this)">
> > +          <radio id="biffMinutes" value="minutes" label="&biffMinutes.label;"
> 
> would use SI unit for the magic values. So "min", ("h" for hour), and "d"
> for day
> 

not sure what you meant, but changed the radio value strings as above.

> @@ +88,5 @@
> > +                    oncommand="setPrefs(this)">
> > +          <radio id="biffMinutes" value="minutes" label="&biffMinutes.label;"
> > +                 accesskey="&biffMinutes.accesskey;">
> > +            <observes element="updateValue" attribute="disabled"/>
> > +          </radio>  
> 
> nit: trailing space, also a few lines down
> 

ok.

> ::: mailnews/extensions/newsblog/content/feed-parser.js
> @@ +885,5 @@
> >  
> >      return null;
> >    },
> >  
> > +  findSyUpdateTags: function(aFeed, aChannel)
> 
> what's the sy abbreviation? Please add documentation for the function.
> (Ok, so it's the namespace abbreviation for the rss1 syndication spec.
> Wasn't obvious.)
> 

made a comment.

> ::: mailnews/extensions/newsblog/content/feed-subscriptions.js
> @@ +1835,5 @@
> > +        // Update lastUpdateTime if successful.
> > +        let options = feed.options;
> > +        options.updates.lastUpdateTime = Date.now();
> > +        feed.options = options;
> > +//        FeedUtils.setStatus(feed.folder, feed.url, "lastUpdateTime", now);
> 
> remove?
> 

done.

> ::: mailnews/extensions/newsblog/content/newsblogOverlay.js
> @@ +19,5 @@
> >    kOpenWebPage:             0,
> >    kOpenSummary:             1,
> >    kOpenToggleInMessagePane: 2,
> >    kOpenLoadInBrowser:       3,
> > + 
> 
> nit: trailing space
> 

ok. all files in this patch have had a strip trailing space run on them.

> @@ +20,5 @@
> >    kOpenSummary:             1,
> >    kOpenToggleInMessagePane: 2,
> >    kOpenLoadInBrowser:       3,
> > + 
> > +  FeedAccountType: ["rss"],
> 
> so this is to allow other types of feed accounts, or what?
> Why isn't it FeedAccountTypes as it's an array?

changed.

yes, this is meant to enable a completely new implementation of feeds and stop hardcoding in the numerous UI type tests embedded in the front end.

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 8830942 [details] [diff] [review]
feedUpdates.patch

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

LGTM! r=mkmelin with a couple of nits

::: mailnews/extensions/newsblog/content/FeedUtils.jsm
@@ +286,5 @@
>          FeedUtils.progressNotifier.init(aMsgWindow, false);
>  
>          // We need to kick off a download for each feed.
> +        let now = Date.now();
> +        let msgDbOk = null;

I don't like mixing booleans and objects. Please initialize to false

@@ +313,5 @@
> +          // add a message to a folder with an unavailable msgDatabase will
> +          // throw later. After the async reparse the folder will be ready for
> +          // the next poll cycle to pick up this folder's feeds, whose update
> +          // time will not have changed.
> +          if (msgDbOk === null)

... and then you can just compare if (!msgDbOk)

@@ +316,5 @@
> +          // time will not have changed.
> +          if (msgDbOk === null)
> +            msgDbOk = FeedUtils.isMsgDatabaseOpenable(folder, true);
> +          if (msgDbOk !== true)
> +            continue;

if (!msgDbOk)
  break;

@@ +1166,5 @@
> +      enabled: true,
> +      // User set.
> +      updateMinutes: 100,
> +      // User set: 0=minutes, 1=days
> +      updateUnits: 0,

can we make these too use the SI units since they exist. "min" and "d"

Comment 25

[alta88]

(In reply to Magnus Melin from comment #24)
> Comment on attachment 8830942 [details] [diff] [review]
> feedUpdates.patch
> 
> Review of attachment 8830942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM! r=mkmelin with a couple of nits
> 
> ::: mailnews/extensions/newsblog/content/FeedUtils.jsm
> @@ +286,5 @@
> >          FeedUtils.progressNotifier.init(aMsgWindow, false);
> >  
> >          // We need to kick off a download for each feed.
> > +        let now = Date.now();
> > +        let msgDbOk = null;
> 
> I don't like mixing booleans and objects. Please initialize to false
> 
> @@ +313,5 @@
> > +          // add a message to a folder with an unavailable msgDatabase will
> > +          // throw later. After the async reparse the folder will be ready for
> > +          // the next poll cycle to pick up this folder's feeds, whose update
> > +          // time will not have changed.
> > +          if (msgDbOk === null)
> 
> ... and then you can just compare if (!msgDbOk)
> 
> @@ +316,5 @@
> > +          // time will not have changed.
> > +          if (msgDbOk === null)
> > +            msgDbOk = FeedUtils.isMsgDatabaseOpenable(folder, true);
> > +          if (msgDbOk !== true)
> > +            continue;
> 
> if (!msgDbOk)
>   break;
> 

this was all from the ancient patch so that all url state objects could be printed/tested for the update or not algo even if the msgdb test failed.  not necessary anymore and it's been fixed to be first in the urlarray loop and break immediately, so good catch.

> @@ +1166,5 @@
> > +      enabled: true,
> > +      // User set.
> > +      updateMinutes: 100,
> > +      // User set: 0=minutes, 1=days
> > +      updateUnits: 0,
> 
> can we make these too use the SI units since they exist. "min" and "d"

sure.

Comment 26

[alta88]

Attachment: feedUpdates.patch

Comment 27

[Jorg K (CEST = GMT+2)]

checkin-needed cleared? Is there another update coming?

Comment 28

[alta88]

maybe, i noticed bustage on a test profile (old and likely corrupt) but still would like to understand it.  there's no problem on a clean profile.

Comment 29

[alta88]

Attachment: feedUpdates-followup.patch

no further issue was found on old profile/accounts upgrades, so the pref error and resetting prefs in a test env is the culprit. some issues were found and are addressed, in patch file order:
1. silence a js warning.
2. don't forget about rdf feeds in the sy update tags function.
3. missed a place for the SI units change (not that minor after all).
4. ensure prefs we need exist even if user footguns.

this patch needs to be applied on top of the others.

Comment 30

[Magnus Melin [:mkmelin]]

Comment on attachment 8833680 [details] [diff] [review]
feedUpdates-followup.patch

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

Didn't test it but looks reasonable to me. r=mkmelin

::: mailnews/extensions/newsblog/content/FeedUtils.jsm
@@ +1210,5 @@
>        optionsAcct = JSON.parse(Services.prefs.getCharPref(optionsAcctPref));
>        // Add the server specific biff enabled state.
>        optionsAcct["doBiff"] = Services.prefs.getBoolPref(check_new_mail);
>      }
> +    catch (ex) {}

this try-catch could be removed now it seems

::: mailnews/extensions/newsblog/content/feed-parser.js
@@ +889,5 @@
>    /**
>     * Find RSS Syndication extension tags.
>     * http://web.resource.org/rss/1.0/modules/syndication/
>     */
> +  findSyUpdateTags: function(aFeed, aChannel, aDS)

please document aDS

Comment 31

[alta88]

Attachment: feedUpdates-followup.patch

(In reply to Magnus Melin from comment #30)
> Comment on attachment 8833680 [details] [diff] [review]
> feedUpdates-followup.patch
> 
> Review of attachment 8833680 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Didn't test it but looks reasonable to me. r=mkmelin
> 
> ::: mailnews/extensions/newsblog/content/FeedUtils.jsm
> @@ +1210,5 @@
> >        optionsAcct = JSON.parse(Services.prefs.getCharPref(optionsAcctPref));
> >        // Add the server specific biff enabled state.
> >        optionsAcct["doBiff"] = Services.prefs.getBoolPref(check_new_mail);
> >      }
> > +    catch (ex) {}
> 
> this try-catch could be removed now it seems
> 

the optionsAcctPref may also not exist.

> ::: mailnews/extensions/newsblog/content/feed-parser.js
> @@ +889,5 @@
> >    /**
> >     * Find RSS Syndication extension tags.
> >     * http://web.resource.org/rss/1.0/modules/syndication/
> >     */
> > +  findSyUpdateTags: function(aFeed, aChannel, aDS)
> 
> please document aDS

done.

Comment 32

[alta88]

Attachment: feedUpdates-lastDownload.patch

very small addition to track last item download time, it's here so as not to have to rev the options object version.

Comment 33

[alta88]

the first 4 should be applied first, then feedUpdates-followup.patch, and feedUpdates-lastDownload.patch last (if it matters). thanks.

Comment 34

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/8571f68411c759762160b13257eb68626b9b00ea
(Folded all patches into one)

Comment 35

[alta88]

Attachment: feedUpdates-tweaks.patch

1. not sure why i moved the dbok check ahead of the time to update check, but it's definitely not what we want.
2. fix context menu for folder with no feeds but with subfolders.
3. fix js warning.

Comment 36

[Jorg K (CEST = GMT+2)]

Can you please fix the formatting error here:
https://hg.mozilla.org/comm-central/rev/8571f68411c759762160b13257eb68626b9b00ea#l16.256
And add braces here
https://hg.mozilla.org/comm-central/rev/8571f68411c759762160b13257eb68626b9b00ea#l16.264
since the 'else' has it.

Comment 37

[alta88]

Attachment: feedUpdates-tweaks.patch

(In reply to Jorg K (GMT+1) from comment #36)
> Can you please fix the formatting error here:
> https://hg.mozilla.org/comm-central/rev/
> 8571f68411c759762160b13257eb68626b9b00ea#l16.256
> And add braces here
> https://hg.mozilla.org/comm-central/rev/
> 8571f68411c759762160b13257eb68626b9b00ea#l16.264
> since the 'else' has it.

sure.  don't you have enough fires tho?  ;)

Comment 38

[Jorg K (CEST = GMT+2)]

(In reply to alta88 from comment #37)
> sure.  don't you have enough fires tho?  ;)
Sadly so. But I saw this when I scanned the merged patch I had landed. I couldn't bother to fix it myself, so here's my chance. Thanks.

Comment 39

[Jorg K (CEST = GMT+2)]

I'll land it in the morning after the next M-I->M-C merge.

Comment 40

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/247903d556cec7f4a1aab92495c8ef21f5cedacd
Can you do me a huge favour, please: Provide meaningful check-in comments. And perhaps add your e-mail address to the user.