1454656 - Unify URLWorker and URLMainThread

Status: RESOLVED FIXED

(Core :: DOM: Workers, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

URL API was split in 2 parts because nsIURI was not thread-safe. Finally, nsIURI can be touched on any thread and we can unify URL implementations.

Only 4 methods need to have custom implementations for workers and main-thread: Constructor, SetHref, SetProtocol, GetOrigin. This methods neeed to create new nsIURI objects and that must happen on the main-thread (for GetOrigin, see nsContentUtils::GetUTFOrigin() when dealing with blobURL).

Comment 1

[Andrea Marchesini [:baku]]

Attachment: url.patch

I had this patch in one of my local repository. I think it's nice to land it.
There is no needs to use mStdURL anymore except for GetOrigin().
And if we really want, we can check if the scheme is 'blob' and only in that scenario, jump to main-thread.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: url.patch

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8969174 [details] [diff] [review]
url.patch

>+void
>+URL::GetHref(nsAString& aHref) const
>+{
>+  MOZ_ASSERT(mURI);
>+
>+  aHref.Truncate();
>+
>+  nsAutoCString href;
>+  nsresult rv = mURI->GetSpec(href);
>+  if (NS_SUCCEEDED(rv)) {
>+    CopyUTF8toUTF16(href, aHref);
>+  }
>+}
Why you don't use URL_GETTER here?

>+
>+void
>+URL::GetProtocol(nsAString& aProtocol) const
>+{
>+  MOZ_ASSERT(mURI);
>+
>+  nsAutoCString protocol;
>+  if (NS_SUCCEEDED(mURI->GetScheme(protocol))) {
>+    aProtocol.Truncate();
>+  }
>+
>+  CopyASCIItoUTF16(protocol, aProtocol);
>+  aProtocol.Append(char16_t(':'));
>+}
Same here (would need to append :)
worker version of this method does truncate before calling the uri method, main thread doesn't.
In practice it doesn't probably matter which one is done.

>+++ b/dom/url/URLMainThread.cpp
>@@ -85,17 +85,18 @@ URLMainThread::Constructor(nsISupports* 
>                           nsContentUtils::GetIOService());
>   if (NS_FAILED(rv)) {
>     // No need to warn in this case. It's common to use the URL constructor
>     // to determine if a URL is valid and an exception will be propagated.
>     aRv.ThrowTypeError<MSG_INVALID_URL>(aURL);
>     return nullptr;
>   }
> 
>-  RefPtr<URLMainThread> url = new URLMainThread(aParent, uri.forget());
>+  RefPtr<URLMainThread> url = new URLMainThread(aParent);
>+  url->SetURI(uri.forget());
I don't understand this change. I don't actually understand why we need SetURI.
But fine.

>+    nsCOMPtr<nsIURI> clone;
>+    nsresult rv = NS_MutateURI(mURI)
>+                    .SetScheme(NS_ConvertUTF16toUTF8(mValue))
>+                    .Finalize(clone);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return true;
>     }
> 
>-    if (NS_WARN_IF(rv.Failed())) {
>-      rv.SuppressException();
>-      mFailed = true;
>+    nsAutoCString href;
>+    rv = clone->GetSpec(href);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return true;
>     }
> 
>+    nsCOMPtr<nsIURI> uri;
>+    rv = NS_NewURI(getter_AddRefs(uri), href);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return true;
>+    }
>+
>+    mRetval = Move(uri);
huh, our APIs are horrible.


r+, if you could use URL_GETTER consistently, or explain why it can't be used

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5051b2393f2
Unify URLWorker and URLMainThread, r=smaug

Comment 5

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/b5051b2393f2