Closed Bug 232133 Opened 20 years ago Closed 20 years ago

nsHTMLSelectElement cleanup and deCOMtamination

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jst, Assigned: jst)

Details

Attachments

(1 file)

Fix
15.67 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
Found silly stuff while browsing code, patch coming up.
Attached patch Fix — — Splinter Review 

    
  

        Attachment #139851 -
        Flags: superreview?(bz-vacation)

        Attachment #139851 -
        Flags: review?(bz-vacation)

    
    

    
  
Comment on attachment 139851 [details] [diff] [review]
Fix

>Index: content/html/content/src/nsHTMLSelectElement.cpp

> nsHTMLOptionCollection::SetOption(PRInt32 aIndex,

>+  if (mSelect && (aIndex >= 0) && (aIndex <= (PRInt32)mElements.Count())) {

No need for that cast (or the extra parens, for that matter).

> nsHTMLOptionCollection::NamedItem(const nsAString& aName,

>+  PRUint32 count = mElements.Count();

PRInt32

>   for (PRUint32 i = 0; i < count && !*aReturn; i++) {

Same.

>+        break;

Given that, do you need the !*aReturn test in the for?	I doubt it, and this
seems right -- we shouldn't be ending up with nsIContent in there that has a
name or id attr but doesn't QI to nsIDOMNode...

r+sr=bzbarsky with those nits.

        Attachment #139851 -
        Flags: superreview?(bz-vacation)

        Attachment #139851 -
        Flags: superreview+

        Attachment #139851 -
        Flags: review?(bz-vacation)

        Attachment #139851 -
        Flags: review+

    
    

    
  
Fixed. Thanks for the review!
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED

    
  
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general

          You need to log in
          before you can comment on or make changes to this bug.