240466 - nsDocument uses wrong interface to get intl.accept_languages preference

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Scooter Morris]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040413
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040413

The intl.accept_languages preference is of type nsIPrefLocalizedString, but
nsDocument::RetreiveRelevantHeaders treats it as a CharPref.


Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Scooter Morris]

Attachment: First-shot patch

Here is a patch that fixes this.  I'm not too happy about the need to do the
CopyUTF16toUTF8.  Is there another way?

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 146078 [details] [diff] [review]
First-shot patch

>   if (!have_contentLanguage) {
>+    nsCOMPtr<nsIPrefLocalizedString> prefString;

You don't use always use this in this code branch (for isntance, if getting the
pref branch fails).  Move it to the code branch where it is used.

>     nsCOMPtr<nsIPrefBranch> prefBranch =
>       do_GetService(NS_PREFSERVICE_CONTRACTID);
> 
>     if (prefBranch) {
>-      prefBranch->GetCharPref("intl.accept_languages",
>-                              getter_Copies(mContentLanguage));
>+      prefBranch->GetComplexValue("intl.accept_languages",
>+                                  NS_GET_IID(nsIPrefLocalizedString),
>+                                  getter_AddRefs(prefString));
>+      if (prefString) {
>+        // Unfortunately, nsIPrefLocalizedString doesn't provide an interface
>+        // for nsXPIDLCStrings, only nsXPIDLStrings...

No it doesn't.	It provides an API for extracting a wstring (or PRUnichar in
c++).  The fact that you can use nsXPIDLStrings to wrap them is of no concern
to the interface.

That aside, please do not complain about what an interface does or doesn't
provide in comments.  That doesn't solve anything.  If you are unhappy with an
interface, file a bug against the interface,
fix/replace/subclass/obsolete/whatever the interface, and update callers.  Or
just deal and move on.	;-)


r=caillon, apart from those two nits.

Comment 3

[Scooter Morris]

Attachment: Updated patch with caillon's comments incorporated

Comment 4

[Scooter Morris]

(In reply to comment #2)
> (From update of attachment 146078 [details] [diff] [review])
> >   if (!have_contentLanguage) {
> >+    nsCOMPtr<nsIPrefLocalizedString> prefString;
> 
> You don't use always use this in this code branch (for isntance, if getting the
> pref branch fails).  Move it to the code branch where it is used.
Absolutely.  Good catch.
> 
> >     nsCOMPtr<nsIPrefBranch> prefBranch =
> >       do_GetService(NS_PREFSERVICE_CONTRACTID);
> > 
> >     if (prefBranch) {
> >-      prefBranch->GetCharPref("intl.accept_languages",
> >-                              getter_Copies(mContentLanguage));
> >+      prefBranch->GetComplexValue("intl.accept_languages",
> >+                                  NS_GET_IID(nsIPrefLocalizedString),
> >+                                  getter_AddRefs(prefString));
> >+      if (prefString) {
> >+        // Unfortunately, nsIPrefLocalizedString doesn't provide an interface
> >+        // for nsXPIDLCStrings, only nsXPIDLStrings...
> 
> No it doesn't.	It provides an API for extracting a wstring (or PRUnichar in
> c++).  The fact that you can use nsXPIDLStrings to wrap them is of no concern
> to the interface.
> 
> That aside, please do not complain about what an interface does or doesn't
> provide in comments.  That doesn't solve anything.  If you are unhappy with an
> interface, file a bug against the interface,
> fix/replace/subclass/obsolete/whatever the interface, and update callers.  Or
> just deal and move on.	;-)
Er, I was moving on, and it wasn't meant as a complaint, but as documentation as
to why the copy was necessary.  Nevertheless, I removed the comment.
> 
> 
> r=caillon, apart from those two nits.
> 

Thanks for the review!

Comment 5

[Scooter Morris]

Attachment: Here is an updated patch that uses the new getLocalizedStringPref interface

Comment 6

[Scooter Morris]

Attachment: Oops -- left some extra includes in....

Comment 7

[Boris Zbarsky [:bzbarsky]]

Patch was checked in at 2004-05-27 22:18 PDT.  Marking fixed.