This is a spin-off from bug 158006. When an image with non-ASCII chars. in its name is opened in a standalone browserpane (a tab or a window) with 'view image' or 'clicking on the link to the image', the 'document' charset of the standalone image is set to ISO-8859-1. As a result, Mozilla prefills filepicker with a mangled filename. How mangled? For instance, Mozilla interprets '0xB0 0xA1.jpg'(ignore space and interpret 0xB0 and 0xA1 as octets corresponding to hex numbers. '가.jpg' : set your encoding to UTF-8 and Korean string will be rendered correctly) in EUC-KR as if it's ISO-8859-1 and comes up with a suggested filename of 'U+00B0 U+00A1 .jpg'(again ignore spaces: °¡.jpg ) ) instead of 'U+AC00 .jpg' (0xB0 0xA1 in EUC-KR is U+AC00). How to reproduce: go to the URL listed in the URL field. Right click on the third blueball in the page and select 'view image'. A small blue ball will come up in a window of its own. Now, right-click on it and select 'save image as'. Filepicker will show up with 'ÆĶõ°ø.jpg' instead of '파란공.jpg' (set charset to UTF-8 to see all these characters.) The fix can be pretty simple. Set charset of a _standalone_ non-textual document to the charset of the referrer (from which non-textual 'document' originates. BTW, if you have Win2k/XP, MacOS X or Unix-like OS with UTF-8 locale, you'll see that the window title bar has the correct Korean filename ('파란공.jpg'). So what I suggested above is hard to implement, there should be another way to retrieve that information...

OOops. I'm sorry I forgot to set the encoding to UTF-8 when reporting this (Bugzilla should use UTF-8 by default !) mangled name (euc-kr interpreted as iso-8859-1) is 'ÆĶõ°ø.jpg' and Korean name is '파란공.jpg'.

This solves the problem when an image is loaded into its own browser pane by clicking on the link to the image. In that case, originCharset is set to the doc. charset of the referrer so that by retrieving that value works fine. However, when an image is loaded into its own browser pane with 'View Image', originCharset is set to UTF-8 and setting the doccharset of the standalone synthetic document to originCharset doesn't help at all. It took me much longer than necessary to figure this out because I didn't realize that change in content/html/document gets only effective after recompiling in layout/...

Adding a few more people to CC. The case that works with attachment 118129 [details] [diff] [review] is clicking on links like below: <a href="non-ascii_imagename.jpg">link name</a> (in http://jshin.net/download.html, the link named '모질라.jpg'. view this with charset set toUTF-8) The case that doesn't work with it is right-clicking on links as below and selecting 'view image'. <img src="non-ascii_image.jpg"> (in the url above, a 'blueball' next to '파란공') BTW, I put a call to 'UpdateDocumentCharacterSet' in |CreateSynthetic...|, but it's just tentative (as a proof of concept) so that there may be a better place(way) to call that. Anyway, to solve this problem for both cases, I'll try another approach (as used in my patch to bug 162765)

I tried using aChannel in |StartDocumentLoad| hoping that uri obtained from aChannel may have charset of the referring document as |originCharset|, but it's not the case. When I right-click on an image and select 'view image', |originCharset| of the image loaded in its own window is set to UTF-8 while |mCharacterSet| of 'stand-alone image document' is ISO-8859-1. I hoped to correct it with |originCharset|, but it's UTF-8 so that it's of no use. Anyway, it's the least important of three cases : - Click on an image link and load the linked image in a stand-alone browser pane : 'save as image' works fine with my patch - Right click on an image and select 'save as image' : ditto - Right click on an image and select 'view image' and then in the stand-alone image pane, right click on the image and select 'save as image' : does NOT work. More important two cases work fine. As for the third case, I need to figure out where |originCharset| of channel/documentURL is set to UTF-8 (see bug 158006 comment #12) and do what's necessary there. Or, if that's not desirable, I have to find out the earliest place where I can set |mDocumentCharacterSet| to that of the referring document instead of ISO-8859-1 (which is the default value set in the ctor of |nsDocument|). Anyway, the third case is least important and rarely used, but the first two cases are frequently used and are often a source of complaints from those who want to convert to Mozilla/Netscape from MS IE. So, let's solve it first. The question is where to call a new function (|UpdateDocumentCharacterSet| or equivalent) added in my patch. It can be called in StartDocumentLoad, OnStartRequest, OnStopRequest, CreateSyntheticDocument or somehwer else. Darin and Christian, any opinion? This patch is similar to the patch for bug 163998 and maybe we can save some lines of code by putting my patch in |OnStopRequest| where |GetOriginCharset| is called anyway for image title set-up.

A new patch. SetDocumentCharacterSet invocation is now in OnStopRequest to share originCharset obtained via mDocumentURL with UpdateTitle.

Comment on attachment 118132 [details] [diff] [review] a new patch So... why not just call UpdateTitle() as it happens now and just have UpdateTitle() get the charset and set it if necessary? Also, do we not care to set the charset if the URI is not a URL? Finally, should this code live in nsImageDocument, or is any of it relevant for plug-ins? Does it need to move up into nsMediaDocument?

handle URI case.

Per Boris' comment, I renamed UpdateTitle to UpdateTitleAndCharset and put two tasks together to avoid repeating if-if twice in caller and callee. > Finally, should this code live in nsImageDocument, What's the role of nsMediaDocument? Is it responsible for (to-me) annoying empty window that comes up when I click on the link to a file of a (non-textual, non-visual) mimetype for which I haven't yet specified helper appl. (or which is to be handled by Mozilla)? For instance, <a href="xxxx.yyy">Link</a> where c-t for 'xxxx.yyy' is to be handled by Mozilla. > or is any of it relevant for plug-ins? Could be, but have to check. Let's say, there's a link to a PDF file (or a flash animation) <a href="non-asciiname.pdf">non-ascii link name</a> When it's clicked, does Mozilla do something similar to what's done in nsImageDocument.cpp, (|CreateSynthetic...|: rolling out <object> or <applet>....)? If that's the case, we have to take care of that case as well. Another reason we have to set charset is that plug-ins may behave differently on charset(I don't know if there's any, but there might be...) The best way is to set charset to that of the referring document as up in the hierarchy as possible. |nsDocument| ctor inits it to ISO-8859-1... > Does it need to move up into nsMediaDocument? If both nsImageDocument and nsMediaDocument need this, doesn't it have to move up to nsHTMLDocument (or even higher if plug-ins also need this? or is nsMediaDocument in charge of plug-ins as well?)?

Somehow my comment added yesterday is not here(perhaps I forgot to submit..) Anyway, I'm sorry I didn't realize that nsImageDocument now inherits nsMediaDocument (my tree was not up to date.) So does a new nsPluginDocument. Then, yes, it makes sense to move UpdateTitleAndCharset (or at least UpdateCharset part if we just want to use the default title for plugin documents) up to nsMediaDocument because both image and plugin document need it. For instance, <a href="non-asciiname.pdf">non-ascii link name</a> is clicked on and opened, 'File|Save Page as' brings in a filepicker filled with a garbled suggested filename because DocumentCharset is init'd to ISO-8859-1 in ctro of nsDocument.

I moved UpdateTitleAndCharset to nsMediaDocument and made some changes as necessary in ns(Image|Media|Plugin)Document. This works well with PDF and Acroread plugin and should work with other plugins and mimetype pairs.

One problem not yet taken care of is whether or not to make a separate string bundle for non-image plugin document. In the current patch, I recycled communicator/locale/layout/ImageDocument.properties for both nsPluginDocument. The result is that the titlebar for non-image document (e.g. PDF) reads documentname.pdf (application/pdf: Image) Mozilla (build .....) It'd be fine if there were no 'Image'. I'm adding Tao to CC to seek his opinion because this also has to do with L10N. I'm not sure whether the name of properties file has to match the class name in which stringbundle is used.

As a bonus, with this patch, tabs with PDF (and other plugin doc) have also more informative titles displayed (e.g. 'doc1.pdf (application/pdf.....'). Without 'Image', I'd be very much tempted to check this in after r/sr...

>I'm not sure whether the name of properties >file has to match the class name No, it can be anything.

>>I'm not sure whether the name of properties file has to match the class name >No, it can be anything. Thanks for the answer. I was not clear, but I was asking if there's any convention/guideline when it comes to name a properties file. Anyway, which would you like more, making a new *separate* properties file for plugin document or just recycling the existing properties file for plugin docs (with 'Image' problem)? A third way might be to add a couple of new strings for plugin doc to the existing file and use them for plugin docs. I'm inclined to the third one with a possible name change in the file name.

I added two strings for media documents and changed the way stringbundle is accessed. I like this pretty much.

Comment on attachment 118523 [details] [diff] [review] a new patch with two new localizable strings added this is the extension of bug 163998 so that I'm asking darin for sr while asking r for Christian who's been monitoring this. BTW, I took the third approach to 'stringbundle'.

nsMedia/Image/Plugin Document.cpp have changed since my last patch. I updated my patch to refleect the change.

Comment on attachment 118925 [details] [diff] [review] a new patch +const PRUnichar* nsMediaDocument::sFormatNames[4] = firstly, please make it static, and secondly - why did you make this array local in nsImageDocument, but global in nsMediaDocument? (also make it static in nsImageDOcument please) + const PRUnichar *formatStrings[4] = {fileStr.get(), + PromiseFlatString(aTypeStr).get(), widthStr.get(), heightStr.get()}; + mStringBundle->FormatStringFromName(aFormatNames[3], formatStrings, 4, I believe that the temporary from PromiseFlatString will be destroyed by the time you call FormatStringFromName, so the second array element points to invalid data... same in the else part + if (valUni) { please use: if (!valUni.IsEmpty())

Thank you for review. > +const PRUnichar* nsMediaDocument::sFormatNames[4] = > firstly, please make it static, I guess I don't have to(and VC++ wouldn't allow it) because it's the definition of a static member array declared in nsMediaDocument class declaration as static const PRUnichar* sFormatNames[4]; > and secondly - why did you make this array > local in nsImageDocument, but global in nsMediaDocument? Because I want to use sFormatNames as the default value for the 2nd argument of nsMediaDocument::UpdateTitleAndCharset. + nsresult UpdateTitleAndCharset(const nsAString& aTypeStr, + const PRUnichar** aFormatNames = sFormatNames, + nscoord aWidth = 0, + nscoord aHeight = 0 ); + + nsCOMPtr<nsIStringBundle> mStringBundle; + static const PRUnichar* sFormatNames[4]; In nsPluginDocument, it's invoked with only the first argument (aTypeStr) while in nsImageDocument, the second argument(sFormatNames) is overriden by the locally defined value (|formatNames|). > (also make it static in nsImageDOcument please) > I believe that the temporary from PromiseFlatString will be destroyed by the > time you call FormatStringFromName, > please use: > if (!valUni.IsEmpty()) Thanks, these are taken care of.

Can you review this, instead? Thanks

Comment on attachment 119150 [details] [diff] [review] a new patch with Christian's concern addressed >definition of a static member array sigh. I didn't realize that, I'm sorry. r=me

Comment on attachment 119150 [details] [diff] [review] a new patch with Christian's concern addressed Thank you, Christian. Darin, can I get sr?

Comment on attachment 119150 [details] [diff] [review] a new patch with Christian's concern addressed darin seems busy. asking bz for sr because he's kept his eyes on this.

It looks like I'm chasing a moving target. nsImage/MediaDocument.cpp have changed since my last patch. Christopher, can I get your r carried over to this patch? Boris, can I get sr?

Comment on attachment 120146 [details] [diff] [review] same patch modified to match recent changes in the tree >Index: layout/html/forms/src/ImageDocument.properties >+#LOCALIZATION NOTE (MediaTitleWithFile): first %s is filename, second %S is type Make both %S uppercase, please. Also, we should really find a better place for this file to live, eventually... like content/html/document Maybe "Object" instead of "Media"? Not sure what would be best here, but "(application/x-shockwave-flash Media)" looks odd to me. >Index: content/html/document/src/nsImageDocument.cpp >- nsresult UpdateTitle(); >+ nsresult UpdateTitleAndCharset(); > > nsCOMPtr<nsIStringBundle> mStringBundle; > nsCOMPtr<nsIDOMElement> mImageElement; Don't we want to remove mStringBundle here? >+ if (!mStringBundle) >+ return NS_OK; So we don't need to update the charset if there is no stringbundle? Don't we need the charset for save as anyway? >+ static const PRUnichar* formatNames[4] = >+ { >+ NS_LITERAL_STRING("ImageTitleWithNeitherDimensionsNorFile").get(), >+ NS_LITERAL_STRING("ImageTitleWithoutDimensions").get(), >+ NS_LITERAL_STRING("ImageTitleWithDimesions").get(), >+ NS_LITERAL_STRING("ImageTitleWithDimensionsAndFile").get() >+ }; This will crash on Linux (unless you build with GCC 3.2 and have --fshort-wchar enabled) and other platforms where NS_LITERAL_STRING("foo") doesn't just expand to L"foo". Please come up with an alternate way to do it (eg make this a const char const * and do runtime conversion as needed in nsMediaDocument. >Index: content/html/document/src/nsMediaDocument.cpp >+const PRUnichar* nsMediaDocument::sFormatNames[4] = >+{ >+ NS_LITERAL_STRING("MediaTitleWithoutFile").get(), >+ NS_LITERAL_STRING("MediaTitleWithFile").get(), >+ NS_LITERAL_STRING("").get(), >+ NS_LITERAL_STRING("").get(), >+}; Same here. >+nsresult >+nsMediaDocument::UpdateTitleAndCharset(const nsAString& aTypeStr, Why? Make it an nsACString; then you can drop a lot of conversions in the callers.... >+ if (NS_FAILED(rv)) >+ fileStr.Assign(NS_ConvertUTF8toUCS2(fileName)); >+ } >+ } else { >+ fileStr.Assign(NS_ConvertUTF8toUCS2(fileName)); >+ } You could combine those into a single |if (fileStr.IsEmpty())| check, no? >+ nsXPIDLString valUni; This needs a better name. >+ // if this is an imageDoc and we got a valid size (sometimes we do not). Just say "if we got a valid size". The fact that nsImageDocument even exists should be a matter of sublime indifference to nsMediaDocument >+ const PRUnichar *formatStrings[4] = {fileStr.get(), >+ PromiseFlatString(aTypeStr).get(), widthStr.get(), heightStr.get()}; What if PromiseFlatString has to construct an object? This will dereference garbage memory.. You want to hold on to the flatString for the duration of the FormatStringFromName call (this will continue to apply when you switch to nsACString; then you will want: |NS_ConvertUTF8toUCS2 unicodeType(aTypeStr); const PRUnichar * ....| This applies some places below too. >+ mStringBundle->FormatStringFromName(aFormatNames[3], formatStrings, 4, Could we have a nice enum (scoped to nsMediaDocument only) for the indices into the name array instead of using the raw numbers? >+ if (valUni) { >+ // set it on the document >+ SetTitle(valUni); We want to set the title unconditionally, even if everything fails -- otherwise we will be showing the title from the previous page, no? In any case, test |!valUni.IsEmpty()| instead of using the conversion operator.... >Index: content/html/document/src/nsMediaDocument.h >+#include "nsCoord.h" Why? More below. >+ // nsIHTMLDocument >+ nsresult Init(); Shouldn't that be NS_IMETHOD then? >- virtual nsresult CreateSyntheticDocument(); >+ nsresult CreateSyntheticDocument(); This needs to stay virtual, since subclasses may need to override (and nsImageDocument does!) >+ nsresult UpdateTitleAndCharset(const nsAString& aTypeStr, >+ const PRUnichar** aFormatNames = sFormatNames, >+ nscoord aWidth = 0, >+ nscoord aHeight = 0 ); This needs some documentation. Like the expected length of aFormatNames. And what the width and height are of. Make aTypeStr an nsACString. Don't use nscoord. Use PRInt32 or something. Those lengths are lenghts in pixels, not nscoords..... My apologies for taking so long; please fix the above and I promise to be more prompt. ;)

.

Thanks a lot for your thorough review. A few points had been addressed in my second last patch per Christian's review comment and on my own, but somehow my last patch got mix-ups from an earlier patch. However, there were several other points I hadn't thought about before all of which I took care of in this patch. Can you take a look again? TIA. BTW, I moved ImageDocument.properites to content/html/document/src and renamed it as MediaDocument.properties.

Comment on attachment 120181 [details] [diff] [review] a new patch per Boris' comment Christian is away. Asking Simon and Boris for r/sr

Comment on attachment 120181 [details] [diff] [review] a new patch per Boris' comment >Index: content/html/document/src/MediaDocument.properties Don't you need to add this file to the build system? The file itself looks fine. >Index: content/html/document/src/nsMediaDocument.h >+ // nsIHTMLDocument >+ NS_IMETHOD Init(); So I just went and checked, and this is in fact not an nsIHTMLDocument method, so the answer to my question is "No, it should not be NS_IMETHOD" and that misleading comment should be removed. >+ // |aFormatNames[]| need needs. > to have four elements, format name with filename >+ // and dimension, with dimension but w/o filename, with filename but w/o "elements: a format name with filename and dimension, a format name with dimension but w/o filename, a format name with filename but w/o dimension, and a format name with neither of them". Don't you want to say what order the names should come in? The order in which you list them here does not match the order they actually need to be in in the array.... >+ // dimension, and with neither of them. See |nsImageDocument| for example. Just put an example in the comment; no reason to send people off to some rather .cpp that may even go away while this .h stays to look for some code in that cpp. Shouldn't UpdateTitleAndCharset be virtual? >Index: content/html/document/src/nsMediaDocument.cpp > NS_IMETHODIMP >+nsMediaDocument::Init() so this would become just an nsresult return. >+ if (!fileName.IsEmpty() && NS_FAILED(rv)) >+ fileStr.Assign(NS_ConvertUTF8toUCS2(fileName)); This seems wrong. What if NS_SUCCEEDED(rv) but originCharset was empty (a URI which never got an origin charset set)? I'd just do if (fileStr.IsEmpty()) { fileStr.Assign(NS_ConvertUTF8toUCS2(fileName)); } here. >+ nsAutoString typeStr = NS_ConvertASCIItoUCS2(aTypeStr); NS_ConvertASCIItoUCS2 typeStr(aTypeStr); then you only copy once, not twice. >+ nsDependentCString fmtName(aFormatNames[eWithDimAndFile]); >+ mStringBundle->FormatStringFromName(NS_ConvertASCIItoUCS2(fmtName).get(), >+ formatStrings, 4, getter_Copies(title)); NS_ConvertASCIItoUCS2 fmtName(aFormatNames[eWithDimAndFile]); then just use fmtName.get() Same in the other branches. >+ else { // string bundle not available. use hard-coded string. Um. Why not set the title to an empty string in this case? I think that would be preferable to doing non-localized stuff.... The rest looks fine.

Thank you for review. I'll fix what you pointed out. > Shouldn't UpdateTitleAndCharset be virtual? Did you mean |UpdateTitleAndCharset(.....)| in nsMediaDocument? In that case, UpdateTitleAndCharset(void) in nsImageDocument has to be renamed to avoid hiding UpdateTitleAndCharset(.....) in nsMediaDocument, doesn't it? Perhaps, what can be done is to add virtual |UpdateTitleAndCharset(void)| to nsMediaDocument the default implementation of which is empty. Optionally, we can rename |UpdateTitleAndCharset(.....)| (that does most of chores) in nsMediaDocument. BTW, the return value of UpdateTitleAndCharset is not checked anywhere so that I'm tempted to change its return type void. What would you say? >>+ else { // string bundle not available. use hard-coded string. >Um. Why not set the title to an empty string in this case? I think that would >be preferable to doing non-localized stuff.... I'm not sure although I can go either way. It seems to me that either leaving the title of the previous page (that is likely to carry some relevant info.) alone(as in older patches) or doing non-localized stuff (as in the last patch, well, except for 'pixels' and 'Image', it's just made of MIME type and filename.) is more friendly than setting the title to an empty string.

> Did you mean |UpdateTitleAndCharset(.....)| in nsMediaDocument? Yes. I just realized that those actually take totally different args... I'm not sure what I think about that, to be truthful... It seems very confusing to me to have two functions with identical names and different arguments in classes that inherit from each other like that.... If no one checks the retval, by all means make it void. Setting the title to an empty string will just make the titlebar say "Mozilla". It's a _lot_ more friendly than leaving the title of the previous page, since the title, if any, should match the content. I fail to see how the title of the previous page can claim to do so. In fact, I see nothing unfriendly about setting the title to an empty string...

How about this in nsMediaDocument.h? // When implemented in a subclass, this should invoke // |nsMediaDocument::UpdateTitleAndCharset| with parameters filled out // as necessary and as appropriate for the subclass. virtual void UpdateTitleAndCharset() = 0; //.... comment on void UpdateTitleAndCharset(.......) void UpdateTitleAndCharset (......); This is still confusing and increases the size of vtbl, but is arguably a bit cleaner than before. > I fail to see how the title of the previous page can claim to do so. It depends. For instance, when the previous page is about topic A with the title indicative of that and the link in the page to a pdf file on topic A (or a related topic) is clicked on and the pdf file is loaded, the prev. page title has some relevance... On the other hand, it could be confusing in a sense.. > In fact, I see nothing unfriendly about setting the title to an empty string... It's less friendly than 'xyz.jpg (JPEG : Image w x h pixels)' or 'abc.pdf (application/pdf)', isn't it? :-) Well, I guess this is not that important so that I'd not insist.

> This is still confusing Yep. It's no less confusing than the existing patch... Perhaps we should change the imagedocument function to "OnImageSizeAvailable" or something? As in, name it based on when it's called, not based on what it does...

addressing Boris' concerns except for making Update... virtual, which we decided not to do over IRC.

Comment on attachment 120296 [details] [diff] [review] yet another patch >Index: content/html/document/src/jar.mn >+ locale/en-US/communicator/content/MediaDocument.properties Keep this in communicator/layout/ like it was before ("content" in a jar and "content" the root dir have nothing to do with each other). Don't forget to also change that in nsImageDocument.cpp >Index: content/html/document/src/nsMediaDocument.h >+ nsresult Init(); This needs to be virtual, since subclasses need to be able to override it, no? Why not just look at the superclass (nsHTMLDocument) when declaring such things and declare them the same way? This patch is missing the CVS removal of the current ImageDocument.properties No need for a new patch for these nits; just make sure to fix them when you check in.

It seems that this was checked in on Apr 12 17:40, can this be marked fixed?

Ooops. I'm sorry I forgot to change the status after checking in the patch. Thank you all for your help.

Verified in the 2003-04-22-08 macho and win32 trunk builds.