887364 - Implement URL API

Status: RESOLVED FIXED

Description

[Andrea Marchesini [:baku]]

Comment 1

[Anne (:annevk)]

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

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

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

This first patch updates the URLUtils interface.

Comment 4

[Andrea Marchesini [:baku]]

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

Comment 5

[Andrea Marchesini [:baku]]

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

Comment 6

[Andrea Marchesini [:baku]]

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

Comment 7

[Masatoshi Kimura [:emk]]

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

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

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

(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 (needinfo please, extremely long backlog)]

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 (needinfo please, extremely long backlog)]

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

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

Comment 14

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

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

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

Comment 16

[Andrea Marchesini [:baku]]

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

Comment 17

[Andrea Marchesini [:baku]]

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

Comment 18

[Andrea Marchesini [:baku]]

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

Comment 19

[Andrea Marchesini [:baku]]

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

Comment 20

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

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

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

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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)]

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

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

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)]

Answered over IRC.

Comment 27

[Andrea Marchesini [:baku]]

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

Comment 28

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Why do we still need trace and finalize hooks?

Comment 29

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

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

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

Comment 31

[Andrea Marchesini [:baku]]

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

Comment 32

[Andrea Marchesini [:baku]]

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

Comment 33

[Andrea Marchesini [:baku]]

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

green on try.

Comment 34

[Andrea Marchesini [:baku]]

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

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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)]

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

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