Mozilla/5.0 (Windows; U; Windows NT 5.1; sv-SE; rv:1.6b) Gecko/20031112 Firebird/0.7+ The error pages that are displayed when a network error has occurred do no longer show the right non-ASCII characters. This is a regression between the 20031111 and 20031112 Firebird nightlies. Steps to reproduce: 1. Set the pref "user_pref("browser.xul.error_pages.enabled", true);" 2. In the en-US.jar file, add a non-ASCII character to the locale/en-US/global/appstrings.properties file. For example, add a "ö" (o with two dots) by changing the line: "dnsNotFound=%S could not be found. Please check the name and try again." to "dnsNotFound=%S could not be found. Please check the name and tr\u00F6 again." 3. Repack the en-US.jar file. 4. Open the browser and try open a bogus address, for example, www.yyyyyyyy.org. Actual Results: Error page tells me that, "www.yyyyyyyy.org could not be found. Please check the name and trö again." Expected Results: See a error page with the message: "www.yyyyyyyy.org could not be found. Please check the name and trö again."

seems to be a regression of my fix for bug 44272. Somewhere, xul handling code is treating UTF-8 as the charset of the current locale (in this case iso-8859-1). Possibly related with bug 91190, but not sure.

The escaping is done at http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#2674 (nsDocShell::LoadErrorPage) The unescaping is done at http://lxr.mozilla.org/seamonkey/source/docshell/resources/content/netError.js#9 and company. This is another impedance mismatch between nsEscape and JS escape(), this time in the opposite direction (nsEscape and JS unescape() doing different things -- nsEscape produces %XX%XX for the UTF-8 encoding of non-ASCII chars, while JS treats that as being two Unicode chars). The real solution is that we need a JS-compatible mode for nsEscape/nsUnescape, I think... Depending on server support for %u escapes, we may want to make that the default mode.

Thanks for pinpointing where the problem lies. > The real solution is that we need a JS-compatible > mode for nsEscape/nsUnescape, How about the other way around? I know doing that way is more work, but if we pretend that Mozilla-specific URIs are comformant to IRIs (see below), we have to do that way. That is, we have to add unescapeURI (or something) (to DOM to( use in url-unescaping. This is to resurrect the code I just removed in bug 44272 with a different name.... To fix view-selected-source, we also need escapeURI (or sth. like that). Hmm.. this is a full cycle... > Depending on server support for %u escapes, we may want > to make that the default mode. No, this is out of question both practically and in terms of the standard compliance. It'd have been great if URL-escaping had been defined that way (or URL had been made to be always in UTF-8, IRI) from the very beginning. Unfortunately, that didn't happen and the best thing we can ask for is to make all http servers understand url-escaped UTF-8 (IRI). See http://www.w3.org/2003/Talks/0904-IUC-IRI/

Wait. So JS escape() does NOT do "url-escaping"? In other words, escaping a string with JS escape() and constructing a URI out of it is likely to fail when someone actually tries to send it to a server? I find it somewhat hard to believe the the escape() in IE has such behavior...

Or do you mean that what escape() produces is correct per IRI spec but not understood by servers in practice?

What JS escape() does is to escape per ECMA 262 (ECMAscript standard edition 3). However, JS escape() in that standard is DIFFERENT from IRI escaping. In IRI escaping, all characters have to be represented in UTF-8 and escaped with their byte values (%HH%HH for characters bet. U+0080 and U+07FF, %HH%HH%HH for chars bet. U+0800 and U+0FFFF and so forth). In JS escape(), characters below U+0100 is escaped with '%HH' but those above U+0100 are escaped with %uHHHH (and presumably, non-BMP characters are escaped with '%uHHHH%uLLLL') That's why I wrote 'if ... had been defined ...'. We've had enough hard time making servers understand IRI escaping so that it's all but impossible to make them understand ECMAscript escape() as well (there's no justification because ECMAscript escape() is different from IRI spec.)

Oh. So in other words, as I said, using escape() in JS does not produce a usable URI? I still find it hard to believe that that's what IE does, but if it's so it's so... In any case, sounds like we just nead IRI-escaping and IRI-unescaping in JS. Luckily for us, see sections 15.1.3.1 and 15.1.3.3 (decodeURI and encodeURI) of http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf Oh, and we happen to implement those. ;) Note that those methods will in fact just convert the native UTF-16 representation of JS to UTF-8 and escape (or conversely, unescape and do a UTF8-to-UTF16 conversion). So they're not quite like what we used to have with its native encoding stuff... but for chrome, which is all UTF-8 anyway, it's all the same.

tested both parts of that patch, even.... (well, testes that a URI of 'ö') gives the right error page; if someone wants to test the error message that would be nice too. We should probably do a sweep of all escape() and unescape() calls in lxr and change them as needed...

I've just made exactly the same patch but you went there first :-) Yes, I agree that we have to do tree-wide sweep. Do you want to do it here or just fill two holes (already found) first and do that later? As for IE, I think either most JS script authors don't bother to escape URLs in their scripts or are more familiar with ECMAscript standard than Mozilla developers have been so that they use encodeURI(Component). Note that MS IE turns on 'Always send URLs in UTF-8' by default despite the fact that IRI is not so well supported. It's one of rare occasions that IE shows the willingness to support the standard breaking the backward compatibility. Or they may have already fixed their IIS to support IRI well, in which case using encodeURI(Component) would just work for IIS served pages.

Either way for the sweep. We have a goodly number of consumers of this stuff in the tree (72 JS callers of escape(), and 25 of unescape()). Some of these could continue using escape/unescape, I bet, and some others probably really want to be using nsITextToSubURI..... If it looks like the patch will need non-rubberstamp reviews, then multiple smaller patches are better, of course.

In addition to those 97 cases in JS files (which agrees with my tally), there are one escape and one unescape in xul files. You're right that some of them are not trivial replacement. So, as the first installment of smaller patches, why don't you go ahead with attachment 1355438?

Comment on attachment 135543 [details] [diff] [review] like this Alright, then. ;)

Comment on attachment 135543 [details] [diff] [review] like this r=jshin

Comment on attachment 135543 [details] [diff] [review] like this btw, don't forget to fix toolkit/components/viewsource/content/viewPartialSource.js for firebird.

Just to give some 'feel' for what has to be done. Most cases are pretty obvious, but in some cases I'm not sure (especially in mailnews) It turned out that there are three *xml files with escape/unescape(). Is there any other file type than js, xul and xml?

Quick comments from a quick glance: 1) There is no such thing as "unencodeURIComponent". You want "decodeURIComponent". Same for "unencodeURI". 2) contentAreaUtils.js -- escaping is done by nsStandardURL or somesuch; decodeURIComponent is correct there. 3) Did line endings change in pref-manager.js or something? 4) In irc/xul/content/commands.js, would it be better to concatenate it all together and then simply encodeURI? I'm really not sure which is better... 5) I'd assume ecmaUnescape would get replaced with decodeURIComponent? That's about it, though ecmaUnescape could likely be removed altogether.... Unless we have no window object in that code?

Something got wrong with me to use 'unencode' in place of 'decode' :-). I removed ecmaEscape/ecmaUnescape completely. I fixed the line ending problem in pref-manager.js (OT: my source tree is exported via NFS/samba from another computer to my build machine - Linux/Win2k dual boot - and I hadn't had this problem before, but somehow it's happening now. perhaps because I changed my line-ending convention in cygwin to Unix from DOS.....) So far, history search, bookmark search, search side bar, url bar, and mail search have been verified to work fine with non-ASCII characters. I still hava to check mail attachment, irc, mailto: and other things.

Comment on attachment 135644 [details] [diff] [review] update Boris, rbs can you please review.

*** Bug 225874 has been marked as a duplicate of this bug. ***

Comment on attachment 135644 [details] [diff] [review] update Not by freeze. And some of this (eg the venkman changes) needs actual module owner review. I will try to review as much of this as I can on Thursday, but I'm not likely to have time before then.

Comment on attachment 135543 [details] [diff] [review] like this And this is no good; the view-source change should really use encodeURIFragment.

I do think that if we can roust up reviews and testing for this quickly this is worth holding beta for.... :( If we can't, then we can't.

bz, please, note that I did not ask for r/sr because I hadn't tested it sufficiently. (it was not much more than a global search and replacement.) I was thinking about splitting the patch into smaller pieces (mailnews/mail, irc, verkman, navigator, etc) to expedite testing and reviewing. Anyway, I'd be glad to see it go in 1.6b, but as you wrote, if we can, we can, but we can't if we can't :-)

This is a sneaky bug due the uncertainties of those numerous places in the tree with escape/unescape. One cannot tell what is going to happen. I cannot help feeling unconfident about the product. Because bug 44272 was made very late in the cycle, it should perhaps be backed out and be re-checked in at the next cylce, together with the fixups of this bug. There isn't (and hasn't been) any particular urgency with bug 44272. What do you think?

> There isn't (and hasn't been) any particular urgency with bug 44272 Actually, it fixed a number of outstanding problems we had on various sites...

These are/were known, and it seems to me that if they have waited that long, they can wait just another cycle. Compared with the unknown that has been created. What I am saying is that the fix for bug 44272 is good, but was perhaps poorly timed.

How do you want to move forward for 1.6b and 1.6? One approach is to back out bug 44272. Another is to get this patch in soon so that it gets testing for beta and we can try to fix problems in time for final. Which do you prefer?

I don't have cycles to spend on fixing this in the near future, though I could review (but not till after thanksgiving weekend, at this point).... If jshin doesn't have time to fix it either, then we should probably back out.

I have a little cycle today so that I want to give it a shot before backing out (I have to admit that the timing was not great for bug 44272 fix). Let me try to split the latest patch into three parts, mailnews part, extension part (irc, verkman) and the rest and see if we can make it. The part I'm least sure of is mailnews. I'm adding people working on irc, verkman and mailnews to cc. Some of them are outside the US so probably they can help this weekend.

IIRC in msgHdrViewOverlay.js you need to use encodeURIComponent - displayName eventually becomes a &filename= URI parameter.

>@@ -1274,15 +1274,15 @@ function ComposeStartup(recycled, aParam > if (params.bodyIsLink) > { > var body = msgCompFields.body; > if (gMsgCompose.composeHTML) > { > var cleanBody; > try { >- cleanBody = unescape(body); >+ cleanBody = decodeURIComponent(body); > } catch(e) { cleanBody = body;} > > msgCompFields.body = "<BR><A HREF=\"" + body + "\">" + cleanBody + "</A><BR>"; > } > else > msgCompFields.body = "\n<" + body + ">\n"; > } body is a URI, we're cleaning it so we can put unicode into the message, but IMHO decodeURI would do it right where decodeURIComponent might not. (Of course, there'a a side issue of a URI that contains an HTML entity...)

This patch include everything other than mailnews, mail and extension. I'm pretty sure that this works fine.

Thanks, neil, for catching bugs. In one of cases, I was inconsistent (in mail I used on and in mailnews, I used the other).

I fixed several typos ('URiComponent' in place of 'URIComponent')

neil and rbs (both not in the US :-)) do you care to review? re. comment #21 and attachment 135443 [details] [diff] [review], I forgot to include it in my patch, but replacing escape with encodeURIComponent (instead of using encodeURI for the whole thing) should work, as you wrote.

Note that the first patch (to viewpartialsource and neterror) needs to be updated too... it's not included in jshin's patches. :(

I added bz's patch with the change he mentioned. It also includes the corresponding change for firebird (int toolkit directory)

*** Bug 226765 has been marked as a duplicate of this bug. ***

Comment on attachment 136481 [details] [diff] [review] update for browser part (include view source patch) asking for r/sr. Given a lot of bugs have been filed for a short period (since JS escape/unescape fix), I guess we'll be able to catch any new bug (that is not caught in r/sr process) pretty soon. Let's be brave :-) I've been using an optimized build with this patch for the last 24hours(?) and haven't yet come across a problem (in keyword search, bookmark, mail search, url bar) although it doesn't tell much.

Comment on attachment 136363 [details] [diff] [review] mail/mailnews patch (thunderbird included) asking for r/sr. a couple of places I have some doubt about: In mail/components/prefwindow/content/pref-themes.js: > - file = 'file:///' + escape(filePath.replace(/\\/g,'/')); > + file = 'file:///' + encodeURIComponent(filePath.replace(/\\/g,'/')); > InstallTrigger.installChrome(InstallTrigger.SKIN, file, getName(file)); We might be better off using encodeURI over the whole thing(including 'file:///')

Comment on attachment 136364 [details] [diff] [review] extension patch : IRC, Verkman, asking for r/sr.

*** Bug 226982 has been marked as a duplicate of this bug. ***

Comment on attachment 136363 [details] [diff] [review] mail/mailnews patch (thunderbird included) >- dump("SaveAsFile from XUL\n"); >- var subject = document.getElementById('msgSubject').value; >- GetCurrentEditor().setDocumentTitle(subject); >+ dump("SaveAsFile from XUL\n"); >+ var subject = document.getElementById('msgSubject').value; >+ GetCurrentEditor().setDocumentTitle(subject); Note sure what this is doing here... > var file = ''; >- file = 'file:///' + escape(fp.file.path.replace(/\\/g,'/')); >+ file = 'file:///' + encodeURIComponent(fp.file.path.replace(/\\/g,'/')); > doXPIInstall(file, getName(file)); I'm not surprised you were confused by this, it makes certain possibly unjustified assumptions about files and URLs. Here's some code which makes no assumptions: const nsIIOService = Components.interfaces.nsIIOService; const nsIFileProtocolHandler = Components.interfaces.nsIFileProtocolHandler; const nsIURL = Components.interfaces.nsIURL; var ioService = Components.classes['@mozilla.org/network/io-service;1'].getService(nsIIOService ); var fileProtocolHandler = ioService.getProtocolHandler("file").QueryInterface(nsIFileProtocolHandler); var url = fileProtocolHandler.newFileURI(fp.file).QueryInterface(nsIURL); // extensions var xpi = {}; xpi[decodeURIComponent(url.fileBaseName)] = url.spec; InstallTrigger.install(xpi); // themes InstallTrigger.installChrome(InstallTrigger.SKIN, url.spec, decodeURIComponent(url.fileBaseName)); In the mean time I suggest you encode the whole file:/// URI. Other than that, what sort of testing (if any) were you looking for? So far I haven't tested anything, I've only read the differences.

*** Bug 226999 has been marked as a duplicate of this bug. ***

I can see this also on Mac OS 10.2.8. Changing Hardware to All.

*** Bug 227270 has been marked as a duplicate of this bug. ***

Thanks, neil. The first chunk you're not sure of is spurrious and should haven't been there. As for the second, I'll call encodeURI() over the whole thing as you suggested. In mailnews, I tested search with non-ASCII strings and saving an attachment and attachments with non-ASCII filenames. Testing addressbook would be nice, but I guess it'd be all right if you don't find anything obviously wrong in the patch. neil, do you want me to upload a new file? Otherwise, please stamp 'r' on attachement 136363 with the understanding that I'll make changes outlined above. mscott, it'd be nice if you can review attachment 136363 [details] [diff] [review] as you see fit. There are a lot of dupes and I guess we all want to get this and other two patches in.

Comment on attachment 136363 [details] [diff] [review] mail/mailnews patch (thunderbird included) OK, so based on comment #47, I'm convinced. Somehow I don't expect anybody to fix that faulty file to url conversion this year ;-)

Comment on attachment 136363 [details] [diff] [review] mail/mailnews patch (thunderbird included) can you fix the missing newlines while you're at it?

Comment on attachment 136363 [details] [diff] [review] mail/mailnews patch (thunderbird included) thanks for r/sr. asking for a1.6 hope it's not too late. it'd be nice get r/sr for other two patches, soon

Comment on attachment 136481 [details] [diff] [review] update for browser part (include view source patch) >Index: ./xpfe/components/related/resources/related-panel.js >@@ -65,17 +65,17 @@ function (oSubject, sMessage, sContextUr >- return escape(sCDT); >+ return encodeURI(sCDT); This should be encodeURIComponent, imo. With that, r=bzbarsky

Comment on attachment 136481 [details] [diff] [review] update for browser part (include view source patch) sr=rbs Useless XXX comment: + try { // XXX_JSHIN + item.setAttribute("tooltiptext", decodeURI(attachment.url)); } catch(e) { item.setAttribute("tooltiptext", attachment.url); } Care to also post on n.p.m.seamonkey that people shouldn't use escape/unescape anymore to tweak URIs, as they are now in parity with IE: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/script56/html/ js56jsmthencodeuricomponent.asp

*** Bug 226997 has been marked as a duplicate of this bug. ***

Comment on attachment 136481 [details] [diff] [review] update for browser part (include view source patch) thanks for r/sr. I'll fix two issues raised in the review comment. I'll post to the newsgroup about escape/unescape vs en/decodeURI(Component).BTW, it's not so much about IE parity as about the standard compliance :-). asking for 1.6b.There's some risk, but we have to either back out JS escape/unescape fix (, which would keep Mozilla NOT compliant to the ECMAscript standard) or take the risk. Given that a number of bugs have been filed over a short time-span (since JS escape/unescape was fixed), I'm pretty sure that whatever bug not fixed/introduced with this fix will be caught during the beta period.

Comment on attachment 136364 [details] [diff] [review] extension patch : IRC, Verkman, >Index: ./extensions/irc/xul/content/static.js >@@ -1201,17 +1201,17 @@ function parseIRCURL (url) > ary = rest.match (/^\/([^\,\?\s\/]*)?\/?(,[^\?]*)?(\?.*)?$/); > if (!ary) > { > dd ("parseIRCURL: rest split failed ``" + rest + "''"); > return null; > } > > rv.target = arrayHasElementAt(ary, 1) ? >- ecmaUnescape(ary[1]).replace("\n", "\\n") : ""; >+ encodeURIComponent(ary[1]).replace("\n", "\\n") : ""; I'll do : s/encodeURIComponent/decodeURIComponent/ >Index: extensions/irc/xul/content/static.js >@@ -1201,17 +1201,17 @@ function parseIRCURL (url) > > rv.target = arrayHasElementAt(ary, 1) ? >- ecmaUnescape(ary[1]).replace("\n", "\\n") : ""; >+ encodeURIComponent(ary[1]).replace("\n", "\\n") : ""; The same here.

Comment on attachment 136364 [details] [diff] [review] extension patch : IRC, Verkman, Am I correct in assuming that encode/decodeURI deal with unicode characters by converting them to UTF-8 and then escaping with %XX? If so, then ecmaEscape and ecmaUnescape are still useful. They work according to the ECMA spec, which is to say: For those characters being replaced whose code point value is 0xFF or less, a two-digit escape sequence of the form %xx is used. For those characters being replaced whose code point value is greater than 0xFF, a four-digit escape sequence of the form %uxxxx is used. While Mozilla's escape/unescape may have finally been fixed to comply, the latest versions of chatzilla and venkman can be installed on older, broken versions of mozilla. We try to make them work there, wherever possible. @@ -207,27 +207,27 @@ function pm_arrayupdate(prefName) PrefManager.prototype.stringToArray = function pm_s2a(string) { if (string.search(/\S/) == -1) return []; var ary = string.split(/\s*;\s*/); for (var i = 0; i < ary.length; ++i) - ary[i] = toUnicode(unescape(ary[i]), PREF_CHARSET); + ary[i] = decodeURI(ary[i]); return ary; } PrefManager.prototype.arrayToString = function pm_a2s(ary) { var escapedAry = new Array() for (var i = 0; i < ary.length; ++i) - escapedAry[i] = escape(fromUnicode(ary[i], PREF_CHARSET)); + escapedAry[i] = encodeURI(ary[i]); This code deals with seralizing javascript arrays into a string pref in a general way. It is not URI specific. This should probably become ecmaUnescape and ecmaEscape to be safe. Also remember that if you change the escape mechanism for prefs, you're going to break everyone's profile, which is a Bad Thing. - display(unescape(MSG_TEST_CTLCHR), "PRIVMSG", sampleUser, me); - display(unescape(MSG_TEST_COLOR), "PRIVMSG", sampleUser, me); + // XXX unescape() may just work as well here? + display(decodeURIComponent(MSG_TEST_CTLCHR), "PRIVMSG", sampleUser, me); + display(decodeURIComponent(MSG_TEST_COLOR), "PRIVMSG", sampleUser, me); This is not a URI either. It should be left as is, or changed to ecmaUnescape. var encodedMatchText = fromUnicode(matchText, eventData.sourceObject); var anchor = document.createElementNS("http://www.w3.org/1999/xhtml", "html:a"); anchor.setAttribute ("href", eventData.network.getURL() + - ecmaEscape(encodedMatchText)); + encodeURIComponent(encodedMatchText)); This has already been encoded, as indicated by the name and the call to fromUnicode. If you use encodeURIComponent here, you're going to double encode the URL. The correct fix is probably to call encodeURIComponent on matchText, and remove the fromUnicode call. rv.target = arrayHasElementAt(ary, 1) ? - ecmaUnescape(ary[1]).replace("\n", "\\n") : ""; + encodeURIComponent(ary[1]).replace("\n", "\\n") : ""; You've replaced an unescape with an encode. Sounds backwards to me. +++ extensions/irc/xul/content/prefs.js 26 Nov 2003 15:17:15 -0000 @@ -139,17 +139,17 @@ function initPrefs() client.charset = client.prefs["charset"]; initAliases(); } function pref_mungeName(name) { var safeName = name.replace(/\./g, "-").replace(/:/g, "_").toLowerCase(); - return ecmaEscape(safeName); + return encodeURIComponent(safeName); } That's not a URI, it's the name of a pref. Some folks might lose their network prefs if you did that. I don't want to see this land without some real testing. Please fix these issues and we'll spin an XPI for hacksrus.com.

Thanks for review and I'll make changes. There are some issues to resolve, though. > Am I correct in assuming that encode/decodeURI deal with unicode characters by > converting them to UTF-8 and then escaping with %XX? Yes, that's what ECMA 262 15.1.3 says and what Mozilla JS engine implemented. Note that en/decodeURI(Component) haven't been touched. They've been always compliant to the ECMA 262. en/decodeURIComponent() are not much URI-specific and work similarly to the old broken escape/unescape() when the 'document' encoding is UTF-8, which is the case in IRC and Venkman. Unfortunately, en/decodeURIComponent() are not a drop-in replacement for the old escape/unescape because they don't agree on which characters to escape and which not to. > - escapedAry[i] = escape(fromUnicode(ary[i], PREF_CHARSET)); > + escapedAry[i] = encodeURI(ary[i]); > This code deals with seralizing javascript arrays into a string pref in a > general way. It is not URI specific. This should probably become ecmaUnescape > and ecmaEscape to be safe. > Also remember that if you change the escape mechanism for prefs, you're going > to break everyone's profile, which is a Bad Thing. As long as PREF_CHARSET is UTF-8 (which is the case as far as I can tell), my change(assuming I use en/decodeURIComponent() instead of en/decodeURI) wouldn't make a great difference. Still, there's a difference as noted above. Using ecmaEscape() instead of encodeURIComponent(which would for sure a good idea if we began from the scratch) would introduce a even more extensive backward compatibility problem you're afraid of, wouldn't it? I don't know how to preserve old profiles at the same time making a new version work with older Mozilla here(if ecmaEscape()/ecmaUnescape() had been used, it'd be possible as in the following case). > function pref_mungeName(name) > { > var safeName = name.replace(/\./g, "-").replace(/:/g, "_").toLowerCase(); > - return ecmaEscape(safeName); > + return encodeURIComponent(safeName); > } > That's not a URI, it's the name of a pref. Some folks might lose their > network prefs if you did that. OK. To keep old profiles work with a new version and to accomodate older Mozilla, we have to keep ecmaEscape() around. Where is the corresponding 'unmunge' code, btw?

FYI, the mail patch breaks the ability to install extensions and themes and thunderbird when they are local files. That's the primary way we install themes and extensions too. We probably need to fix that before checking this in. I'm looking into it.

Ignore that last comment. When I applied the patch, I did not read Neil's comment: "In the mean time I suggest you encode the whole file:/// URI. Other than that, what sort of testing (if any) were you looking for? So far I haven't tested anything, I've only read the differences." for pref-themes.js and pref-extensions.js. I made that change and themes/extensions work again.

I addressed rgina's concerns. As I wrote before, for some of them, we need to decide what to do, in terms of backward compatibility (with old profiles and with old versions of Mozilla) so that this patch still has some rough edges to smooth out. Anyway, as bz wrote to me in an email, we may not have to block 1.6b for IRC and Venkman. rgina also thinks that it needs to be tested more thoroughly before landing. So, I guess we'll go with two other patches(pending 1.6b approval) for 1.6b and bake this patch (for IRC and Venkman) for a while. BTW, mscott, thanks for testing the mail(news) patch.

Comment on attachment 136363 [details] [diff] [review] mail/mailnews patch (thunderbird included) a=asa (on behalf of drivers) for checkin to 1.6beta

Comment on attachment 136481 [details] [diff] [review] update for browser part (include view source patch) a=asa (on behalf of drivers) for checkin to 1.6beta

browser and mailnews/mail patches checked in. thanks all. btw, the seemingly spurrious diff. (comment #43) turned out due to that the old file had spurrious CR at the end of three lines affected. (no CR is used elsewhere in the file). As for the missing newlines, I guess they were missing in the original file so that my check-in added them, which is harmless.

Now that two patches (for components more frequently used than those fixed by the third) have been landed, what should I do? The blocking flag was removed, but it's still listed in the blocking bug list(I realized that the URL in the tinder box was made manually). Should I mark this as fixed and work on the rest in a separate bug or just keep this open with the understanding that it doesn't block 1.6b? rgina thinks that Venkman patch is rather risk-free compared with IRC patch. Besides, Venkman seems to get more exposures than chatzilla. Therefore, we might want to get that part in.

This is rather straightforward replacing escape/unescape with encodeURIComponent/decodeURIComponent. I replaced two instances of encodeURI() in attachment 136740 [details] [diff] [review] with encodeURIComponent()

I think you checked in a file with a conflict. I just fixed it but be careful next time :). It's a dangerous world out there... http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/mail/components/compose/content&command=DIFF_FRAMESET&file=MsgComposeCommands.js&rev1=1.26&rev2=1.27&root=/cvsroot

Ooops. sorry and thanks for fixing it. I removed the identical conflict in the corresponding file in mailnews tree, but apparently missed it in TB tree. I just 'grepped' all ohter files I checked in to make sure there's no more conflict sneaked in and there is no more.

*** Bug 128716 has been marked as a duplicate of this bug. ***

Comment on attachment 136770 [details] [diff] [review] Venkman only patch r=rginda

Comment on attachment 136770 [details] [diff] [review] Venkman only patch Asking for sr. In a recent poll (conducted at builders.com??), Mozilla composer was ranked the third as a 'web developer' platform thanks largely to Venkman. Given that, I guess we don't want to break it in 1.6(b). If possible, let's get this in as well.

Comment on attachment 136364 [details] [diff] [review] extension patch : IRC, Verkman, sorry for spamming.

Comment on attachment 136770 [details] [diff] [review] Venkman only patch sr=me, and please get it in tonight -- thanks! /be

Comment on attachment 136770 [details] [diff] [review] Venkman only patch brendan, thansk for quick sr. while you're at it, can you put a1.6b stamp as well. I guess you effectively did, but...

Thanks. Fix just got landed. If necessary (for 1.6b release tracking convenience), I'll mark this as fixed and open a separate bug for chatzilla.

BTW, for var fileProtocolHandler = ioService.getProtocolHandler("file").QueryInterface(nsIFileProtocolHandler); var url = fileProtocolHandler.newFileURI(fp.file).QueryInterface(nsIURL); there's a handy shortcut: var url = ioService.newFileURI(fp.file).QueryInterface(nsIURL);

*** Bug 227025 has been marked as a duplicate of this bug. ***

*** Bug 227509 has been marked as a duplicate of this bug. ***

I just did what Neil suggested in comment #43 to show his hunch wrong :-)

Comment on attachment 137750 [details] [diff] [review] extra patch for thunderbird theme/extension installer asking for r/sr

Comment on attachment 137750 [details] [diff] [review] extra patch for thunderbird theme/extension installer Any review I did of this would probably break three rules of review...

Comment on attachment 137750 [details] [diff] [review] extra patch for thunderbird theme/extension installer Thanks for sr. Because it's neil's patch, asking him for review broke rules (as he wrote). I should have incorporated it in the first round. BTW, I'm changing the function signature of 'installTheme' to accept a file picker instead of a file path. No other caller was found in lxr on mozilla so I think it's safe. I considered merging installTheme() with installSkin() (the only caller), but just left it alone.

jshin, you might have missed a few (un)escapes? LXR shows these: /editor/ui/dialogs/content/EditorPublish.js, line 160 -- gDialog.FilenameInput.value = unescape(filename); /xpfe/components/prefwindow/resources/content/pref-applications.js, line 111 -- type = unescape(neverAskSave[i]); /xpfe/components/prefwindow/resources/content/pref-applications.js, line 120 -- type = unescape(neverAskOpen[i]); /mailnews/base/resources/content/subscribe.js, line 349 -- SetState(escape(name), state);

Thanks. All of them were in my original list, but somehow I didn't fix them. I ran 'find ... xargs grep ' on my tree and found three more (two in browser/components/bookmarks/src/nsBookmarkProtocolHandler.js that was checked on December 4th - i.e. after I made patches - and one in layout/html/tests/block/bugs/58652.js). - bookmark.url = unescape(rv[1]); + bookmark.url = decodeURI(rv[1]); var propertyData = propertyPairs[i].split("="); - bookmark[propertyData[0]] = unescape(propertyData[1]); + bookmark[propertyData[0]] = decodeURIComponent(propertyData[1]); I made a preliminary patch, but am not sure if that's right because I can't find the corresponding escape/unescape for all cases. I'm gonna uploade it soon

thank you, neil, for catching them.

I see what you mean - subscribe.js weirdly only escapes some of the time...

Yeah, that's what I'm least sure of. (see bug 126453 as well) As for 'pref-applications.js', I guess we can just do what I did in the latest patch (because MIME type would be just ASCII with '/' and '-' if I'm right about what 'AskForSave' is). The most conspicuous one would be 'nsBookmarkProtocolHandler.js' (the new one). We probably have to fix it before firebird 0.8 ships. 'Publish' (composer) part I have to try it myself.

Comment on attachment 137829 [details] [diff] [review] additional patch for cases found by neil (and a new file) asking for r/sr. 1. EditorPublish.js : I went through the file and am pretty sure that we have to use decodeURIComponent(). 2. subscribe.js : It should be all right. 3. nsBookmarkProtocol.js : I should have pushed a little harder and a little earlier. I think this should go in for firebird 0.8release

Comment on attachment 137750 [details] [diff] [review] extra patch for thunderbird theme/extension installer mscott landed almost identical patch in bug 228300 a couple of weeks ago.

*** Bug 241596 has been marked as a duplicate of this bug. ***

*** Bug 242174 has been marked as a duplicate of this bug. ***

What is keeping this bug from being resolved?

jshin, will you check in that last patch, or does it need more work not reported here? /be

Attachment 137829 [details] [diff] was checked in (I'm sorry I haven't noted it here) in Feburary last year. What keep this from being resolved is that IRC patch hasn't been applied yet. I didn't ask for review of IRC/Vekman patch (attachment 136740 [details] [diff] [review]) and uploaded Vekman only patch(that was landed) perhaps because IRC patch was regarded as a bit too dangerous. (see comment #64, comment #60).

Perhaps it would be worthwhile to update the summary?