324738 - Remove nsIParser::RegisterDTD and related code

Status: RESOLVED FIXED

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

Description

[Blake Kaplan (:mrbkap) (inactive)]

nsIParser::RegisterDTD causes the parser to do a dance every time we want to parse a web page. Nobody in Gecko actually uses these facilities, and anybody else trying to would be stymied by the assumptions that the code makes anyway. I have a patch to remove it and move on with parser simplification.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make it go away

This simplifies things some, but there's more to do!

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 209658 [details] [diff] [review]
Make it go away

The changes in netwerk/base/src/nsProxyAutoConfig.js look very unrelated to this bug :)

 inline nsresult NS_NewNavHTMLDTD(nsIDTD** aInstancePtrResult)
 {
-  NS_DEFINE_CID(kNavDTDCID, NS_CNAVDTD_CID);
-  return CallCreateInstance(kNavDTDCID, aInstancePtrResult);
+  NS_IF_ADDREF(*aInstancePtrResult = new CNavDTD());
+  return *aInstancePtrResult ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
 }

You could eliminate a double if-check and double dereference of *aInstancePtrResult (oh how I hate that name :) if you'd break this out onto a few more lines of code, i.e. like this:

 inline nsresult NS_NewNavHTMLDTD(nsIDTD** aInstancePtrResult)
 {
   *aInstancePtrResult = new CNavDTD();
   if (!*aInstancePtrResult)
     return NS_ERROR_OUT_OF_MEMORY;

   NS_ADDREF(*aInstancePtrResult);

   return NS_OK;
 }

Or better yet, use a local variable to cut down further on the number of times we dereference *aInstancePtrResult. But whatever :)

 nsresult NS_NewViewSourceHTML(nsIDTD** aInstancePtrResult)
 {
   CViewSourceHTML* it = new CViewSourceHTML();
 
   if (it == 0) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
-  return it->QueryInterface(kClassIID, (void **) aInstancePtrResult);
+  NS_ADDREF(it);
+  return NS_OK;
 }

Hey now, don't be hasty :) You still need to pass "it" to *aInstancePtrResult somewhere :)

sr=jst with that fixed.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With that fixed

This has some comments addressed, and I'm carrying forward jst's review.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 209661 [details] [diff] [review]
With that fixed

>Index: mailnews/compose/src/nsMsgComposeService.cpp
>@@ -572,32 +571,26 @@ NS_IMETHODIMP nsMsgComposeService::GetPa
...
>+            rv = parser->Parse(rawBody, 0, NS_LITERAL_CSTRING("text/html"), PR_FALSE, PR_TRUE);
>+            if (NS_FAILED(rv))
>+              // Something went horribly wrong with parsing for html format
>+              // in the body.  Set composeHTMLFormat to PR_FALSE so we show the
>+              // plain text mail compose.
>+              composeHTMLFormat = PR_FALSE;

Please put braces around here.

>Index: parser/htmlparser/src/CNavDTD.h
...
> inline nsresult NS_NewNavHTMLDTD(nsIDTD** aInstancePtrResult)
> {
>-  NS_DEFINE_CID(kNavDTDCID, NS_CNAVDTD_CID);
>-  return CallCreateInstance(kNavDTDCID, aInstancePtrResult);
>+  nsIDTD* temp = new CNavDTD();
>+  if (!temp)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  NS_ADDREF(*aInstancePtrResult = temp);
>+  return NS_OK;
> }

Couldn't you just nuke this method and let consumers do |new CNavDTD|?

>Index: parser/htmlparser/src/nsViewSourceHTML.cpp
...
> nsresult NS_NewViewSourceHTML(nsIDTD** aInstancePtrResult)
> {
>   CViewSourceHTML* it = new CViewSourceHTML();
> 
>   if (it == 0) {
>     return NS_ERROR_OUT_OF_MEMORY;
>   }
> 
>-  return it->QueryInterface(kClassIID, (void **) aInstancePtrResult);
>+  NS_ADDREF(*aInstancePtrResult = it);
>+  return NS_OK;
> }

Same as above, can this be nuked? Same for NS_NewExpatDriver?

r=me with that

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch I just checked in

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.