Bug 958576 part 2. Move FakeDependentString to the binding_detail namespace. r=peterv
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 22 Jan 2014 14:37:10 -0500
changeset 180858 1ee233ff37f80df3d2820eccf728bc5e8ce60422
parent 180857 669c460f673c90a899372f833b39f27920d5f8a8
child 180859 c1d65e90d4c297f3358efdb8b9fcfe59af09952f
push id3343
push userffxbld
push dateMon, 17 Mar 2014 21:55:32 +0000
treeherdermozilla-beta@2f7d3415f79f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspeterv
bugs958576
milestone29.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 958576 part 2. Move FakeDependentString to the binding_detail namespace. r=peterv I took the opportunity to move away from the NonNull<nsAString> hack we had for string arguments, since just passing in a FakeDependentString works fine and callees are now less likely to declare their arg as being of that type. In the process of doing that, I ran into what looks like a substantive bug in the "owning union with string default value" case: We were doing mValue.mString.Value() without ever having constructed mValue.mString!
content/html/content/src/nsGenericHTMLElement.cpp
dom/bindings/BindingDeclarations.h
dom/bindings/BindingUtils.h
dom/bindings/Codegen.py
dom/events/nsJSEventListener.cpp
--- a/content/html/content/src/nsGenericHTMLElement.cpp
+++ b/content/html/content/src/nsGenericHTMLElement.cpp
@@ -3149,17 +3149,17 @@ nsGenericHTMLElement::SetItemValue(JSCon
                                    ErrorResult& aError)
 {
   if (!HasAttr(kNameSpaceID_None, nsGkAtoms::itemprop) ||
       HasAttr(kNameSpaceID_None, nsGkAtoms::itemscope)) {
     aError.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);
     return;
   }
 
-  FakeDependentString string;
+  binding_detail::FakeDependentString string;
   JS::Rooted<JS::Value> value(aCx, aValue);
   if (!ConvertJSValueToString(aCx, value, &value, eStringify, eStringify, string)) {
     aError.Throw(NS_ERROR_UNEXPECTED);
     return;
   }
   SetItemValueText(string);
 }
 
--- a/dom/bindings/BindingDeclarations.h
+++ b/dom/bindings/BindingDeclarations.h
@@ -317,17 +317,19 @@ public:
   }
 };
 
 // Specialization for strings.
 // XXXbz we can't pull in FakeDependentString here, because it depends on
 // internal strings.  So we just have to forward-declare it and reimplement its
 // ToAStringPtr.
 
+namespace binding_detail {
 struct FakeDependentString;
+} // namespace binding_detail
 
 template<>
 class Optional<nsAString>
 {
 public:
   Optional() : mPassed(false) {}
 
   bool WasPassed() const
@@ -339,17 +341,17 @@ public:
   {
     MOZ_ASSERT(str);
     mStr = str;
     mPassed = true;
   }
 
   // If this code ever goes away, remove the comment pointing to it in the
   // FakeDependentString class in BindingUtils.h.
-  void operator=(const FakeDependentString* str)
+  void operator=(const binding_detail::FakeDependentString* str)
   {
     MOZ_ASSERT(str);
     mStr = reinterpret_cast<const nsDependentString*>(str);
     mPassed = true;
   }
 
   const nsAString& Value() const
   {
--- a/dom/bindings/BindingUtils.h
+++ b/dom/bindings/BindingUtils.h
@@ -1451,16 +1451,18 @@ HasPropertyOnPrototype(JSContext* cx, JS
 // shadowPrototypeProperties is false then skip properties that are also
 // present on the proto chain of proxy.  If shadowPrototypeProperties is true,
 // then the "proxy" argument is ignored.
 bool
 AppendNamedPropertyIds(JSContext* cx, JS::Handle<JSObject*> proxy,
                        nsTArray<nsString>& names,
                        bool shadowPrototypeProperties, JS::AutoIdVector& props);
 
+namespace binding_detail {
+
 // A struct that has the same layout as an nsDependentString but much
 // faster constructor and destructor behavior
 struct FakeDependentString {
   FakeDependentString() :
     mFlags(nsDependentString::F_TERMINATED)
   {
   }
 
@@ -1528,29 +1530,31 @@ private:
                     "Offset of mLength should match");
       static_assert(offsetof(FakeDependentString, mFlags) ==
                       offsetof(DepedentStringAsserter, mFlags),
                     "Offset of mFlags should match");
     }
   };
 };
 
+} // namespace binding_detail
+
 enum StringificationBehavior {
   eStringify,
   eEmpty,
   eNull
 };
 
 // pval must not be null and must point to a rooted JS::Value
 static inline bool
 ConvertJSValueToString(JSContext* cx, JS::Handle<JS::Value> v,
                        JS::MutableHandle<JS::Value> pval,
                        StringificationBehavior nullBehavior,
                        StringificationBehavior undefinedBehavior,
-                       FakeDependentString& result)
+                       binding_detail::FakeDependentString& result)
 {
   JSString *s;
   if (v.isString()) {
     s = v.toString();
   } else {
     StringificationBehavior behavior;
     if (v.isNull()) {
       behavior = nullBehavior;
@@ -2043,16 +2047,49 @@ T& NonNullHelper(OwningNonNull<T>& aArg)
 }
 
 template<typename T>
 const T& NonNullHelper(const OwningNonNull<T>& aArg)
 {
   return aArg;
 }
 
+inline
+void NonNullHelper(NonNull<binding_detail::FakeDependentString>& aArg)
+{
+  // This overload is here to make sure that we never end up applying
+  // NonNullHelper to a NonNull<binding_detail::FakeDependentString>. If we
+  // try to, it should fail to compile, since presumably the caller will try to
+  // use our nonexistent return value.
+}
+
+inline
+void NonNullHelper(const NonNull<binding_detail::FakeDependentString>& aArg)
+{
+  // This overload is here to make sure that we never end up applying
+  // NonNullHelper to a NonNull<binding_detail::FakeDependentString>. If we
+  // try to, it should fail to compile, since presumably the caller will try to
+  // use our nonexistent return value.
+}
+
+inline
+void NonNullHelper(binding_detail::FakeDependentString& aArg)
+{
+  // This overload is here to make sure that we never end up applying
+  // NonNullHelper to a FakeDependentString before we've constified it.  If we
+  // try to, it should fail to compile, since presumably the caller will try to
+  // use our nonexistent return value.
+}
+
+MOZ_ALWAYS_INLINE
+const nsAString& NonNullHelper(const binding_detail::FakeDependentString& aArg)
+{
+  return aArg;
+}
+
 // Reparent the wrapper of aObj to whatever its native now thinks its
 // parent should be.
 nsresult
 ReparentWrapper(JSContext* aCx, JS::Handle<JSObject*> aObj);
 
 /**
  * Used to implement the hasInstance hook of an interface object.
  *
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -3631,36 +3631,40 @@ for (uint32_t i = 0; i < length; ++i) {
             if isMember == "Variadic":
                 # The string is kept alive by the argument, so we can just
                 # depend on it.
                 assignString = "${declName}.Rebind(str.Data(), str.Length())"
             else:
                 assignString = "${declName} = str"
             return JSToNativeConversionInfo(
                 "{\n"
-                "  FakeDependentString str;\n"
+                "  binding_detail::FakeDependentString str;\n"
                 "%s\n"
                 "  %s;\n"
                 "}\n" % (
                     CGIndenter(CGGeneric(getConversionCode("str"))).define(),
                     assignString),
                 declType=declType, dealWithOptional=isOptional)
 
         if isOptional:
             declType = "Optional<nsAString>"
-        else:
-            declType = "NonNull<nsAString>"
+            holderType = CGGeneric("binding_detail::FakeDependentString")
+            conversionCode = ("%s\n"
+                              "${declName} = &${holderName};" %
+                              getConversionCode("${holderName}"))
+        else:
+            declType = "binding_detail::FakeDependentString"
+            holderType = None
+            conversionCode = getConversionCode("${declName}")
 
         # No need to deal with optional here; we handled it already
         return JSToNativeConversionInfo(
-            ("%s\n"
-             "${declName} = &${holderName};" %
-             getConversionCode("${holderName}")),
+            conversionCode,
             declType=CGGeneric(declType),
-            holderType=CGGeneric("FakeDependentString"))
+            holderType=holderType)
 
     if type.isByteString():
         assert not isEnforceRange and not isClamp
 
         nullable = toStringBool(type.nullable())
 
         conversionCode = (
             "if (!ConvertJSValueToByteString(cx, ${val}, ${mutableVal},"
@@ -6937,17 +6941,17 @@ class CGUnionStruct(CGThing):
                 if self.ownsMembers:
                     methods.append(vars["setter"])
                     if t.isString():
                         methods.append(
                             ClassMethod("SetStringData", "void",
                                 [Argument("const nsString::char_type*", "aData"),
                                  Argument("nsString::size_type", "aLength")],
                                 inline=True, bodyInHeader=True,
-                                body="mValue.mString.Value().Assign(aData, aLength);"))
+                                body="SetAsString().Assign(aData, aLength);"))
 
             body = string.Template('MOZ_ASSERT(Is${name}(), "Wrong type!");\n'
                                    'mValue.m${name}.Destroy();\n'
                                    'mType = eUninitialized;').substitute(vars)
             methods.append(ClassMethod("Destroy" + vars["name"],
                                        "void",
                                        [],
                                        visibility="private",
@@ -7136,17 +7140,17 @@ class CGUnionConversionStruct(CGThing):
                                            bodyInHeader=True,
                                            body=body,
                                            visibility="private"))
                 if t.isString():
                     methods.append(ClassMethod("SetStringData", "void",
                                      [Argument("const nsDependentString::char_type*", "aData"),
                                       Argument("nsDependentString::size_type", "aLength")],
                                      inline=True, bodyInHeader=True,
-                                     body="mStringHolder.SetData(aData, aLength);"))
+                                     body="SetAsString().SetData(aData, aLength);"))
 
             if vars["holderType"] is not None:
                 members.append(ClassMember("m%sHolder" % vars["name"],
                                            vars["holderType"]))
 
         return CGClass(structName + "Argument",
                        members=members,
                        constructors=[ctor],
@@ -7960,17 +7964,17 @@ class CGProxyNamedOperation(CGProxySpeci
             unwrapString = ("nameVal = %s;\n" % self.value) + unwrapString
 
         # Sadly, we have to set up nameVal even if we have an atom id,
         # because we don't know for sure, and we can end up needing it
         # so it needs to be higher up the stack.  Using a Maybe here
         # seems like probable overkill.
         return ("JS::Rooted<JS::Value> nameVal(cx);\n" +
                 idDecl +
-                ("FakeDependentString %s;\n" % argName) +
+                ("binding_detail::FakeDependentString %s;\n" % argName) +
                 unwrapString +
                 ("\n"
                  "\n"
                  "%s* self = UnwrapProxy(proxy);\n" %
                  self.descriptor.nativeType) +
                 CGProxySpecialOperation.define(self))
 
 class CGProxyNamedGetter(CGProxyNamedOperation):
@@ -11596,16 +11600,18 @@ struct PrototypeTraits;
         (includes, implincludes,
          declarations, unions) = UnionTypes(config.getDescriptors(),
                                             config.getDictionaries(),
                                             config.getCallbacks(),
                                             config)
         includes.add("mozilla/dom/OwningNonNull.h")
         includes.add("mozilla/dom/UnionMember.h")
         includes.add("mozilla/dom/BindingDeclarations.h")
+        # Need BindingUtils.h for FakeDependentString
+        includes.add("mozilla/dom/BindingUtils.h")
         implincludes.add("mozilla/dom/PrimitiveConversions.h")
 
         # Wrap all of that in our namespaces.
         curr = CGNamespace.build(['mozilla', 'dom'], unions)
 
         curr = CGWrapper(curr, post='\n')
 
         namespaces = []
--- a/dom/events/nsJSEventListener.cpp
+++ b/dom/events/nsJSEventListener.cpp
@@ -168,17 +168,17 @@ nsJSEventListener::HandleEvent(nsIDOMEve
 
     NS_ENSURE_TRUE(aEvent, NS_ERROR_UNEXPECTED);
     InternalScriptErrorEvent* scriptEvent =
       aEvent->GetInternalNSEvent()->AsScriptErrorEvent();
     if (scriptEvent &&
         (scriptEvent->message == NS_LOAD_ERROR ||
          scriptEvent->typeString.EqualsLiteral("error"))) {
       errorMsg = scriptEvent->errorMsg;
-      msgOrEvent.SetAsString() = static_cast<nsAString*>(&errorMsg);
+      msgOrEvent.SetAsString().SetData(errorMsg.Data(), errorMsg.Length());
 
       file = scriptEvent->fileName;
       fileName = &file;
 
       lineNumber.Construct();
       lineNumber.Value() = scriptEvent->lineNr;
     } else {
       msgOrEvent.SetAsEvent() = aEvent->InternalDOMEvent();