212125 - add NS_LoadPeristentPropertiesFromURI

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jungshik Shin]

darin replied to my posting quoted below that because xpcom is not supposed to
know anything about nsIURI, I have to go with NS_LoadPersistentPropertiesFromURI.  


re: nntp://news.mozilla.org/be6cet$[e-mail]
    nntp://news.mozilla.org/[e-mail]

--------------quote----------
While working on bug 208213 (http://bugzilla.mozilla.org/show_bug.cgi?id=208213),
I realized that the following pattern is  used in several places (mostly under
gfx but also under layout/mathml) :

1. get a url with a const string (url spec)
2. get an input stream
3. create an instance of nsIPer...Prop.
4. load from the input stream

See,  for instance,

http://lxr.mozilla.org/seamonkey/source/gfx/src/mac/nsMacUnicodeFontInfo.cpp#481

(or http://lxr.mozilla.org/seamonkey/search?string=nsIPersistentProperties)

If not a method in the interface, how abuot  NS_LoadPeristenPropertiesFromURI?
------quote-------------

Comment 1

[Jungshik Shin]

Attachment: a patch

I added |NS_InitPersistentPropertiesFromURISpec| to NetUtils.h 
"Init" seems to be better than "Load" because all the old functions replaced by
this new one use 'Init' in their names.

Comment 2

[Jungshik Shin]

Comment on attachment 127327 [details] [diff] [review]
a patch

Most of files touched fall under rbs' umbrella	:-) so that I'm asking him for
r and darin for sr (for NetUtils.h)

Comment 3

[Darin Fisher]

Comment on attachment 127327 [details] [diff] [review]
a patch

>Index: netwerk/base/public/nsNetUtil.h

>+inline nsresult
>+NS_InitPersistentPropertiesFromURISpec(nsIPersistentProperties **result,
>+                                       const nsAString& spec)

"Init" seems like the wrong verb here.	you're instantiating something,
so it seems like the verb should be "New", but perhaps "Load" is still
best.


>+{
>+    nsCOMPtr<nsIURI> uri;
>+    nsresult rv = NS_NewURI(getter_AddRefs(uri), spec);

also, i'm not sure i like the fact that you are hiding the other
parameters to NS_NewURI.  what about the |originCharset| and the
|baseURI|?  perhaps this function should just take a nsIURI instead.
you could then have another version that takes a |const nsACString&|.
it might also be good to offer an optional nsIIOService parameter
that should be forwarded to NS_OpenURI.

NS_LoadPersistentPropertiesFromURI(nsIPersistentProperties **result,
				   nsIURI		    *uri,
				   nsIIOService 	    *ios = nsnull)
{
  ...
}

also, be sure to keep the indentation and bracket style consistent
with nsNetUtil.h

Comment 4

[Jungshik Shin]

Attachment: a new patch with darin's concerns addressed

Thanks for the review, darin.

I added NS_LoadPeristenPropertiesFromURI and added optional arguments (charset,
baseURI, ioService) to NS_LoadPersistenPropertiesFromURISpec. The latter is a
thin wrapper over the former. In addition, this patch uses |const nsACString&|
instead of |const nsAString&| for |spec|

Comment 5

[Darin Fisher]

Comment on attachment 127404 [details] [diff] [review]
a new patch with darin's concerns addressed

>Index: netwerk/base/public/nsNetUtil.h

>+inline nsresult
>+NS_LoadPersistentPropertiesFromURI(nsIPersistentProperties **result,
>+                                   nsIURI                  *uri,
>+                                   nsIIOService            *ioService = nsnull)
>+{
>+    nsCOMPtr<nsIInputStream> in;
>+    nsresult rv = NS_OpenURI(getter_AddRefs(in), uri, ioService);
>+    if (NS_SUCCEEDED(rv)) {
>+        nsCOMPtr<nsIPersistentProperties> properties = 
>+            do_CreateInstance(NS_PERSISTENTPROPERTIES_CONTRACTID, &rv);
>+        if (NS_SUCCEEDED(rv)) {
>+            *result = properties;
>+            NS_ADDREF(*result);
>+            rv = (*result)->Load(in);
>+        }
>+    }
>+    return rv;
>+}

suppose someone writes code like this:

  nsIPersistentProperties *prop;
  rv = NS_LoadPersistentPropertiesFromURI(&prop, uri);
  if (NS_FAILED(rv)) return rv;

now suppose the function call fails because
nsIPersistentProperties::Load happens to fail.
won't we leak |prop|?

bottom line: you should not assign to |*result| until
you know the function will succeed.  do this instead:

NS_LoadPersistentPropertiesFromURI(nsIPersistentProperties **result,
...
{
...
	if (NS_SUCCEEDED(rv)) {
	    rv = properties->Load(in);
	    if (NS_SUCCEEDED(rv))
		NS_ADDREF(*result = properties);
	}
    }
    return rv;
}

>+    if (NS_SUCCEEDED(rv)) {
>+        rv = NS_LoadPersistentPropertiesFromURI(result, uri, ioService);
>+    }

nit: the existing style of nsNetUtil.h is to leave out brackets
when they are not necessary.  please make this code consistent.

Comment 6

[Jungshik Shin]

Attachment: patch (potential leak filled up)

Thanks for the catch. As for 'braces', I should have looked  further up than
|NS_NewPostDataStream| which has if-block with a single statement enc. by
braces ;-)

Comment 7

[Jungshik Shin]

Comment on attachment 127410 [details] [diff] [review]
patch (potential leak filled up)

I'm sorry for spamming. I wish bugzilla allowed me to do all these small steps
in a single shot...

Comment 8

[rbs]

Comment on attachment 127410 [details] [diff] [review]
patch (potential leak filled up)

r=rbs

Comment 9

[Jungshik Shin]

thanks. fix checked into trunk

Comment 10

[benc]

is there a whitebox or API test for this change?