688415 - Outparamdel nsDocument::GetRadioGroup

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 1

[Mounir Lamouri (:mounir)]

Comment on attachment 561704 [details] [diff] [review]
Patch v1

Review of attachment 561704 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the null check issue addressed.

::: content/base/src/nsDocument.cpp
@@ +6432,1 @@
>       ToLowerCase(tmKey); //should case-insensitive.

nit: use { }

@@ +6432,5 @@
>       ToLowerCase(tmKey); //should case-insensitive.
> +
> +  nsRadioGroupStruct* radioGroup;
> +  if (mRadioGroups.Get(tmKey, &radioGroup))
> +    return radioGroup;

nit: use { }

@@ +6434,5 @@
> +  nsRadioGroupStruct* radioGroup;
> +  if (mRadioGroups.Get(tmKey, &radioGroup))
> +    return radioGroup;
> +
> +  nsAutoPtr<nsRadioGroupStruct> newRadioGroup(new nsRadioGroupStruct());

nit: I would prefer: nsAutoPtr<nsRadioGroupStruct> newRadioGroup = new nsRadioGroupStruct();

@@ +6435,5 @@
> +  if (mRadioGroups.Get(tmKey, &radioGroup))
> +    return radioGroup;
> +
> +  nsAutoPtr<nsRadioGroupStruct> newRadioGroup(new nsRadioGroupStruct());
> +  NS_ENSURE_TRUE(mRadioGroups.Put(tmKey, newRadioGroup), nsnull);

Is that possible in normal circumstance? Maybe an assert would be more appropriate?
If it is, we should remove all null checks in the caller side or change them with an assert.

@@ +6478,5 @@
>      return NS_OK;
>    }
>  
> +  nsRadioGroupStruct* radioGroup = GetRadioGroup(name);
> +  NS_ENSURE_TRUE(radioGroup, NS_ERROR_OUT_OF_MEMORY);

Same as comment above: do we want to assert here?

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #1)
> @@ +6434,5 @@
> > +  nsRadioGroupStruct* radioGroup;
> > +  if (mRadioGroups.Get(tmKey, &radioGroup))
> > +    return radioGroup;
> > +
> > +  nsAutoPtr<nsRadioGroupStruct> newRadioGroup(new nsRadioGroupStruct());
> 
> nit: I would prefer: nsAutoPtr<nsRadioGroupStruct> newRadioGroup = new
> nsRadioGroupStruct();

IIRC, that didn't compile.

> @@ +6435,5 @@
> > +  if (mRadioGroups.Get(tmKey, &radioGroup))
> > +    return radioGroup;
> > +
> > +  nsAutoPtr<nsRadioGroupStruct> newRadioGroup(new nsRadioGroupStruct());
> > +  NS_ENSURE_TRUE(mRadioGroups.Put(tmKey, newRadioGroup), nsnull);
> 
> Is that possible in normal circumstance? Maybe an assert would be more
> appropriate?
> If it is, we should remove all null checks in the caller side or change them
> with an assert.

Our hashtables don't use infallible allocation, AFAIK, so I don't think we should assert.

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to Ms2ger from comment #2)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #1)
> > @@ +6434,5 @@
> > > +  nsRadioGroupStruct* radioGroup;
> > > +  if (mRadioGroups.Get(tmKey, &radioGroup))
> > > +    return radioGroup;
> > > +
> > > +  nsAutoPtr<nsRadioGroupStruct> newRadioGroup(new nsRadioGroupStruct());
> > 
> > nit: I would prefer: nsAutoPtr<nsRadioGroupStruct> newRadioGroup = new
> > nsRadioGroupStruct();
> 
> IIRC, that didn't compile.

Hmm, why?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

nsAutoPtr<nsRadioGroupStruct> newRadioGroup = new nsRadioGroupStruct();

gives

/content/base/src/nsDocument.cpp: In member function ‘nsRadioGroupStruct* nsDocument::GetRadioGroup(const nsAString_internal&)’:
/content/base/src/nsDocument.cpp:6485:72: error: conversion from ‘nsRadioGroupStruct*’ to non-scalar type ‘nsAutoPtr<nsRadioGroupStruct>’ requested

Fixed your other nits.

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 614704 [details] [diff] [review]
Patch v2

Review of attachment 614704 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments taken into account.

::: content/base/src/nsDocument.cpp
@@ +6482,5 @@
> +    return radioGroup;
> +  }
> +
> +  nsAutoPtr<nsRadioGroupStruct> newRadioGroup(new nsRadioGroupStruct());
> +  NS_ENSURE_TRUE(mRadioGroups.Put(tmKey, newRadioGroup), nsnull);

I'm not sure why there is a nsAutoPtr here. Is it really needed?
Also, I would prefer an NS_ASSERTION instead of NS_ENSURE_TRUE. Even if it's not sure the allocation will success, I guess it should *not* fail.

And a nit: can you leave a blank line after this line (before the |return| statement).

@@ +6495,2 @@
>    if (radioGroup) {
>      radioGroup->mSelectedRadioButton = aRadio;

As long as you are here, can you change that code to do:
NS_ENSURE_TRUE(radioGroup, NS_OK);

// real code

@@ +6507,3 @@
>    if (radioGroup) {
>      *aRadio = radioGroup->mSelectedRadioButton;
>      NS_IF_ADDREF(*aRadio);

ditto

@@ +6553,2 @@
>    if (!radioGroup) {
>      return NS_ERROR_FAILURE;

I would prefer NS_ENSURE_SUCCESS(radioGroup, NS_ERROR_FAILURE) just below the assignment like the previous chunk is doing.

@@ +6601,1 @@
>    if (radioGroup) {

Early return with NS_ENSURE_TRUE please :)

@@ +6619,1 @@
>    if (radioGroup) {

ditto

@@ +6640,3 @@
>    if (!radioGroup) {
>      return NS_OK;
>    }

nit: that could be changed to NS_ENSURE_SUCCESS but do whatever you want ;)

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #5)
> Comment on attachment 614704 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 614704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the comments taken into account.
> 
> ::: content/base/src/nsDocument.cpp
> @@ +6482,5 @@
> > +    return radioGroup;
> > +  }
> > +
> > +  nsAutoPtr<nsRadioGroupStruct> newRadioGroup(new nsRadioGroupStruct());
> > +  NS_ENSURE_TRUE(mRadioGroups.Put(tmKey, newRadioGroup), nsnull);
> 
> I'm not sure why there is a nsAutoPtr here. Is it really needed?

if (!mRadioGroups.Put(tmKey, newRadioGroup)) {
  delete newRadioGroup;
  return nsnull;
}

would work too, but nsAutoPtr is safer.

> Also, I would prefer an NS_ASSERTION instead of NS_ENSURE_TRUE. Even if it's
> not sure the allocation will success, I guess it should *not* fail.

I disagree; we shouldn't assert things that can happen, even on OOM. I'll be happy to remove the code that checks for that after bug 734847 lands, but not now.

Comment 7

[Mounir Lamouri (:mounir)]

Ok.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/24ebe492528b