Closed
Bug 368702
Opened 18 years ago
Closed 18 years ago
Effective TLD Service should treat trailing dot properly
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha5
People
(Reporter: ecfbugzilla, Assigned: ecfbugzilla)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•18 years ago
|
||
Assignee | ||
Comment 2•18 years ago
|
||
Attachment #253348 -
Flags: review?(darin.moz)
Comment 3•18 years ago
|
||
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.
Assignee | ||
Comment 4•18 years ago
|
||
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 5•18 years ago
|
||
Comment on attachment 253348 [details]
mochitest compatible testcase
why not make this an xpcshell testcase, like the stuff in netwerk/test/unit?
Assignee | ||
Comment 6•18 years ago
|
||
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.
Attachment #253348 -
Attachment is obsolete: true
Attachment #253990 -
Flags: review?(darin.moz)
Attachment #253348 -
Flags: review?(darin.moz)
Comment 7•18 years ago
|
||
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...
Assignee | ||
Comment 8•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Attachment #253346 -
Flags: review?(darin.moz) → review?(cbiesinger)
Assignee | ||
Updated•18 years ago
|
Attachment #253990 -
Flags: review?(darin.moz) → review?(cbiesinger)
Updated•18 years ago
|
Flags: wanted1.8.1.x+
Updated•18 years ago
|
Attachment #253346 -
Flags: review?(cbiesinger) → review+
Comment 9•18 years ago
|
||
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+
Assignee | ||
Comment 10•18 years ago
|
||
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)
Assignee | ||
Updated•18 years ago
|
Attachment #253990 -
Flags: superreview?(dveditz)
Comment 11•18 years ago
|
||
Comment on attachment 253346 [details] [diff] [review]
Make sure to skip trailing dot for TLD checking
sr=dveditz
Attachment #253346 -
Flags: superreview?(dveditz) → superreview+
Updated•18 years ago
|
Attachment #253990 -
Flags: superreview?(dveditz) → superreview+
Assignee | ||
Comment 12•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Whiteboard: [checkin needed]
Comment 13•18 years ago
|
||
mozilla/netwerk/dns/src/nsEffectiveTLDService.cpp 1.2
mozilla/netwerk/test/unit/test_bug368702.js 1.1
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha5
Updated•18 years ago
|
Attachment #253346 -
Attachment is obsolete: true
Updated•18 years ago
|
Attachment #253990 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•