Comment 1 Anne (:annevk) 2013-06-27 00:06:07 PDT

Bug 668680 was the bug for this so far. But given the wall of text you might want to duplicate that to this bug instead I suppose.

Comment 2 Masatoshi Kimura [:emk] 2013-06-27 00:44:11 PDT

I think we should fix bug 483304 first instead of bringing the broken hash behavior into the brand new API.

Comment 3 Andrea Marchesini [:baku] 2013-06-27 06:59:35 PDT

Created attachment 768289 [details] [diff] [review]
patch 1 - URLUtils interface updated

This first patch updates the URLUtils interface.

Comment 4 Andrea Marchesini [:baku] 2013-06-27 07:00:53 PDT

Created attachment 768291 [details] [diff] [review]
patch 2 - URL

Comment 5 Andrea Marchesini [:baku] 2013-06-27 07:01:27 PDT

Created attachment 768292 [details] [diff] [review]
patch 3 - URL in workers

Comment 6 Andrea Marchesini [:baku] 2013-06-27 07:02:11 PDT

I would like to implement |attribute URLQuery? query| in a follow-up.

Comment 7 Masatoshi Kimura [:emk] 2013-06-27 07:18:42 PDT

Comment on attachment 768292 [details] [diff] [review]
patch 3 - URL in workers

Is the proxy needed? I thought nsIOService and nsIURL implementations are thread-safe.

Comment 8 Andrea Marchesini [:baku] 2013-06-27 07:42:51 PDT

> Is the proxy needed? I thought nsIOService and nsIURL implementations are
> thread-safe.

They are not... would be nice to have it thread-safe.

Comment 9 :Ehsan Akhgari 2013-06-27 08:18:14 PDT

*** Bug 668680 has been marked as a duplicate of this bug. ***

Comment 10 :Ehsan Akhgari 2013-06-27 08:19:47 PDT

(In reply to Andrea Marchesini (:baku) from comment #6)
> I would like to implement |attribute URLQuery? query| in a follow-up.

Sure.

Comment 11 :Ehsan Akhgari 2013-06-27 08:33:20 PDT

Comment on attachment 768289 [details] [diff] [review]
patch 1 - URLUtils interface updated

Review of attachment 768289 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below addressed.

::: content/base/src/Link.cpp
@@ +329,5 @@
>    return;
>  }
>  
>  void
> +Link::GetUsername(nsAString &_username)

Nit: please avoid prefixing names with underscores in new code.  aUsername seems better.

@@ +339,5 @@
> +    return;
> +  }
> +
> +  nsAutoCString username;
> +  (void)uri->GetUsername(username);

These casts are not really necessary.

::: dom/webidl/HTMLAreaElement.webidl
@@ -22,5 @@
>             attribute DOMString shape;
> -  // No support for stringifier attributes yet
> -  //[SetterThrows]
> -  //stringifier attribute DOMString href;
> -  stringifier;

It seems like here you're dropping the stringifier for both HTMLAreaElement and HTMLAnchorElement.  See bug 851891.  I do hope that this patch fails the test added in bug 839913.  :-)

Please restore the stringifier on URLUtils.

Comment 12 :Ehsan Akhgari 2013-06-27 08:52:27 PDT

Comment on attachment 768291 [details] [diff] [review]
patch 2 - URL

Review of attachment 768291 [details] [diff] [review]:
-----------------------------------------------------------------

r- mostly because of the wrappercache-ness.  If this does need to be wrappercached, please let me know!

::: dom/base/URL.cpp
@@ +18,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(URL)

You need to traverse mWindow here.

@@ +35,5 @@
> +}
> +
> +URL::~URL()
> +{
> +}

Nit: please drop the empty dtor.

@@ +223,5 @@
> +  }
> +
> +  if (!aOrigin.EqualsLiteral("null")) {
> +    aOrigin.Assign(origin);
> +  }

Hmm, it would be nice if you could refactor this into a new nsContentUtils method so that you don't have to repeat this same pattern in multiple places.

@@ +259,5 @@
> +  nsAutoCString username;
> +  nsresult rv = mURI->GetUsername(username);
> +  if (NS_SUCCEEDED(rv)) {
> +    CopyUTF8toUTF16(username, aUsername);
> +  }

Please refactor the .Truncate()/rv check/CopyUTF8toUTF16 dance into a private helper.  :-)

::: dom/base/URL.h
@@ +11,5 @@
>  #include "mozilla/dom/URLBinding.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"
> +#include "nsAutoPtr.h"
> +#include "nsIURI.h"

It would be nice to see if you can avoid some of these #includes in the header by forward declaring stuff.  I think you should be able to forward declare nsIURI at least, and maybe ErrorResult as well.

@@ +23,5 @@
>  
>  namespace dom {
>  
> +class URL MOZ_FINAL : public nsISupports,
> +                      public nsWrapperCache

I would prefer a bit if this did not implement nsISupports.  You should be able to declare your own AddRef and Release, use NS_IMPL_CYCLE_COLLECTING_NATIVE_ADDREF and NS_IMPL_CYCLE_COLLECTING_NATIVE_RELEASE to implement them.

Also, why does this need to be a wrapper cache?  Is it possible to recreate the wrapper for an existing native object once gc kills its JSObject wrapper?

::: dom/bindings/Bindings.conf
@@ +1118,1 @@
>  },

I think you should be able to drop the empty braces here.

Comment 13 Andrea Marchesini [:baku] 2013-06-27 08:52:54 PDT

Created attachment 768370 [details] [diff] [review]
patch 1 - URLUtils interface updated

Comment 14 :Ehsan Akhgari 2013-06-27 08:54:30 PDT

Comment on attachment 768292 [details] [diff] [review]
patch 3 - URL in workers

Review of attachment 768292 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, but I'm not very confident in reviewing exposing things to workers.  Can you please ask somebody else to also look at this?  Thanks!

Comment 15 Andrea Marchesini [:baku] 2013-06-28 10:05:42 PDT

Created attachment 769011 [details] [diff] [review]
patch 2 - URL

Comment 16 Andrea Marchesini [:baku] 2013-06-28 10:12:06 PDT

Created attachment 769014 [details] [diff] [review]
patch 3 - URL in workers

Comment 17 Andrea Marchesini [:baku] 2013-06-29 03:19:48 PDT

Would be nice to have nsIOService and nsIURI thread-safe: bug 888604

Comment 18 Andrea Marchesini [:baku] 2013-07-01 10:52:27 PDT

Created attachment 769756 [details] [diff] [review]
patch 2 - URL

Comment 19 Andrea Marchesini [:baku] 2013-07-01 10:53:09 PDT

Created attachment 769757 [details] [diff] [review]
patch 3 - URL in workers

Comment 20 :Ehsan Akhgari 2013-07-02 09:49:10 PDT

Comment on attachment 769756 [details] [diff] [review]
patch 2 - URL

Review of attachment 769756 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsContentUtils.h
@@ +1606,5 @@
>    static nsresult GetASCIIOrigin(nsIURI* aURI, nsCString& aOrigin);
>    static nsresult GetUTFOrigin(nsIPrincipal* aPrincipal,
>                                 nsString& aOrigin);
>    static nsresult GetUTFOrigin(nsIURI* aURI, nsString& aOrigin);
> +  static nsresult GetUTFNonNullOrigin(nsIURI* aURI, nsString& aOrigin);

You're not using the return value of this function anywhere, so please make it void.

Comment 21 Andrea Marchesini [:baku] 2013-07-02 14:04:10 PDT

Created attachment 770404 [details] [diff] [review]
patch 2 - URL

Comment 22 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-08-13 13:49:17 PDT

Comment on attachment 769757 [details] [diff] [review]
patch 3 - URL in workers

Hopefully khuey can grab this.

Comment 23 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-08-14 13:28:14 PDT

Comment on attachment 769757 [details] [diff] [review]
patch 3 - URL in workers

Review of attachment 769757 [details] [diff] [review]:
-----------------------------------------------------------------

This should be updated to use cycle collection.

Comment 24 Andrea Marchesini [:baku] 2013-08-20 02:13:20 PDT

> This should be updated to use cycle collection.

khuey, can you gell me something more? I didn't find any other class with 'cycle' macros in dom/workers. BTW, I already updated the patches for the main-thread implementation.

Comment 25 Andrea Marchesini [:baku] 2013-08-20 02:16:10 PDT

Created attachment 792709 [details] [diff] [review]
patch 2 - URL

Cycle Collection for URL.cpp

Comment 26 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-08-20 08:44:41 PDT

Answered over IRC.

Comment 27 Andrea Marchesini [:baku] 2013-08-20 09:59:56 PDT

Created attachment 792904 [details] [diff] [review]
patch 3 - URL in workers

Comment 28 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-08-29 10:14:52 PDT

Why do we still need trace and finalize hooks?

Comment 29 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-09-03 10:34:49 PDT

Comment on attachment 792904 [details] [diff] [review]
patch 3 - URL in workers

Review of attachment 792904 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/URL.cpp
@@ +28,1 @@
>  using mozilla::dom::WorkerGlobalObject;

You're going to have to rebase.  This is gone.

@@ +622,5 @@
> +
> +    if (NS_FAILED(NS_DispatchToMainThread(runnable))) {
> +      NS_ERROR("Failed to dispatch teardown runnable!");
> +    }
> +  }

Just do this in the dtor.

Comment 30 Andrea Marchesini [:baku] 2013-09-04 05:15:54 PDT

Created attachment 799442 [details] [diff] [review]
patch 2 - URL

Comment 31 Andrea Marchesini [:baku] 2013-09-04 05:16:31 PDT

Created attachment 799443 [details] [diff] [review]
patch 3 - URL in workers

Comment 32 Andrea Marchesini [:baku] 2013-09-04 05:17:54 PDT

https://tbpl.mozilla.org/?tree=Try&rev=62dc9a2b0a6e

Comment 33 Andrea Marchesini [:baku] 2013-09-04 09:44:49 PDT

Created attachment 799596 [details] [diff] [review]
patch 2 - URL

green on try.

Comment 34 Andrea Marchesini [:baku] 2013-09-04 09:45:29 PDT

https://tbpl.mozilla.org/?tree=Try&rev=0808ccf1d57e

Comment 35 Ryan VanderMeulen [:RyanVM] 2013-09-04 10:09:51 PDT

https://hg.mozilla.org/integration/mozilla-inbound/rev/907989350527
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2199d73aef6
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5fda7e3b3f4

Comment 36 Wes Kocher (:KWierso) 2013-09-04 16:23:30 PDT

https://hg.mozilla.org/mozilla-central/rev/907989350527
https://hg.mozilla.org/mozilla-central/rev/f2199d73aef6
https://hg.mozilla.org/mozilla-central/rev/c5fda7e3b3f4

Comment 37 Jean-Yves Perrier [:teoli] 2013-09-06 03:39:24 PDT

Documentation updated/created:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/26
https://developer.mozilla.org/en-US/docs/Web/API/URLUtils
https://developer.mozilla.org/en-US/docs/Web/API/URLUtils.password
https://developer.mozilla.org/en-US/docs/Web/API/URLUtils.username
https://developer.mozilla.org/en-US/docs/Web/API/URLUtils.origin
https://developer.mozilla.org/en-US/docs/Web/API/URL
https://developer.mozilla.org/en-US/docs/Web/API/URL.URL
https://developer.mozilla.org/en-US/docs/Web/API/Location
https://developer.mozilla.org/en-US/docs/Web/API/HTMLAnchorElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLAreaElement
https://developer.mozilla.org/en-US/docs/Web/Guide/Needs_categorization/Functions_available_to_workers