Last Comment Bug 241739 - nsXULDocument seems unable to deal with non-ascii scripts
: nsXULDocument seems unable to deal with non-ascii scripts
Status: RESOLVED FIXED
: intl
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.8beta3
Assigned To: Christian :Biesinger (don't email me, ping me on IRC)
:
Mentors:
http://lxr.mozilla.org/seamonkey/sour...
: 264014 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-04-26 06:36 PDT by Christian :Biesinger (don't email me, ping me on IRC)
Modified: 2008-07-31 03:15 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (282 bytes, application/vnd.mozilla.xul+xml)
2005-06-10 14:55 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details
implements comment 8's suggestion (11.67 KB, patch)
2005-06-10 16:45 PDT, Christian :Biesinger (don't email me, ping me on IRC)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review
patch v2 (11.60 KB, patch)
2005-06-11 08:18 PDT, Christian :Biesinger (don't email me, ping me on IRC)
cbiesinger: review+
cbiesinger: superreview+
asa: approval1.8b3+
Details | Diff | Review

Description Christian :Biesinger (don't email me, ping me on IRC) 2004-04-26 06:36:16 PDT
in the OnStreamComplete impl:
3133         nsString stringStr; stringStr.AssignWithConversion(string, stringLen);

it seems somewhat unlikely that this will do the intended thing for non-ascii
(or, non-latin1) scripts.
Comment 1 Jungshik Shin 2004-04-26 16:08:42 PDT
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsScriptLoader.cpp#735

Can we do something similar to what's done in nsScriptLoader.cpp? 
Comment 2 Boris Zbarsky [:bz] 2004-04-26 16:10:53 PDT
That's exactly what we want to do, in fact...  It just sucks to duplicate code.
  Any way to hook this crud up to the normal scriptloader?
Comment 3 Christian :Biesinger (don't email me, ping me on IRC) 2004-04-26 16:30:15 PDT
nsIScriptLoader:
 89   void processScriptElement(in nsIDOMHTMLScriptElement aElement,
 90                             in nsIScriptLoaderObserver aObserver);

I suspect the type of aElement would have to change, at least.
Comment 4 Christian :Biesinger (don't email me, ping me on IRC) 2004-04-26 16:31:00 PDT
it could possibly use
http://lxr.mozilla.org/seamonkey/source/content/html/content/public/nsIScriptElement.h
although it may need a way to query the URL
Comment 5 Boris Zbarsky [:bz] 2004-04-26 17:06:54 PDT
See bug 235826.  That changes the nsIScriptLoader api already.
Comment 6 Jonas Sicking (:sicking) 2004-04-26 18:59:47 PDT
unfortunatly i suspect it's not as easy in XUL due to the
implement-all-elements-using-a-single-class design :(
Comment 7 Boris Zbarsky [:bz] 2004-10-12 08:57:56 PDT
*** Bug 264014 has been marked as a duplicate of this bug. ***
Comment 8 Boris Zbarsky [:bz] 2004-10-12 08:59:57 PDT
The other option is to move a lot of the scritloader OnStreamComplete guts (at
least the part handling the encoding conversion) out into a static helper or an
nsContentUtils helper and have nsXULDocument reuse it...
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-09 16:51:48 PDT
crazy idea: could we maybe just create an HTMLScriptElement for <xul:script/> nodes?
Comment 10 Jonas Sicking (:sicking) 2005-06-09 18:18:24 PDT
You probably wouldn't be able to take advantage of the prototype stuff for
script elements then. IIRC for scripts we put the actual script in the prototype
to avoid recompiles.
Comment 11 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-10 14:55:20 PDT
Created attachment 185895 [details]
Testcase
Comment 12 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-10 16:45:37 PDT
Created attachment 185903 [details] [diff] [review]
implements comment 8's suggestion

(this patch leaves the method on nsScriptLoader, instead of putting it onto
nsContentUtils)
Comment 13 Boris Zbarsky [:bz] 2005-06-10 20:07:50 PDT
Comment on attachment 185903 [details] [diff] [review]
implements comment 8's suggestion

>Index: base/src/nsScriptLoader.cpp
>+nsScriptLoader::ConvertToUTF16(nsIChannel* aChannel, const PRUint8* aData,

>+  if (NS_FAILED(rv) || characterSet.IsEmpty()) {
>+    if (!aHintCharset.IsEmpty()) {

Why not use && instead of a separate if?

>+      nsString tempStr;

Why bother?  Why not just write to aString directly?

r+sr=bzbarsky with those nits picked.
Comment 14 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-11 08:18:34 PDT
Created attachment 185953 [details] [diff] [review]
patch v2

comments addressed, transferring bz's r+sr

This patch allows non-ASCII characters in XUL <script src> scripts, would be
nice to have in 1.8 - it just reuses the code for HTML/SVG <script>.
Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-12 05:32:38 PDT
Checking in content/base/src/nsScriptLoader.cpp;
/cvsroot/mozilla/content/base/src/nsScriptLoader.cpp,v  <--  nsScriptLoader.cpp
new revision: 1.72; previous revision: 1.71
done
Checking in content/base/src/nsScriptLoader.h;
/cvsroot/mozilla/content/base/src/nsScriptLoader.h,v  <--  nsScriptLoader.h
new revision: 1.10; previous revision: 1.9
done
Checking in content/xul/document/src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <-- 
nsXULDocument.cpp
new revision: 1.659; previous revision: 1.658
done
Comment 16 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-12 16:14:40 PDT
documented at
http://developer.mozilla.org/en/docs/International_characters_in_XUL_JavaScript

Note You need to log in before you can comment on or make changes to this bug.