Bug 1290541 - Fix broken test_fooUrl.js, r=jorgk a=jorgk IGNORE IDL
authorR Kent James <rkent@caspia.com>
Thu, 04 Aug 2016 13:55:34 -0700
changeset 27425 75e7f00fedba73d33d32194a6831f7ae72d5f150
parent 27424 a08bdac74f35aef9b0b56e0f8d025be5da0cb449
child 27426 9b7fd654f3ccec1ce658feb7ff507c32c9582420
push id1850
push userclokep@gmail.com
push dateWed, 08 Mar 2017 19:29:12 +0000
treeherdercomm-esr52@028df196b2d9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjorgk, jorgk
bugs1290541
Bug 1290541 - Fix broken test_fooUrl.js, r=jorgk a=jorgk IGNORE IDL
mailnews/base/public/nsIMsgMailNewsUrl.idl
mailnews/base/util/nsMsgMailNewsUrl.cpp
mailnews/base/util/nsMsgMailNewsUrl.h
mailnews/imap/src/nsImapUrl.cpp
mailnews/imap/src/nsImapUrl.h
mailnews/jsaccount/test/unit/resources/testJaFooUrlComponent.js
mailnews/jsaccount/test/unit/test_fooUrl.js
mailnews/local/src/nsMailboxUrl.cpp
mailnews/local/src/nsMailboxUrl.h
mailnews/news/src/nsNntpUrl.cpp
mailnews/news/src/nsNntpUrl.h
--- a/mailnews/base/public/nsIMsgMailNewsUrl.idl
+++ b/mailnews/base/public/nsIMsgMailNewsUrl.idl
@@ -35,16 +35,41 @@ interface nsIMsgMailNewsUrl : nsIURL {
 
   readonly attribute nsIURI baseURI;
 
   // if you really want to know what the current state of the url is (running or not
   // running) you should look into becoming a urlListener...
   void SetUrlState(in boolean runningUrl, in nsresult aStatusCode);
   void GetUrlState(out boolean runningUrl);
 
+  // These are used by cloneInternal to determine the disposition of the uri
+  // ref when cloning. They should match the RefHandlingEnum enums defined,
+  // for example, in nsStandardURI.h but are not strictly required to do so.
+
+  /// Ignore the ref field, replacing with blank.
+  const unsigned long IGNORE_REF = 0;
+
+  /// Keep the existing ref field.
+  const unsigned long HONOR_REF = 1;
+
+  /// Replace the ref field with a new value.
+  const unsigned long REPLACE_REF = 2;
+
+  /**
+   * The general clone method that other clone methods use.
+   *
+   * This class should NOT be called except from classes that override
+   * a base class.
+   *
+   * @param aRefHandlingMode  one of the Ref constants defined above.
+   * @param aNewRef           the new reference, if needed.
+   * @return                  a cloned version of the current object.
+   */
+  nsIURI cloneInternal(in unsigned long aRefHandlingMode, in AUTF8String aNewRef);
+
   readonly attribute nsIMsgIncomingServer server;
 
   /**
    * The folder associated with this url.
    * 
    * @exception NS_ERROR_FAILURE    May be thrown if the url does not
    *                                relate to a folder, e.g. standalone
    *                                .eml messages.
--- a/mailnews/base/util/nsMsgMailNewsUrl.cpp
+++ b/mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ -512,18 +512,18 @@ nsMsgMailNewsUrl::GetHasRef(bool *result
   return m_baseURL->GetHasRef(result);
 }
 
 NS_IMETHODIMP nsMsgMailNewsUrl::SchemeIs(const char *aScheme, bool *_retval)
 {
   return m_baseURL->SchemeIs(aScheme, _retval);
 }
 
-nsresult
-nsMsgMailNewsUrl::CloneInternal(RefHandlingEnum aRefHandlingMode,
+NS_IMETHODIMP
+nsMsgMailNewsUrl::CloneInternal(uint32_t aRefHandlingMode,
                                 const nsACString& newRef, nsIURI** _retval)
 {
   nsresult rv;
   nsAutoCString urlSpec;
   nsCOMPtr<nsIIOService> ioService =
     mozilla::services::GetIOService();
   NS_ENSURE_TRUE(ioService, NS_ERROR_UNEXPECTED);
   rv = GetSpec(urlSpec);
@@ -535,42 +535,42 @@ nsMsgMailNewsUrl::CloneInternal(RefHandl
   nsCOMPtr<nsIMsgWindow> msgWindow(do_QueryReferent(m_msgWindowWeak));
   if (msgWindow)
   {
     nsCOMPtr<nsIMsgMailNewsUrl> msgMailNewsUrl = do_QueryInterface(*_retval, &rv);
     NS_ENSURE_SUCCESS(rv, rv);
     msgMailNewsUrl->SetMsgWindow(msgWindow);
   }
 
-  if (aRefHandlingMode == eReplaceRef) {
+  if (aRefHandlingMode == nsIMsgMailNewsUrl::REPLACE_REF) {
     rv = (*_retval)->SetRef(newRef);
     NS_ENSURE_SUCCESS(rv, rv);
-  } else if (aRefHandlingMode == eIgnoreRef) {
+  } else if (aRefHandlingMode == nsIMsgMailNewsUrl::IGNORE_REF) {
     rv = (*_retval)->SetRef(EmptyCString());
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   return rv;
 }
 
 NS_IMETHODIMP nsMsgMailNewsUrl::Clone(nsIURI **_retval)
 {
-  return CloneInternal(eHonorRef, EmptyCString(), _retval);
+  return CloneInternal(nsIMsgMailNewsUrl::HONOR_REF, EmptyCString(), _retval);
 }
 
 NS_IMETHODIMP
 nsMsgMailNewsUrl::CloneIgnoringRef(nsIURI** _retval)
 {
-  return CloneInternal(eIgnoreRef, EmptyCString(), _retval);
+  return CloneInternal(nsIMsgMailNewsUrl::IGNORE_REF, EmptyCString(), _retval);
 }
 
 NS_IMETHODIMP
 nsMsgMailNewsUrl::CloneWithNewRef(const nsACString& newRef, nsIURI** _retval)
 {
-  return CloneInternal(eReplaceRef, newRef, _retval);
+  return CloneInternal(nsIMsgMailNewsUrl::REPLACE_REF, newRef, _retval);
 }
 
 NS_IMETHODIMP nsMsgMailNewsUrl::Resolve(const nsACString &relativePath, nsACString &result) 
 {
   // only resolve anchor urls....i.e. urls which start with '#' against the mailnews url...
   // everything else shouldn't be resolved against mailnews urls.
   nsresult rv = NS_OK;
 
--- a/mailnews/base/util/nsMsgMailNewsUrl.h
+++ b/mailnews/base/util/nsMsgMailNewsUrl.h
@@ -42,24 +42,17 @@ public:
     nsMsgMailNewsUrl();
 
     NS_DECL_THREADSAFE_ISUPPORTS
     NS_DECL_NSIMSGMAILNEWSURL
     NS_DECL_NSIURI
     NS_DECL_NSIURL
 
 protected:
-  enum RefHandlingEnum {
-    eIgnoreRef,
-    eHonorRef,
-    eReplaceRef
-  };
   virtual ~nsMsgMailNewsUrl();
-  nsresult CloneInternal(RefHandlingEnum aRefHandlingMode,
-                         const nsACString& newRef, nsIURI** _retval);
 
   nsCOMPtr<nsIURL> m_baseURL;
   nsWeakPtr m_statusFeedbackWeak;
   nsWeakPtr m_msgWindowWeak;
   nsWeakPtr m_loadGroupWeak;
   nsCOMPtr<nsIMimeHeaders> mMimeHeaders;
   nsCOMPtr<nsIMsgSearchSession> m_searchSession;
   nsCOMPtr<nsICacheEntryDescriptor> m_memCacheEntry;
--- a/mailnews/imap/src/nsImapUrl.cpp
+++ b/mailnews/imap/src/nsImapUrl.cpp
@@ -1165,29 +1165,31 @@ NS_IMETHODIMP nsImapUrl::SetMockChannel(
 
 NS_IMETHODIMP nsImapUrl::GetAllowContentChange(bool *result)
 {
   NS_ENSURE_ARG_POINTER(result);
   *result = m_allowContentChange;
   return NS_OK;
 }
 
-NS_IMETHODIMP nsImapUrl::Clone(nsIURI **_retval)
+NS_IMETHODIMP nsImapUrl::CloneInternal(uint32_t aRefHandlingMode,
+                                       const nsACString& newRef,
+                                       nsIURI** _retval)
 {
-  nsresult rv = nsMsgMailNewsUrl::Clone(_retval);
+  nsresult rv =
+    nsMsgMailNewsUrl::CloneInternal(aRefHandlingMode, newRef, _retval);
   NS_ENSURE_SUCCESS(rv, rv);
   // also clone the mURI member, because GetUri below won't work if
   // mURI isn't set due to escaping issues.
   nsCOMPtr <nsIMsgMessageUrl> clonedUrl = do_QueryInterface(*_retval);
   if (clonedUrl)
     clonedUrl->SetUri(mURI.get());
   return rv;
 }
 
-
 NS_IMETHODIMP nsImapUrl::SetUri(const char * aURI)
 {
   mURI= aURI;
   return NS_OK;
 }
 
 NS_IMETHODIMP nsImapUrl::GetUri(char** aURI)
 {
--- a/mailnews/imap/src/nsImapUrl.h
+++ b/mailnews/imap/src/nsImapUrl.h
@@ -24,17 +24,18 @@ class nsImapUrl : public nsIImapUrl, pub
 {
 public:
 
   NS_DECL_ISUPPORTS_INHERITED
 
   // nsIURI override
   NS_IMETHOD SetSpec(const nsACString &aSpec) override;
   NS_IMETHOD SetQuery(const nsACString &aQuery) override;
-  NS_IMETHOD Clone(nsIURI **_retval) override;
+  NS_IMETHOD CloneInternal(uint32_t aRefHandlingMode,
+                           const nsACString& newRef, nsIURI **_retval) override;
 
   //////////////////////////////////////////////////////////////////////////////
   // we support the nsIImapUrl interface
   //////////////////////////////////////////////////////////////////////////////
   NS_DECL_NSIIMAPURL
 
   // nsIMsgMailNewsUrl overrides
   NS_IMETHOD IsUrlType(uint32_t type, bool *isType) override;
--- a/mailnews/jsaccount/test/unit/resources/testJaFooUrlComponent.js
+++ b/mailnews/jsaccount/test/unit/resources/testJaFooUrlComponent.js
@@ -73,30 +73,28 @@ FooUrl.prototype = {
   getInterface: function(iid) {
     for (let iface of FooUrlProperties.extraInterfaces) {
       if (iid.equals(iface))
         return this;
     }
     return this.delegator.QueryInterface(iid);
   },
 
-    // nsIURI overrides
+    // nsIMsgMailNewsUrl overrides
 
-  // Override clone() to always make the pathname capitalized. This is useful
+  // Override cloneInternal() to always make the pathname capitalized. This is useful
   // to test that a method called from C++, but overridden in JS, gets the JS
-  // method, since nsMsgMailNewsUrl::CloneIgnoringRef calls Clone().
-  clone: function()
+  // method, since nsMsgMailNewsUrl::CloneIgnoringRef calls CloneInternal().
+  cloneInternal: function(aRefHandle, aRef)
   {
-    let uriClone = this.cppBase.clone();
+    let uriClone = this.cppBase.cloneInternal(aRefHandle, aRef);
     uriClone.path = uriClone.path.toUpperCase();
     return uriClone;
   },
 
-    // nsIMsgMailNewsUrl overrides
-
   // Override to allow setting from a JS variable.
   IsUrlType(aType) { return aType == this._urlType;},
 
     // msgIFooUrl implementation
 
   // Foo id for item.
   // attribute AString itemId;
   get itemId() { return this._itemId;},
--- a/mailnews/jsaccount/test/unit/test_fooUrl.js
+++ b/mailnews/jsaccount/test/unit/test_fooUrl.js
@@ -28,64 +28,88 @@ var tests = [
     for (let iface of extraInterfaces) {
       let fooUrl = url.getInterface(iface);
       Assert.ok(fooUrl instanceof iface);
       Assert.ok(fooUrl.QueryInterface(iface));
     }
   },
   function test_msgIOverride() {
     let url = newURL().QueryInterface(Ci.msgIOverride);
-    // test of access to wrapped JS object
+
+    // test of access to wrapped JS object.
+
+    // Access the ._hidden attribute using the XPCOM interface,
+    // where it is not defined.
     Assert.equal(typeof url.jsDelegate._hidden, "undefined");
+
+    // Get the JS object, where _hidden IS defined.
     Assert.equal(url.jsDelegate.wrappedJSObject._hidden, "IAmHidden");
   },
   function test_nsIURI() {
     let url = newURL().QueryInterface(Ci.nsIURI);
+    url instanceof Ci.nsIMsgMailNewsUrl; // so we see cloneInternal
 
-    // test attributes
+    // test methods that mostly use the baseURL in nsMsgMailNewsUrl
     Assert.ok("spec" in url);
     url.spec = "https://test.invalid/folder?isFoo=true&someBar=theBar";
     Assert.equal(url.host, "test.invalid");
 
-    // test non-attributes
+    // test another method from nsMsgMailNewsUrl.
     // url.resolve is overridden in nsMsgMailNewsUrl to only work if starts with "#"
     Assert.equal("https://test.invalid/folder?isFoo=true&someBar=theBar#modules",
                  url.resolve("#modules"));
 
     // Test JS override of method called virtually in C++.
-    // nsMsgMailNewsUrl::CloneIgnoringRef calls Clone(). We overrode the JS to
-    // capitalize the path.
+    // We overrode cloneInteral in JS to capitalize the path.
     url.spec = "https://test.invalid/folder#modules";
-    Assert.equal(url.clone().spec, "https://test.invalid/FOLDER#MODULES");
+    Assert.equal(url.cloneInternal(Ci.nsIMsgMailNewsUrl.HONOR_REF, null).spec,
+                 "https://test.invalid/FOLDER#MODULES");
+
+    // But then it gets tricky.
+    // This is not overridden, so you are calling JaBaseCppUrl::CloneIgnoringRef
+    // which inherits from nsMsgMailNewsUrl::CloneIgnoringRef(). So why is the path
+    // capitalized? Because nsMsgMailNewsUrl::CloneIgnoringRef calls the virtual
+    // method CloneInternal(), which IS overridden to give the capitalized value,
+    // showing polymorphic behavior.
     Assert.equal(url.cloneIgnoringRef().spec, "https://test.invalid/FOLDER");
 
     // Demo of differences between the various versions of the object. The
     // standard XPCOM constructor returns the JS implementation, as was tested
-    // above. Try the same tests using the JS delegate, which should give the
-    // same (overridden) results (overridden for clone, C++ class
-    // for cloneIgnoringRef.
+    // above. Try the same tests using the wrapped JS delegate, which should give
+    // the same (overridden) results (overridden for cloneInternal, C++ class
+    // for cloneIgnoringRef with polymorphic override).
     let jsDelegate = url.QueryInterface(Ci.msgIOverride).jsDelegate.wrappedJSObject;
-    Assert.equal(jsDelegate.clone().spec, "https://test.invalid/FOLDER#MODULES");
+    Assert.equal(jsDelegate.cloneInternal(Ci.nsIMsgMailNewsUrl.HONOR_REF, null).spec,
+                                          "https://test.invalid/FOLDER#MODULES");
     Assert.equal(jsDelegate.cloneIgnoringRef().spec, "https://test.invalid/FOLDER");
 
-    // Not sure why you would want to do this, but you call also call the C++
-    // object that does delegation, and get the same result. This is actually
-    // what we expect C++ callers to see.
-    let cppDelegator = jsDelegate.delegator.QueryInterface(Ci.nsIURI);
-    Assert.equal(cppDelegator.clone().spec, "https://test.invalid/FOLDER#MODULES");
-    Assert.equal(cppDelegator.cloneIgnoringRef().spec, "https://test.invalid/FOLDER");
+    // The cppBase object will not have the overrides. cppBase is from
+    // JaCppUrlDelegator::GetCppBase which returns an instance of the Super()
+    // object. This instance will always call the C++ objects and never the
+    // JS objects.
+    let cppBase = url.QueryInterface(Ci.msgIOverride).cppBase.QueryInterface(Ci.nsIURI);
+    cppBase instanceof Ci.nsIMsgMailNewsUrl; // so it sees cloneInternal
 
-    // The cppBase object will not have the overrides.
-    let cppBase = url.QueryInterface(Ci.msgIOverride).cppBase.QueryInterface(Ci.nsIURI); 
-    Assert.equal(cppBase.clone().spec, "https://test.invalid/folder#modules");
+    // nsMsgMailNewsUrl::CloneInternal does gives the normal, lower-case spec.
+    Assert.equal(cppBase.cloneInternal(Ci.nsIMsgMailNewsUrl.HONOR_REF, null).spec,
+                 "https://test.invalid/folder#modules");
 
-    // But then it gets tricky. We can call cloneIgnoringRef in the C++ base
-    // but it will use the virtual clone which is overridden.
+    // But again, when calling a C++ class we get the polymorphic behavior.
     Assert.equal(cppBase.cloneIgnoringRef().spec, "https://test.invalid/FOLDER");
 
+    // Not sure why you would want to do this, but you could also call the C++
+    // object that does delegation, and get the same result. That is, call
+    // JaCppUrlDelegator:: classes using the forwarding macros to either a JS
+    // class or a C++ class. This is actually what we expect C++ callers to see,
+    // so this matches what we see above.
+    let cppDelegator = jsDelegate.delegator.QueryInterface(Ci.nsIURI);
+
+    Assert.equal(cppDelegator.cloneInternal(Ci.nsIMsgMailNewsUrl.HONOR_REF, null).spec,
+                                            "https://test.invalid/FOLDER#MODULES");
+    Assert.equal(cppDelegator.cloneIgnoringRef().spec, "https://test.invalid/FOLDER");
   },
   function test_nsIURL() {
     let url = newURL().QueryInterface(Ci.nsIURL);
     Assert.ok("filePath" in url);
     url.spec = "https://test.invalid/folder?isFoo=true&someBar=theBar";
     Assert.equal(url.query, "isFoo=true&someBar=theBar");
     // Note that I tried here to test getCommonSpec, but that does not work
     // because nsStandardURL.cpp makes an assumption that the URL is directly
--- a/mailnews/local/src/nsMailboxUrl.cpp
+++ b/mailnews/local/src/nsMailboxUrl.cpp
@@ -125,19 +125,22 @@ nsresult nsMailboxUrl::SetMessageSize(ui
 }
 
 NS_IMETHODIMP nsMailboxUrl::SetUri(const char * aURI)
 {
   mURI= aURI;
   return NS_OK;
 }
 
-NS_IMETHODIMP nsMailboxUrl::Clone(nsIURI **_retval)
+NS_IMETHODIMP nsMailboxUrl::CloneInternal(uint32_t aRefHandlingMode,
+                                          const nsACString& newRef,
+                                          nsIURI **_retval)
 {
-  nsresult rv = nsMsgMailNewsUrl::Clone(_retval);
+  nsresult rv = nsMsgMailNewsUrl::CloneInternal(aRefHandlingMode,
+                                                newRef, _retval);
   NS_ENSURE_SUCCESS(rv, rv);
   // also clone the mURI member, because GetUri below won't work if
   // mURI isn't set due to nsIFile fun.
   nsCOMPtr <nsIMsgMessageUrl> clonedUrl = do_QueryInterface(*_retval);
   if (clonedUrl)
     clonedUrl->SetUri(mURI.get());
   return rv;
 }
--- a/mailnews/local/src/nsMailboxUrl.h
+++ b/mailnews/local/src/nsMailboxUrl.h
@@ -56,17 +56,19 @@ public:
   {
     m_curMsgIndex = aIndex;
     return NS_OK;
   }
 
   NS_IMETHOD GetFolder(nsIMsgFolder **msgFolder);
 
   // nsIMsgMailNewsUrl override
-  NS_IMETHOD Clone(nsIURI **_retval) override;
+  NS_IMETHOD CloneInternal(uint32_t aRefHandlingMode,
+                           const nsACString& newRef,
+                           nsIURI **_retval) override;
 
   // nsMailboxUrl
   nsMailboxUrl();
   NS_DECL_NSIMSGMESSAGEURL
   NS_DECL_ISUPPORTS_INHERITED
   NS_DECL_NSIMSGI18NURL
 
 protected:
--- a/mailnews/news/src/nsNntpUrl.cpp
+++ b/mailnews/news/src/nsNntpUrl.cpp
@@ -548,20 +548,22 @@ NS_IMETHODIMP nsNntpUrl::GetCharsetOverR
 }
 
 NS_IMETHODIMP nsNntpUrl::SetCharsetOverRide(const char * aCharacterSet)
 {
   mCharsetOverride = aCharacterSet;
   return NS_OK;
 }
 
-NS_IMETHODIMP nsNntpUrl::Clone(nsIURI **_retval)
+NS_IMETHODIMP nsNntpUrl::CloneInternal(uint32_t aRefHandlingMode,
+                                       const nsACString& newRef,
+                                       nsIURI **_retval)
 {
   nsresult rv;
-  rv = nsMsgMailNewsUrl::Clone(_retval);
+  rv = nsMsgMailNewsUrl::CloneInternal(aRefHandlingMode, newRef, _retval);
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsCOMPtr<nsIMsgMessageUrl> newsurl = do_QueryInterface(*_retval, &rv);
   NS_ENSURE_SUCCESS(rv, rv);
 
   return newsurl->SetUri(mURI.get());
 }
 
--- a/mailnews/news/src/nsNntpUrl.h
+++ b/mailnews/news/src/nsNntpUrl.h
@@ -21,17 +21,18 @@ public:
   // nsIURI over-ride...
   NS_IMETHOD SetSpec(const nsACString &aSpec);
 
   NS_IMETHOD IsUrlType(uint32_t type, bool *isType);
 
   // nsIMsgMailNewsUrl overrides
   NS_IMETHOD GetServer(nsIMsgIncomingServer **server);
   NS_IMETHOD GetFolder(nsIMsgFolder **msgFolder);
-  NS_IMETHOD Clone(nsIURI **_retval);
+  NS_IMETHOD CloneInternal(uint32_t aRefHandlingMode,
+                           const nsACString& newRef,nsIURI **_retval);
 
   // nsNntpUrl
   nsNntpUrl();
 
   NS_DECL_ISUPPORTS_INHERITED
 
 private:
   virtual ~nsNntpUrl();