Closed
Bug 188218
Opened 22 years ago
Closed 21 years ago
Change to create instance of IDN interface as default.
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
VERIFIED
FIXED
mozilla1.4beta
People
(Reporter: nhottanscp, Assigned: nhottanscp)
References
Details
(Keywords: intl, Whiteboard: [adt1])
Attachments
(1 file, 3 obsolete files)
6.22 KB,
patch
|
darin.moz
:
review+
alecf
:
superreview+
|
Details | Diff | Splinter Review |
Currently, the pref "network.enableIDN" has to be added to prefs.js in order to activate IDN. Once bug 112979 is fixed, the interface has to be created always and the pref should only be used if the user wants to turn it off.
Assignee | ||
Updated•22 years ago
|
Assignee | ||
Comment 1•21 years ago
|
||
GetBoolPref() always returns false for NS_NET_PREF_ENABLEIDN even it is defined as true in all.js. I guess the pref system is not ready at that moment. So, I get IDN service without checking the pref. It can be disabled later when the pref observer is called.
Assignee | ||
Updated•21 years ago
|
Attachment #117927 -
Flags: review?(darin)
Assignee | ||
Comment 2•21 years ago
|
||
IDN is currently disabled as default even the pref value "network.enableIDN" in all.js is set as true, so "network.enableIDN" has to be defined again in prefs.js in each profile.
Keywords: nsbeta1
Assignee | ||
Comment 3•21 years ago
|
||
One side effect of enabling IDN early at start up is that other prefs cannot be read at that time. That is not a problem for the official "xn--" prefix which does not need any additional preference. But for the test bed which requires RACE ("bq--"), it would not work with this patch. I think I need a separate patch to install pref callbacks for those prefs.
Assignee | ||
Comment 4•21 years ago
|
||
Attachment #117927 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #118822 -
Flags: review?(darin)
Updated•21 years ago
|
Attachment #117927 -
Flags: review?(darin)
Comment 5•21 years ago
|
||
i18n triage team: nsbeta1+/adt1
Comment 6•21 years ago
|
||
Comment on attachment 118822 [details] [diff] [review] Added pref callbacks. >Index: dns/src/nsIDNService.h >+ static PRBool mMultilingualTestBed; // if true generates extra node for mulitlingual testbed >+ static char mACEPrefix[kACEPrefixLen+1]; why make these static? oh, because you want your pref observer to be able to set these variable. ic. well, why don't you make nsIDNService implement nsIObserver instead? the only reason for nsStandardURL::nsPrefObserver is the fact that nsStandardURL is not a service (i.e., you wouldn't want each instance of nsStandardURL to observe preferences independently). >Index: dns/src/nsIDNService.cpp >+ if (!nsCRT::strcmp(data, NS_LITERAL_STRING(NS_NET_PREF_IDNTESTBED).get())) { nit: do this instead: NS_LITERAL_STRING(NS_NET_PREF_IDNTESTBED).Equals(data) (sorry for not reviewing this last week!)
Attachment #118822 -
Flags: review?(darin) → review-
Assignee | ||
Comment 7•21 years ago
|
||
> (i.e., you wouldn't want each instance
> of nsStandardURL to observe preferences independently)
so I cannot add observers at the constructor, right?
darin, is there a mechanism to manage observers for a service?
Comment 8•21 years ago
|
||
generally, the rule of thumb is to create an Init method (see nsIOService::Init). from there you can setup |this| to be an observer for pref changes.
Assignee | ||
Comment 9•21 years ago
|
||
But where can I remove the observer, or no need to do that for a service?
Assignee | ||
Comment 10•21 years ago
|
||
I use NS_XPCOM_SHUTDOWN_OBSERVER_ID to remove the observers but the destructor was called first and it was never invoked. darin, do you know a way to remove observer for a service? Or can we put the observer at the constructor since the current prefs are only for testing purpose and we can remove the code later?
Assignee | ||
Comment 11•21 years ago
|
||
Attachment #118822 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #120083 -
Flags: review?(darin)
Comment 12•21 years ago
|
||
Comment on attachment 120083 [details] [diff] [review] Changed the IDN service to implement pref observers. >Index: dns/src/nsIDNService.h >+ static PRBool mInitialized; all fine, except no need to make this static. the service manager will ensure that one and only one instance of your class will be created. >Index: dns/src/nsIDNService.cpp >+nsresult nsIDNService::Init() >+{ >+ if (!mInitialized) { >+ mInitialized = PR_TRUE; in fact, no need to even have a mInitialized. >+ nsCOMPtr<nsIPrefService> prefService(do_GetService(NS_PREFSERVICE_CONTRACTID)); >+ if (prefService) { >+ nsCOMPtr<nsIPrefBranch> prefBranch; >+ prefService->GetBranch(nsnull, getter_AddRefs(prefBranch)); >+ if (prefBranch) { >+ nsCOMPtr<nsIPrefBranchInternal> prefInternal(do_QueryInterface(prefBranch)); >+ if (prefInternal) { you can rewrite this code like so: nsCOMPtr<nsIPrefBranchInternal> prefInternal(do_GetService(NS_PREFSERVICE_CONTRACTID)); if (prefInternal) { yup! the pref service implements nsIPrefBranch =) >+ strncpy(nsIDNService::mACEPrefix, prefix.get(), kACEPrefixLen); strncpy is not guaranteed to null terminate your string (some systems do, but others don't). use PL_strncpyz instead.
Attachment #120083 -
Flags: review?(darin) → review-
Assignee | ||
Comment 13•21 years ago
|
||
Attachment #120083 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #120112 -
Flags: review?(darin)
Comment 14•21 years ago
|
||
Comment on attachment 120112 [details] [diff] [review] Removed mInitialized, changed to use PL_strncpyz. r=darin
Attachment #120112 -
Flags: review?(darin) → review+
Assignee | ||
Updated•21 years ago
|
Attachment #120112 -
Flags: superreview?(alecf)
Comment 15•21 years ago
|
||
Comment on attachment 120112 [details] [diff] [review] Removed mInitialized, changed to use PL_strncpyz. sr=alecf
Assignee | ||
Comment 16•21 years ago
|
||
checked in to the trunk
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.4beta
Comment 17•21 years ago
|
||
Comment on attachment 120112 [details] [diff] [review] Removed mInitialized, changed to use PL_strncpyz. sr=alecf
Attachment #120112 -
Flags: superreview?(alecf) → superreview+
You need to log in
before you can comment on or make changes to this bug.
Description
•