Closed
Bug 481450
Opened 15 years ago
Closed 15 years ago
new XML() to cal.safeNewXML()
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
RESOLVED
FIXED
1.0b1
People
(Reporter: pgoerzen, Assigned: pgoerzen)
Details
Attachments
(1 file, 1 obsolete file)
4.36 KB,
patch
|
mschroeder
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.6) Gecko/2009020409 Iceweasel/3.0.6 (Debian-3.0.6-1) Build Identifier: There are several occurrences of new XML() that ought to use the wrapper. Reproducible: Always
Assignee | ||
Comment 1•15 years ago
|
||
Tested the ICS code, and it works for me. I don't have a Google account so, I couldn't test those providers. Also, I'm guessing that for the Google occurrences, aData.substring(38) can be reduced to aData since safeNewXML takes care of stripping out the <?xml...
Updated•15 years ago
|
Assignee: nobody → pgoerzen
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Updated•15 years ago
|
Attachment #365465 -
Flags: review?(philipp)
Comment 2•15 years ago
|
||
Comment on attachment 365465 [details] [diff] [review] Use the safeNewXML wrapper > // A feed was passed back, parse it. Due to bug 336551 we need to > // filter out the <?xml...?> part. >- var xml = new XML(aData.substring(38)); >+ var xml = cal.safeNewXML(aData.substring(38)); ... >- if (str.indexOf("<?xml ") == 0) { >- str = str.substring(str.indexOf('<', 2)); >- } >- var multistatus = new XML(str); >+ var multistatus = cal.safeNewXML(str); We can even go a step further and make safeNewXML remove all types of <?xml ...?> processing instructions. I'd use regex to do so. Mind posting a new patch to do so?
Assignee | ||
Comment 3•15 years ago
|
||
(In reply to comment #2) > We can even go a step further and make safeNewXML remove all types of <?xml > ...?> processing instructions. I'd use regex to do so. Mind posting a new patch > to do so? I'm a little bit confused. It looks like safeNewXML already removes <?xml. . .?>: return new XML(aStr.replace(/^\s*<\?xml[^>]*>/g, "").trimRight()); Do you want to remove all patterns of the form <?...?> ? If so, I'll modify the regex to do that. Thanks, Peter
Assignee | ||
Comment 4•15 years ago
|
||
(In reply to comment #3) > Do you want to remove all patterns of the form <?...?> ? If so, I'll modify the > regex to do that. Actually, it looks like XML.ignoreProcessingInstructions is turned on by default, which should ignore all processing instructions except the <?xml. . .?> element.
Comment 5•15 years ago
|
||
Oh sorry, I didn't read safeNewXML in detail. What I noticed though that in the calling code, the xml part is removed by hand. In gdata for example, I used aData.substring(38) to remove the <?xml part, this could be shortened to just pass aData since safeNewXML takes care. The comment about the respective bug should be moved into safeNewXML. The same goes for the caldav code that removes it with substring/indexOf. str could also be passed directly here.
Assignee | ||
Comment 6•15 years ago
|
||
Ok, I've changed aData.substring(38) to aData and documented the bug number in safeNewXML. All the substring/indexOf code appears to be removed from caldav already.
Attachment #365465 -
Attachment is obsolete: true
Attachment #365465 -
Flags: review?(philipp)
Updated•15 years ago
|
Attachment #365654 -
Flags: review?(philipp)
Comment 7•15 years ago
|
||
Thanks for taking care! Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/6fadebfb15d1> -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Comment 8•15 years ago
|
||
Comment on attachment 365654 [details] [diff] [review] remove substring(38), document bug number in safeNewXML This patch got reviewed and checked in, but not marked as r+.
Attachment #365654 -
Flags: review?(philipp) → review+
Updated•14 years ago
|
Target Milestone: 1.0 → 1.0b1
You need to log in
before you can comment on or make changes to this bug.
Description
•