Bug 883827. Make Optional<NonNull<T>> and Optional<OwningNonNull<T>> nicer to use by having their const Value() method return a T&. r=peterv

content/base/src/nsFormData.cpp

@@ -114,8 +114,7 @@ nsFormData::Constructor(const GlobalObject& aGlobal,
 {
   nsRefPtr<nsFormData> formData = new nsFormData(aGlobal.Get());
   if (aFormElement.WasPassed()) {
-    // TODO: this should ...Value().WalkFromElements(formData); - Bug 883827
-    aRv = aFormElement.Value().get()->WalkFormElements(formData);
+    aRv = aFormElement.Value().WalkFormElements(formData);
   }
   return formData.forget();
 }

content/media/webaudio/AudioContext.cpp

@@ -366,7 +366,7 @@ AudioContext::DecodeAudioData(const ArrayBuffer& aBuffer,
 
   nsCOMPtr<DecodeErrorCallback> failureCallback;
   if (aFailureCallback.WasPassed()) {
-    failureCallback = aFailureCallback.Value().get();
+    failureCallback = &aFailureCallback.Value();
   }
   nsAutoPtr<WebAudioDecodeJob> job(
     new WebAudioDecodeJob(contentType, this,

dom/bindings/BindingDeclarations.h

@@ -265,16 +265,23 @@ public:
     mImpl.construct(t1, t2);
   }
 
-  const InternalType& Value() const
+  const T& Value() const
   {
     return mImpl.ref();
   }
 
+  // Return InternalType here so we can work with it usefully.
   InternalType& Value()
   {
     return mImpl.ref();
   }
 
+  // And an explicit way to get the InternalType even if we're const.
+  const InternalType& InternalValue() const
+  {
+    return mImpl.ref();
+  }
+
   // If we ever decide to add conversion operators for optional arrays
   // like the ones Nullable has, we'll need to ensure that Maybe<> has
   // the boolean before the actual data.
@@ -284,6 +291,7 @@ private:
   Optional_base(const Optional_base& other) MOZ_DELETE;
   const Optional_base &operator=(const Optional_base &other) MOZ_DELETE;
 
+protected:
   Maybe<InternalType> mImpl;
 };
 
@@ -318,6 +326,20 @@ public:
   Optional(JSContext* cx, const T& aValue) :
     Optional_base<JS::Handle<T>, JS::Rooted<T> >(cx, aValue)
   {}
+
+  // Override the const Value() to return the right thing so we're not
+  // returning references to temporaries.
+  JS::Handle<T> Value() const
+  {
+    return this->mImpl.ref();
+  }
+
+  // And we have to override the non-const one too, since we're
+  // shadowing the one on the superclass.
+  JS::Rooted<T>& Value()
+  {
+    return this->mImpl.ref();
+  }
 };
 
 // A specialization of Optional for JSObject* to make sure that when someone
@@ -379,6 +401,51 @@ public:
   }
 };
 
+// A specialization of Optional for NonNull that lets us get a T& from Value()
+template<typename U> class NonNull;
+template<typename T>
+class Optional<NonNull<T> > : public Optional_base<T, NonNull<T> >
+{
+public:
+  // We want our Value to actually return a non-const reference, even
+  // if we're const.  At least for things that are normally pointer
+  // types...
+  T& Value() const
+  {
+    return *this->mImpl.ref().get();
+  }
+
+  // And we have to override the non-const one too, since we're
+  // shadowing the one on the superclass.
+  NonNull<T>& Value()
+  {
+    return this->mImpl.ref();
+  }
+};
+
+// A specialization of Optional for OwningNonNull that lets us get a
+// T& from Value()
+template<typename U> class OwningNonNull;
+template<typename T>
+class Optional<OwningNonNull<T> > : public Optional_base<T, OwningNonNull<T> >
+{
+public:
+  // We want our Value to actually return a non-const reference, even
+  // if we're const.  At least for things that are normally pointer
+  // types...
+  T& Value() const
+  {
+    return *this->mImpl.ref().get();
+  }
+
+  // And we have to override the non-const one too, since we're
+  // shadowing the one on the superclass.
+  OwningNonNull<T>& Value()
+  {
+    return this->mImpl.ref();
+  }
+};
+
 // 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

dom/bindings/Codegen.py

@@ -8017,7 +8017,7 @@ class CGDictionary(CGThing):
             memberData = memberLoc
         else:
             # The data is inside the Optional<>
-            memberData = "%s.Value()" % memberLoc
+            memberData = "%s.InternalValue()" % memberLoc
 
         if self.workers:
             propDef = (

dom/src/notification/Notification.cpp

@@ -382,7 +382,7 @@ Notification::RequestPermission(const GlobalObject& aGlobal,
 
   NotificationPermissionCallback* permissionCallback = nullptr;
   if (aCallback.WasPassed()) {
-    permissionCallback = aCallback.Value().get();
+    permissionCallback = &aCallback.Value();
   }
   nsCOMPtr<nsIRunnable> request =
     new NotificationPermissionRequest(principal, window, permissionCallback);