Get rid of nsISAXXMLReader::parseFromStream

NEW
Unassigned

Status

()

defect
7 years ago
4 years ago

People

(Reporter: hsivonen, Unassigned)

Tracking

({addon-compat})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P2])

Attachments

(1 attachment, 1 obsolete attachment)

This API does synchronous IO on the main thread. All callers should migrate to using nsISAXXMLReader::parseAsync.

Updated

7 years ago
Whiteboard: [Snappy] → [Snappy:P2]
Posted patch WIP (obsolete) — Splinter Review
Attachment #620429 - Flags: feedback?(hsivonen)
Comment on attachment 620429 [details] [diff] [review]
WIP

Looks reasonable assuming that the JavaScript method that's being removed has no callers.
Attachment #620429 - Flags: feedback?(hsivonen) → feedback+
Posted patch Patch v2Splinter Review
There was a test calling it, I've modified it.
The failures in the try run (https://tbpl.mozilla.org/?tree=Try&rev=f5bb26a5fea5) seem unrelated.
Attachment #620429 - Attachment is obsolete: true
Attachment #621040 - Flags: review?(hsivonen)
Comment on attachment 621040 [details] [diff] [review]
Patch v2

Sorry about the delay.

>+  nsCOMPtr<nsIInputStream> aStream;

Please use the variable name "stream" here. The form "aFoo" is reserved for attributes.

>+    cstream.init(fstream, "UTF-8", 0, 0);
>+
>+    let (str = {}) {
>+      let read = 0;
>+      do {
>+        read = cstream.readString(0xffffffff, str);
>+        data += str.value;
>+      } while (read != 0);
>+    }
>+

This does the sort of badness in JS that this bug aims to remove from the C++ side. Can this test be reasonably modified to use nsISAXXMLReader::parseAsync?
Comment on attachment 621040 [details] [diff] [review]
Patch v2

For clarity, setting the review flagged to a non-? value. Setting to r- due to the test case issue, since r+ wouldn't be right.
Attachment #621040 - Flags: review?(hsivonen) → review-
Component: RSS Discovery and Preview → General
Product: Firefox → Core
QA Contact: rss.preview → general
Could you please also add a comment to the IDL interface that asynchronous observers will be required?
Also, when this goes away, please close bug 334494.
(In reply to Marco Castelluccio [:marco] from comment #3)
> Created attachment 621040 [details] [diff] [review]
> Patch v2

There's another problem with this patch:  it breaks parseFromString.

(In reply to Henri Sivonen (:hsivonen) from comment #0)
> This API does synchronous IO on the main thread. All callers should migrate
> to using nsISAXXMLReader::parseAsync.

Henri, could you or someone write up a code sample on how to do this?  Specifically, how to take a string and parse it using parseAsync?
(In reply to Henri Sivonen (:hsivonen) from comment #2)
> Comment on attachment 620429 [details] [diff] [review]
> WIP
> 
> Looks reasonable assuming that the JavaScript method that's being removed
> has no callers.

There are a few callers in comm-central: http://mxr.mozilla.org/comm-central/search?string=parseFromStream
(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> (In reply to Henri Sivonen (:hsivonen) from comment #2)
> > Comment on attachment 620429 [details] [diff] [review]
> > WIP
> > 
> > Looks reasonable assuming that the JavaScript method that's being removed
> > has no callers.
> 
> There are a few callers in comm-central:
> http://mxr.mozilla.org/comm-central/search?string=parseFromStream

I'm not sure those callers are actually calling nsISAXXMLReader.parseFromStream, but other functions on different interfaces.
(In reply to Mark Banner (:standard8) from comment #10)
> (In reply to Florian Quèze [:florian] [:flo] from comment #9)
> > (In reply to Henri Sivonen (:hsivonen) from comment #2)
> > > Comment on attachment 620429 [details] [diff] [review]
> > > WIP
> > > 
> > > Looks reasonable assuming that the JavaScript method that's being removed
> > > has no callers.
> > 
> > There are a few callers in comm-central:
> > http://mxr.mozilla.org/comm-central/search?string=parseFromStream
> 
> I'm not sure those callers are actually calling
> nsISAXXMLReader.parseFromStream, but other functions on different interfaces.

Oh right, it's on the nsIDOMParser interface, sorry for the noise.
(In reply to Alex Vincent [:WeirdAl] from comment #8)
> (In reply to Henri Sivonen (:hsivonen) from comment #0)
> > This API does synchronous IO on the main thread. All callers should migrate
> > to using nsISAXXMLReader::parseAsync.
> 
> Henri, could you or someone write up a code sample on how to do this? 

See
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/feeds/FeedProcessor.js
and
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/feeds/FeedProcessor.js

> Specifically, how to take a string and parse it using parseAsync?

Presumably you need to have an nsIChannel that exposes the data of the string. I don’t know how to do that.
You need to log in before you can comment on or make changes to this bug.