92618 - int -> roman number code in two places

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P2)

Description

[Daniel Bratell]

There are code to convert between an integer and a roman number (7 -> VII) in 
both nsBulletFrame.cpp in layout and in nsDTDUtils.cpp in htmlparser. It seems 
to be the exact same code, though the code in nsBulletFrame is a little more 
evolved and uses more modern string apis.

For display, it's the nsBulletFrame code that is used. I don't know when, if 
ever, the nsDTDUtils code is used.

If not, it should be removed. If it's used, the code should be moved to a common 
place.

Comment 1

[Daniel Bratell]

RomanString is used by GetFormattedString which is used by GetNextValueAsString 
and GetValueAsString in CAbacus. CAbacus is used by 
nsDTDContext::IncrementCounter which is called by CNavDTD::DidHandleStartTag in 
the case of eHTMLTag_counter (what's that?) and by CCounterElement::OpenContext.

So it seems as if the method in htmlparser is used (though I don't know when). 
Left is to move the code to a common place. Where could that be? 

Comment 2

[harishd]

The code is nsDTDUtils got added ( by rickg ) a while back. I barely recall what
it's meant for.

My guess: The code should kick in if you use <counter></counter>. 

Suggestion: We probably need nsLayoutUtils.h under mozilla/layout/base/public
and nsLayoutUtils.cpp under mozilla/layout/base/src and move the common code there.

Comment 3

[harishd]

typo: "The code is nsDTDUtils" should be "The code in nsDTDUtils"

Comment 4

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 5

[Cathleen]

harish, will this provide a good win for footprint?

asa mass changed milestone to post 1.0.1, but this is one of those bugs on my
footprint improvemnt list...

Comment 6

[harishd]

Cathleen: I doubt a big win. But still worth removing redundant code. Moving to
0.9.8.

Comment 7

[harishd]

Attachment: Patch v1.0 [ Needs testing ]

Moved counter related code into DEBUG flag.

Comment 8

[Daniel Bratell]

Whats the purpose of the patch? Won't this break something?

Comment 9

[harishd]

Daniel: It shouldn't break anything. AFAIK this code shouldn't belong in the
optimized build.

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 61634 [details] [diff] [review]
Patch v1.0 [ Needs testing ]

r=heikki

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 61634 [details] [diff] [review]
Patch v1.0 [ Needs testing ]

sr=jst

Comment 12

[harishd]

FIXED

Comment 13

[Moied]

Marking verified patch checked in CVS (Rev 3.368)