Skip installing fields which have no text. Bug 397924, r+sr+a=sicking
authorbzbarsky@mit.edu
Tue, 02 Oct 2007 07:38:35 -0700
changeset 6535 635ec289e4ef11c322dcd30d3f2512941fa0ae12
parent 6534 d7cb799f78e527ab3819e0a135f3b44411c09543
child 6536 a898fb4991b2d10265fe8dba6f9d324f7d95eccc
push id1
push userroot
push dateTue, 26 Apr 2011 22:38:44 +0000
treeherdermozilla-beta@bfdb6e623a36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs397924
milestone1.9a9pre
Skip installing fields which have no text. Bug 397924, r+sr+a=sicking
content/xbl/src/nsXBLBinding.cpp
content/xbl/src/nsXBLProtoImplField.cpp
content/xbl/src/nsXBLProtoImplField.h
content/xbl/test/Makefile.in
content/xbl/test/test_bug397934.xhtml
--- a/content/xbl/src/nsXBLBinding.cpp
+++ b/content/xbl/src/nsXBLBinding.cpp
@@ -199,27 +199,33 @@ XBLResolve(JSContext *cx, JSObject *obj,
 
   nsCOMPtr<nsIScriptContext> context = global->GetContext();
   if (!context) {
     return JS_TRUE;
   }
 
 
   // Now we either resolve or fail
-  *objp = origObj;
+  PRBool didInstall;
   nsresult rv = field->InstallField(context, origObj,
-                                    protoBinding->DocURI());
+                                    protoBinding->DocURI(),
+                                    &didInstall);
   if (NS_FAILED(rv)) {
     if (!::JS_IsExceptionPending(cx)) {
       nsDOMClassInfo::ThrowJSException(cx, rv);
     }
 
     return JS_FALSE;
   }
 
+  if (didInstall) {
+    *objp = origObj;
+  }
+  // else we didn't resolve this field after all
+
   return JS_TRUE;
 }
 
 nsXBLJSClass::nsXBLJSClass(const nsAFlatCString& aClassName)
 {
   memset(this, 0, sizeof(nsXBLJSClass));
   next = prev = static_cast<JSCList*>(this);
   name = ToNewCString(aClassName);
--- a/content/xbl/src/nsXBLProtoImplField.cpp
+++ b/content/xbl/src/nsXBLProtoImplField.cpp
@@ -89,59 +89,65 @@ nsXBLProtoImplField::AppendFieldText(con
     mFieldText = ToNewUnicode(aText);
     mFieldTextLength = aText.Length();
   }
 }
 
 nsresult
 nsXBLProtoImplField::InstallField(nsIScriptContext* aContext,
                                   JSObject* aBoundNode,
-                                  nsIURI* aBindingDocURI) const
+                                  nsIURI* aBindingDocURI,
+                                  PRBool* aDidInstall) const
 {
   NS_PRECONDITION(aBoundNode,
                   "uh-oh, bound node should NOT be null or bad things will "
                   "happen");
 
-  jsval result = JSVAL_VOID;
+  *aDidInstall = PR_FALSE;
+
+  if (mFieldTextLength == 0) {
+    return NS_OK;
+  }
+
+  jsval result = JSVAL_NULL;
   
   // EvaluateStringWithValue and JS_DefineUCProperty can both trigger GC, so
   // protect |result| here.
   nsresult rv;
   nsAutoGCRoot root(&result, &rv);
   if (NS_FAILED(rv))
     return rv;
 
-  if (mFieldTextLength != 0) {
-    nsCAutoString uriSpec;
-    aBindingDocURI->GetSpec(uriSpec);
+  nsCAutoString uriSpec;
+  aBindingDocURI->GetSpec(uriSpec);
   
-    // compile the literal string
-    // XXX Could we produce a better principal here?  Should be able
-    // to, really!
-    PRBool undefined;
-    nsCOMPtr<nsIScriptContext> context = aContext;
-    rv = context->EvaluateStringWithValue(nsDependentString(mFieldText,
-                                                            mFieldTextLength), 
-                                          aBoundNode,
-                                          nsnull, uriSpec.get(),
-                                          mLineNumber, nsnull,
-                                          (void*) &result, &undefined);
-    if (NS_FAILED(rv))
-      return rv;
+  // compile the literal string
+  // XXX Could we produce a better principal here?  Should be able
+  // to, really!
+  PRBool undefined;
+  nsCOMPtr<nsIScriptContext> context = aContext;
+  rv = context->EvaluateStringWithValue(nsDependentString(mFieldText,
+                                                          mFieldTextLength), 
+                                        aBoundNode,
+                                        nsnull, uriSpec.get(),
+                                        mLineNumber, nsnull,
+                                        (void*) &result, &undefined);
+  if (NS_FAILED(rv))
+    return rv;
 
-    if (undefined) {
-      result = JSVAL_VOID;
-    }
+  if (undefined) {
+    result = JSVAL_VOID;
   }
 
   // Define the evaluated result as a JS property
   nsDependentString name(mName);
   JSContext* cx = (JSContext*) aContext->GetNativeContext();
   JSAutoRequest ar(cx);
   if (!::JS_DefineUCProperty(cx, aBoundNode,
                              reinterpret_cast<const jschar*>(mName), 
                              name.Length(), result, nsnull, nsnull,
                              mJSAttributes)) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
-  
+
+  *aDidInstall = PR_TRUE;
   return NS_OK;
 }
--- a/content/xbl/src/nsXBLProtoImplField.h
+++ b/content/xbl/src/nsXBLProtoImplField.h
@@ -58,18 +58,19 @@ public:
   void SetLineNumber(PRUint32 aLineNumber) {
     mLineNumber = aLineNumber;
   }
   
   nsXBLProtoImplField* GetNext() const { return mNext; }
   void SetNext(nsXBLProtoImplField* aNext) { mNext = aNext; }
 
   nsresult InstallField(nsIScriptContext* aContext,
-                        JSObject* aBoundNode, nsIURI*
-                        aBindingDocURI) const;
+                        JSObject* aBoundNode,
+                        nsIURI* aBindingDocURI,
+                        PRBool* aDidInstall) const;
 
   const PRUnichar* GetName() const { return mName; }
 
 protected:
   nsXBLProtoImplField* mNext;
   PRUnichar* mName;
   PRUnichar* mFieldText;
   PRUint32 mFieldTextLength;
--- a/content/xbl/test/Makefile.in
+++ b/content/xbl/test/Makefile.in
@@ -46,12 +46,13 @@ relativesrcdir  = content/xbl/test
 include $(DEPTH)/config/autoconf.mk
 include $(topsrcdir)/config/rules.mk
 
 _TEST_FILES =	\
 		test_bug296375.xul \
 		test_bug366770.html \
 		test_bug371724.xhtml \
 		test_bug372769.xhtml \
+		test_bug397934.xhtml \
 		$(NULL)
 
 libs:: $(_TEST_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)
new file mode 100644
--- /dev/null
+++ b/content/xbl/test/test_bug397934.xhtml
@@ -0,0 +1,116 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=397934
+-->
+<head>
+  <title>Test for Bug 397934</title>
+  <script type="text/javascript" src="/MochiKit/packed.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+  <bindings xmlns="http://www.mozilla.org/xbl">
+    <binding id="ancestor">
+      <implementation>
+        <field name="testAncestor">"ancestor"</field>
+      </implementation>
+    </binding>
+    <binding id="test1" extends="#ancestor">
+      <implementation>
+        <constructor>
+          this.storage = window;
+        </constructor>
+        <field name="window"></field>
+        <field name="storage">null</field>
+        <field name="parentNode"></field>
+        <field name="ownerDocument">"ownerDocument"</field>
+        <field name="emptyTest1"></field>
+        <field name="emptyTest1">"empty1"</field>
+        <field name="emptyTest2">"empty2"</field>        
+        <field name="emptyTest2"></field>
+        <field name="testAncestor"></field>
+        <field name="testEvalOnce">window.counter++; undefined</field>
+      </implementation>
+    </binding>
+    <binding id="test2" extends="#ancestor">
+      <implementation>
+        <constructor>
+          this.storage = window;
+        </constructor>
+        <field name="window">undefined</field>
+        <field name="storage">null</field>
+        <field name="parentNode">undefined</field>
+        <field name="ownerDocument">"ownerDocument"</field>
+        <field name="emptyTest1">undefined</field>
+        <field name="emptyTest1">"empty1"</field>
+        <field name="emptyTest2">"empty2"</field>        
+        <field name="emptyTest2">undefined</field>
+        <field name="testAncestor">undefined</field>
+      </implementation>
+    </binding>
+  </bindings>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=397934">Mozilla Bug 397934</a>
+<p id="display1" style="-moz-binding: url(#test1)"></p>
+<p id="display2" style="-moz-binding: url(#test2)"></p>
+<div id="content" style="display: none">
+  
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+<![CDATA[
+
+/** Test for Bug 397934 **/
+SimpleTest.waitForExplicitFinish();
+addLoadEvent(function() {
+  var d;
+  d = $("display1");
+  is(d.storage, window,
+     "test1" +
+       ": |window| in the constructor should have resolved to our window");
+  is(d.ownerDocument, "ownerDocument",
+     "test1" + ": Control to test field overriding DOM prop");
+  is(d.parentNode, document.body, "test1" + ": unexpected parent");
+  is(d.emptyTest1, undefined,
+     "test1" + ": First field wins even if undefined");
+  is(typeof(d.emptyTest1), "undefined",
+     "test1" + ": First field wins even if undefined, double-check");
+  is(d.emptyTest2, "empty2",
+     "test1" + ": First field wins");
+  is(d.testAncestor, "ancestor",
+     "test1" + ": Empty field should not override ancestor binding");
+
+  window.counter = 0;
+  d.testEvalOnce;
+  d.testEvalOnce;
+  is(window.counter, 1, "Field should have evaluated once and only once");
+
+  d = $("display2");
+  is(d.storage, undefined,
+     "test2" +
+       ": |window| in the constructor should have resolved to undefined");
+  is(typeof(d.storage), "undefined",
+     "test2" +
+       ": |window| in the constructor should really have resolved to undefined");
+  is(d.ownerDocument, "ownerDocument",
+     "test2" + ": Control to test field overriding DOM prop");
+  is(d.parentNode, undefined, "test2" + ": unexpected parent");
+  is(typeof(d.parentNode), "undefined", "test2" + ": unexpected parent for real");
+  is(d.emptyTest1, undefined,
+     "test2" + ": First field wins even if undefined");
+  is(typeof(d.emptyTest1), "undefined",
+     "test2" + ": First field wins even if undefined, double-check");
+  is(d.emptyTest2, "empty2",
+     "test2" + ": First field wins");
+  is(d.testAncestor, undefined,
+     "test2" + ": Undefined field should override ancestor binding");
+  is(typeof(d.testAncestor), "undefined",
+     "test2" + ": Undefined field should really override ancestor binding");
+});
+addLoadEvent(SimpleTest.finish);
+
+]]>
+</script>
+</pre>
+</body>
+</html>
+