Open
Bug 717564
Opened 12 years ago
Updated 2 years ago
Get rid of nsISAXXMLReader::parseFromStream
Categories
(Core :: General, defect)
Core
General
Tracking
()
NEW
People
(Reporter: hsivonen, Unassigned)
Details
(Keywords: addon-compat, Whiteboard: [Snappy:P2])
Attachments
(1 file, 1 obsolete file)
8.08 KB,
patch
|
hsivonen
:
review-
|
Details | Diff | Splinter Review |
This API does synchronous IO on the main thread. All callers should migrate to using nsISAXXMLReader::parseAsync.
Updated•12 years ago
|
Whiteboard: [Snappy] → [Snappy:P2]
Comment 1•12 years ago
|
||
Attachment #620429 -
Flags: feedback?(hsivonen)
Reporter | ||
Comment 2•12 years ago
|
||
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+
Comment 3•12 years ago
|
||
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)
Reporter | ||
Comment 4•12 years ago
|
||
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?
Reporter | ||
Comment 5•12 years ago
|
||
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-
Updated•12 years ago
|
Component: RSS Discovery and Preview → General
Product: Firefox → Core
QA Contact: rss.preview → general
Comment 6•12 years ago
|
||
Could you please also add a comment to the IDL interface that asynchronous observers will be required?
Comment 7•12 years ago
|
||
Also, when this goes away, please close bug 334494.
Comment 8•12 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
(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.
Reporter | ||
Comment 12•11 years ago
|
||
(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.
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•