106940 - MozillaBrowser.cpp should use the newer nsIPrefService APIs instead of nsIPref

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Chip Clark]

This is part of a larger clean up project to move from nsIPref to nsIPrefService
and nsIPrefBranch

Comment 1

[Maureen Gerlofs (IBM)]

Attachment: Replace nsIPref with nsIPrefBranch and friends

Replaced refrences to nsIPref from mozilla/src/embedding/browser/activex and
replaced with nsIPrefBranch and friends.

Comment 2

[Mike Kaply [:mkaply]]

Comment on attachment 152944 [details] [diff] [review]
Replace nsIPref with nsIPrefBranch and friends

In some of the comments, you changed prefs to prefBranch.

In those cases, prefs makes more sense.

r=mkaply

Comment 3

[Darin Fisher]

Comment on attachment 152944 [details] [diff] [review]
Replace nsIPref with nsIPrefBranch and friends

>Index: common/IWebBrowserImpl.h

>         CComBSTR bstrUrl(L"http://home.netscape.com/");
> 
>+        // Find the home page stored in prefBranch
>+        nsCOMPtr<nsIPrefBranch> prefBranch;
>+        if (NS_SUCCEEDED(GetPrefs(getter_AddRefs(prefBranch))))
>+        {
>+            nsCOMPtr<nsIPrefLocalizedString> homePage;
>+            prefBranch->GetComplexValue("browser.startup.homepage",
>+                                             NS_GET_IID(nsIPrefLocalizedString),
>+                                             getter_AddRefs(homePage));
>+
>+            if (homePage)
>             {
>+               nsXPIDLString homePageString;
>+               nsresult rv=homePage->ToString(getter_Copies(homePageString));
>+               if (rv == NS_OK)
>+               {
>+                 bstrUrl = homePageString.get();
>+               }
>             }
>         }

a few nits in the above code:

 - indentation is not uniform; should be 4 white spaces
 - add whitespace around '=' sign, |nsresult rv = homepage...|
 - |if (rv == NS_OK)| should be |if (NS_SUCCEEDED(rv))|


>+        // Find the home page stored in prefBranch
>+        nsCOMPtr<nsIPrefBranch> prefBranch;
>+        if (NS_SUCCEEDED(GetPrefs(getter_AddRefs(prefBranch))))
>         {
>+            // TODO find and navigate to the search page stored in prefBranch
>             //      and not this hard coded address
>         }

seems to me that this code should just be commented out since
it does nothing at the moment.


>Index: control/StdAfx.h

>+#include "nsIPrefBranch.h"
>+#include "nsIPrefService.h"

the fact that StdAfx.h includes nsIPrefBranch.h and nsIPrefService.h
seems to suggest that the .cpp files shouldn't need to include them
explicitly.  is that true?  if so, how about removing the redundant
includes from the .cpp files.


>Index: plugin/XPCBrowser.cpp

>+nsresult IEBrowser::GetPrefs(nsIPrefBranch **aPrefBranch)
> {
>+    NS_ENSURE_ARG_POINTER(aPrefBranch);
>     nsresult rv;
>+    nsCOMPtr<nsIPrefBranch> prefBranch = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+    *aPrefBranch = prefBranch;
>+    NS_IF_ADDREF(*aPrefBranch);
>     return rv;
> }

you can also write this function like so:

  nsresult IEBrowser::GetPrefs(nsIPrefBranch **aPrefBranch)
  {
      return CallGetService(NS_PREFSERVICE_CONTRACTID, aPrefBranch);
  }


sr=darin with these revisions.

Comment 4

[Maureen Gerlofs (IBM)]

Attachment: Updated patch based on mkaply and darin's comments.

No updates were made to /mozilla/embedding/browser/activex/src/control/StdAfx.h
from the last patch, since it is used only in the control subdirectory.

Comment 5

[Darin Fisher]

fixed-on-trunk