Closed Bug 1488897 Opened Last year Closed Last year

Use fetch() to read feed opml file, get rid of xpcom method

Categories

(MailNews Core :: Feed Reader, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: alta88, Assigned: alta88)

Details

Attachments

(1 file, 1 obsolete file)

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

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

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +2240,5 @@
>  
>  /**
>   * Import feeds opml file Open filepicker function.
>   *
> + * @returns {Promise} nsIFilePicker or null.

This is a strange thing to return. 
I suggest returning the file url spec, or possibly [spec, filename] (but it's easy enough to get the filename form the file url)
It doesn't look like you use the file for anything but the leafe name anymore

@@ +2459,5 @@
> +    // Get file and file url to open from filepicker.
> +    let fp = await this.opmlPickOpenFile();
> +    let openFile = fp.file;
> +    let openFileUrl = fp.fileURL.spec;
> +    if (!openFile || !openFileUrl) {

neither will ever happen. fp is null if nothing's selected

@@ +2495,5 @@
>      if (aServer && (aServer instanceof Ci.nsIMsgIncomingServer)) {
>        this.mRSSServer = aServer;
>      }
>  
> +    if (!aFile || !aFileUrl || !this.mRSSServer || !aCallback) {

Existing code, but I don't think we should be checking aCallback here. Let that blow up, it's a programming error, no?

@@ +2516,5 @@
> +        return response.text();
> +      })
> +      .then(function(responseText) {
> +        if (responseText) {
> +          opmlDom = (new DOMParser()).parseFromString(responseText, "text/xml");

why not "application/xml" anymore?

@@ +2522,5 @@
> +              opmlDom.documentElement.namespaceURI == FeedUtils.MOZ_PARSERERROR_NS ||
> +              opmlDom.documentElement.tagName != "opml" ||
> +              !(opmlDom.querySelector("body") &&
> +                opmlDom.querySelector("body").childElementCount)) {
> +            // If the OPML file is invalid or empty.

well, not empty, we checked there's responseText. Looks like there's no statusReport for empty response now actually
Attachment #9006676 - Flags: review?(mkmelin+mozilla) → review-
(In reply to Magnus Melin from comment #2)
> Comment on attachment 9006676 [details] [diff] [review]
> opmlFilePicker.patch
> 
> Review of attachment 9006676 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/extensions/newsblog/content/feed-subscriptions.js
> @@ +2240,5 @@
> >  
> >  /**
> >   * Import feeds opml file Open filepicker function.
> >   *
> > + * @returns {Promise} nsIFilePicker or null.
> 
> This is a strange thing to return. 
> I suggest returning the file url spec, or possibly [spec, filename] (but
> it's easy enough to get the filename form the file url)
> It doesn't look like you use the file for anything but the leafe name anymore

not sure why you say it's strange, but since resolving with an array is how i initially did it (tho it seemed clunky), fine.

> 
> @@ +2459,5 @@
> > +    // Get file and file url to open from filepicker.
> > +    let fp = await this.opmlPickOpenFile();
> > +    let openFile = fp.file;
> > +    let openFileUrl = fp.fileURL.spec;
> > +    if (!openFile || !openFileUrl) {
> 
> neither will ever happen. fp is null if nothing's selected

removed.

> 
> @@ +2495,5 @@
> >      if (aServer && (aServer instanceof Ci.nsIMsgIncomingServer)) {
> >        this.mRSSServer = aServer;
> >      }
> >  
> > +    if (!aFile || !aFileUrl || !this.mRSSServer || !aCallback) {
> 
> Existing code, but I don't think we should be checking aCallback here. Let
> that blow up, it's a programming error, no?

sure.

> 
> @@ +2516,5 @@
> > +        return response.text();
> > +      })
> > +      .then(function(responseText) {
> > +        if (responseText) {
> > +          opmlDom = (new DOMParser()).parseFromString(responseText, "text/xml");
> 
> why not "application/xml" anymore?
> 

according to rfc7303 it doesn't matter, but reverted anyway. the DOMParser returns a pretty printed format for debugging even with application/xml.
https://stackoverflow.com/questions/44842217/how-to-choose-either-application-xml-or-text-xml-as-mediatype?noredirect=1

> @@ +2522,5 @@
> > +              opmlDom.documentElement.namespaceURI == FeedUtils.MOZ_PARSERERROR_NS ||
> > +              opmlDom.documentElement.tagName != "opml" ||
> > +              !(opmlDom.querySelector("body") &&
> > +                opmlDom.querySelector("body").childElementCount)) {
> > +            // If the OPML file is invalid or empty.
> 
> well, not empty, we checked there's responseText. Looks like there's no
> statusReport for empty response now actually

empty as in 0 byte file and empty as in valid xml but with no body child nodes (meaning <outline>s).
udpated.
Attachment #9006676 - Attachment is obsolete: true
Attachment #9007269 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9007269 [details] [diff] [review]
opmlFilePicker.patch

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

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +2240,5 @@
>  
>  /**
>   * Import feeds opml file Open filepicker function.
>   *
> + * @returns {Promise} [{nsIFile} file, {String} fileUrl] or null.

I think I'd still go for just returning the fileUrl.
You can get the filename from it simply by 
 name = decodeURIComponent(fileUrl.match(/.*\/(.*)/)[1]);
Attachment #9007269 - Flags: review?(mkmelin+mozilla) → review+
stuff like that is hacky imo, when it can be passed in from an interface like nsIFile. one could also get the fileUrl from getURLSpecFromFile(nsIFile) but that was an anti pattern for the purpose of not using xpcom when unnecessary.
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2fa2b1ec51ad
Use fetch() to read feed opml file, get rid of xpcom method. r=mkmelin
Status: NEW → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.