241975 - nsSOAPPropertyBag cleanup

Status: RESOLVED FIXED

(Core Graveyard :: Web Services, defect)

Description

[Harshal Pradhan]

.

Comment 1

[Harshal Pradhan]

Attachment: Cleanup

- Use nsCOMArray
- Use nsInterfaceHashtable
- Kill redundant member variable
- Make a method non-virtual

Comment 2

[Harshal Pradhan]

Comment on attachment 147242 [details] [diff] [review]
Cleanup

peterv, you've been doing similar stuff in schema recently... would you please
look at this?

Comment 3

[timeless]

Comment on attachment 147242 [details] [diff] [review]
Cleanup

> nsSOAPPropertyBagMutator::nsSOAPPropertyBagMutator()
> {
>-  mBag = mSOAPBag = new nsSOAPPropertyBag();
>+  mSOAPBag = new nsSOAPPropertyBag();
> }
...
> NS_IMETHODIMP nsSOAPPropertyBagMutator::GetPropertyBag(nsIPropertyBag ** aPropertyBag) {
>   NS_ENSURE_ARG_POINTER(aPropertyBag);
>-  *aPropertyBag = mBag;
>+  *aPropertyBag = mSOAPBag;
>   NS_IF_ADDREF(*aPropertyBag);
>   return NS_OK;
> }

The idl interface says that propertyBag "will never be null"
<http://lxr.mozilla.org/seamonkey/source/extensions/webservices/public/nsISOAPP
ropertyBagMutator.idl#54>. Would you consider adding an nsresult
nsSOAPPropertyBagMutator::Init method, moving the mSOAPBag construction into it
and changing
http://lxr.mozilla.org/seamonkey/source/extensions/webservices/build/src/nsWebS
ervicesModule.cpp#80
< NS_GENERIC_FACTORY_CONSTRUCTOR(nsSOAPPropertyBagMutator)
> NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsSOAPPropertyBagMutator, Init)
?

Then you can fix GetPropertyBag to NS_ADDREF per contract.

Comment 4

[Harshal Pradhan]

Attachment: What timeless said ...

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 147292 [details] [diff] [review]
What timeless said ...

>Index: soap/src/nsSOAPPropertyBag.cpp
>===================================================================

>+nsSOAPPropertyBag::nsSOAPPropertyBag()
> {
>-  /* property initializers and constructor code */
>+  mProperties.Init();

Since this can fail you should probably create nsSOAPPropertyBag::Init and make
that return NS_ERROR_FAILURE if Init returns PR_FALSE. And change
NS_GENERIC_FACTORY_CONSTRUCTOR to _INIT and make nsSOAPPropertyBagMutator::Init
call nsSOAPPropertyBag::Init.


>+nsresult
>     nsSOAPPropertyBag::SetProperty(const nsAString & aName,
>                                    nsIVariant * aValue)
> {
>   NS_ENSURE_ARG_POINTER(&aName);

Bah, remove that.


>+PLDHashOperator PR_CALLBACK PropertyBagEnumFunc(const nsAString& aKey, nsIVariant *aData, void *aClosure)
> {

>+  nsCOMArray<nsIProperty>* properties =
>+      NS_STATIC_CAST(nsCOMArray<nsIProperty>*, aClosure);
>+  properties->AppendObject(new nsSOAPProperty(aKey, aData));

Maybe we should check for failure here and return PL_DHASH_STOP in that case?

>+nsSOAPPropertyBagEnumerator::nsSOAPPropertyBagEnumerator(nsSOAPPropertyBag * aPropertyBag) : mCurrent(0)
> {
>-  aPropertyBag->mProperties->Enumerate(&PropertyBagEnumFunc, mProperties);
>+  aPropertyBag->mProperties.EnumerateRead(PropertyBagEnumFunc, &mProperties);

Which means this call would move to nsSOAPPropertyBagEnumerator::Init and you'd
need to compare the count it returns to aPropertyBag->mProperties.Count().

> NS_IMETHODIMP nsSOAPPropertyBagEnumerator::GetNext(nsISupports ** aItem)
> {

>-    *aItem = mProperties->ElementAt(mCurrent++);
>+    NS_ADDREF(*aItem = mProperties[mCurrent++]);

Technically this needs to QI to nsISupports, though we know it's alright cause
mProperties contains  nsSOAPProperty's. Maybe mProperties can just be
nsCOMArray<nsISupports> after all?

>@@ -191,8 +172,7 @@
>     nsSOAPPropertyBagEnumerator::HasMoreElements(PRBool * aResult)
> {
>   NS_ENSURE_ARG_POINTER(aResult);
>-  PRUint32 count;
>-  mProperties->Count(&count);
>+  PRUint32 count = mProperties.Count();
>   *aResult = mCurrent < count;

I'd just do

  *aResult = mCurrent < mProperties.Count();

Comment 6

[Harshal Pradhan]

Attachment: This should fix those issues

- Sprinkled Init() in places where hashtables need to be inited.
- Handle erros from Init(), and during enumeration.
- Use nsCOMArray<nsISupports> to avoid casts.
- Fix stupid string param checks.
- Cleanup some more whitespace/formatting etc. where
  I am touching code anyway.

Only nsSOAPPropertyBagMutator has an xpcom factory constructor, so,
the other two dont need any changes in nsWebServicesModule.cpp

Comment 7

[Harshal Pradhan]

Attachment: -w version of the above, might be marginally easier to read

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 147382 [details] [diff] [review]
This should fix those issues

>Index: soap/src/nsSOAPPropertyBag.cpp
>===================================================================

> /* readonly attribute nsISimpleEnumerator enumerator; */
> NS_IMETHODIMP
>-    nsSOAPPropertyBag::GetEnumerator(nsISimpleEnumerator * *aEnumerator)
>+nsSOAPPropertyBag::GetEnumerator(nsISimpleEnumerator * *aEnumerator)
> {
>   NS_ENSURE_ARG_POINTER(aEnumerator);

>+
>+  nsSOAPPropertyBagEnumerator* enumerator = new nsSOAPPropertyBagEnumerator();
>+  NS_ENSURE_TRUE(enumerator, NS_ERROR_OUT_OF_MEMORY);
>+
>+  if (!enumerator->Init(this)) {
>+    delete enumerator;

I'd just make enumerator a nsRefPtr<nsSOAPPropertyBagEnumerator>. Either way,
r=peterv.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 147382 [details] [diff] [review]
This should fix those issues

sr=jst

Comment 10

[Harshal Pradhan]

Checked in with peterv's refptr suggestion.