668680 - Implementing the URL APIs

Status: RESOLVED DUPLICATE of bug 887364

(Core :: DOM: Core & HTML, defect)

Description

[Shawn Gong]

Attachment: patch 1; array-related APIs and attributes not implemented yet

Implementing the MozURL APIs according to: https://docs.google.com/document/edit?id=1r_VTFKApVOaNIkocrg0z-t7lZgzisTuGTXkdzAk4gLU&hl=en

Comment 1

[Shawn Gong]

Comment on attachment 543306 [details] [diff] [review]
patch 1; array-related APIs and attributes not implemented yet

Hey Jonas take a brief look first before we move on :)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 543306 [details] [diff] [review]
patch 1; array-related APIs and attributes not implemented yet

>+  aProtocol = NS_ConvertUTF8toUTF16(str);

  CopyUTF8toUTF16(str, aProtocol);

this is in lots of places.

>+  int seperator = 0;

"separator".  And why are you using BeginWriting instead of just using Find() on the string?

In general, I'd much rather we fixed SetHostPort on nsStandardURL (even using this code!) than work around it not working.

GetHost should just GetHostPort on the URI.

The SetPort here doesn't match the behavior of nsLocation::SetPort (not really the spec, but that's a separate issue).  Either nsLocation or the spec need to change; can you file a bug to investigate which?

SetPathName and GetPathName should probably use the filePath getter/setter, no?

GetSearch is pretty clearly not per spec; it needs to prepend '?'.

Similar for GetHash.

In general, it's worth sharing as much code as possible with nsLocation here, since per spec these are supposed to behave identically.

Comment 3

[Shawn Gong]

Thanks Boris for the suggestions - I'll look into them

Comment 4

[Shawn Gong]

Hi Boris: first thing I found is this spec which doesn't want the '?' or '#': http://www.whatwg.org/specs/web-apps/current-work/#url-decomposition-idl-attributes

Comment 5

[Boris Zbarsky [:bzbarsky]]

Per that spec, 

  On getting, if the input is an absolute URL that fulfills the condition given
  in the "getter condition" column corresponding to the attribute in the table
  below, the user agent must return the part of the input URL given in the
  "component" column, with any prefixes specified in the "prefix" column
  appropriately added to the start of the string and any suffixes specified in
  the "suffix" column appropriately added to the end of the string. 

The prefix column contains '?' for "search" and '#' for "hash".

Comment 6

[Shawn Gong]

Ah I see - thanks Boris!

Comment 7

[Shawn Gong]

Attachment: Implemented the methods; test page is very elementary yet

The comments per Boris are still under discussion between Boris and Jonas thus nothing has changed on that part.

Other APIs are all implemented; however, the test page is still very basic for now.

Comment 8

[Shawn Gong]

Attachment: Features all implemented; fixed prefix and Find() issues pointed out by Boris

For the nsLocation sharing issue, more discussions will carry on

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 545994 [details] [diff] [review]
Features all implemented; fixed prefix and Find() issues pointed out by Boris

>--- a/content/base/public/Makefile.in
>+++ b/content/base/public/Makefile.in
> 		nsIDOMFile.idl \
> 		nsIDOMFileReader.idl \
>+		nsIDOMMozURL.idl \
> 		nsIDOMFileList.idl \
> 		nsIDOMFileException.idl \

Weird place.

>--- /dev/null
>+++ b/content/base/public/nsIDOMMozURL.idl
>+ //TODO:stringifier?

Can do that by adding DOMString toString();

>+ attribute DOMString href;

>--- /dev/null
>+++ b/content/base/src/nsDOMMozURL.cpp

>+#include "nsDOMClassInfo.h"
>+#include "nsDOMMozURL.h"

This one first.

>+nsDOMMozURL::Initialize(nsISupports* aOwner,
>+                        JSContext* aCx,
>+                        JSObject* aObj,
>+                        PRUint32 aArgc,
>+                        jsval* aArgv)
>+{
>+  NS_ENSURE_TRUE(aArgc, NS_ERROR_XPC_NOT_ENOUGH_ARGS);
>+  if (aArgc > 0) {

You don't need both checks, do you? You need to handle the aArgc == 0 case per spec.

>+    nsresult rv;
>+    nsCOMPtr<nsIURI> URI;
>+    nsString plainURL, plainBaseURL;
>+    JsvalToString(aCx, aArgv[0], plainURL);

plainBaseURL should be declared inside the branch.

>+
>+    if (aArgc == 2) {  // Both plain & base URLs are provided
>+      JsvalToString(aCx, aArgv[0], plainBaseURL);

aArgv[1]. Why didn't your tests catch that?

>+      rv = NS_NewURI(getter_AddRefs(URI), 
>+                       plainURL, nsnull, baseURI);

Indentation.

>+    }
>+    else {  // Only the plain URL is provided

} else {

>+nsDOMMozURL::JsvalToString(JSContext* aCx,
>+                           jsval aVal,
>+                           nsString& aResult)
>+{
>+  aResult.Assign(depStr);

Is that copy necessary?

>+nsDOMMozURL::CollectParameters()
>+{
>+  // Get rid of the prefix "?" mark
>+  query = Substring(query, 1, query.Length()-1);

"query.Length() - 1" (note spaces). Is that argument necessary, though?

>+
>+  PRInt32 delimiter = -1;
>+  PRInt32 next = query.FindChar('&');
>+  while (true) {
>+    if (next == -1) { // Means we got to the last parameter

I don't know if we guarantee that it's -1 and not an arbitrary negative value. Also elsewhere.

>+    nsString parameter;
>+    parameter = Substring(query, delimiter+1, next-delimiter-1);

Spaces, and this should be one line.

>+    if (!parameter.IsEmpty()) {
>+      PRInt32 equalSign = parameter.FindChar('=');
>+      if (equalSign <= -1) {

< 0

>+      }
>+      else {

} else {. Check the rest of the patch yourself, please.

>+        nsString name;
>+        name = Substring(parameter, 0, equalSign);

One line.

>+        nsString value;
>+        value = Substring(parameter, equalSign+1,
>+                                   parameter.Length()-equalSign+1);

One line, spaces.

>+    next = query.FindChar('&', delimiter+1);

Spaces.

>+nsDOMMozURL::SerializeParameters(nsAString& aResult)
>+{
>+  nsString result;
>+  for (PRUint32 i = 0; i < mParameters.Length(); i++) {

The compiler might not be smart enough, so please call Length() only once.

>+    nsString s;
>+    s = mParameters.ElementAt(i).name;

.

>+    if (!mParameters.ElementAt(i).value.IsEmpty()) {
>+      s += PRUnichar('=');
>+      s += mParameters.ElementAt(i).value;

Looks like you want a local for mParameters.ElementAt(i).

>+  aResult = Substring(result, 0, result.Length()-1);

.

>+nsDOMMozURL::StringArrayToJsval(JSContext* aCx,
>+                                nsTArray<nsString>& aArray,
>+                                jsval* aResult)
>+{
>+  JSAutoRequest ar(aCx);
>+
>+  JSObject* array = JS_NewArrayObject(aCx, 0, NULL);
>+  if (!array) {
>+    NS_WARNING("Failed to make array!");
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

s/make/create/ or NS_ENSURE_TRUE(array, NS_ERROR_OUT_OF_MEMORY);

>+    if (!JS_SetArrayLength(aCx, array, jsuint(aArray.Length()))) {
>+      NS_WARNING("Failed to set array length!");
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }

And here.

>+    jsint count = jsint(aArray.Length());

Does this need to be signed?

>+    for (jsint index = 0; index < count; index++) {

I'd use "i".

>+      nsString name = aArray[index];

Would nsString& work?

>+// methods

That's clear. If you want to put something here, what about "nsIDOMMozURL implementation"?

>+nsDOMMozURL::GetParameterNames(JSContext* aCx, jsval* aResult)
>+{
>+  for (PRUint32 i = 0; i < mParameters.Length(); i++) {

Same here about Length().

>+    if (!names.Contains(mParameters.ElementAt(i).name)) {
>+      names.AppendElement(mParameters.ElementAt(i).name);
>+    }
>+  }

Might be worth looking for an implementation that isn't O(n^2) here.

>+nsDOMMozURL::GetParameter(const nsAString& aName,
>+                          JSContext* aCx,
>+                          nsAString& aResult)
>+{
>+  // Reversely traverse paramters to find the last occurance

"Traverse the parameters backwards, to find the last occurrence.", or something else that pleases a spelling checker.

>+  for (PRInt32 i = mParameters.Length()-1; i >= 0; i--) {

Spaces.

>+nsDOMMozURL::AppendParameter(const nsAString& aName,
>+                             const jsval& aValue,
>+                             JSContext* aCx)
>+{
>+  if (JSVAL_IS_STRING(aValue)) {
>+    ParameterTuple tuple((nsString)aName, value);

nsString(aName)

>+  }
>+  else {  // Should have got an array of parameter values
>+    JSObject *obj = JSVAL_TO_OBJECT(aValue);
>+    NS_ASSERTION(JS_IsArrayObject(aCx, obj), 
>+      "Append Parameter must be either string or array");

You can't do this. We can't have assertions being tickled by random websites.

Figure out whether this should be DOMString[] or sequence<DOMString> and implement the WebIDL requirements.

And check the entire patch for trailing spaces.

>+    jsuint len;
>+    if(!JS_GetArrayLength(aCx, obj, &len)) {

if (

>+nsDOMMozURL::GetProtocol(nsAString &aProtocol)
>+{
>+  nsCString str;
>+  mURL->GetScheme(str);
>+  aProtocol = NS_ConvertUTF8toUTF16(str);

CopyUTF8toUTF16. Same below. And better names than "str", please.

>+NS_IMETHODIMP
>+nsDOMMozURL::SetPort(const nsAString &aPort)
>+{
>+  nsresult rv;
>+  PRInt32 port = PromiseFlatString(aPort).ToInteger(&rv) ;

Fix that space.

>+nsDOMMozURL::GetPathname(nsAString &aPathname)
>+{
>+  if (questionSign == hashSign) {
>+  }
>+  else {
>+    if (questionSign == -1) {
>+    }
>+    else if (hashSign == -1) {
>+    }
>+    else {
>+      PRInt32 delimiter = (questionSign < hashSign)?
>+                      questionSign: hashSign;

NS_MIN

And this code flow looks strange.

>+nsDOMMozURL::GetSearch(nsAString &aSearch)
>+{
>+    if ((hashSign == -1) ||
>+        (questionSign < hashSign)) {

No need to over-brace here.

>+NS_IMETHODIMP
>+nsDOMMozURL::GetHash(nsAString &aHash)
>+{
>+  bool hasHash = (href.Find("#") == -1)?
>+                 false: true;

bool hasHash = href.Find("#") >= 0;

but inline that check in the if.

>+nsDOMMozURL::GetOrigin(nsAString &aOrigin)
>+{
>+  nsString origin;
>+  nsresult rv = nsContentUtils::GetUTFOrigin(mURL,
>+                                             origin);
>+  aOrigin = origin;

This is silly. Why can't GetUTFOrigin take nsAString&?

>--- /dev/null
>+++ b/content/base/src/nsDOMMozURL.h
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation

"the Mozilla Foundation."

Check the rest of your patch.

>+ * Portions created by the Initial Developer are Copyright (C) 2007

2007? Same here.

>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ * Shawn Gong <[e-mail]>

The first letter of your name should be under the "n".

>+#ifndef nsDOMMozURL_h__
>+#define nsDOMMozURL_h__

No trailing underscores; these identifiers are reserved.

>+
>+#include "nsIDOMMozURL.h"
>+
>+#include "nsISupportsUtils.h"
>+#include "nsTArray.h"
>+#include "nsIJSNativeInitializer.h"
>+#include "nsNetUtil.h"
>+#include "nsJSUtils.h"

At least the latter two includes should be in the cpp. Please only include what you *need* here.

>+
>+class nsDOMMozURL : public nsIDOMMozURL,
>+                    public nsIJSNativeInitializer
>+{
>+  struct ParameterTuple
>+  {
>+    nsString name;
>+    nsString value;

mName, mValue.

>+#endif

#endif // nsDOMMozURL_h

>+

No need for this line.

>--- a/content/base/test/Makefile.in
>+++ b/content/base/test/Makefile.in
> _TEST_FILES1 = 	test_bug5141.html \
>+		test_mozURL.html \

If you want to put this here, do it like

_TEST_FILES1 = \
		test_mozURL.html \
		test_bug5141.html \
		test_bug51034.html \

>--- /dev/null
>+++ b/content/base/test/test_mozURL.html
>+var url, r;
>+
>+// Invalid constructor should raise an exception
>+var gotException = new Boolean(false);

= false;

>+try {
>+  url = new MozURL();
>+}
>+catch (e) {
>+  gotException = true;
>+}
>+is(gotException, true, "should got an exception");

if(gotException, false)

>+gotExcpetion = false;

typo

>+try {
>+  url = new MozURL("www.abc.com");
>+}
>+catch (e) {
>+  gotException = true;
>+}
>+is(gotException, true, "should got an exception");

Why?

>+//---
>+//--- Helpers
>+
>+function areArraysEqual(a, b)
>+{
>+  if (!a) {
>+    if (!b) {
>+      return true;
>+    }
>+    else {
>+      return false;
>+    }
>+  }
>+  if (!b) {
>+    return false;
>+  }
>+
>+  if (a.length != b.length) {
>+    return false;
>+  }
>+
>+  for (var i = 0; i < a.length; i++) {
>+    if (a[i] != b[i]) {
>+      return false;
>+    }
>+  }
>+  return true;
>+}

This is reusable, please put it somewhere central?

>+
>+// Unit tests for helpers
>+var a = [2,3,4,5];
>+var b = [2,3,4,5];
>+is(areArraysEqual(a,b), true, "array of ints");
>+a = ["a","b"];
>+b = ["a","b"];
>+is(areArraysEqual(a,b), true, "array of strings");
>+a = ["a", 2];
>+b = [3,4];
>+is(areArraysEqual(a,b), false, "different arrays of combined");
>+a = ["a", 2];
>+b = ["a", 2];
>+is(areArraysEqual(a,b), true, "array of combind");

Typo. And these should go in another file.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And the spec wants this to be the same as window.URL, AIUI. That's silly, and we should get the spec changed.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Here is my proposal for sharing code with nsLocation:

1. Find places where nsLocation and MozURL behaves differently for a
   getter/setter and decide which behavior we want. Align the other to that
   behavior.
2. Make nsLocation::GetURI/GetWriteableURI/SetURI deal with MozURL objects
   rather than nsIURI objects.
3. Change the various getters and setter to use methods on MozURL rather than
   nsIURI.

Does that sound ok?

When 1 means changing nsLocation we should do that as a separate patch which should land ASAP.


As for SetHostPort, it appears that all implementations of SetHostPort are broken. There is only one caller of it which is nsLocation::SetHost, which presumably is broken too.

We can either fix it, or make hostPort be readonly in the idl. I don't have a strong preference for which.

Comment 12

[Shawn Gong]

Hi Ms2ger, thanks for the detailed review!
Where do you think I should put the array comparison helper?
Also I didn't get what you meant by Comment 10, "spec wants this to be same as window.URL" - could you elaborate?

Boris, how do you think about Jonas' proposal?

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #12)
> Where do you think I should put the array comparison helper?

SimpleTest (testing/mochitest/tests/SimpleTest/SimpleTest.js), maybe?

> Also I didn't get what you meant by Comment 10, "spec wants this to be same
> as window.URL" - could you elaborate?

See <http://dev.w3.org/2006/webapi/FileAPI/#creating-revoking>. No need to change your patch here, though.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #13)
> > Also I didn't get what you meant by Comment 10, "spec wants this to be same
> > as window.URL" - could you elaborate?
> 
> See <http://dev.w3.org/2006/webapi/FileAPI/#creating-revoking>. No need to
> change your patch here, though.

This is quite intentional. In fact, the URL object existed before we decided to put the creating/revoking API there. In any case, if it's something you don't agree with please do raise the issue on the webapps list.

Comment 15

[Shawn Gong]

Comparisons between nsLocation and MozURL:

           nsLocation                                        MozURL
GetHash  Unescape thing (need more discussion)
SetHash  Useless inserting “#” in front (underlying 
         nsIURI already does that)
GetHost  Same
SetHost  Using nsIURI->SetHostPort, which won't work    Works by calling SetHostname
                                                        and SetPort seperately
GetHostname Same
SetHostname Same

GetHref  Same
SetHref  Gets context first then SetHrefWithContext     Directly set href from 
         or using “oldHref”?                            incoming parameter
GetPathname Using nsIURL->GetFilePath                   Manually got pathname;   
                                                        will apply nsLocation way
SetPathname SetPath and get rid of the original hash    Only set the pathname and
            and search                                  still keep hash & search;   
                                                        will apply nsLocation way
GetPort  Same
SetPort  Same
GetProtocol Append “:” after                            Didn't append “:”; should 
                                                        apply the nsLocation way
SetProtocol Same
GetSearch   Simpler way                                 Complicated; should apply 
                                                        the nsLocation way
SetSearch Same


Please add your thoughts

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please file a separate bug on nsLocation::GetHash. IE/Chrome/Safari/Opera all do not do the unescaping that we do so we should likely change our behavior. But it needs to happen in a separate bug that we should land ASAP.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Please file a separate bug on nsLocation::GetHash.

That's bug 483304.  It can't land "ASAP" unless we also fix a bunch of other related stuff ASAP (see bug 483304 comment 21)....

> SetHash  Useless inserting “#” in front

It's not quite useless.  Or rather, inserting it when setting to something nonempty not starting with '#' is useless, but inserting it for an empty string does mean something.

This file:

  <script>
    location.hash = "";
  </script>

changes the location URI in Gecko and Presto but not WebKit.  Getting that behavior requires the '#' insertion for empty string, because SetRef(emptystring) will remove the ref component completely.  I believe the spec at http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#url-decomposition-idl-attributes in fact requires the Gecko/Presto behavior, thought it's a bit unclear on it.  Might be worth confirming and getting the spec clarified.

> SetHost

We should just implement SetHostPort.  Worst case, by moving the logic you wrote here to nsStandardURL::SetHostPort.

> Gets context first then SetHrefWithContext 

That's just because for non-absolute hrefs you need to deal with base URIs and the like.  Which the attached patch fails to do, btw....

> SetPathname

Hmm.  The spec seems to say this should not blow away the hash and search.  What do other browsers do?

Comment 18

[Shawn Gong]

Attachment: Multiple changes; details in comment #19

Comment 19

[Shawn Gong]

Attachment: Multiple changes; details in comment #20

Comment 20

[Shawn Gong]

Firstly many thanks to Ms2ger, Boris and Jonas.

Most of Ms2ger's suggestions are taken. Here is a list of suggestions not taken:
> >+ //TODO:stringifier?
> Can do that by adding DOMString toString();
khuey told me the support for stringifier is in progress, so if it's not very important to work around right now, we can wait for that.

> You need to handle the aArgc == 0 case
> per spec.
What should we do if no URL is provided at initialization? Waiting for a href to be set? I didn't find specific details from spec..

> >+nsDOMMozURL::JsvalToString(JSContext* aCx,
> >+                           jsval aVal,
> >+                           nsString& aResult)
> >+{
> >+  aResult.Assign(depStr);
> 
> Is that copy necessary?
It seems we have to convert around JSString and nsString - correct me if it's not the case.

> >+    nsString parameter;
> >+    parameter = Substring(query, delimiter+1, next-delimiter-1);
> 
> Spaces, and this should be one line.
It appears the nsString disabled the copy constructor so we have to make it 2 lines - let me know if there're other ways around.

> >+      }
> >+      else {
> 
> } else {. Check the rest of the patch yourself, please.
The separate lined "else" is preferred style by sicking and bent.

> >+  for (PRUint32 i = 0; i < mParameters.Length(); i++) {
> 
> The compiler might not be smart enough, so please call Length() only once.
khuey said the compiler is smart enough, and this is the preferred style.
  
> >+    if (!names.Contains(mParameters.ElementAt(i).name)) {
> >+      names.AppendElement(mParameters.ElementAt(i).name);
> >+    }
> >+  }
> 
> Might be worth looking for an implementation that isn't O(n^2) here.
The more efficient algorithms come into my mind would consists of sorting, but we want to keep the order of the parameter names; also, typically there won't be more than 20 parameters, so it might not bring big impact.
Please share your ideas :)
 
> >+nsDOMMozURL::GetOrigin(nsAString &aOrigin)
> >+{
> >+  nsString origin;
> >+  nsresult rv = nsContentUtils::GetUTFOrigin(mURL,
> >+                                             origin);
> >+  aOrigin = origin;
> 
> This is silly. Why can't GetUTFOrigin take nsAString&?
Workaround is  nsContentUtils::GetUTFOrigin(mURL,
                                            (nsString&)aOrigin);

> >+
> >+#include "nsIDOMMozURL.h"
> >+
> >+#include "nsISupportsUtils.h"
> >+#include "nsTArray.h"
> >+#include "nsIJSNativeInitializer.h"
> >+#include "nsNetUtil.h"
> >+#include "nsJSUtils.h"
> 
> At least the latter two includes should be in the cpp. Please only include
> what you *need* here.
Moved nsJSUtils.h to the cpp; the nsNetUtil.h is needed for the nsIURL.

> 
> >+try {
> >+  url = new MozURL("www.abc.com");
> >+}
> >+catch (e) {
> >+  gotException = true;
> >+}
> >+is(gotException, true, "should got an exception");
> 
> Why?
URL without a scheme is invalid.


Per Boris' comment:
> > SetHash  Useless inserting “#” in front
> 
> It's not quite useless.  Or rather, inserting it when setting to something
> nonempty not starting with '#' is useless, but inserting it for an empty
> string does mean something.
Now the MozURL SetHash the same way nsLocation does it. From spec can't get the clear result, will email whatwg.org.

> > SetHost
> 
> We should just implement SetHostPort.  Worst case, by moving the logic you
> wrote here to nsStandardURL::SetHostPort.
Implemented nsStandardURL::SetHostPort.

> > Gets context first then SetHrefWithContext 
> 
> That's just because for non-absolute hrefs you need to deal with base URIs
> and the like.  Which the attached patch fails to do, btw....
Now the SetHref deals with 3 types of URLs: absolute URL, anchor URL (start with '#') and relative URL. (it looks for "://" to identify absolute URL, which might be naive - please share your thought).

Since the nsLocation uses window context to find the base URL, it might not share this function with MozURL.

> > SetPathname
> 
> Hmm.  The spec seems to say this should not blow away the hash and search. 
> What do other browsers do?
They keep the hash and search, so I kept the MozURL way as it is.


I appreciate your time and please share your ideas!

Comment 21

[Boris Zbarsky [:bzbarsky]]

> please share your thought

What's the behavior we're trying to implement?  I'd like to get that pinned down before looking at the code.  (In any case, looking for "://" is definitely wrong.  Think "about:blank" as a trivial example.)

> They keep the hash and search, so I kept the MozURL way as it is.

And made nsLocation match it, I assume?

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #20)
> > >+ //TODO:stringifier?
> > Can do that by adding DOMString toString();
> khuey told me the support for stringifier is in progress, so if it's not
> very important to work around right now, we can wait for that.

OK.

> > You need to handle the aArgc == 0 case
> > per spec.
> What should we do if no URL is provided at initialization? Waiting for a
> href to be set? I didn't find specific details from spec..

Hmm. Looks like a spec bug; could you email whatwg about it?

> > >+      }
> > >+      else {
> > 
> > } else {. Check the rest of the patch yourself, please.
> The separate lined "else" is preferred style by sicking and bent.

They should know that the style guide requires } else { on one line.

> > >+  for (PRUint32 i = 0; i < mParameters.Length(); i++) {
> > 
> > The compiler might not be smart enough, so please call Length() only once.
> khuey said the compiler is smart enough, and this is the preferred style.

OK.

> > >+    if (!names.Contains(mParameters.ElementAt(i).name)) {
> > >+      names.AppendElement(mParameters.ElementAt(i).name);
> > >+    }
> > >+  }
> > 
> > Might be worth looking for an implementation that isn't O(n^2) here.
> The more efficient algorithms come into my mind would consists of sorting,
> but we want to keep the order of the parameter names; also, typically there
> won't be more than 20 parameters, so it might not bring big impact.
> Please share your ideas :)

Couldn't think of anything myself, I just hoped that you would. I guess it's fine, though.

>  
> > >+nsDOMMozURL::GetOrigin(nsAString &aOrigin)
> > >+{
> > >+  nsString origin;
> > >+  nsresult rv = nsContentUtils::GetUTFOrigin(mURL,
> > >+                                             origin);
> > >+  aOrigin = origin;
> > 
> > This is silly. Why can't GetUTFOrigin take nsAString&?
> Workaround is  nsContentUtils::GetUTFOrigin(mURL,
>                                             (nsString&)aOrigin);

That looks even worse. Could you file a followup to figure you if we can change the signature, though?

> > >+
> > >+#include "nsIDOMMozURL.h"
> > >+
> > >+#include "nsISupportsUtils.h"
> > >+#include "nsTArray.h"
> > >+#include "nsIJSNativeInitializer.h"
> > >+#include "nsNetUtil.h"
> > >+#include "nsJSUtils.h"
> > 
> > At least the latter two includes should be in the cpp. Please only include
> > what you *need* here.
> Moved nsJSUtils.h to the cpp; the nsNetUtil.h is needed for the nsIURL.

OK, thanks.

> > 
> > >+try {
> > >+  url = new MozURL("www.abc.com");
> > >+}
> > >+catch (e) {
> > >+  gotException = true;
> > >+}
> > >+is(gotException, true, "should got an exception");
> > 
> > Why?
> URL without a scheme is invalid.

Er, yes, I guess it would be.

Thanks for your thoughtful reply!

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 547855 [details] [diff] [review]
Multiple changes; details in comment #20

And a couple more nits:

>--- /dev/null
>+++ b/content/base/public/nsIDOMMozURL.idl
>+[scriptable, uuid(7e5011ba-7f11-484f-8da1-860edb56cdb3)]
>+
>+interface nsIDOMMozURL : nsISupports

Remove that blank line, and use two-space indentation below.

>--- /dev/null
>+++ b/content/base/src/nsDOMMozURL.cpp
>+
>+#include "nsDOMClassInfo.h"
>+#include "nsDOMMozURL.h"

Include this first.

>+#include "nsJSUtils.h"
>+#include "xpcprivate.h"
>+#include "xpcquickstubs.h"
>+#include "CharTokenizer.h"

Add 'using namespace mozilla;' after the includes here.

>+  mozilla::CharTokenizer<'&'> tokenizer(query);

And make this just 'CharTokenizer<'&'> tokenizer(query);'.

>+nsDOMMozURL::StringArrayToJsval(JSContext* aCx,
>+                                nsTArray<nsString>& aArray,
>+                                jsval* aResult)
>+{
>+    nsresult rv = JS_SetArrayLength(aCx, array,
>+                                    jsuint(aArray.Length()));

I'm pretty sure this doesn't return nsresult.

And use nsAString& foo instead of nsAString &foo throughout.

Comment 24

[Shawn Gong]

Attachment: Multiple changes; details in comment #25

Comment 25

[Shawn Gong]

(In reply to comment #21)
> > please share your thought
> 
> What's the behavior we're trying to implement?  I'd like to get that pinned
> down before looking at the code.  (In any case, looking for "://" is
> definitely wrong.  Think "about:blank" as a trivial example.)

Talked with Jonas and Adam Barth (author of the URL spec linked above), and we decided to only support absolute URL or hash with SetHref right now. The SetHref will create a new underlying URI object, because different protocols might need different implementations; as a result an invalid URL will result in an error.

> > They keep the hash and search, so I kept the MozURL way as it is.
> 
> And made nsLocation match it, I assume?

Yes.


(In reply to comment #22)
> > > You need to handle the aArgc == 0 case
> > > per spec.
> > What should we do if no URL is provided at initialization? Waiting for a
> > href to be set? I didn't find specific details from spec..
> 
> Hmm. Looks like a spec bug; could you email whatwg about it?

Emailed whatwg and Adam Barth corrected the spec - now the first argument is mandatory.

> > > >+nsDOMMozURL::GetOrigin(nsAString &aOrigin)
> > > >+{
> > > >+  nsString origin;
> > > >+  nsresult rv = nsContentUtils::GetUTFOrigin(mURL,
> > > >+                                             origin);
> > > >+  aOrigin = origin;
> > > 
> > > This is silly. Why can't GetUTFOrigin take nsAString&?
> > Workaround is  nsContentUtils::GetUTFOrigin(mURL,
> >                                             (nsString&)aOrigin);
> 
> That looks even worse. Could you file a followup to figure you if we can
> change the signature, though?

Changed the signature of GetUTFOrigin.



(In reply to comment #23)
> >--- /dev/null
> >+++ b/content/base/src/nsDOMMozURL.cpp
> >+
> >+#include "nsDOMClassInfo.h"
> >+#include "nsDOMMozURL.h"
> 
> Include this first.

Change the order will cause compile errors - seems the DOMClassInfo is needed at the first place. Any thoughts?


Jonas, the "&&" case handling will come up in the next patch.

Thanks for your time and input everyone (:

Comment 26

[Shawn Gong]

Attachment: Final patch candidate. Details in Comment #25

Now the "&&" behavior is as discussed - parameters with empty names will be stored as well.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Is there a reason to use jsval where just using array return values in IDL seems like it would work fine?  Then you could nix the JSContext dependencies, too (including the one that doesn't even need to be there in the patch as it is!).

I can't find the spec for appendParameter.  Could you point me to that?

The code in NewURL() is broken, imo.  You want something more like:

  NS_ADDREF(*aNewObject = new nsDOMMozURL);

if you insist on having the COM-like signature.  But I can't figure out why you want that signature to start with, because the function seems to be unused...

Your JsvalToString returns errors on failure, but callers don't consistently check for those.

Is it intended that passing a data: URI to the constructor in JS will set mURL to null?

In SetProtocol, the Substring() call can just be replaces with Truncate(colon).

The hackery in GetHash is really unfortunate, since the nsIURI has all the information you're looking for there.  We should just add an API to get it; please file a followup.  Sadly my proposal that GetRef return a void string on no ref were overruled.  :(  For now, can we please skip the expensive GetHref and so forth in the common case of aHash nonempty?

It's still a bit sad to have all this code duplication with nsLocation.  :(

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 548914 [details] [diff] [review]
Final patch candidate. Details in Comment #25

>--- /dev/null
>+++ b/content/base/src/nsDOMMozURL.cpp
>+nsDOMMozURL::SetProtocol(const nsAString& aProtocol)
>+{
>+  PRInt32 colon = protocol.Find(":");
>+  if (colon >=0) {

">= 0"

>+    if (colon != protocol.Length() - 1) {

"PRUint32(colon)" (This causes a build warning.)

>+      return NS_ERROR_FAILURE;
>+    } else {

No else after return. There's a couple more in the patch, could you fix those too?

>+      protocol = Substring(protocol, 0, protocol.Length()-1);

"protocol.Length() - 1"

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And for inclusions, please use

nsIDOMMozURL.idl:

#include "nsISupports.idl"

%{C++
#include "jspubtd.h"
%}

nsDOMMozURL.h:

#include "nsIDOMMozURL.h"
#include "nsIJSNativeInitializer.h"

#include "nsCOMPtr.h"
#include "nsIURL.h"
#include "nsStringGlue.h"
#include "nsTArray.h"

#include "jspubtd.h"

nsDOMMozURL.cpp:

#include "nsDOMMozURL.h"

#include "nsDOMClassInfo.h"
#include "nsISupportsUtils.h"
#include "nsNetUtil.h"
#include "nsJSUtils.h"
#include "xpcprivate.h"
#include "xpcquickstubs.h"
#include "mozilla/CharTokenizer.h"

That should be enough.

Comment 30

[Shawn Gong]

(In reply to comment #27)
Hi Boris, thanks for the points.

> Is there a reason to use jsval where just using array return values in IDL
> seems like it would work fine?  Then you could nix the JSContext
> dependencies, too (including the one that doesn't even need to be there in
> the patch as it is!).

It seems we don't have array return value support in IDLs? If we do, could you point me to the right places to look up?

> I can't find the spec for appendParameter.  Could you point me to that?

The spec for appendParameter is in Adam Barth's document in the URL link of this bug. (https://docs.google.com/document/edit?id=1r_VTFKApVOaNIkocrg0z-t7lZgzisTuGTXkdzAk4gLU&hl=en)

> The code in NewURL() is broken, imo.  You want something more like:
>
>   NS_ADDREF(*aNewObject = new nsDOMMozURL);
>
> if you insist on having the COM-like signature.  But I can't figure out why
> you want that signature to start with, because the function seems to be
> unused...

That style is referenced from other JS interface implementations - could you suggest a better way to do it?

> Your JsvalToString returns errors on failure, but callers don't consistently
> check for those.

Ah right, I'll fix them!

> Is it intended that passing a data: URI to the constructor in JS will set
> mURL to null?

Data URI was not in consideration for MozURL; do you think we should deal with it?

> In SetProtocol, the Substring() call can just be replaces with
> Truncate(colon).

Will do!

> The hackery in GetHash is really unfortunate, since the nsIURI has all the
> information you're looking for there.  We should just add an API to get it;
> please file a followup.  Sadly my proposal that GetRef return a void string
> on no ref were overruled.  :(  For now, can we please skip the expensive
> GetHref and so forth in the common case of aHash nonempty?

Yes the GetHref should be avoided - my mistake. Do you mean we should add a GetHash in nsIURI to make a "#" prefixed GetRef?

> It's still a bit sad to have all this code duplication with nsLocation.  :(

As discussed above, nsLocation will use most of the code from MozURL (: But it will be in a separate patch.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> If we do, could you point me to the right places to look up?

Here's some IDL for returning a string array: http://hg.mozilla.org/mozilla-central/file/982a5835fba1/dom/interfaces/html/nsIDOMHTMLInputElement.idl#l118

> The spec for appendParameter is in Adam Barth's document in the URL link of
> this bug. 

Oh, I see.  It just hijacks the browser's find with a broken one... ok.

> could you suggest a better way to do it?

Can you explain what you're trying to do?

> Data URI was not in consideration for MozURL

Meaning what?  Right now, if a MozURL() is create with a data: URI and then the caller tries to do something with it, your code will crash.  That seems undesirable.

> Do you mean we should add a GetHash in nsIURI to make a "#" prefixed GetRef?

I mean we should add an API to tell apart the "no ref at all" and "ref is empty string" cases.  That's what you really need here, right?

Comment 32

[Shawn Gong]

(In reply to comment #31)
> Here's some IDL for returning a string array:
> http://hg.mozilla.org/mozilla-central/file/982a5835fba1/dom/interfaces/html/
> nsIDOMHTMLInputElement.idl#l118

This needs the extra length parameter, which would not be nice for the intaking parameter in appendParameter(). Correct me if I got it wrong :)

> > could you suggest a better way to do it?
> 
> Can you explain what you're trying to do?

Changed to NS_ADDREF. The NewURL is used in DOMClassInfo.
 
> > Data URI was not in consideration for MozURL
> 
> Meaning what?  Right now, if a MozURL() is create with a data: URI and then
> the caller tries to do something with it, your code will crash.  That seems
> undesirable.

So in addition to the mURL, we plan to store an extra mURI object to handle data: URI - does it sound good?

> > Do you mean we should add a GetHash in nsIURI to make a "#" prefixed GetRef?
> 
> I mean we should add an API to tell apart the "no ref at all" and "ref is
> empty string" cases.  That's what you really need here, right?

Ah I see.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> This needs the extra length parameter

Yes, why is that a problem?

> which would not be nice for the intaking parameter in appendParameter().

  void getParameterAll(in DOMString aName, [optional] out unsigned long aLength,
                       [array,size_is(aLength), retval]
                         out wstring aParameters);

which JS could then call like this:

  var paramArray = getParameterAll("myname");

I'm not sure what the problem is....

> Changed to NS_ADDREF. The NewURL is used in DOMClassInfo.

Ah, ok.  Yeah, the classinfo use does need this silly signature for now.

> So in addition to the mURL, we plan to store an extra mURI object to handle
> data: URI - does it sound good?

It's not just data:.  But yes, you should store both, or just store the nsIURI and QI to nsIURL in the setters/getters that need an nsIURL.  Either way.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Oh, wait.  You were talking about appendParameter.  That should just use jsval for now; that's fine, because it's an overload.  The only other option is to use an nsIVariant, and you already have this working so it's ok to just let it be.

Comment 35

[Shawn Gong]

Attachment: Patch 6

Fixes according to Ms2ger and Boris.

Boris, about the [array,size_is] syntax, it seems the compiler only understands it inside function parameters; for attributes like the parameterNames, putting it in front (readonly attribute [array,size_is(aLength)] out DOMString parameterNames) could not be recognized by the compiler. Am I doing it wrong?

Thanks!

Comment 36

[Shawn Gong]

Also for the SetPathname, changing the path but keep the original hash and search doesn't make much sense; other browsers don't keep the hash either. So MozURL now doesn't keep the hash - which is consistent to nsLocation's original behavior.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Hrm....  yeah, the array thing won't work for an attribute.  I guess let's not worry about it.

For the rest, please make sure you report spec bugs for any places where what we're doing doesn't match what the spec says (e.g. SetPathname)?

Comment 38

[Shawn Gong]

Attachment: Patch 7. Added null-check for each query.

Comment 39

[Shawn Gong]

Will report spec bugs Boris - let me know which others you think don't match the specs.

Bug# 676049 follows up about the nsLocation migration.

Comment 40

[Shawn Gong]

Attachment: Patch 7. Added null-check for each query.

Comment 41

[Boris Zbarsky [:bzbarsky]]

> let me know which others you think don't match the specs.

Uh... Just look through your code and see whether it does what the spec says... ;)

Comment 42

[Shawn Gong]

Attachment: Patch 8. Added some tests from whatwg example usage, and fixed a few bugs.

The spec's example shows that a single "?" also needs to be reflected in GetSearch (same for "#"), so added support for that.

Comment 43

[Shawn Gong]

Attachment: Patch 9. Fixed a few bugs.

Comment 44

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Jonas, are you planning to review?

Comment 45

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Jonas, still alive?

Comment 47

[Masatoshi Kimura [:emk]]

- I prefer implementing this without "Moz" prefix.
- Now WebIDL bindings should be used to implement this.

Comment 48

[(no longer active)]

We're working on this over in bug 887364.