Closed Bug 1025548 Opened 8 years ago Closed 8 years ago

Preliminary perf/code org tweaks for Bug 257037

Categories

(MailNews Core :: Feed Reader, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: alta88, Assigned: alta88)

References

Details

Attachments

(4 files, 4 obsolete files)

The Bug 257037 patch contained a number of tangential changes which are better to split out for ease of review etc.

Part1: implement isBiff in the interface and move, word for word, downloadFeed() and subscribeToFeed() to FeedUtils.jsm.  updateSubscriptionsDS() has already been moved.
Part2: reorg/genericise some functions.
Part3: some perf/snappy tweaks, and active delete of dead objects, for Bug 950561 type cases.
Assignee: nobody → alta88
Attachment #8440327 - Flags: review?(mkmelin+mozilla)
Attached patch feedUpdatesPart2.patch (obsolete) — Splinter Review
Attachment #8440328 - Flags: review?(mkmelin+mozilla)
Attached patch feedUpdatesPart3.patch (obsolete) — Splinter Review
Attachment #8440329 - Flags: review?(mkmelin+mozilla)
Blocks: 257037
OS: Linux → All
Hardware: x86_64 → All
Attachment #8440329 - Attachment is obsolete: true
Attachment #8440329 - Flags: review?(mkmelin+mozilla)
Attachment #8442095 - Flags: review?(mkmelin+mozilla)
Attached patch feedUpdatesPart4.patch (obsolete) — Splinter Review
Part 4 - move FeedMessageHandler to newsblogOverlay.js where i should have put it in the first place; UTF8 tweaks.
Attachment #8448045 - Flags: review?(mkmelin+mozilla)
Attachment #8440327 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8440328 [details] [diff] [review]
feedUpdatesPart2.patch

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

::: mailnews/extensions/newsblog/content/FeedUtils.jsm
@@ +480,5 @@
>      }
>  
>      return feedUrlArray.length ? feedUrlArray : null;
>    },
> + 

whitespace

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +54,4 @@
>      {
> +      this.mMainWin.FeedFolderNotificationService = MailServices.mfn;
> +      this.mMainWin.FeedFolderNotificationService.
> +                    addListener(this.FolderListener, this.FOLDER_ACTIONS);

please when breaking lines, put . on the new line
Attachment #8440328 - Flags: review?(mkmelin+mozilla) → review+
Attachment #8442095 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8448045 [details] [diff] [review]
feedUpdatesPart4.patch

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

While touching this you could fix the double spacing in documentations, and the space before function (e.g. function (aFoo....) -> function(aFoo
Attachment #8448045 - Flags: review?(mkmelin+mozilla) → review+
Attachment #8440328 - Attachment is obsolete: true
Attachment #8451357 - Flags: review+
Attached patch feedUpdatesPart4.patch (obsolete) — Splinter Review
Attachment #8448045 - Attachment is obsolete: true
Attachment #8451358 - Flags: review+
updated parts 2 and 4 for comments;  moved gShowFeedSummary global into newsblogOverlay.js.
Keywords: checkin-needed
Backed out for Mozmill failures.
https://hg.mozilla.org/comm-central/rev/b64ae233cdde

https://tbpl.mozilla.org/php/getParsedLog.php?id=43749551&tree=Thunderbird-Trunk
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 33.0 → ---
part4 was missing an include in SearchDialog.xul.
Attachment #8451358 - Attachment is obsolete: true
Attachment #8455647 - Flags: review+
Keywords: checkin-needed
Backed out for the test-message-header.js perma-fail seen in the log below (not the known perma-fail tracked in Bug 1038647). Please verify that this is green on Try before requesting checkin again.
https://hg.mozilla.org/comm-central/rev/43c7f777a37f

https://tbpl.mozilla.org/php/getParsedLog.php?id=43834979&tree=Thunderbird-Trunk
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 33.0 → ---
This might actually be a new perma-fail from m-c. Still investigating.
Thanks for the notice.  (This code doesn't touch anything near header display and couldn't be the culprit for that particular test failure).

I'll take the liberty of adding checkin-needed to get on the queue once mozmill is sorted; you can adjust as you see fit.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.