Last Comment Bug 887364 - Implement URL API
: Implement URL API
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla26
Assigned To: Andrea Marchesini (:baku)
:
Mentors:
http://url.spec.whatwg.org/#api
: 668680 (view as bug list)
Depends on: 483304 888604
Blocks: url 887836
  Show dependency treegraph
 
Reported: 2013-06-26 10:58 PDT by Andrea Marchesini (:baku)
Modified: 2013-10-25 10:08 PDT (History)
16 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1 - URLUtils interface updated (9.84 KB, patch)
2013-06-27 06:59 PDT, Andrea Marchesini (:baku)
ehsan: review+
Details | Diff | Review
patch 2 - URL (29.43 KB, patch)
2013-06-27 07:00 PDT, Andrea Marchesini (:baku)
ehsan: review-
Details | Diff | Review
patch 3 - URL in workers (30.03 KB, patch)
2013-06-27 07:01 PDT, Andrea Marchesini (:baku)
ehsan: feedback+
Details | Diff | Review
patch 1 - URLUtils interface updated (10.07 KB, patch)
2013-06-27 08:52 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch 2 - URL (31.27 KB, patch)
2013-06-28 10:05 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch 3 - URL in workers (30.02 KB, patch)
2013-06-28 10:12 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch 2 - URL (31.99 KB, patch)
2013-07-01 10:52 PDT, Andrea Marchesini (:baku)
ehsan: review+
Details | Diff | Review
patch 3 - URL in workers (30.98 KB, patch)
2013-07-01 10:53 PDT, Andrea Marchesini (:baku)
khuey: review-
Details | Diff | Review
patch 2 - URL (31.97 KB, patch)
2013-07-02 14:04 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch 2 - URL (33.07 KB, patch)
2013-08-20 02:16 PDT, Andrea Marchesini (:baku)
ehsan: review+
Details | Diff | Review
patch 3 - URL in workers (31.08 KB, patch)
2013-08-20 09:59 PDT, Andrea Marchesini (:baku)
khuey: review+
Details | Diff | Review
patch 2 - URL (33.12 KB, patch)
2013-09-04 05:15 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch 3 - URL in workers (31.08 KB, patch)
2013-09-04 05:16 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch 2 - URL (34.60 KB, patch)
2013-09-04 09:44 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review

Description Andrea Marchesini (:baku) 2013-06-26 10:58:52 PDT

    
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 (busy, don't ask for review please) 2013-06-27 08:18:14 PDT
*** Bug 668680 has been marked as a duplicate of this bug. ***
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 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 (busy, don't ask for review please) 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 (busy, don't ask for review please) 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 (busy, don't ask for review please) 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 (busy, don't ask for review please) 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] (khuey@mozilla.com) (Away until 6/13) 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] (khuey@mozilla.com) (Away until 6/13) 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] (khuey@mozilla.com) (Away until 6/13) 2013-08-29 10:14:52 PDT
Why do we still need trace and finalize hooks?
Comment 29 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 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

Note You need to log in before you can comment on or make changes to this bug.