Andrea Marchesini [:baku] Assignee
	Description • 11 years ago

      No description provided.

	Anne (:annevk)
	Comment 1 • 11 years ago

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.

	Masatoshi Kimura [:emk]
	Comment 2 • 11 years ago

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

	Andrea Marchesini [:baku] Assignee
	Comment 3 • 11 years ago

This first patch updates the URLUtils interface.

	Andrea Marchesini [:baku] Assignee
	Comment 6 • 11 years ago

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

	Masatoshi Kimura [:emk]
	Comment 7 • 11 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 8 • 11 years ago

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

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

	(no longer active)
	Comment 10 • 11 years ago

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

Sure.

	(no longer active)
	Comment 11 • 11 years ago

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.

	(no longer active)
	Comment 12 • 11 years ago

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.

	(no longer active)
	Comment 14 • 11 years ago

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!

	Andrea Marchesini [:baku] Assignee
	Comment 17 • 11 years ago

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

	(no longer active)
	Comment 20 • 11 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 22 • 11 years ago

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

Hopefully khuey can grab this.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 23 • 11 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 24 • 11 years ago

> 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.

	Andrea Marchesini [:baku] Assignee
	Comment 25 • 11 years ago

Cycle Collection for URL.cpp

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 26 • 11 years ago

Answered over IRC.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 28 • 11 years ago

Why do we still need trace and finalize hooks?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 29 • 11 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 32 • 11 years ago

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

	Andrea Marchesini [:baku] Assignee
	Comment 33 • 11 years ago

green on try.

	Andrea Marchesini [:baku] Assignee
	Comment 34 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 11 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 36 • 11 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 37 • 11 years ago

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