188410 - Change nsIIDNService to take AUTF8String and ACString

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[nhottanscp]

separated from bug 112979

quoted from bug 112979, comment #50
> the methods on nsIIDNService should actually take AUTF8String and ACString
> types instead of "string" types.  as it currently exists the interface is
> not scriptable because it uses "string" to represent UTF8 data.  xpconnect
> will fail to convert UCS2 to UTF8 before calling ConvertUTF8toACE.  of
> course the methods are currently marked |noscript| because of this fact.
> we should file a bug to fix the interface or if you have time it would be
> great if you could fix the interface now.  doing so would also cut down on
> the number of buffer copies since you should not have to call ToNewCString
> at the end of each method.
> 
> here's what the interface should look like:
> 
> interface nsIIDNService : nsISupports
> {
>   ACString    convertUTF8toACE(in AUTF8String input);
>   AUTF8String convertACEtoUTF8(in ACString input);
> };
> 
> also, we probably need to have a method on this interface to test whether
> an ASCII string should be converted to UTF8 using convertACEtoUTF8.  this
> method would be called by nsStandardURL in order to provide UTF8 to the
> caller of nsIURI::GetHost.

Comment 1

[nhottanscp]

Attachment: Change nsIIDNService to take AUTF8String and ACString.

Comment 2

[Darin Fisher]

Comment on attachment 117036 [details] [diff] [review]
Change nsIIDNService to take AUTF8String and ACString.

>Index: base/src/nsStandardURL.cpp

>+        nsCAutoString ace;
>+        rv = gIDNService->ConvertUTF8toACE(Host(), ace);
>         if (NS_SUCCEEDED(rv)) {
>+            mHostA = ToNewCString(ace);
>+            NS_ENSURE_TRUE(mHostA, NS_ERROR_OUT_OF_MEMORY);
>             result = mHostA;
>             return NS_OK;
>         }

hmm.. too many buffer copies.  do this instead:

	  rv = gIDNService->ConvertUTF8toACE(Host(), result);
	  if (NS_SUCCEEDED(rv)) {
	      mHostA = ToNewCString(result);
	      return NS_OK;
	  }

no need to null check mHostA since null value is ok.  printf would
probably fail if system is out of memory, so no point in doing
anything.  also, please fix this warning message while you're at it:

      NS_WARNING("UTF8ToIDNHostName failed");


>Index: dns/public/nsIIDNService.idl

>+    /**
>+	 * Checks if the input string is ACE encoded or not.
>+     */
>+	boolean encodedInACE(in ACString input);

please replace tabs with spaces.  |isACE| might be a better name
for this method ("is ascii compatible encoding").


>Index: dns/src/nsDnsService.cpp

>+            nsCAutoString hostNameACE;
...
>+            lookup = FindOrCreateLookup(PromiseFlatCString(hostNameACE).get());

no need to call PromiseFlatCString here.  that's just extra code.  do this
instead:

	      lookup = FindOrCreateLookup(hostNameACE.get());


>Index: dns/src/nsIDNService.cpp

>+  nsCAutoString lowerCase(Substring(input, 0, 4));
>+  ToLowerCase(lowerCase);
>+  *_retval = lowerCase.Equals(mACEPrefix);

    *_retval = Substring(input, 0, 4).Equals(nsDependentCString(mACEPrefix, 4),
					    
nsCaseInsensitiveCStringComparator());

yeah, i know.. not exactly pretty :-/

Comment 3

[nhottanscp]

Attachment: Updated per review's comment.

Comment 4

[nhottanscp]

Attachment: tab -> space

Comment 5

[Darin Fisher]

Comment on attachment 117100 [details] [diff] [review]
tab -> space

>Index: dns/public/nsIIDNService.idl

>+    ACString ConvertUTF8toACE(in AUTF8String input);
>+    AUTF8String ConvertACEtoUTF8(in ACString input);

sorry i didn't catch this yesterday, but these should be "interCap" style
especially now that they are scriptable.  (e.g., convertUTF8toACE).

>+        converter->ConvertUTF8toACE(nsCAutoString("»OÆW.¤½¥q"), buf);

FWIW, this can be simply:  NS_LITERAL_CSTRING("»OÆW.¤½¥q") instead.  doesn't
really matter since this is a test program, but thought i'd point it out since
it's good programming practice to avoid extra needless buffer copies ;-)

r=darin with these changes.

Comment 6

[nhottanscp]

Attachment: Update per reviewer's comment.

Comment 7

[Darin Fisher]

Comment on attachment 117103 [details] [diff] [review]
Update per reviewer's comment.

>+            rv = mIDNConverter->ConvertUTF8toACE(nsCAutoString(hostName), hostNameACE);

oops... you want this instead:

>+            rv = mIDNConverter->ConvertUTF8toACE(nsDependentCString(hostName), hostNameACE);

remember, nsCAutoString creates a copy of your string.	please see
http://www.mozilla.org/projects/xpcom/string-guide.html for more info.

r=darin with that change.

Comment 8

[nhottanscp]

Attachment: Changed to use nsDependentCString.

Comment 9

[Alec Flett]

Comment on attachment 117115 [details] [diff] [review]
Changed to use nsDependentCString.

sr=alecf

Comment 10

[Darin Fisher]

Comment on attachment 117115 [details] [diff] [review]
Changed to use nsDependentCString.

r=darin

Comment 11

[nhottanscp]

checked in to the trunk