Bug 484121 (4/6) - Let the parser call nsIContentSink::{Will,Did}BuildModel directly. r+sr=mrbkap

parser/htmlparser/public/nsIDTD.h

@@ -102,7 +102,7 @@ public:
      * @param   anErrorCode - contains error code resulting from parse process
      * @return
      */
-    NS_IMETHOD DidBuildModel(nsresult anErrorCode, PRBool aNotifySink,
+    NS_IMETHOD DidBuildModel(nsresult anErrorCode,
                              nsIParser* aParser,
                              nsIContentSink* aSink) = 0;
 
@@ -174,7 +174,7 @@ NS_DEFINE_STATIC_IID_ACCESSOR(nsIDTD, NS_IDTD_IID)
 
 #define NS_DECL_NSIDTD \
     NS_IMETHOD WillBuildModel(  const CParserContext& aParserContext, nsITokenizer* aTokenizer, nsIContentSink* aSink);\
-    NS_IMETHOD DidBuildModel(nsresult anErrorCode,PRBool aNotifySink,nsIParser* aParser,nsIContentSink* aSink);\
+    NS_IMETHOD DidBuildModel(nsresult anErrorCode,nsIParser* aParser,nsIContentSink* aSink);\
     NS_IMETHOD BuildModel(nsIParser* aParser,nsITokenizer* aTokenizer,nsITokenObserver* anObserver,nsIContentSink* aSink);\
     NS_IMETHOD HandleToken(CToken* aToken,nsIParser* aParser);\
     NS_IMETHOD_(PRBool) CanContain(PRInt32 aParent,PRInt32 aChild) const;\

parser/htmlparser/src/CNavDTD.cpp

@@ -206,22 +206,15 @@ CNavDTD::WillBuildModel(const CParserContext& aParserContext,
   mBodyContext->SetNodeAllocator(&mNodeAllocator);
 
   if (!aParserContext.mPrevContext && aSink) {
-    STOP_TIMER();
-    MOZ_TIMER_DEBUGLOG(("Stop: Parse Time: CNavDTD::WillBuildModel(), this=%p\n", this));
-    
-    result = aSink->WillBuildModel(GetMode());
-    
-    MOZ_TIMER_DEBUGLOG(("Start: Parse Time: CNavDTD::WillBuildModel(), this=%p\n", this));
-    START_TIMER();
 
-    if (NS_SUCCEEDED(result) && !mSink) {
+    if (!mSink) {
       mSink = do_QueryInterface(aSink, &result);
       if (NS_FAILED(result)) {
         mFlags |= NS_DTD_FLAG_STOP_PARSING;
         return result;
       }
     }
-    
+
     // Let's see if the environment is set up for us to write output to
     // a logging sink. If so, then we'll create one, and make it the
     // proxy for the real sink we're given from the parser.
@@ -369,7 +362,6 @@ CNavDTD::BuildNeglectedTarget(eHTMLTags aTarget,
 
 nsresult
 CNavDTD::DidBuildModel(nsresult anErrorCode,
-                       PRBool aNotifySink,
                        nsIParser* aParser,
                        nsIContentSink* aSink)
 {
@@ -378,7 +370,7 @@ CNavDTD::DidBuildModel(nsresult anErrorCode,
   }
 
   nsresult result = NS_OK;
-  if (aParser && aNotifySink) {
+  if (aParser) {
     if (NS_OK == anErrorCode) {
       if (!(mFlags & NS_DTD_FLAG_HAS_MAIN_CONTAINER)) {
         // This document is not a frameset document, however, it did not contain
@@ -422,11 +414,7 @@ CNavDTD::DidBuildModel(nsresult anErrorCode,
       mFlags &= ~NS_DTD_FLAG_ENABLE_RESIDUAL_STYLE;
       while (mBodyContext->GetCount() > 0) { 
         result = CloseContainersTo(mBodyContext->Last(), PR_FALSE);
-        if (NS_FAILED(result)) {
-          //No matter what, you need to call did build model.
-          aSink->DidBuildModel();
-          return result;
-        }
+        NS_ENSURE_SUCCESS(result, result);
       } 
     } else {
       // If you're here, then an error occured, but we still have nodes on the stack.
@@ -448,8 +436,7 @@ CNavDTD::DidBuildModel(nsresult anErrorCode,
     }
   }
 
-  // No matter what, you need to call did build model.
-  return aSink->DidBuildModel(); 
+  return result;
 }
 
 NS_IMETHODIMP_(void) 

parser/htmlparser/src/nsExpatDriver.cpp

@@ -1294,7 +1294,7 @@ nsExpatDriver::WillBuildModel(const CParserContext& aParserContext,
   // XML must detect invalid character convertion
   aParserContext.mScanner->OverrideReplacementCharacter(0xffff);
 
-  return aSink->WillBuildModel(GetMode());
+  return mInternalState;
 }
 
 NS_IMETHODIMP
@@ -1308,21 +1308,14 @@ nsExpatDriver::BuildModel(nsIParser* aParser,
 
 NS_IMETHODIMP
 nsExpatDriver::DidBuildModel(nsresult anErrorCode,
-                             PRBool aNotifySink,
                              nsIParser* aParser,
                              nsIContentSink* aSink)
 {
   // Check for mSink is intentional. This would make sure
   // that DidBuildModel() is called only once on the sink.
-  nsresult result = NS_OK;
-  if (mSink) {
-    result = aSink->DidBuildModel();
-    mSink = nsnull;
-  }
-
+  mSink = nsnull;
   mExtendedSink = nsnull;
-
-  return result;
+  return NS_OK;
 }
 
 NS_IMETHODIMP

parser/htmlparser/src/nsParser.cpp

@@ -1487,6 +1487,57 @@ nsParser::CancelParsingEvents()
 
 ////////////////////////////////////////////////////////////////////////
 
+/**
+ * Evalutes EXPR1 and EXPR2 exactly once each, in that order.  Stores the value
+ * of EXPR2 in RV is EXPR2 fails, otherwise RV contains the result of EXPR1
+ * (which could be success or failure).
+ *
+ * To understand the motivation for this construct, consider these example
+ * methods:
+ *
+ *   nsresult nsSomething::DoThatThing(nsIWhatever* obj) {
+ *     nsresult rv = NS_OK;
+ *     ...
+ *     return obj->DoThatThing();
+ *     NS_ENSURE_SUCCESS(rv, rv);
+ *     ...
+ *     return rv;
+ *   }
+ *
+ *   void nsCaller::MakeThingsHappen() {
+ *     return mSomething->DoThatThing(mWhatever);
+ *   }
+ *
+ * Suppose, for whatever reason*, we want to shift responsibility for calling
+ * mWhatever->DoThatThing() from nsSomething::DoThatThing up to
+ * nsCaller::MakeThingsHappen.  We might rewrite the two methods as follows:
+ *
+ *   nsresult nsSomething::DoThatThing() {
+ *     nsresult rv = NS_OK;
+ *     ...
+ *     ...
+ *     return rv;
+ *   }
+ *
+ *   void nsCaller::MakeThingsHappen() {
+ *     nsresult rv;
+ *     PREFER_LATTER_ERROR_CODE(mSomething->DoThatThing(),
+ *                              mWhatever->DoThatThing(),
+ *                              rv);
+ *     return rv;
+ *   }
+ *
+ * *Possible reasons include: nsCaller doesn't want to give mSomething access
+ * to mWhatever, nsCaller wants to guarantee that mWhatever->DoThatThing() will
+ * be called regardless of how nsSomething::DoThatThing behaves, &c.
+ */
+#define PREFER_LATTER_ERROR_CODE(EXPR1, EXPR2, RV) {                          \
+  nsresult RV##__temp = EXPR1;                                                \
+  RV = EXPR2;                                                                 \
+  if (NS_FAILED(RV)) {                                                        \
+    RV = RV##__temp;                                                          \
+  }                                                                           \
+}
 
 /**
  * This gets called just prior to the model actually
@@ -1525,7 +1576,16 @@ nsParser::WillBuildModel(nsString& aFilename)
   nsresult rv = mParserContext->GetTokenizer(mDTD, mSink, tokenizer);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  return mDTD->WillBuildModel(*mParserContext, tokenizer, mSink);
+  rv = mDTD->WillBuildModel(*mParserContext, tokenizer, mSink);
+  nsresult sinkResult = mSink->WillBuildModel(mDTD->GetMode());
+  // nsIDTD::WillBuildModel used to be responsible for calling
+  // nsIContentSink::WillBuildModel, but that obligation isn't expressible
+  // in the nsIDTD interface itself, so it's sounder and simpler to give that
+  // responsibility back to the parser. The former behavior of the DTD was to
+  // NS_ENSURE_SUCCESS the sink WillBuildModel call, so if the sink returns
+  // failure we should use sinkResult instead of rv, to preserve the old error
+  // handling behavior of the DTD:
+  return NS_FAILED(sinkResult) ? sinkResult : rv;
 }
 
 /**
@@ -1545,7 +1605,16 @@ nsParser::DidBuildModel(nsresult anErrorCode)
       PRBool terminated = mInternalState == NS_ERROR_HTMLPARSER_STOPPARSING;
       if (mDTD && mSink &&
           mSink->ReadyToCallDidBuildModel(terminated)) {
-        result = mDTD->DidBuildModel(anErrorCode,PR_TRUE,this,mSink);
+        nsresult dtdResult =  mDTD->DidBuildModel(anErrorCode,this,mSink),
+                sinkResult = mSink->DidBuildModel();
+        // nsIDTD::DidBuildModel used to be responsible for calling
+        // nsIContentSink::DidBuildModel, but that obligation isn't expressible
+        // in the nsIDTD interface itself, so it's sounder and simpler to give
+        // that responsibility back to the parser. The former behavior of the
+        // DTD was to NS_ENSURE_SUCCESS the sink DidBuildModel call, so if the
+        // sink returns failure we should use sinkResult instead of dtdResult,
+        // to preserve the old error handling behavior of the DTD:
+        result = NS_FAILED(sinkResult) ? sinkResult : dtdResult;
       }
 
       //Ref. to bug 61462.

parser/htmlparser/src/nsViewSourceHTML.cpp

@@ -313,9 +313,8 @@ nsresult CViewSourceHTML::WillBuildModel(const CParserContext& aParserContext,
 
   mLineNumber = 1;
 
-  result = mSink->WillBuildModel(GetMode());
-
   START_TIMER();
+
   return result;
 }
 
@@ -536,7 +535,7 @@ void CViewSourceHTML::AddAttrToNode(nsCParserStartNode& aNode,
  * @param
  * @return
  */
-NS_IMETHODIMP CViewSourceHTML::DidBuildModel(nsresult anErrorCode,PRBool aNotifySink,nsIParser* aParser,nsIContentSink* aSink){
+NS_IMETHODIMP CViewSourceHTML::DidBuildModel(nsresult anErrorCode,nsIParser* aParser,nsIContentSink* aSink){
   nsresult result= NS_OK;
 
   //ADD CODE HERE TO CLOSE OPEN CONTAINERS...
@@ -547,7 +546,7 @@ NS_IMETHODIMP CViewSourceHTML::DidBuildModel(nsresult anErrorCode,PRBool aNotify
     STOP_TIMER();
 
     mSink=(nsIHTMLContentSink*)aParser->GetContentSink();
-    if((aNotifySink) && (mSink)) {
+    if (mSink) {
         //now let's close automatically auto-opened containers...
 
 #ifdef DUMP_TO_FILE
@@ -564,7 +563,6 @@ NS_IMETHODIMP CViewSourceHTML::DidBuildModel(nsresult anErrorCode,PRBool aNotify
         mSink->CloseContainer(eHTMLTag_body);
         mSink->CloseContainer(eHTMLTag_html);
       }
-      result = mSink->DidBuildModel();
     }
 
     START_TIMER();