Bug 883827. Make Optional<NonNull<T>> and Optional<OwningNonNull<T>> nicer to use by having their const Value() method return a T&. r=peterv
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 19 Jun 2013 14:48:43 -0400
changeset 135657 4be8af5b96c7e3f65afab0c97489533e933d56e7
parent 135656 6d4a482b6aa77194d489ea8f9b483696a2b587c4
child 135658 af5a1ae148db63fbccb0ec0576ea83e0a34a722d
push id24847
push userkwierso@gmail.com
push dateWed, 19 Jun 2013 23:38:15 +0000
treeherdermozilla-central@8ea92aeab783 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspeterv
bugs883827
milestone24.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 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
content/media/webaudio/AudioContext.cpp
dom/bindings/BindingDeclarations.h
dom/bindings/Codegen.py
dom/src/notification/Notification.cpp
--- a/content/base/src/nsFormData.cpp
+++ b/content/base/src/nsFormData.cpp
@@ -109,18 +109,17 @@ nsFormData::WrapObject(JSContext* aCx, J
 
 /* static */ already_AddRefed<nsFormData>
 nsFormData::Constructor(const GlobalObject& aGlobal,
                         const Optional<NonNull<HTMLFormElement> >& aFormElement,
                         ErrorResult& aRv)
 {
   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();
 }
 
 // -------------------------------------------------------------------------
 // nsIXHRSendable
 
 NS_IMETHODIMP
--- a/content/media/webaudio/AudioContext.cpp
+++ b/content/media/webaudio/AudioContext.cpp
@@ -361,17 +361,17 @@ AudioContext::DecodeAudioData(const Arra
   // Failed type sniffing will be handled by AsyncDecodeMedia.
   nsAutoCString contentType;
   NS_SniffContent(NS_DATA_SNIFFER_CATEGORY, nullptr,
                   aBuffer.Data(), aBuffer.Length(),
                   contentType);
 
   nsCOMPtr<DecodeErrorCallback> failureCallback;
   if (aFailureCallback.WasPassed()) {
-    failureCallback = aFailureCallback.Value().get();
+    failureCallback = &aFailureCallback.Value();
   }
   nsAutoPtr<WebAudioDecodeJob> job(
     new WebAudioDecodeJob(contentType, this,
                           &aSuccessCallback, failureCallback));
   mDecoder.AsyncDecodeMedia(contentType.get(),
                             aBuffer.Data(), aBuffer.Length(), *job);
   // Transfer the ownership to mDecodeJobs
   mDecodeJobs.AppendElement(job.forget());
--- a/dom/bindings/BindingDeclarations.h
+++ b/dom/bindings/BindingDeclarations.h
@@ -260,35 +260,43 @@ public:
   }
 
   template <class T1, class T2>
   void Construct(const T1 &t1, const T2 &t2)
   {
     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.
 
 private:
   // Forbid copy-construction and assignment
   Optional_base(const Optional_base& other) MOZ_DELETE;
   const Optional_base &operator=(const Optional_base &other) MOZ_DELETE;
 
+protected:
   Maybe<InternalType> mImpl;
 };
 
 template<typename T>
 class Optional : public Optional_base<T, T>
 {
 public:
   Optional() :
@@ -313,16 +321,30 @@ public:
     Optional_base<JS::Handle<T>, JS::Rooted<T> >()
   {
     this->Construct(cx);
   }
 
   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
 // calls Construct() on it we will pre-initialized the JSObject* to nullptr so
 // it can be traced safely.
 template<>
 class Optional<JSObject*> : public Optional_base<JSObject*, JSObject*>
 {
@@ -374,16 +396,61 @@ public:
 
   template <class T1>
   void Construct(const T1& t1)
   {
     Optional_base<JS::Value, JS::Value>::Construct(t1);
   }
 };
 
+// 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
 // ToAStringPtr.
 
 struct FakeDependentString;
 
 template<>
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -8012,17 +8012,17 @@ class CGDictionary(CGThing):
     def getMemberDefinition(self, memberInfo):
         member = memberInfo[0]
         declType = memberInfo[1].declType
         memberLoc = self.makeMemberName(member.identifier.name)
         if member.defaultValue:
             memberData = memberLoc
         else:
             # The data is inside the Optional<>
-            memberData = "%s.Value()" % memberLoc
+            memberData = "%s.InternalValue()" % memberLoc
 
         if self.workers:
             propDef = (
                 'JS_DefineProperty(cx, obj, "%s", temp, nullptr, nullptr, JSPROP_ENUMERATE)' %
                 member.identifier.name)
         else:
             propDef = (
                 'JS_DefinePropertyById(cx, obj, %s, temp, nullptr, nullptr, JSPROP_ENUMERATE)' %
--- a/dom/src/notification/Notification.cpp
+++ b/dom/src/notification/Notification.cpp
@@ -377,17 +377,17 @@ Notification::RequestPermission(const Gl
   if (!sop) {
     aRv.Throw(NS_ERROR_UNEXPECTED);
     return;
   }
   nsCOMPtr<nsIPrincipal> principal = sop->GetPrincipal();
 
   NotificationPermissionCallback* permissionCallback = nullptr;
   if (aCallback.WasPassed()) {
-    permissionCallback = aCallback.Value().get();
+    permissionCallback = &aCallback.Value();
   }
   nsCOMPtr<nsIRunnable> request =
     new NotificationPermissionRequest(principal, window, permissionCallback);
 
   NS_DispatchToMainThread(request);
 }
 
 NotificationPermission