Bug 453496. Split up SetForm() into SetForm and ClearForm and rationalize the args. r+sr=jst

content/html/content/public/nsIFormControl.h

@@ -69,9 +69,8 @@ class nsIFormSubmission;
 #define NS_FORM_OBJECT         21
 
 #define NS_IFORMCONTROL_IID   \
-{ 0x119c5ce8, 0xf4b0, 0x456c, \
-  {0x83, 0x4d, 0xb1, 0x90, 0x3d, 0x99, 0x9f, 0xb3} }
-
+{ 0x52dc1f0d, 0x1683, 0x4dd7, \
+ { 0xae, 0x0a, 0xc4, 0x76, 0x10, 0x64, 0x2f, 0xa8 } }
 
 /**
  * Interface which all form controls (e.g. buttons, checkboxes, text,
@@ -92,25 +91,23 @@ public:
 
   /**
    * Set the form for this form control.
-   * @param aForm the form.  Either this, or the control's current
-   *        form must be null.
+   * @param aForm the form.  This must not be null.
+   *
+   * @note that when setting the form the control is not added to the
+   * form.  It adds itself when it gets bound to the tree thereafter,
+   * so that it can be properly sorted with the other controls in the
+   * form.
+   */
+  virtual void SetForm(nsIDOMHTMLFormElement* aForm) = 0;
+
+  /**
+   * Tell the control to forget about its form.
    *
-   * XXXbz We should really have a SetForm method (which takes aForm
-   * and nothing else) and a ClearForm() method (which takes
-   * aRemoveFromForm fand aNotify).
-   * 
    * @param aRemoveFromForm set false if you do not want this element removed
    *        from the form.  (Used by nsFormControlList::Clear())
    * @param aNotify If true, send nsIDocumentObserver notifications as needed.
-   *
-   * @note that when setting a new form aNotify is ignored and the
-   * control is not added to the form itself.  It adds itself when it
-   * gets bound to the tree thereafter, so that it can be properly
-   * sorted with the other controls in the form.
    */
-  NS_IMETHOD SetForm(nsIDOMHTMLFormElement* aForm,
-                     PRBool aRemoveFromForm,
-                     PRBool aNotify) = 0;
+  virtual void ClearForm(PRBool aRemoveFromForm, PRBoqol aNotify) = 0;
 
   /**
    * Get the type of this control as an int (see NS_FORM_* above)

content/html/content/src/nsGenericHTMLElement.cpp

@@ -2286,7 +2286,7 @@ nsGenericHTMLFormElement::~nsGenericHTMLFormElement()
 
   // Clean up.  Set the form to nsnull so it knows we went away.
   // Do not notify as the content is being destroyed.
-  SetForm(nsnull, PR_TRUE, PR_FALSE);
+  ClearForm(PR_TRUE, PR_FALSE);
 }
 
 NS_IMPL_QUERY_INTERFACE_INHERITED1(nsGenericHTMLFormElement,
@@ -2307,42 +2307,47 @@ nsGenericHTMLFormElement::SaveSubtreeState()
   nsGenericHTMLElement::SaveSubtreeState();
 }
 
-NS_IMETHODIMP
-nsGenericHTMLFormElement::SetForm(nsIDOMHTMLFormElement* aForm,
-                                  PRBool aRemoveFromForm,
-                                  PRBool aNotify)
+void
+nsGenericHTMLFormElement::SetForm(nsIDOMHTMLFormElement* aForm)
 {
-  NS_ASSERTION(!mForm || HasFlag(ADDED_TO_FORM),
-               "Form control should have had flag set.");
-  NS_ASSERTION(!mForm || !aForm,
+  NS_PRECONDITION(aForm, "Don't pass null here");
+  NS_ASSERTION(!mForm,
                "We don't support switching from one non-null form to another.");
 
-  if (mForm) {
-    if (aRemoveFromForm) {
-      nsAutoString nameVal, idVal;
-      GetAttr(kNameSpaceID_None, nsGkAtoms::name, nameVal);
-      GetAttr(kNameSpaceID_None, nsGkAtoms::id, idVal);
+  // keep a *weak* ref to the form here
+  CallQueryInterface(aForm, &mForm);
+  mForm->Release();
+}
 
-      mForm->RemoveElement(this, aNotify);
+void
+nsGenericHTMLFormElement::ClearForm(PRBool aRemoveFromForm,
+                                    PRBool aNotify)
+{
+  NS_ASSERTION((mForm != nsnull) == HasFlag(ADDED_TO_FORM),
+               "Form control should have had flag set correctly");
 
-      if (!nameVal.IsEmpty()) {
-        mForm->RemoveElementFromTable(this, nameVal);
-      }
+  if (!mForm) {
+    return;
+  }
+  
+  if (aRemoveFromForm) {
+    nsAutoString nameVal, idVal;
+    GetAttr(kNameSpaceID_None, nsGkAtoms::name, nameVal);
+    GetAttr(kNameSpaceID_None, nsGkAtoms::id, idVal);
 
-      if (!idVal.IsEmpty()) {
-        mForm->RemoveElementFromTable(this, idVal);
-      }
+    mForm->RemoveElement(this, aNotify);
+
+    if (!nameVal.IsEmpty()) {
+      mForm->RemoveElementFromTable(this, nameVal);
     }
 
-    UnsetFlags(ADDED_TO_FORM);
-    mForm = nsnull;
-  } else if (aForm) {
-    // keep a *weak* ref to the form here
-    CallQueryInterface(aForm, &mForm);
-    mForm->Release();
+    if (!idVal.IsEmpty()) {
+      mForm->RemoveElementFromTable(this, idVal);
+    }
   }
 
-  return NS_OK;
+  UnsetFlags(ADDED_TO_FORM);
+  mForm = nsnull;
 }
 
 NS_IMETHODIMP
@@ -2444,7 +2449,7 @@ nsGenericHTMLFormElement::BindToTree(nsIDocument* aDocument,
     // probably changed _somewhere_.
     nsCOMPtr<nsIDOMHTMLFormElement> form = FindForm();
     if (form) {
-      SetForm(form, PR_FALSE, PR_FALSE);
+      SetForm(form);
     }
   }
 
@@ -2481,12 +2486,12 @@ nsGenericHTMLFormElement::UnbindFromTree(PRBool aDeep, PRBool aNullParent)
     // Might need to unset mForm
     if (aNullParent) {
       // No more parent means no more form
-      SetForm(nsnull, PR_TRUE, PR_TRUE);
+      ClearForm(PR_TRUE, PR_TRUE);
     } else {
       // Recheck whether we should still have an mForm.
       nsCOMPtr<nsIDOMHTMLFormElement> form = FindForm(mForm);
       if (!form) {
-        SetForm(nsnull, PR_TRUE, PR_TRUE);
+        ClearForm(PR_TRUE, PR_TRUE);
       } else {
         UnsetFlags(MAYBE_ORPHAN_FORM_ELEMENT);
       }

content/html/content/src/nsGenericHTMLElement.h

@@ -824,9 +824,8 @@ public:
 
   // nsIFormControl
   NS_IMETHOD GetForm(nsIDOMHTMLFormElement** aForm);
-  NS_IMETHOD SetForm(nsIDOMHTMLFormElement* aForm,
-                     PRBool aRemoveFromForm,
-                     PRBool aNotify);
+  virtual void SetForm(nsIDOMHTMLFormElement* aForm);
+  virtual void ClearForm(PRBool aRemoveFromForm, PRBool aNotify);
 
   NS_IMETHOD SaveState()
   {

content/html/content/src/nsHTMLFormElement.cpp

@@ -754,7 +754,7 @@ CollectOrphans(nsINode* aRemovalRoot, nsTArray<nsIFormControl*> aArray
     if (node->HasFlag(MAYBE_ORPHAN_FORM_ELEMENT)) {
       node->UnsetFlags(MAYBE_ORPHAN_FORM_ELEMENT);
       if (!nsContentUtils::ContentIsDescendantOf(node, aRemovalRoot)) {
-        control->SetForm(nsnull, PR_TRUE, PR_TRUE);
+        control->ClearForm(PR_TRUE, PR_TRUE);
 #ifdef DEBUG
         removed = PR_TRUE;
 #endif
@@ -2075,12 +2075,12 @@ nsFormControlList::Clear()
   // Null out childrens' pointer to me.  No refcounting here
   PRInt32 i;
   for (i = mElements.Length()-1; i >= 0; i--) {
-    mElements[i]->SetForm(nsnull, PR_FALSE, PR_TRUE);
+    mElements[i]->ClearForm(PR_FALSE, PR_TRUE);
   }
   mElements.Clear();
 
   for (i = mNotInElements.Length()-1; i >= 0; i--) {
-    mNotInElements[i]->SetForm(nsnull, PR_FALSE, PR_TRUE);
+    mNotInElements[i]->ClearForm(PR_FALSE, PR_TRUE);
   }
   mNotInElements.Clear();
 

content/html/document/src/nsHTMLContentSink.cpp

@@ -523,10 +523,10 @@ MaybeSetForm(nsGenericHTMLElement* aContent, nsHTMLTag aNodeType,
   NS_ASSERTION(formControl,
                "nsGenericHTMLElement didn't implement nsIFormControl");
   nsCOMPtr<nsIDOMHTMLFormElement> formElement(do_QueryInterface(form));
-  NS_ASSERTION(!form || formElement,
+  NS_ASSERTION(formElement,
                "nsGenericHTMLElement didn't implement nsIDOMHTMLFormElement");
 
-  formControl->SetForm(formElement, PR_TRUE, PR_FALSE);
+  formControl->SetForm(formElement);
 }
 
 /**