Last Comment Bug 688415 - Outparamdel nsDocument::GetRadioGroup
: Outparamdel nsDocument::GetRadioGroup
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: :Ms2ger
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-22 05:03 PDT by :Ms2ger
Modified: 2012-04-15 04:39 PDT (History)
2 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (7.70 KB, patch)
2011-09-22 05:03 PDT, :Ms2ger
mounir: review+
Details | Diff | Review
Patch v2 (7.75 KB, patch)
2012-04-13 02:12 PDT, :Ms2ger
mounir: review+
Details | Diff | Review

Description :Ms2ger 2011-09-22 05:03:02 PDT
Created attachment 561704 [details] [diff] [review]
Patch v1
Comment 1 Mounir Lamouri (:mounir) 2011-09-22 14:47:33 PDT
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 2011-09-23 01:13:46 PDT
(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) 2011-09-23 01:17:15 PDT
(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 2012-04-13 02:12:28 PDT
Created attachment 614704 [details] [diff] [review]
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) 2012-04-13 04:31:54 PDT
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 2012-04-13 04:40:44 PDT
(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) 2012-04-13 05:48:56 PDT
Ok.

Note You need to log in before you can comment on or make changes to this bug.