Closed
Bug 1078026
Opened 10 years ago
Closed 10 years ago
Clean up XPCOM string usage in nsDNSService2.cpp
Categories
(Core :: Networking: DNS, defect)
Core
Networking: DNS
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: neil, Assigned: neil)
Details
Attachments
(1 file, 1 obsolete file)
9.24 KB,
patch
|
sworkman
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
* 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 2•10 years ago
|
||
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 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/19917edca5d7
Comment 7•10 years ago
|
||
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.
Description
•