63188 - What is the aMode parameter in CToken::Consume?

Status: RESOLVED FIXED

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

Description

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

I ran into this while trying to work on mode detection stuff...

The |aMode| parameter in |CToken::Consume| is used inconsistently.  In some
subclasses of CToken (such as CStartToken, CEndToken), there is a comment saying:

@param   aMode -- 1=HTML; 0=text (or other ML)

However, in CTextToken::ConsumeUntil and CCommentToken::Consume, aMode is
treated as an |nsDTDMode| (which is an |enum| type).

In CAttributeToken, there is no |aMode| parameter in the implementation (it is
called |aRetainWhitespace| instead|, although it is called |aMode| in the header
file.  In most of the other subclasses in nsHTMLTokens, |aMode| is just left unused.

Should this be consistent?  (I would think so, since |Consume| is a virtual
method.)  If not, why is |aMode| a parameter in the definition of CToken and in
all of the classes where it is unused?  If so, how should we fix it?  In some
places it's used like a PRBool (in 2 different ways), and others it's used like
an nsDTDMode.

Any thoughts on what was intended here?

Comment 1

[bsharma]

updated qa contact.

Comment 2

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

Reality check.  Moving out to 0.9.1.

Comment 3

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

->harishd

Comment 4

[harishd]

aMode parameter has been replaced with aFlag ( may be it should be called aFlags
)and is being used consistently.

Marking fixed.

Comment 5

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

It's still called |aMode| in the header and |aFlag| in the implementation. 
That's annoyingly inconsistent.  Reopening, although it's not as serious as the
original problem.

Comment 6

[harishd]

ok ok sigh!!!!

Comment 7

[harishd]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug, or if it blocks your work in some way
-- please attach your concern to the bug for reconsideration. 

Comment 8

[Christopher Hoess (gone)]

Attachment: Changes aMode to aFlag

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 119782 [details] [diff] [review]
Changes aMode to aFlag

r+sr=jst, go ahead and land this whenever anyone has a chance.

Comment 10

[Boris Zbarsky [:bzbarsky]]

timeless landed this earlier today.