717564 - Get rid of nsISAXXMLReader::parseFromStream

Status: NEW

(Core :: General, defect)

Description

[Henri Sivonen (:hsivonen)]

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

Comment 1

[Marco Castelluccio [:marco]]

Attachment: WIP

Comment 2

[Henri Sivonen (:hsivonen)]

Comment on attachment 620429 [details] [diff] [review]
WIP

Looks reasonable assuming that the JavaScript method that's being removed has no callers.

Comment 3

[Marco Castelluccio [:marco]]

Attachment: Patch v2

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.

Comment 4

[Henri Sivonen (: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 5

[Henri Sivonen (:hsivonen)]

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.

Comment 6

[Alex Vincent [:WeirdAl]]

Could you please also add a comment to the IDL interface that asynchronous observers will be required?

Comment 7

[Alex Vincent [:WeirdAl]]

Also, when this goes away, please close bug 334494.

Comment 8

[Alex Vincent [:WeirdAl]]

(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?

Comment 9

[Florian Quèze [:florian]]

(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

Comment 10

[Mark Banner (:standard8)]

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

Comment 11

[Florian Quèze [:florian]]

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

Comment 12

[Henri Sivonen (:hsivonen)]

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