Created attachment 79694 [details] [diff] [review] check for xml and clearly check for text/ at the beginning of the string The current code works for any mime type beginning with 'text', and will treat a mime type of texture/map as if it were text. It's my belief that we probably don't want that. Although I'm adding the same sort of bug by checking for xml instead of +xml or -xml or xml as the last n chars or ... The other problem with Find("text") is from a readability perspective, it does a Find without comparing against 0 when it really wants to know if the first n chars are text. this costs a bit in the failure case and confuses people who know that sometimes people think that 0 is a failure case and wonder if the code should use kNotFound... Anyways. the key fix for this bug is to check for xml because that's what xul, xbl and svg are. fwiw biesi wrote this patch first...
So... why are we doing this? If I understand rfc 2397 and rfc2396 correctly, spaces in a data: url should be encoded as %20 (rfc 2396, section 2.4.3) and spaces that are not encoded should be stripped, per RFC 2396, appendix E....
this is parity, we're already doing it for text/* (well text*), i'd like us to do it for xml things. Currently you can't use %20 because of some other bug. for testing purposes, encoding spaces is a royal pain even if it were to work.
I'd probably have used something like Substring(mContentType, 0, 5).Equals("text/") darin: Why is this whitespace removing done, anyway? A base64 decoder should ignore whitespace anyway (so we could remove the lBase64 from the if, in any case); why do we want to remove them from non-text/ mimetypes?
eh, nevermind my last comment, I just read bz's comment. However, I'd still argue that we shouldn't strip them if the data is base64 encoded, because the B64 decoder should strip spaces anyway, so the extra stripping just takes time.
Comment on attachment 79694 [details] [diff] [review] check for xml and clearly check for text/ at the beginning of the string can you use strncmp(...) == 0 instead of !strncmp(...), I find that more readable r=biesi with that change
Attachment #79694 - Flags: review+
darin, could you super-review?
Comment on attachment 79694 [details] [diff] [review] check for xml and clearly check for text/ at the beginning of the string sr=darin (sorry for not reviewing this sooner)
Attachment #79694 - Flags: superreview+
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
*** Bug 142523 has been marked as a duplicate of this bug. ***
I click on the link, and nothing happens. What should happen? (Mozilla 1.2., Win98)
benc: Hm, interesting that it doesn't load - what should happen is that an empty (I think) page should load. What should not happen is that you get an XML Parsing error; that error would occur because spaces were stripped (which was what this patch changed). try pasting the url in the urlbar and pressing enter maybe.
No response when pasting into Location (Mozilla 1.1, Win98).
benc: that's strange... here, on a recent nightly, I get the expected blank page. do you get any message in the JS Console?
Is that all it's supposed to do? Oh. that is what happens to me. QA people like testcases that say "this is working great!" if everything works. Lots of us are blackbox testers, so we want to just click and go.
benc, for your testing pleasure, a testcase that shows a "this works" message :) (click the changed URL)
VERIFIED: Mozilla 1.3a, Win98+MacOS X
VERIFIED: Mozilla 1.3a, Linux
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.