If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nsIRDFResource should support IDN

RESOLVED FIXED in mozilla1.7beta

Status

()

Core
RDF
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Jungshik Shin, Assigned: Benjamin Smedberg)

Tracking

(Blocks: 1 bug, {intl})

Trunk
mozilla1.7beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

14 years ago
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.

Updated

14 years ago
Summary: nsIRDFResource should support IDN → nsIRDFResource should support IDN
(Assignee)

Comment 1

14 years ago
Have patch, will travel.
Assignee: rjc → bsmedberg
(Assignee)

Updated

14 years ago
Blocks: 216788
(Assignee)

Comment 2

14 years ago
Created attachment 143055 [details] [diff] [review]
add nsIRDFResource.valueUTF8
(Assignee)

Updated

14 years ago
Attachment #143055 - Flags: superreview?(darin)
Attachment #143055 - Flags: review?(axel)

Comment 3

14 years ago
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.
Attachment #143055 - Flags: superreview?(darin) → superreview-
(Assignee)

Comment 4

14 years ago
Created attachment 143121 [details] [diff] [review]
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.
Attachment #143055 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #143121 - Flags: superreview?(darin)
Attachment #143121 - Flags: review?(axel)

Updated

14 years ago
Attachment #143055 - Flags: review?(axel)

Comment 5

14 years ago
(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

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

Comment 7

14 years ago
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.
Attachment #143121 - Flags: review?(axel) → review+

Comment 8

14 years ago
> +    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

14 years ago
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
Attachment #143121 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 10

14 years ago
checked in, with one win32 bustage fix in nsAbOutlookCard.cpp
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.7beta
You need to log in before you can comment on or make changes to this bug.