The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mounir, Assigned: Ms2ger)

Tracking

Trunk
mozilla17
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 561755 [details] [diff] [review]
Patch v1

Was this what you were thinking?
Assignee: nobody → Ms2ger
Status: NEW → ASSIGNED
Attachment #561755 - Flags: review?(mounir)
(Reporter)

Comment 2

6 years ago
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?
Attachment #561755 - Flags: review?(mounir) → review-
(Assignee)

Comment 3

5 years ago
Created attachment 615071 [details] [diff] [review]
Patch v2

Something like this?
Attachment #561755 - Attachment is obsolete: true
Attachment #615071 - Flags: review?(mounir)
(Reporter)

Comment 4

5 years ago
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
Attachment #615071 - Flags: review?(mounir) → review+
(Reporter)

Comment 5

5 years ago
Thanks for this patch btw ;)
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 643322 [details] [diff] [review]
Part b: Remove null checks

I noticed hashtables are infallible now.
Attachment #643322 - Flags: review?(mounir)
(Reporter)

Updated

5 years ago
Attachment #643322 - Flags: review?(mounir) → review+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/29ba0ce22560
https://hg.mozilla.org/mozilla-central/rev/42c33d3dd1b0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Backed out with the mass tree revert to get rid of the OS X M5 orange:
https://hg.mozilla.org/mozilla-central/rev/c801b99d726f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a4ffaef8acd
https://hg.mozilla.org/integration/mozilla-inbound/rev/84666ccc06cf
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/1a4ffaef8acd
https://hg.mozilla.org/mozilla-central/rev/84666ccc06cf
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.