899931 - Cleanup and decom nsListControlFrame

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Comment 1

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

Attachment: Part a: Remove IsContentSelected / IsContentSelectedByIndex

These are unused.

Comment 2

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

Attachment: Part b: Introduce a nice nsListControlFrame::GetOption

There's a number of methods to get at options, but none quite useful.

Comment 3

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

Attachment: Part c: Cleanup nsListControlFrame::GetOptionText

Comment 4

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

Attachment: Part d: Cleanup nsListControlFrame::GetCurrentOption()

Comment 5

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

Attachment: Part e: Cleanup nsListControlFrame::ToggleOptionSelectedFromFrame

Comment 6

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

Attachment: Part f: Cleanup nsListControlFrame::GetIndexFromDOMEvent

I inlined two helpers that I think obscured more than they helped.

Comment 7

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

Attachment: Part g: Cleanup nsListControlFrame::ScrollToIndex / ScrollToFrame

Also removes nsListControlFrame::GetOptionAsContent / GetOptionContent which are now unused.

Comment 8

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

Attachment: Part h: Cleanup nsListControlFrame::GetOptions()

Also removes nsListControlFrame::GetOption, which is replaced with GetOptions()->ItemAsOption().

Comment 9

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

Attachment: Part i: Inline nsListControlFrame::GetSizeAttribute

... and use the WebIDL API instead of the mess it has now.

Comment 10

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

Attachment: Part j: nsListControlFrame::GetSelectedIndex()

Comment 11

[David Zbarsky (:dzbarsky)]

Comment on attachment 783640 [details] [diff] [review]
Part c: Cleanup nsListControlFrame::GetOptionText

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

::: layout/forms/nsListControlFrame.cpp
@@ -1168,5 @@
> -        GetOption(options, aIndex);
> -      if (optionElement) {
> -#if 0 // This is for turning off labels Bug 4050
> -        nsAutoString text;
> -        optionElement->GetLabel(text);

Heh.

Comment 12

[David Zbarsky (:dzbarsky)]

Comment on attachment 783643 [details] [diff] [review]
Part d: Cleanup nsListControlFrame::GetCurrentOption()

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

::: layout/forms/nsListControlFrame.cpp
@@ +1191,3 @@
>  
> +  for (uint32_t i = 0, length = selectElement->Length(); i < length; ++i) {
> +    dom::HTMLOptionElement* node = selectElement->Item(i);

Wow, this was pretty messy.  Nice cleanup.

Comment 13

[David Zbarsky (:dzbarsky)]

Comment on attachment 783644 [details] [diff] [review]
Part e: Cleanup nsListControlFrame::ToggleOptionSelectedFromFrame

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

::: layout/forms/nsListControlFrame.cpp
@@ +1356,5 @@
>  nsListControlFrame::ToggleOptionSelectedFromFrame(int32_t aIndex)
>  {
> +  nsRefPtr<dom::HTMLOptionElement> option =
> +    GetOption(static_cast<uint32_t>(aIndex));
> +  NS_ENSURE_TRUE(option, false);

SafeCast?

Comment 14

[David Zbarsky (:dzbarsky)]

Comment on attachment 783647 [details] [diff] [review]
Part g: Cleanup nsListControlFrame::ScrollToIndex / ScrollToFrame

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

Should we be using uint32_t for index in more places in this file?

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 783637 [details] [diff] [review]
Part b: Introduce a nice nsListControlFrame::GetOption

Null-checking 'select' seems unnecessary.  I think I'd prefer to just
crash there if that were to happen somehow.

Comment 16

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

(In reply to David Zbarsky (:dzbarsky) from comment #14)
> Comment on attachment 783647 [details] [diff] [review]
> Part g: Cleanup nsListControlFrame::ScrollToIndex / ScrollToFrame
> 
> Review of attachment 783647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Should we be using uint32_t for index in more places in this file?

I'd say yes, but there are a lot of variables that seem to be >= -1 rather than >= 0. I'd need to do some more careful auditing than I wanted to do in this bug.

(In reply to Mats Palmgren (:mats) from comment #15)
> Comment on attachment 783637 [details] [diff] [review]
> Part b: Introduce a nice nsListControlFrame::GetOption
> 
> Null-checking 'select' seems unnecessary.  I think I'd prefer to just
> crash there if that were to happen somehow.

There are a lot of seemingly pointless null-checks in this code; I'd prefer staying on the safe side in this bug and removing them all in a followup.

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/136f1614f092
https://hg.mozilla.org/mozilla-central/rev/ad1c42bfcc98
https://hg.mozilla.org/mozilla-central/rev/0fc75bf359ef
https://hg.mozilla.org/mozilla-central/rev/a38da0a3d632
https://hg.mozilla.org/mozilla-central/rev/fa76408d51da
https://hg.mozilla.org/mozilla-central/rev/1a76ce7c35fd
https://hg.mozilla.org/mozilla-central/rev/3e2c0996c820
https://hg.mozilla.org/mozilla-central/rev/350781773544
https://hg.mozilla.org/mozilla-central/rev/f81a11940564
https://hg.mozilla.org/mozilla-central/rev/2e99a8e6be1c