Last Comment Bug 636123 - nsDocument::GetRadioGroup should be split in two methods: one to get the radio group and one to create it
: nsDocument::GetRadioGroup should be split in two methods: one to get the radi...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-23 04:03 PST by Mounir Lamouri (:mounir)
Modified: 2012-08-04 18:40 PDT (History)
1 user (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (4.20 KB, patch)
2011-09-22 08:41 PDT, :Ms2ger (⌚ UTC+1/+2)
mounir: review-
Details | Diff | Splinter Review
Patch v2 (8.86 KB, patch)
2012-04-14 11:42 PDT, :Ms2ger (⌚ UTC+1/+2)
mounir: review+
Details | Diff | Splinter Review
Part b: Remove null checks (4.78 KB, patch)
2012-07-18 05:03 PDT, :Ms2ger (⌚ UTC+1/+2)
mounir: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-02-23 04:03:33 PST
This method is currently creating a radio group if none is found and we might not want to do that in some cases (when we want to read a value, we don't want to bother creating an entry if none is found).
That would also allow us make GetRadioGroup const.
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2011-09-22 08:41:01 PDT
Created attachment 561755 [details] [diff] [review]
Patch v1

Was this what you were thinking?
Comment 2 Mounir Lamouri (:mounir) 2011-09-22 14:55:55 PDT
Comment on attachment 561755 [details] [diff] [review]
Patch v1

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

So what I wanted was:
nsRadioGroupStruct* GetRadioGroup(const nsAString&) const;
nsRadioGroupStruct* CreateRadioGroup(const nsAString&);

And fix the callers to have them call CreateRadioGrup() when they actually need to do so.

I do not like the GetRadioGroup()/GetExistingRadioGroup() naming. The former is still confusing and it's not obvious that it will create a group if none is found.
In addition, I would prefer GetRadioGroupInternal() instead of GetRadioGroupLC(). I'm not even sure what LC means here.

BTW, if you do that, some null-checks after GetRadioGroup() will still be needed (re: the bug where I told you you could remove them)

Does that make sense to you?
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2012-04-14 11:42:54 PDT
Created attachment 615071 [details] [diff] [review]
Patch v2

Something like this?
Comment 4 Mounir Lamouri (:mounir) 2012-04-16 02:29:49 PDT
Comment on attachment 615071 [details] [diff] [review]
Patch v2

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

I would have preferred something like:
nsRadioGroupStruct* radioGroup = GetRadioGroup(str);
if (!radioGroup) {
  radioGroup = CreateRadioGroup(str);
  NS_ENSURE_TRUE(radioGroup, OOM_ERROR);
}

But I understand it could be painful and error-prone to do that and I will try to not be stubborn ;)

::: content/base/src/nsDocument.cpp
@@ +6498,5 @@
> +  if (IsHTML()) {
> +    ToLowerCase(tmKey); //should case-insensitive.
> +  }
> +
> +  if (nsRadioGroupStruct* radioGroup = GetRadioGroupInternal(tmKey)) {

Hmm, I think I would prefer:
nsRadioGroupStruct* radioGroup = GetRadioGroupInternal(tmKey);
if (radioGroup) {
  return radioGroup;
}

@@ +6517,1 @@
>    NS_ENSURE_TRUE(radioGroup, NS_OK);

Shouldn't that return OOM instead? Given that if radioGroup is null that means we failed to create a new entry.

@@ +6528,1 @@
>    NS_ENSURE_TRUE(radioGroup, NS_OK);

ditto

@@ +6573,1 @@
>    NS_ENSURE_TRUE(radioGroup, NS_ERROR_FAILURE);

ditto

@@ +6619,1 @@
>    NS_ENSURE_TRUE(radioGroup, NS_OK);

ditto

@@ +6636,1 @@
>    NS_ENSURE_TRUE(radioGroup, NS_OK);

ditto

@@ +6656,1 @@
>    NS_ENSURE_TRUE(radioGroup, NS_OK);

ditto
Comment 5 Mounir Lamouri (:mounir) 2012-04-16 02:30:09 PDT
Thanks for this patch btw ;)
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-04-16 03:01:42 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> Comment on attachment 615071 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 615071 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would have preferred something like:
> nsRadioGroupStruct* radioGroup = GetRadioGroup(str);
> if (!radioGroup) {
>   radioGroup = CreateRadioGroup(str);
>   NS_ENSURE_TRUE(radioGroup, OOM_ERROR);
> }
> 
> But I understand it could be painful and error-prone to do that and I will
> try to not be stubborn ;)

I thought you would, but I given the number of times I'd have to repeat this pattern, I'd rather not.

> @@ +6517,1 @@
> >    NS_ENSURE_TRUE(radioGroup, NS_OK);
> 
> Shouldn't that return OOM instead? Given that if radioGroup is null that
> means we failed to create a new entry.

Fair point, but I didn't want to change behaviour in this patch.
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-07-18 05:03:30 PDT
Created attachment 643322 [details] [diff] [review]
Part b: Remove null checks

I noticed hashtables are infallible now.
Comment 9 Ed Morley [:emorley] 2012-08-04 11:28:43 PDT
Backed out with the mass tree revert to get rid of the OS X M5 orange:
https://hg.mozilla.org/mozilla-central/rev/c801b99d726f

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