Closed Bug 1345141 Opened 4 years ago Closed 4 years ago

Miscellaneous feed code deduplication, modernization, simplification, and fix of a deprecation

Categories

(MailNews Core :: Feed Reader, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 55.0

People

(Reporter: alta88, Assigned: alta88)

Details

Attachments

(4 files, 2 obsolete files)

No description provided.
Attached patch newFeed.patch (obsolete) — Splinter Review
Assignee: nobody → alta88
Attachment #8844507 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8844507 [details] [diff] [review]
newFeed.patch

Part1: Deduplicate and simplify code for new Feed object creation
Bug 1345141 - Part2: Modernize feed xpcom factory code
Attachment #8844508 - Flags: review?(mkmelin+mozilla)
Attached patch parser.patchSplinter Review
Bug Bug 1345141 - Part3: Ensure parser returns array, use includes not indexof, use for..of in node loops
Attachment #8844511 - Flags: review?(mkmelin+mozilla)
Attached patch xmlbase.patch (obsolete) — Splinter Review
Bug 1345141 - Part4: - Remove baseURI from Atom parser, as xml:base is deprecated by Bug 903372; handle relative urls
Attachment #8844515 - Flags: review?(mkmelin+mozilla)
Attached patch newFeed.patchSplinter Review
Bug 1345141 - Part1: Deduplicate and simplify code for new Feed object creation
Attachment #8844507 - Attachment is obsolete: true
Attachment #8844507 - Flags: review?(mkmelin+mozilla)
Attachment #8845011 - Flags: review?(mkmelin+mozilla)
Attachment #8844508 - Flags: review?(mkmelin+mozilla) → review+
Attachment #8844511 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8844515 [details] [diff] [review]
xmlbase.patch

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

::: mailnews/extensions/newsblog/content/feed-parser.js
@@ +580,4 @@
>      tags = this.childrenByTagNameNS(channel, FeedUtils.ATOM_IETF_NS, "link");
> +    aFeed.link = this.findAtomLink("self", tags, contentBase);
> +    if (!aFeed.link)
> +      aFeed.link = this.findAtomLink("alternate", tags, contentBase);

would be slightly nicer as:

aFeed.link = this.findAtomLink("self", tags, contentBase) || this.findAtomLink("alternate", tags, contentBase);

@@ +695,3 @@
>        }
>        else
> +        item.xmlContentBase = contentBase;

while you're here, please ensure all these if-elses have braces
Attachment #8844515 - Flags: review?(mkmelin+mozilla) → review+
Attachment #8845011 - Flags: review?(mkmelin+mozilla) → review+
Attached patch xmlbase.patchSplinter Review
updated for comments.
Attachment #8844515 - Attachment is obsolete: true
Attachment #8846657 - Flags: review+
xmlbase.patch should be applied last.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.