Cleanup and decom nsListControlFrame

RESOLVED FIXED in mozilla25

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments)

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
Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 783635 [details] [diff] [review]
Part a: Remove IsContentSelected / IsContentSelectedByIndex

These are unused.
Attachment #783635 - Flags: review?(dzbarsky)
(Assignee)

Comment 2

5 years ago
Created attachment 783637 [details] [diff] [review]
Part b: Introduce a nice nsListControlFrame::GetOption

There's a number of methods to get at options, but none quite useful.
Attachment #783637 - Flags: review?(dzbarsky)
(Assignee)

Comment 3

5 years ago
Created attachment 783640 [details] [diff] [review]
Part c: Cleanup nsListControlFrame::GetOptionText
Attachment #783640 - Flags: review?(dzbarsky)
(Assignee)

Comment 4

5 years ago
Created attachment 783643 [details] [diff] [review]
Part d: Cleanup nsListControlFrame::GetCurrentOption()
Attachment #783643 - Flags: review?(dzbarsky)
(Assignee)

Comment 5

5 years ago
Created attachment 783644 [details] [diff] [review]
Part e: Cleanup nsListControlFrame::ToggleOptionSelectedFromFrame
Attachment #783644 - Flags: review?(dzbarsky)
(Assignee)

Comment 6

5 years ago
Created attachment 783645 [details] [diff] [review]
Part f: Cleanup nsListControlFrame::GetIndexFromDOMEvent

I inlined two helpers that I think obscured more than they helped.
Attachment #783645 - Flags: review?(dzbarsky)
(Assignee)

Comment 7

5 years ago
Created attachment 783647 [details] [diff] [review]
Part g: Cleanup nsListControlFrame::ScrollToIndex / ScrollToFrame

Also removes nsListControlFrame::GetOptionAsContent / GetOptionContent which are now unused.
Attachment #783647 - Flags: review?(dzbarsky)
(Assignee)

Comment 8

5 years ago
Created attachment 783649 [details] [diff] [review]
Part h: Cleanup nsListControlFrame::GetOptions()

Also removes nsListControlFrame::GetOption, which is replaced with GetOptions()->ItemAsOption().
Attachment #783649 - Flags: review?(dzbarsky)
(Assignee)

Comment 9

5 years ago
Created attachment 783651 [details] [diff] [review]
Part i: Inline nsListControlFrame::GetSizeAttribute

... and use the WebIDL API instead of the mess it has now.
Attachment #783651 - Flags: review?(dzbarsky)
(Assignee)

Comment 10

5 years ago
Created attachment 783652 [details] [diff] [review]
Part j: nsListControlFrame::GetSelectedIndex()
Attachment #783652 - 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.
(Assignee)

Comment 16

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