210238 - Dead code removal in htmlparser/src/nsElementTable.cpp

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Mats Palmgren (inactive)]

Dead code removal in htmlparser/src/nsElementTable.cpp:

int nsHTMLElement::GetSynonymousGroups(eHTMLTags aTag) {
  int result=0;

  int theGroup=gHTMLElements[aTag].mParentBits;
  switch(theGroup) {

    case kPhrase:
    case kSpecial:
    case kFontStyle: 
    case kHTMLContent:
    case kHeadContent:
    case kHeadMisc:
    case kFormControl:
    case kPreformatted:
    case kHeading:
    case kBlockMisc:
    case kBlock:
    case kList:
    case kPCDATA:
    case kExtensions:
    case kTable:
    case kSelf:
    case kInlineEntity:
    case kBlockEntity:
    case kFlowEntity:
    case kAllTags:
    default:
      break;
  }

  if(eHTMLTag_font==aTag)  //hack for backward compatibility
    result&=kFontStyle;

  return result;
}



That 'switch' seems pretty useless to me, let's remove it, giving:


int nsHTMLElement::GetSynonymousGroups(eHTMLTags aTag) {
  int result=0;

  if(eHTMLTag_font==aTag)  //hack for backward compatibility
    result&=kFontStyle;

  return result;
}


Hmm, 'result&=kFontStyle' will always be zero, right?
I suggest we just remove this function and update the callers (only one).
Patch coming up...

Comment 1

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 126208 [details] [diff] [review]
Patch rev. 1

Why not get rid of

> static
> PRInt32 GetIndexOfChildOrSynonym(nsDTDContext& aContext,eHTMLTags aChildTag) {

and have the current callers call nsHTMLElement::GetIndexOfChildOrSynonym
directly?

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

As David suggested above

Comment 4

[harishd]

Comment on attachment 126232 [details] [diff] [review]
Patch rev. 2


>Index: htmlparser/src/nsElementTable.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/htmlparser/src/nsElementTable.cpp,v
>retrieving revision 3.150
>diff -u -r3.150 nsElementTable.cpp
>--- htmlparser/src/nsElementTable.cpp	11 Jun 2003 04:24:22 -0000	3.150
>+++ htmlparser/src/nsElementTable.cpp	22 Jun 2003 14:28:51 -0000
>@@ -1474,56 +1474,8 @@
>     if(theSynTags) {
>       theChildIndex=LastOf(aContext,*theSynTags);
>     } 
>-    else{
>-      PRInt32 theGroup=nsHTMLElement::GetSynonymousGroups(aChildTag);
>-      if(theGroup) {
>-        theChildIndex=aContext.GetCount();
>-        while(-1<--theChildIndex) {
>-          eHTMLTags theTag=aContext[theChildIndex];
>-          if(gHTMLElements[theTag].IsMemberOf(theGroup)) {
>-            break;   
>-          }
>-        }
>-      }
>-    } 
>   }
>   return theChildIndex;
>-}
>-
>-int nsHTMLElement::GetSynonymousGroups(eHTMLTags aTag) {
>-  int result=0;
>-
>-  int theGroup=gHTMLElements[aTag].mParentBits;
>-  switch(theGroup) {
>-
>-    case kPhrase:
>-    case kSpecial:
>-    case kFontStyle: 
>-    case kHTMLContent:
>-    case kHeadContent:
>-    case kHeadMisc:
>-    case kFormControl:
>-    case kPreformatted:
>-    case kHeading:
>-    case kBlockMisc:
>-    case kBlock:
>-    case kList:
>-    case kPCDATA:
>-    case kExtensions:
>-    case kTable:
>-    case kSelf:
>-    case kInlineEntity:
>-    case kBlockEntity:
>-    case kFlowEntity:
>-    case kAllTags:
>-    default:
>-      break;
>-  }
>-
>-  if(eHTMLTag_font==aTag)  //hack for backward compatibility
>-    result&=kFontStyle;
>-
>-  return result;
> }
> 

Why are you removing these lines?

Comment 5

[Mats Palmgren (inactive)]

Since |nsHTMLElement::GetSynonymousGroups| doesn't do anything useful
(it always returns zero) I see no point in keeping it.
See explanation in comment 0.

Comment 6

[harishd]

The comment "hack for backward compatibility" concerns me.

I believe result&=kFontStyle; should have been result = kFontStyle. What do you
think?

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3 (updated to trunk, no changes since rev. 2)

Comment 8

[Mats Palmgren (inactive)]

My point is that the code *currently* doesn't do anything, so it should be
removed. Activating this code now, as suggested in comment 6 seems riscy.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 170327 [details] [diff] [review]
Patch rev. 3 (updated to trunk, no changes since rev. 2)

It would have been nice to figure out what bug that "hack for backwards
compatibility" was trying to fix, but it hasn't worked for 4 or so years (and
of course bonsai is useless in that file), so r=mrbkap

Comment 10

[Mats Palmgren (inactive)]

Checked in 2005-01-16 09:07 PDT

-> FIXED

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using LXR for code inspection.