636123 - nsDocument::GetRadioGroup should be split in two methods: one to get the radio group and one to create it

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

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 (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Was this what you were thinking?

Comment 2

[Mounir Lamouri (:mounir)]

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 (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Something like this?

Comment 4

[Mounir Lamouri (:mounir)]

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)]

Thanks for this patch btw ;)

Comment 6

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

(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 (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Remove null checks

I noticed hashtables are infallible now.

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/29ba0ce22560
https://hg.mozilla.org/mozilla-central/rev/42c33d3dd1b0

Comment 9

[Ed Morley [:emorley]]

Backed out with the mass tree revert to get rid of the OS X M5 orange:
https://hg.mozilla.org/mozilla-central/rev/c801b99d726f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a4ffaef8acd
https://hg.mozilla.org/integration/mozilla-inbound/rev/84666ccc06cf

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a4ffaef8acd
https://hg.mozilla.org/mozilla-central/rev/84666ccc06cf