201271 - Fix interface inconsistencies in parser

Status: RESOLVED FIXED

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

Description

[Christopher Hoess (gone)]

Similar to bug 63188. There are various places in parser where parameters in the
headers do not match those in the implementation. Patch coming up.

Comment 1

[Christopher Hoess (gone)]

Attachment: First round

This could get more involved then I thought...anyway, here's the first patch.

Comment 2

[Christopher Hoess (gone)]

Attachment: Fixed first round.

Missed a function...

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 119930 [details] [diff] [review]
Fixed first round.

sr=jst if Harish agrees with these changes.

Comment 4

[harishd]

Comment on attachment 119930 [details] [diff] [review]
Fixed first round.

r=harishd

Comment 5

[Christopher Hoess (gone)]

Attachment: Second round: more extensive.

Cleans up interfaces, also removes a few unused functions, moves a function out
of nsDTDUtils.h to CNavDTD.cpp to eliminate a compiler warning, and does style
cleanup of function(void) to remove the unnecessary parameter. A big grab bag.

Comment 6

[harishd]

Comment on attachment 120293 [details] [diff] [review]
Second round: more extensive.

>Index: public/nsHTMLTags.h
>Index: public/nsHTMLTokens.h
>+    virtual eContainerInfo GetContainerInfo() const {return eFormUnknown;}

Should break this into three lines. ( I'll do that before landing ).

>Index: public/nsIContentSink.h
>+  NS_IMETHOD WillBuildModel()=0;

spaces around =. Will do that as well :)

>Index: public/nsIParser.h
>Index: public/nsIParserNode.h
>Index: public/nsITokenizer.h
>Index: public/nsToken.h
>Index: src/CNavDTD.cpp
>Index: src/CNavDTD.h
>Index: src/COtherElements.h

This file should go away. Or at least it should be stripped down to fit editor
needs ( if editor still depends on it ). I believe there is a bug on this.
Anyway, would be a good candidate for code bloat reduction. Chris,  willing to
take a look ? ;). I'll look into it if time permits.

>Index: src/nsDTDUtils.cpp
>Index: src/nsDTDUtils.h
Index: src/nsElementTable.cpp
>Index: src/nsElementTable.h
>Index: src/nsExpatDriver.cpp
>Index: src/nsExpatDriver.h
>Index: src/nsHTMLNullSink.cpp
>Index: src/nsHTMLTokenizer.cpp
>Index: src/nsHTMLTokenizer.h
>Index: src/nsHTMLTokens.cpp

>+//void CAttributeToken::BindKey(nsScanner* aScanner, 
>+//                              nsReadingIterator<PRUnichar>& aStart, 
>+//                              nsReadingIterator<PRUnichar>& aEnd)
>+//{
>+//  aScanner->BindSubstring(mTextKey, aStart, aEnd);
>+//}

Why is this commented out?

>Index: src/nsParser.cpp
>Index: src/nsParser.h
>Index: src/nsScanner.cpp
>Index: src/nsScanner.h

will review the rest later...

Comment 7

[Christopher Hoess (gone)]

I can take a look at COtherElements.h (bug 153998?). I missed the commented-out
function when I was deleting unused functions; that should just be removed.

Comment 8

[Christopher Hoess (gone)]

Harish, do you know when you'll be able to finish the review? I know the patch
is extensive, but most of it is cosmetic (and it's built and run for some time),
and I'd like to get in some more substantial changes on some of these files. Thanks.

Comment 9

[Frank Wein [:mcsmurf]]

Christopher Hoess: You need someone else to review your patch, harisd is gone now.

Comment 10

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

Comment on attachment 120293 [details] [diff] [review]
Second round: more extensive.

General notice - this patch seems to be changing non-frozen interfaces. Do we
want to rev UUIDs?

>Index: public/nsHTMLTokens.h
>===================================================================
>-      //the following info is used to set well-formedness state on start tags...
>-    virtual eContainerInfo GetContainerInfo(void) const {return mContainerInfo;}
>-    virtual void           SetContainerInfo(eContainerInfo aContainerInfo) {mContainerInfo=aContainerInfo;}

How can you delete these, there is code getting and settings values?

>-    virtual PRBool         IsWellFormed(void) const {return PRBool(eWellFormed==mContainerInfo);}

Isn't there code calling this as well?

>-    virtual const char* GetClassName(void);

Also isn't this also used, how can you remove it? This happens all over the
patch...

>+//unused?
>+//    virtual void          BindKey(nsScanner* aScanner, nsReadingIterator<PRUnichar>& aStart, nsReadingIterator<PRUnichar>& aEnd);

Yeah, it seems to me BindKey is unused so we I think we should remove it to
save a little space.

>Index: src/CNavDTD.cpp
>===================================================================
>+/**
>+ *
>+ * This method used to live in nsDTDUtils.h, but only CNavDTD uses it now.
>+ */
>+static void
>+InPlaceConvertLineEndings( nsAString& aString )

It seems like this already lives in CNavDTD.
>Index: src/CNavDTD.h
>===================================================================
>-    virtual eHTMLTags GetTopNode() const;

How can you remove this, isn't it used?

>-    nsresult  CloseTransientStyles(eHTMLTags aChildTag);

Yes, another unused function bites the dust! But shouldn't it be removed from
the .cpp too?

>-		nsresult        CollectSkippedContent(nsIParserNode& aNode,PRInt32& aCount);

How can you remove this, isn't it used?

>Index: src/COtherElements.h
>===================================================================
>+#ifdef DEBUG
>     PRInt32   theCount=aContext->GetCount();
>+#endif

Move the call into the DEBUG section below, less work and clearer.

>Index: src/nsDTDUtils.h
>===================================================================


>+/*
>+*/

This stuff has been removed already.

>Index: src/nsElementTable.h
>===================================================================
>-  eHTMLTags       GetTag(void) const {return mTagID;}

Are you sure this is not used?

>Index: src/nsHTMLTokens.cpp
>===================================================================
>+//void CAttributeToken::BindKey(nsScanner* aScanner, 
>+//                              nsReadingIterator<PRUnichar>& aStart, 
>+//                              nsReadingIterator<PRUnichar>& aEnd)
>+//{
>+//  aScanner->BindSubstring(mTextKey, aStart, aEnd);
>+//}

Just take it out completely.

>Index: src/nsScanner.cpp
>===================================================================
>-  nsresult  result=Peek(theChar);

I don't think you can take out that line, theChar is used later

>Index: src/nsTagHandler.h
>===================================================================
>-class nsTagHandler : public nsITagHandler {

I think this part has also been removed already.

Answer some questions, fix those few issues, and attach a fresh patch please.

Comment 11

[Johnny Stenback (:jst)]

Over to the default owner...

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

I don't think it's worth using this patch as a base for further work (it's so out of date). Over to choess and marking fixed. If someone wants to use it, then please reopen.