Closed Bug 899931 Opened 6 years ago Closed 6 years ago

Cleanup and decom nsListControlFrame

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Details

Attachments

(10 files)

2.19 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
2.51 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
3.34 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
4.21 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
1.65 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
5.72 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
6.32 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
8.59 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
2.66 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
1000 bytes, patch
dzbarsky
: review+
Details | Diff | Splinter Review
No description provided.
These are unused.
Attachment #783635 - Flags: review?(dzbarsky)
There's a number of methods to get at options, but none quite useful.
Attachment #783637 - Flags: review?(dzbarsky)
I inlined two helpers that I think obscured more than they helped.
Attachment #783645 - Flags: review?(dzbarsky)
Also removes nsListControlFrame::GetOptionAsContent / GetOptionContent which are now unused.
Attachment #783647 - Flags: review?(dzbarsky)
Also removes nsListControlFrame::GetOption, which is replaced with GetOptions()->ItemAsOption().
Attachment #783649 - Flags: review?(dzbarsky)
... and use the WebIDL API instead of the mess it has now.
Attachment #783651 - Flags: review?(dzbarsky)
Attachment #783635 - Flags: review?(dzbarsky) → review+
Attachment #783637 - Flags: review?(dzbarsky) → review+
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.
Attachment #783640 - Flags: review?(dzbarsky) → review+
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.
Attachment #783643 - Flags: review?(dzbarsky) → review+
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?
Attachment #783644 - Flags: review?(dzbarsky) → review+
Attachment #783651 - Flags: review?(dzbarsky) → review+
Attachment #783652 - Flags: review?(dzbarsky) → review+
Attachment #783645 - Flags: review?(dzbarsky) → review+
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?
Attachment #783647 - Flags: review?(dzbarsky) → review+
Attachment #783649 - Flags: review?(dzbarsky) → review+
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.
(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.
You need to log in before you can comment on or make changes to this bug.