307437 - xpfe and suite should use the newer nsIPrefService APIs instead of nsIPref.

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Vidar Haarr (not reading bugmail)]

Part of the ongoing work I'm doing to rid the source of nsIPref.

Comment 1

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.1

First stab at nsIPref removal from xpfe/

Neil: Could you take a look at this patch?

Several things I'm not too sure about in this patch.
* General string usage - I'm not confident about chars/string API yet.
* In nsXULWindow, I change a NewURI(nsDependentCString(urlStr)... to
NewURI(urlStr..., as urlStr was previously a char* but is now a nsXPIDLCString,
and I read somewhere in the string guide that nsDependentStrings should only be
used when dealing with raw char pointers.
* The string building in nsBrowserInstance::GetHomePageGroup - I have tested
this (if it is the reader for tab groups as home page) and it works, but I'm
not sure I'm freeing and addref/releasing what needs to be.
* nsRelatedLinksHandler::Init, the mRLServerURL->AssignWithConversion, should
that be done differently? I seem to remember there being an effort to move away
from *WithConversion.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 195205 [details] [diff] [review]
version 0.1

	urlStr = "chrome://navigator/content/navigator.xul";

urlStr.AssignLiteral("chrome://..."); would be slightly more efficient

+  rv = aPref->GetComplexValue(PREF_BROWSER_STARTUP_HOMEPAGE,
NS_GET_IID(nsIPrefLocalizedString),
+				 getter_AddRefs(uri));

second line is a bit wrongly indented

+	tmpUri->GetData(getter_Copies(uriString));

another wrong indentation

-      if (NS_SUCCEEDED(rv) && *aDefaultArgs)
+      if (NS_SUCCEEDED(rv)) {

this no longer checks whether the string is nonempty...

				mRLServerURL->AssignWithConversion(prefVal);

yep, please avoid AssignWithConversion. the right thing is probably
nsISupportsString here (as a complex value).

-			prefs->UnregisterCallback("browser.search.mode",
searchModePrefCallback, this);

why no replacement for this?

Comment 3

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.2

Thank you, biesi.

I did not touch that AssignWithConversion (is that code still in use? the
fallback is www-rl.netscape.com :-), because I wasn't sure if going from a char
-> SupportsString pref without any migration code works?

Comment 4

[[e-mail]]

(In reply to comment #2)
> -			prefs->UnregisterCallback("browser.search.mode",
> searchModePrefCallback, this);
> 
> why no replacement for this?
IIRC pref observers work differently; either
a) you use a strong ref so can't unregister the callback in your destructor
b) you use a weak ref so you don't care about unregistering

Comment 5

[[e-mail]]

Comment on attachment 195205 [details] [diff] [review]
version 0.1

>+  nsXPIDLString uriString;
>+  uri->GetData(getter_Copies(uriString));
>+
>   // The "homepage" is a group of pages, put them in uriList separated by '\n'
>-  nsAutoString uriList(uri);
>+  nsAutoString uriList(uriString);
A home page group is unlikely to fit in the stack buffer. Just make uriList an
nsXPIDLString instead, then you can GetData directly into it too.

>+		nsCOMPtr<nsIPrefBranchInternal> prefBranchInternal(do_QueryInterface(prefs));
I think we're supposed to use nsIPrefBranch2 these days. Also, as
nsIPrefBranch2 inherits from nsIPrefBranch you can use a single variable to do
all the operations here.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #3)
>  I wasn't sure if going from a char
> -> SupportsString pref without any migration code works?

yes, that works. I have no idea whether the code is still used...

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 196954 [details] [diff] [review]
version 0.2

ok then, a real review ;)
(sorry for the delay)

appshell/src/nsXULWindow.cpp
      if (NS_SUCCEEDED(prefres) && urlStr[0] == '\0') {

that second part should be IsEmpty() now

browser/src/nsBrowserInstance.cpp
+      rv = prefs->GetComplexValue(PREF_HOMEPAGE_OVERRIDE_URL,
NS_GET_IID(nsIPrefLocalizedString),

can you wrap this so it fits into 80 chars?

+      if (NS_SUCCEEDED(rv))
+	 overrideURL->ToString(aDefaultArgs);
+      if (*aDefaultArgs) {
	 return NS_OK;

doesn't this leak if the pref is an empty string?

components/related/src/nsRelatedLinksHandler.cpp
-				&prefVal)) && (prefVal))
+				getter_Copies(prefVal))) &&
(!prefVal.IsEmpty()))

not quite equivalent, but probably doesn't matter

components/search/src/nsInternetSearchService.cpp
+			pb->RemoveObserver("browser.search.mode", this);

hm... this is in the destructor, so as neil pointed out, it's not very
useful...

-  nsXPIDLString defCharset;
+  nsXPIDLString defCharsetStr;

what was the motivation behing this change?


If you don't want to do that nsISupportsString thing, that's fine. but if so,
please file a bug about it. thanks :)

Comment 8

[Ryan Jones-Ward [:sciguyryan]]

Looks like this has already been fixed. Can anyone else confirm?

Comment 9

[Alfred Peng]

No, still there.

Comment 10

[[e-mail]]

-> me

Comment 11

[[e-mail]]

Attachment: updated patch

updated to trunk, and tweaked per reviewer comments.

Comment 12

[[e-mail]]

Comment on attachment 295659 [details] [diff] [review]
updated patch

>+    prefs->AddObserver("browser.search.mode", this, PR_FALSE);
This never gets removed. Either make the data source support weak references or remove it on xpcom shutdown or some such.

>         stringEncoding.Assign(NS_ConvertASCIItoUTF16(encodingList[i].stringEncoding));
I can't believe I sr'd this.

>   nsString defCharset;
>+  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREF_CONTRACTID));
>+  if (prefs) {
>+    nsCOMPtr<nsIPrefLocalizedString> defCharset;
>+    prefs->GetComplexValue("intl.charset.default",
>+                           NS_GET_IID(nsIPrefLocalizedString),
>+                           getter_AddRefs(defCharset));
>+    defCharset->GetData(getter_Copies(defCharsetStr));
>+  }
I can't believe that this compiles.

>Index: xpfe/browser/src/nsBrowserInstance.cpp
Only bits of this code are used any more; my makefile fu isn't up to working out whether Camino compiles this but if not then nobody compiles the #ifdef

>Index: xpfe/components/xremote/src/XRemoteService.cpp
And I'm fairly sure that this file is dead.

Comment 13

[[e-mail]]

(In reply to comment #12)
> This never gets removed. Either make the data source support weak references or
> remove it on xpcom shutdown or some such.

so, this shouldn't be a problem, if i'm reading http://mxr.mozilla.org/mozilla/source/modules/libpref/public/nsIPrefBranch2.idl#79 right: the prefbranch will itself observe xpcom-shutdown and remove all the observers, exactly to prevent situations like this. if you want i can do the weak ref thing though, what do you think?

> I can't believe that this compiles.

i suck :(

> >Index: xpfe/browser/src/nsBrowserInstance.cpp
> Only bits of this code are used any more; my makefile fu isn't up to working
> out whether Camino compiles this but if not then nobody compiles the #ifdef

just checked with the camino lads and it seems camino builds this file and doesn't #define MOZ_XUL_APP - so this code is live?

> >Index: xpfe/components/xremote/src/XRemoteService.cpp
> And I'm fairly sure that this file is dead.

ok; mind if i leave these little changes in, just to zap all reference to nsIPref? for great justice!

Comment 14

[[e-mail]]

(In reply to comment #13)
> the prefbranch will itself observe xpcom-shutdown and remove all the observers
Can you find someone to verify that contract?

> > >Index: xpfe/browser/src/nsBrowserInstance.cpp
> > Only bits of this code are used any more; my makefile fu isn't up to working
> > out whether Camino compiles this but if not then nobody compiles the #ifdef
> just checked with the camino lads and it seems camino builds this file and
> doesn't #define MOZ_XUL_APP - so this code is live?
For as long as they don't #define MOZ_XUL_APP ;-)

> > >Index: xpfe/components/xremote/src/XRemoteService.cpp
> > And I'm fairly sure that this file is dead.
> 
> ok; mind if i leave these little changes in, just to zap all reference to
> nsIPref? for great justice!
I guess so, but I can only review these by inspection, so don't break Camino.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #14)
> (In reply to comment #13)
> > the prefbranch will itself observe xpcom-shutdown and remove all the observers
> Can you find someone to verify that contract?

That's really what all services holding on to observers should do.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

But yeah, nsIPrefBranch2 even guarantees that.

Comment 17

[[e-mail]]

Attachment: v2

now with compile goodness.

Comment 18

[[e-mail]]

Comment on attachment 296226 [details] [diff] [review]
v2

I'm not sure we should be using nsXPIDLString in "new" code - change to nsAutoString perhaps? (getter_Copies still works)

Comment 19

[[e-mail]]

checked in, with s/nsXPIDLString/nsString/ (nsAutoString is just overhead, since it's always adopting an allocated buffer).

thanks!

Comment 20

[[e-mail]]

(In reply to comment #19)
>checked in, with s/nsXPIDLString/nsString/ (nsAutoString is just overhead,
>since it's always adopting an allocated buffer).
Sorry, I was thinking of nsString, but the patch actually showed you apparently changing an nsAutoString to an nsXPIDLString, which is what I was objecting to.