188218 - Change to create instance of IDN interface as default.

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[nhottanscp]

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.

Comment 1

[nhottanscp]

Attachment: Initialize IDN service at nsStandardURL constructor.

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.

Comment 2

[nhottanscp]

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.

Comment 3

[nhottanscp]

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.

Comment 4

[nhottanscp]

Attachment: Added pref callbacks.

Comment 5

[Samir Gehani]

i18n triage team: nsbeta1+/adt1

Comment 6

[Darin Fisher]

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!)

Comment 7

[nhottanscp]

> (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

[Darin Fisher]

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.

Comment 9

[nhottanscp]

But where can I remove the observer, or no need to do that for a service?

Comment 10

[nhottanscp]

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?

Comment 11

[nhottanscp]

Attachment: Changed the IDN service to implement pref observers.

Comment 12

[Darin Fisher]

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.

Comment 13

[nhottanscp]

Attachment: Removed mInitialized, changed to use PL_strncpyz.

Comment 14

[Darin Fisher]

Comment on attachment 120112 [details] [diff] [review]
Removed mInitialized, changed to use PL_strncpyz.

r=darin

Comment 15

[Alec Flett]

Comment on attachment 120112 [details] [diff] [review]
Removed mInitialized, changed to use PL_strncpyz.

sr=alecf

Comment 16

[nhottanscp]

checked in to the trunk

Comment 17

[Alec Flett]

Comment on attachment 120112 [details] [diff] [review]
Removed mInitialized, changed to use PL_strncpyz.

sr=alecf

Comment 18

[Yuying Long]

Mark as verified as it works on latest build.