Cleanup HTMLSelectElement

RESOLVED FIXED in mozilla25

Status

()

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

(15 attachments)

4.43 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
6.69 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
2.37 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
1.02 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
3.58 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
1.10 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
1.08 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
747 bytes, patch
dzbarsky
: review+
Details | Diff | Splinter Review
20.31 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
17.54 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
1.20 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
1.08 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
1.26 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
2.17 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
1.31 KB, patch
dzbarsky
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 775765 [details] [diff] [review]
Part a: Add a IsOptionDisabled overload without an outparam
Attachment #775765 - Flags: review?(dzbarsky)
(Assignee)

Comment 2

5 years ago
Created attachment 775767 [details] [diff] [review]
Part b: Return void from InsertOptionsIntoList{,Recurse}
Attachment #775767 - Flags: review?(dzbarsky)
(Assignee)

Comment 3

5 years ago
Created attachment 775771 [details] [diff] [review]
Part c: Cleanup HTMLSelectElement::InsertOptionsIntoList
Attachment #775771 - Flags: review?(dzbarsky)
(Assignee)

Comment 4

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

Comment 5

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

Comment 6

5 years ago
Created attachment 775777 [details] [diff] [review]
Part f: Cleanup IsOptionSelectedByIndex
Attachment #775777 - Flags: review?(dzbarsky)
(Assignee)

Comment 7

5 years ago
Created attachment 775778 [details] [diff] [review]
Part g: Cleanup OnOptionSelected
Attachment #775778 - Flags: review?(dzbarsky)
(Assignee)

Comment 8

5 years ago
Created attachment 775780 [details] [diff] [review]
Part h: Cleanup FindSelectedIndex
Attachment #775780 - Flags: review?(dzbarsky)
(Assignee)

Comment 9

5 years ago
Created attachment 775781 [details] [diff] [review]
Part i: Cleanup SetOptionsSelectedByIndex
Attachment #775781 - Flags: review?(dzbarsky)
(Assignee)

Comment 11

5 years ago
Created attachment 775786 [details] [diff] [review]
Part j: Cleanup SaveState
Attachment #775786 - Flags: review?(dzbarsky)
(Assignee)

Comment 12

5 years ago
Created attachment 775788 [details] [diff] [review]
Park k: Cleanup RestoreStateTo
Attachment #775788 - Flags: review?(dzbarsky)
(Assignee)

Comment 13

5 years ago
Created attachment 775789 [details] [diff] [review]
Part l: Cleanup Reset
Attachment #775789 - Flags: review?(dzbarsky)
(Assignee)

Comment 14

5 years ago
Created attachment 775791 [details] [diff] [review]
Part m: Cleanup SubmitNamesValues

I hope you appreciate the removed QI in particular.
Attachment #775791 - Flags: review?(dzbarsky)
(Assignee)

Comment 15

5 years ago
Created attachment 775792 [details] [diff] [review]
Part n: Cleanup IsValueMissing
Attachment #775792 - Flags: review?(dzbarsky)
Attachment #775767 - Flags: review?(dzbarsky) → review+
Attachment #775771 - Flags: review?(dzbarsky) → review+
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?
Attachment #775777 - Flags: review?(dzbarsky) → review+
Attachment #775780 - Flags: review?(dzbarsky) → review+
Attachment #775786 - Flags: review?(dzbarsky) → review+
Attachment #775788 - Flags: review?(dzbarsky) → review+
Attachment #775778 - Flags: review?(dzbarsky) → review+
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
Attachment #775792 - Flags: review?(dzbarsky) → review+
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?
Attachment #775789 - Flags: review?(dzbarsky) → review+
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.
Attachment #775765 - Flags: review?(dzbarsky) → review+
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.
Attachment #775775 - Flags: review?(dzbarsky) → review+
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.
Attachment #775776 - Flags: review?(dzbarsky) → review+
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?
Attachment #775784 - Flags: review+
Attachment #775781 - Flags: review?(dzbarsky) → review+
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.
Attachment #775791 - Flags: review?(dzbarsky) → review+
(Assignee)

Comment 24

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

Updated

5 years ago
Depends on: 896273
You need to log in before you can comment on or make changes to this bug.