Closed Bug 1078026 Opened 10 years ago Closed 10 years ago

Clean up XPCOM string usage in nsDNSService2.cpp

Categories

(Core :: Networking: DNS, defect)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: neil, Assigned: neil)

Details

Attachments

(1 file, 1 obsolete file)

Previous code (simply blamed as hg@1 and I can't be bothered to spelunk bonsai) had some interesting use of the string API which may have helped confuse the author of bug 722197 which would explain the existence of bug 1026656 to help clean up. Additionally there is conversion from ASCII to UTF16 to UTF8 which seems unnecessary.
Attached patch Proposed patch (obsolete) — Splinter Review
* Use a char rather than a unicode tokeniser
* Stop fiddling with pointers to arguments
I also renamed the hostname parameters to aHostname to match CancelAsyncResolve.
Attachment #8500162 - Flags: feedback?(mcmanus)
Comment on attachment 8500162 [details] [diff] [review]
Proposed patch

Review of attachment 8500162 [details] [diff] [review]:
-----------------------------------------------------------------

steve is maintaining ths code now.. thanks for the patch!
Attachment #8500162 - Flags: feedback?(mcmanus) → feedback?(sworkman)
Comment on attachment 8500162 [details] [diff] [review]
Proposed patch

Review of attachment 8500162 [details] [diff] [review]:
-----------------------------------------------------------------

Good patch - thanks! Looks about right to me. If you can, pull out that repeated logic into its own (inline) function. And then run this through try to get some kind of verification. f+

::: netwerk/dns/nsDNSService2.cpp
@@ +683,5 @@
> +    } else if (!idn || IsASCII(aHostname)) {
> +        hostname = aHostname;
> +    } else if (!IsUTF8(aHostname) ||
> +               NS_FAILED(idn->ConvertUTF8toACE(aHostname, hostname))) {
> +        return NS_ERROR_FAILURE;

Oh, I had to stare at this for a while to see if there was a change in the logic. The current code is not so clear, huh? ;)

@@ +750,5 @@
> +    } else if (!idn || IsASCII(aHostname)) {
> +        hostname = aHostname;
> +    } else if (!IsUTF8(aHostname) ||
> +               NS_FAILED(idn->ConvertUTF8toACE(aHostname, hostname))) {
> +        return NS_ERROR_FAILURE;

Good catch - no localhost conversion in current code.

@@ +793,5 @@
> +    } else if (!idn || IsASCII(aHostname)) {
> +        hostname = aHostname;
> +    } else if (!IsUTF8(aHostname) ||
> +               NS_FAILED(idn->ConvertUTF8toACE(aHostname, hostname))) {
> +        return NS_ERROR_FAILURE;

This is repeated three times - could you pull it out into an inline function?
Attachment #8500162 - Flags: feedback?(sworkman) → feedback+
Attached patch Proposed patchSplinter Review
I created an inline function, although it still looks a little ugly.

I'd actually pushed the previous patch to try although I don't have the treeherder URL any more, so here's the URL for this patch:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4371a630d958
Assignee: nobody → neil
Attachment #8500162 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8502064 - Flags: review?(sworkman)
Comment on attachment 8502064 [details] [diff] [review]
Proposed patch

Review of attachment 8502064 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the function renamed.

Thanks for the patch!

::: netwerk/dns/nsDNSService2.cpp
@@ +655,5 @@
> +        aACE = aInput;
> +        return true;
> +    }
> +
> +    return IsUTF8(aInput) && NS_SUCCEEDED(aIDN->ConvertUTF8toACE(aInput, aACE));

I think this is fine and a lot less ugly than before :)

One more nit: s/ConvertUTF8ToACE/PreprocessHostname/ - it does more that UTF8 to ACE conversion, so let's name it a little more generically.
Attachment #8502064 - Flags: review?(sworkman) → review+
https://hg.mozilla.org/mozilla-central/rev/19917edca5d7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.