893909 - Cleanup HTMLSelectElement

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Part a: Add a IsOptionDisabled overload without an outparam

Comment 2

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

Attachment: Part b: Return void from InsertOptionsIntoList{,Recurse}

Comment 3

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

Attachment: Part c: Cleanup HTMLSelectElement::InsertOptionsIntoList

Comment 4

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

Attachment: Part d: Cleanup Remove

Comment 5

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

Attachment: Part e: Cleanup SetLength

Comment 6

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

Attachment: Part f: Cleanup IsOptionSelectedByIndex

Comment 7

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

Attachment: Part g: Cleanup OnOptionSelected

Comment 8

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

Attachment: Part h: Cleanup FindSelectedIndex

Comment 9

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

Attachment: Part i: Cleanup SetOptionsSelectedByIndex

Comment 10

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

Attachment: Part i -w

Comment 11

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

Attachment: Part j: Cleanup SaveState

Comment 12

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

Attachment: Park k: Cleanup RestoreStateTo

Comment 13

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

Attachment: Part l: Cleanup Reset

Comment 14

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

Attachment: Part m: Cleanup SubmitNamesValues

I hope you appreciate the removed QI in particular.

Comment 15

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

Attachment: Part n: Cleanup IsValueMissing

Comment 16

[David Zbarsky (:dzbarsky)]

Comment on attachment 775777 [details] [diff] [review]
Part f: Cleanup IsOptionSelectedByIndex

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

::: content/html/content/src/HTMLSelectElement.cpp
@@ +817,5 @@
>    return mOptions->GetOptionIndex(option->AsElement(), aStartIndex, aForward, aIndex);
>  }
>  
>  bool
>  HTMLSelectElement::IsOptionSelectedByIndex(int32_t aIndex)

This is only called from one place.  Why don't you change this and the loop in HTMLSelectElement::FindSelectedIndex to use uint32_t?

Comment 17

[David Zbarsky (:dzbarsky)]

Comment on attachment 775792 [details] [diff] [review]
Part n: Cleanup IsValueMissing

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

::: content/html/content/src/HTMLSelectElement.cpp
@@ +1777,1 @@
>      if (!value.IsEmpty()) {

So GetValue should be void but we're using the XPIDL method? ok

Comment 18

[David Zbarsky (:dzbarsky)]

Comment on attachment 775791 [details] [diff] [review]
Part m: Cleanup SubmitNamesValues

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

::: content/html/content/src/HTMLSelectElement.cpp
@@ -1719,1 @@
>    }

Why do we eat this return value?

Comment 19

[David Zbarsky (:dzbarsky)]

Comment on attachment 775765 [details] [diff] [review]
Part a: Add a IsOptionDisabled overload without an outparam

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

::: content/html/content/src/HTMLSelectElement.cpp
@@ +1146,1 @@
>      while (1) {

how about
for (nsCOMPtr<Element> node = aOption->GetParentElement();
     node;
     node = node->GetParentElement())

@@ +1163,2 @@
>          // If you put something else between you and the optgroup, you're a
>          // moron and you deserve not to have optgroup disabling work.

Nice.

Comment 20

[David Zbarsky (:dzbarsky)]

Comment on attachment 775775 [details] [diff] [review]
Part d: Cleanup Remove

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

::: content/html/content/src/HTMLSelectElement.cpp
@@ +685,1 @@
>    return NS_OK;

Hrm.  Remove() ends up calling IndexOf twice.  Probably not a big deal because we have the cache, but kind of unfortunate.

Comment 21

[David Zbarsky (:dzbarsky)]

Comment on attachment 775776 [details] [diff] [review]
Part e: Cleanup SetLength

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

::: content/html/content/src/HTMLSelectElement.cpp
@@ +738,2 @@
>      }
>      

whitespace

@@ -733,3 @@
>      }
>      
> -    // This violates the W3C DOM but we do this for backwards compatibility

Any idea what this comment was referring to?

@@ +742,5 @@
>      nsContentUtils::NameChanged(mNodeInfo, nsGkAtoms::option,
>                                  getter_AddRefs(nodeInfo));
>  
> +    nsCOMPtr<nsINode> node = NS_NewHTMLOptionElement(nodeInfo.forget());
> +    if (!node) {

This is infallible.

Comment 22

[David Zbarsky (:dzbarsky)]

Comment on attachment 775784 [details] [diff] [review]
Part i -w

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

::: content/html/content/src/HTMLSelectElement.h
@@ +321,5 @@
>                                         int32_t aEndIndex,
>                                         bool aIsSelected,
>                                         bool aClearAll,
>                                         bool aSetDisabled,
> +                                 bool aNotify);

Can you change this to use an enum instead of bool flags?

Comment 23

[David Zbarsky (:dzbarsky)]

Comment on attachment 775791 [details] [diff] [review]
Part m: Cleanup SubmitNamesValues

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

::: content/html/content/src/HTMLSelectElement.cpp
@@ -1719,1 @@
>    }

I guess this is fine since we don't check the return value anyway.

Comment 24

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

(In reply to David Zbarsky (:dzbarsky) from comment #16)
> Comment on attachment 775777 [details] [diff] [review]
> Part f: Cleanup IsOptionSelectedByIndex
> 
> Review of attachment 775777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/HTMLSelectElement.cpp
> @@ +817,5 @@
> >    return mOptions->GetOptionIndex(option->AsElement(), aStartIndex, aForward, aIndex);
> >  }
> >  
> >  bool
> >  HTMLSelectElement::IsOptionSelectedByIndex(int32_t aIndex)
> 
> This is only called from one place.  Why don't you change this and the loop
> in HTMLSelectElement::FindSelectedIndex to use uint32_t?

I considered, but it seems non-equivalent if aStartIndex is negative.

(In reply to David Zbarsky (:dzbarsky) from comment #17)
> Comment on attachment 775792 [details] [diff] [review]
> Part n: Cleanup IsValueMissing
> 
> Review of attachment 775792 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/HTMLSelectElement.cpp
> @@ +1777,1 @@
> >      if (!value.IsEmpty()) {
> 
> So GetValue should be void but we're using the XPIDL method? ok

Yeah; I'll leave it to you to fix that :)

(In reply to David Zbarsky (:dzbarsky) from comment #18)
> Comment on attachment 775791 [details] [diff] [review]
> Part m: Cleanup SubmitNamesValues
> 
> Review of attachment 775791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/HTMLSelectElement.cpp
> @@ -1719,1 @@
> >    }
> 
> Why do we eat this return value?

(In reply to David Zbarsky (:dzbarsky) from comment #23)
> Comment on attachment 775791 [details] [diff] [review]
> Part m: Cleanup SubmitNamesValues
> 
> Review of attachment 775791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/HTMLSelectElement.cpp
> @@ -1719,1 @@
> >    }
> 
> I guess this is fine since we don't check the return value anyway.

Yeah; we did it before, and I don't want to change that.

(In reply to David Zbarsky (:dzbarsky) from comment #19)
> Comment on attachment 775765 [details] [diff] [review]
> Part a: Add a IsOptionDisabled overload without an outparam
> 
> Review of attachment 775765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/HTMLSelectElement.cpp
> @@ +1146,1 @@
> >      while (1) {
> 
> how about
> for (nsCOMPtr<Element> node = aOption->GetParentElement();
>      node;
>      node = node->GetParentElement())

Nice.

(In reply to David Zbarsky (:dzbarsky) from comment #20)
> Comment on attachment 775775 [details] [diff] [review]
> Part d: Cleanup Remove
> 
> Review of attachment 775775 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/HTMLSelectElement.cpp
> @@ +685,1 @@
> >    return NS_OK;
> 
> Hrm.  Remove() ends up calling IndexOf twice.  Probably not a big deal
> because we have the cache, but kind of unfortunate.

Ignored as discussed on IRC.

(In reply to David Zbarsky (:dzbarsky) from comment #21)
> Comment on attachment 775776 [details] [diff] [review]
> Part e: Cleanup SetLength
> 
> Review of attachment 775776 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/HTMLSelectElement.cpp
> @@ +738,2 @@
> >      }
> >      
> 
> whitespace

Removed

> @@ -733,3 @@
> >      }
> >      
> > -    // This violates the W3C DOM but we do this for backwards compatibility
> 
> Any idea what this comment was referring to?

Nope; the old spec didn't even say anything about the setter.

> @@ +742,5 @@
> >      nsContentUtils::NameChanged(mNodeInfo, nsGkAtoms::option,
> >                                  getter_AddRefs(nodeInfo));
> >  
> > +    nsCOMPtr<nsINode> node = NS_NewHTMLOptionElement(nodeInfo.forget());
> > +    if (!node) {
> 
> This is infallible.

Removed.

(In reply to David Zbarsky (:dzbarsky) from comment #22)
> Comment on attachment 775784 [details] [diff] [review]
> Part i -w
> 
> Review of attachment 775784 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/HTMLSelectElement.h
> @@ +321,5 @@
> >                                         int32_t aEndIndex,
> >                                         bool aIsSelected,
> >                                         bool aClearAll,
> >                                         bool aSetDisabled,
> > +                                 bool aNotify);
> 
> Can you change this to use an enum instead of bool flags?

Not in this bug.

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/1db8a12e53e9
https://hg.mozilla.org/mozilla-central/rev/01da392cf670
https://hg.mozilla.org/mozilla-central/rev/b6e69ee84ad3
https://hg.mozilla.org/mozilla-central/rev/8a1ecc6236c1
https://hg.mozilla.org/mozilla-central/rev/3ec0906fcf68
https://hg.mozilla.org/mozilla-central/rev/39fc7ef972b8
https://hg.mozilla.org/mozilla-central/rev/1f1718431c33
https://hg.mozilla.org/mozilla-central/rev/effaed213a6c
https://hg.mozilla.org/mozilla-central/rev/f034d7318487
https://hg.mozilla.org/mozilla-central/rev/92cdfc957668
https://hg.mozilla.org/mozilla-central/rev/7bc2e4b2bfc5
https://hg.mozilla.org/mozilla-central/rev/ba2e6e147d30
https://hg.mozilla.org/mozilla-central/rev/0820c5ec7117
https://hg.mozilla.org/mozilla-central/rev/c8619b95ee10