15089 - XML Parser errors are not localizable.

Status: VERIFIED FIXED

(Core :: XML, defect, P2)

Description

[Nisheeth Ranjan]

I'm keeping this on my plate for post-beta work.  CCing you guys to keep you in
the loop.

Comment 1

[tao]

CC Erik.

Comment 2

[Frank Tang]

nisheeth, can you add Jim Clark to the CC list ?

Comment 3

[Nisheeth Ranjan]

James, I'm adding you here to keep you in the loop.  Any suggestions regarding
this bug are most welcome.  Thanks.

Comment 4

[James Clark]

I've no idea how you normally handle error message localization.  Take the error
code returned by XML_GetErrorCode() (which will always be a member of the
XML_Error enum) and do whatever you usually do to generate a localized message
from an error code.

Comment 5

[Nisheeth Ranjan]

Moving bugs out by one milestone...

Comment 6

[tao]

Hi, Nisheeth:


Will end users see the error messages generated from expat? 

Since expat is primary C code and can't get access to xpcom objects such as
chrome registry, nsILocale, and stringBundle, it seems to me the best way to 
solve this problem is to have expat proprogate the error code up to the caller
in C++ or JS and use the err code as the key to access the error messsage in 
the properties files.

How does this sound? Let me know!

It seems to me 
if the users are going to see the messages

Comment 7

[Nisheeth Ranjan]

Tao, yes, end users will see the expat error messages if they try to load a 
malformed XML file.  The error codes are already propagated from expat to 
nsExpatTokenizer.  Could you point me to some code that uses properties files 
for localization?  If the property files can be accessed and manipulated through  
objects that are accessible from the component manager, then I can put the error 
message localization code in nsExpatTokenizer.  Currently, the expat tokenizer 
generates tokens for displaying the error message and always uses English error 
messages.

Comment 8

[tao]

Here is the idl of the stringBundle:
  http://lxr.mozilla.org/mozilla/source/intl/strres/public/nsIStringBundle.idl

And, its usage in Wallet:

  http://lxr.mozilla.org/mozilla/source/extensions/wallet/src/wallet.cpp#707


Note that the GetStringFromID() methods passed the resulting strings as 
PRUnichar*. Please do not convert it to CString, otherwise, non-ascii texts
will be truncated.



Thanks

Comment 9

[ekrock's old account (dead)]

FYI: This is a "nice to have" but not required for FCS. Reasoning: if XML
content developers and XML app developers are doing their jobs correctly, users
should never be presented with invalid/error-generating XML. As a result, these
error messages should (usually) be seen only by XML developers during
development, not by end users. (Obviously developers will make mistakes, but
then they're making themselves and their site/app look bad and have an incentive
to fix it. And developers outside the U.S. are, sadly, already used to dealing
with English-language error messages.) As a result, it's not the end of the
world if these error messages aren't localized for FCS. Nisheeth--you're free to
mark FUTURE if you have other higher-priority work and are strapped for time.

Comment 10

[Nisheeth Ranjan]

Marking future based on Eric's analysis.  Will keep it in mind for work that 
Heikki might do if he comes on board on time.  Thanks, Eric, for that analysis.

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Taking, but if somebody has the cycles feel free to take this from me.

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix, might need a little polish

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Proposed fix 1

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Harish, can you review?

I am not so sure about where to place the string properties; I put them in
communicator/locale/layout. Also, I am still open to renaming nsParserMsgUtils
to something else, if you can think of a better one. The class is made so that
if we later want to make localized messages for the HTML parser, we don't need
to change any code, just add new properties files and edit makefiles.

I still need to do some Mac build changes as well, but this can be reviewed for
other platforms.

nsBranch, moving to 0.9.4 and upping priority.

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix 1 with different cvs options (cvs -uwN)

Comment 17

[Johnny Stenback (:jst)]

sr=jst

Comment 18

[harishd]

+  nsCOMPtr<nsIIOService> pNetService(do_GetService(kIOServiceCID, &rv));
+  if (NS_SUCCEEDED(rv) && pNetService) {

Checking for NS_SUCCEEDED(rv) should be sufficient...right? 

+    rv = pNetService->NewURI(aPropFileName, nsnull, getter_AddRefs(uri));
+    if (NS_SUCCEEDED(rv) && uri) {

Same as above. Also, are you sure about propagating all failure messages? Or
should we just worry about NS_ERROR_OUT_OF_MEMORY?

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

You are right that NS_SUCCEEDED() is enough for do_GetService() and NewURI(), I
fixed that. Regarding the error returns, we are ignoring the return values for
CreateErrorText() and PushXMLErrorTokens() so we don't propagate stuff up from
there.

The most likely reason why the GetLocalizedStringBy*() functions would fail is
missing localization file or something like that. With the patch as is, we would
not update the content area which looks bad. So as a compromise, I am going to
ignore the return value of those functions in PushXMLErrorTokens() so that we at
least get a page giving the user some indication an error happened even though
some information is missing. jst ok'd this change.

I'll attach the latest patch.

Comment 20

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Proposed fix 2

Comment 21

[Frank Tang]

r=ftang for "localization, location of string resource etc."

Comment 22

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fixed.

Comment 23

[Chris Petersen]

Since I 'm not exactly sure how to test this, I'm marking verified based on the
last comments

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d888c25cd9ca
[fenix] For https://github.com/mozilla-mobile/fenix/issues/15089 - Disable recently closed in multiselect