234392 - nsIRDFResource should support IDN

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[Jungshik Shin]

Currently, nsIRDFResource is written under the assumption that URIs are always
in ASCII. With IDNs(internationalized domain names), that's not the case any more. 
We have to replace 'string' with 'AUTF8String'. There are a number of
'customers' (callers, child classes/implementations) so that it's a significant
amount of work.

Comment 1

[Benjamin Smedberg]

Have patch, will travel.

Comment 2

[Benjamin Smedberg]

Attachment: add nsIRDFResource.valueUTF8

Comment 3

[Darin Fisher]

Comment on attachment 143055 [details] [diff] [review]
add nsIRDFResource.valueUTF8

>Index: mailnews/imap/src/nsImapMailFolder.cpp

>-  nsAutoString uri;
>-  uri.AppendWithConversion(mURI);
>+  NS_ConvertUTF8toUTF16 uri(mURI);
>   uri.Append(PRUnichar('/'));
> 
>   uri.Append(*name);
>   char* uriStr = ToNewCString(uri);

so this is very interesting.... you are taking care to treat mURI as
UTF-8 when converting to UTF-16.  however, the code calls ToNewCString
which does a lossy conversion (assuming |uri| is ASCII).  that doesn't
seem ok to me.


>+  nsAdoptingCString escapedName(nsEscape(mURI.get() + rootURI.Length(),
>+                                         url_Path));
>+  if (escapedName.IsEmpty()) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+  }

hmm.. are you sure |mURI.get() + rootURI.Length()| will never be an
empty string?


>Index: mailnews/news/src/nsNewsFolder.cpp

>+#if 0
>+      nsAutoString sep;
>+      rv = nsGetNewsFolderSeparator(sep);
>+      if (NS_FAILED(rv)) return rv;
>       str += sep;
>+#endif

maybe ask a mailnews guy if he really wants to keep this old |#if 0|'d code??


the rest looks fine to me.

Comment 4

[Benjamin Smedberg]

Attachment: nsIRDFResource.valueUTF8

Updated with darin's nits. I added an assertion for the
nsImapMailFolder::GetFolderURL, because it should always be a longer URI. I
left in the #if 0 code, and will send mail hoping for an answer, but don't want
to hold the patch for that.  Pike: I think the interCaps naming is acceptable
here, because this method is designed for scripting use; it's just a
one-character change in the patch, though, so I'll do what you prefer.

Comment 5

[Axel Hecht]

(In reply to comment #4)
> Pike: I think the interCaps naming is acceptable
> here, because this method is designed for scripting use; it's just a
> one-character change in the patch, though, so I'll do what you prefer.

If I knew. We should probably have drivers vote on whether we ever want to move
RDF to interCaps at all. Though we need a plan for that, and this bug is prolly a 
bit more urgent. How about adding a @note that we may change to LeadingCaps?

Comment 6

[Chase Tingley]

Side note: the general interCaps issue for the RDF code is in bug 189120.

Comment 7

[Axel Hecht]

Comment on attachment 143121 [details] [diff] [review]
nsIRDFResource.valueUTF8

-    for (const char* p = mURI; *p && *p != ':'; ++p)
+    for (const char* p = mURI.get(); *p && *p != ':'; ++p)
	 contractID.Append(*p);

This is not your yacky string code, so you don't have to fix it. Note to self,
eventually someone should.

Comment 8

[Darin Fisher]

> +    for (const char* p = mURI.get(); *p && *p != ':'; ++p)
> 	 contractID.Append(*p);

how about this:

  PRInt32 i = mURI.FindChar(':');
  if (i != kNotFound)
    contractID += StringHead(mURI, i - 1);

Comment 9

[Darin Fisher]

Comment on attachment 143121 [details] [diff] [review]
nsIRDFResource.valueUTF8

my vote is for ValueUTF8 to be consistent with the rest of the RDF interfaces. 
"when in rome, stay in rome."

does the RDF standard support IDN?  if not, should we be doing this?  i think
it's reasonable, but i'm just asking ;-)

> +  nsCAutoString uri = mURI + NS_LITERAL_CSTRING('/');

does this compile?  double quotes should be needed.

sr=darin

Comment 10

[Benjamin Smedberg]

checked in, with one win32 bustage fix in nsAbOutlookCard.cpp