Closed Bug 368702 Opened 13 years ago Closed 13 years ago
Effective TLD Service should treat trailing dot properly
Testcase: var tld = Components.classes["@mozilla.org/network/effective-tld-service;1"] .getService(Components.interfaces.nsIEffectiveTLDService); alert(tld.getEffectiveTLDLength("google.com.")); Currently this shows 0 though 4 is the correct value. I should have a patch soon.
Assignee: nobody → trev
Status: NEW → ASSIGNED
Attachment #253346 - Flags: review?(darin.moz)
Should we really count the trailing dot? There's already some confusion about what the length means because an IDN domain would be converted to punycode and the length returned based on the new length. This only affects effectiveTLD's that are not pure ascii, but we do in fact have a couple of those already in our domain list. Then again if Necko isn't stripping the trailing dot then maybe it does count. I'd like this made explicit in comments in the interface file either way.
If we get a domain with a trailing dot passed we certainly need to count this dot in our result, otherwise you won't be able to take a substring of given length at the end of the domain name and say that it is the effective TLD. And yes, Necko doesn't strip the trailing dot. It actually treats "domain.com" and "domain.com." as different domains (which makes sense since some servers do in fact treat them as different).
Comment on attachment 253348 [details] mochitest compatible testcase why not make this an xpcshell testcase, like the stuff in netwerk/test/unit?
No good reason not to make an xpcshell testcase but I don't know this test harness and it doesn't seem to be documented. The script to run the tests doesn't work on Windows so I had to do it manually but this seems to work.
docs are at http://developer.mozilla.org/en/docs/Writing_xpcshell-based_unit_tests make check is supposed to work on windows, I hope it still does...
Thanks for pointing me to the docs and thanks for telling about "make check" - yes, it works and even makes things quite a lot easier.
Attachment #253346 - Flags: review?(darin.moz) → review?(cbiesinger)
Attachment #253990 - Flags: review?(darin.moz) → review?(cbiesinger)
13 years ago
Attachment #253346 - Flags: review?(cbiesinger) → review+
Comment on attachment 253990 [details] [diff] [review] Unit test I'm somewhat surprised that for a host w/o TLD the length is the string length instead of 0, but ok - this was clearly the case before this patch too. I'd make Cc and Ci constants
Attachment #253990 - Flags: review?(cbiesinger) → review+
Comment on attachment 253346 [details] [diff] [review] Make sure to skip trailing dot for TLD checking The comment will need to be fixed before checking in. "ignore the trailing if one is present" should be "ignore the trailing *dot* if one is present" of course.
Attachment #253346 - Flags: superreview?(dveditz)
Comment on attachment 253346 [details] [diff] [review] Make sure to skip trailing dot for TLD checking sr=dveditz
Attachment #253346 - Flags: superreview?(dveditz) → superreview+
Attachment #253990 - Flags: superreview?(dveditz) → superreview+
This is both the fix and the unit test in one patch. The comment is fixed, also Cc and Ci in the unit test are made constants.
mozilla/netwerk/dns/src/nsEffectiveTLDService.cpp 1.2 mozilla/netwerk/test/unit/test_bug368702.js 1.1
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha5
You need to log in before you can comment on or make changes to this bug.