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

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, enhancement)

Comment 1

[alta88]

Attachment: opmlFilePicker.patch

From bug 1356780.

Comment 2

[Magnus Melin [:mkmelin]]

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

Comment 3

[alta88]

(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).

Comment 4

[alta88]

Attachment: opmlFilePicker.patch

udpated.

Comment 5

[Magnus Melin [:mkmelin]]

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]);

Comment 6

[alta88]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2fa2b1ec51ad
Use fetch() to read feed opml file, get rid of xpcom method. r=mkmelin