Closed Bug 1312813 Opened 3 years ago Closed 3 years ago

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

Categories

(MailNews Core :: Feed Reader, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 54.0

People

(Reporter: alta88, Assigned: alta88)

References

(Blocks 1 open bug)

Details

(Whiteboard: l10n impact)

Attachments

(7 files, 9 obsolete files)

10.88 KB, patch
alta88
: review+
Details | Diff | Splinter Review
7.98 KB, patch
alta88
: review+
Details | Diff | Splinter Review
8.40 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
84.06 KB, patch
alta88
: review+
Details | Diff | Splinter Review
7.35 KB, patch
alta88
: review+
Details | Diff | Splinter Review
2.33 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
6.92 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch feedUpdates-strings.patch (obsolete) — Splinter Review
tb and sm strings.
Assignee: nobody → alta88
Attachment #8804370 - Flags: review?(mkmelin+mozilla)
Attached patch feedUpdates-css.patch (obsolete) — Splinter Review
css rules.
Attachment #8804370 - Attachment is obsolete: true
Attachment #8804370 - Flags: review?(mkmelin+mozilla)
Attachment #8804371 - Flags: review?(richard.marti)
Attached patch feedUpdates-strings.patch (obsolete) — Splinter Review
tb and sm strings, again.
Attachment #8804373 - Flags: review?(mkmelin+mozilla)
Attached patch feedUpdates.patch (obsolete) — Splinter Review
main part.
Attachment #8804374 - Flags: review?(mkmelin+mozilla)
Attached patch feedUpdates-folderpane.patch (obsolete) — Splinter Review
tb folderpane and folderpane context.
Attachment #8804376 - Flags: review?(mkmelin+mozilla)
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
Depends on: 1308727
Whiteboard: l10n impact
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 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/status@2x.png and chrome://global/skin/icons/error.png? And if the commented out rule stays add one with chrome://messenger/skin/icons/loading@2x.png.

::: 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.
Attachment #8804371 - Flags: review?(richard.marti) → review+
(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.
updated css.
Attachment #8804371 - Attachment is obsolete: true
Attachment #8804650 - Flags: review+
Attachment #8804373 - Flags: review?(iann_bugzilla)
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.
Attachment #8804373 - Flags: review?(iann_bugzilla) → review+
Attached patch feedUpdates-strings.patch (obsolete) — Splinter Review
updated for comment 11.
Attachment #8804373 - Attachment is obsolete: true
Attachment #8804373 - Flags: review?(mkmelin+mozilla)
Attachment #8806328 - Flags: review?(mkmelin+mozilla)
Large patch, anyone has review cycles?
"Large" refers to 'feedUpdates.patch'? I have cycles, but sadly no idea :-(
Yes that's the largest one - though I assume the patches are all quite entangled with each other.
Duplicate of this bug: 257037
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?
Attachment #8804374 - Flags: review?(mkmelin+mozilla) → review-
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)
Attachment #8804376 - Flags: review?(mkmelin+mozilla)
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.
Attachment #8806328 - Flags: review?(mkmelin+mozilla) → review+
(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...
fixed trailing spaces (comment 19).
Attachment #8806328 - Attachment is obsolete: true
Attachment #8830864 - Flags: review+
changed to U (comment 18).
Attachment #8804376 - Attachment is obsolete: true
Attachment #8830865 - Flags: review?(mkmelin+mozilla)
Attached patch feedUpdates.patch (obsolete) — Splinter Review
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.
Attachment #8804374 - Attachment is obsolete: true
Attachment #8830942 - Flags: review?(mkmelin+mozilla)
Attachment #8830865 - Flags: review?(mkmelin+mozilla) → review+
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"
Attachment #8830942 - Flags: review?(mkmelin+mozilla) → review+
(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.
Attachment #8830942 - Attachment is obsolete: true
Attachment #8832697 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
checkin-needed cleared? Is there another update coming?
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.
Attached patch feedUpdates-followup.patch (obsolete) — Splinter Review
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.
Attachment #8833680 - Flags: review?(mkmelin+mozilla)
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
Attachment #8833680 - Flags: review?(mkmelin+mozilla) → review+
(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.
Attachment #8833680 - Attachment is obsolete: true
Attachment #8833691 - Flags: review+
very small addition to track last item download time, it's here so as not to have to rev the options object version.
Attachment #8833694 - Flags: review?(mkmelin+mozilla)
Attachment #8833694 - Flags: review?(mkmelin+mozilla) → review+
the first 4 should be applied first, then feedUpdates-followup.patch, and feedUpdates-lastDownload.patch last (if it matters). thanks.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/8571f68411c759762160b13257eb68626b9b00ea
(Folded all patches into one)
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch feedUpdates-tweaks.patch (obsolete) — Splinter Review
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.
Attachment #8834696 - Flags: review?(mkmelin+mozilla)
(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?  ;)
Attachment #8834696 - Attachment is obsolete: true
Attachment #8834696 - Flags: review?(mkmelin+mozilla)
Attachment #8834951 - Flags: review?(mkmelin+mozilla)
(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.
Attachment #8834951 - Flags: review?(mkmelin+mozilla) → review+
I'll land it in the morning after the next M-I->M-C merge.
Keywords: checkin-needed
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.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.