Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla26
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 11 obsolete attachments)

10.07 KB, patch
Details | Diff | Splinter Review
31.08 KB, patch
Details | Diff | Splinter Review
34.60 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)

Comment 1

4 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.
I think we should fix bug 483304 first instead of bringing the broken hash behavior into the brand new API.
Depends on: 483304
(Assignee)

Comment 3

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

This first patch updates the URLUtils interface.
Attachment #768289 - Flags: review?(ehsan)
(Assignee)

Comment 4

4 years ago
Created attachment 768291 [details] [diff] [review]
patch 2 - URL
Attachment #768291 - Flags: review?(ehsan)
(Assignee)

Comment 5

4 years ago
Created attachment 768292 [details] [diff] [review]
patch 3 - URL in workers
Attachment #768292 - Flags: review?(ehsan)
(Assignee)

Comment 6

4 years ago
I would like to implement |attribute URLQuery? query| in a follow-up.
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.
(Assignee)

Comment 8

4 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.
Duplicate of this bug: 668680
(In reply to Andrea Marchesini (:baku) from comment #6)
> I would like to implement |attribute URLQuery? query| in a follow-up.

Sure.
(Assignee)

Updated

4 years ago
Blocks: 887836
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.
Attachment #768289 - Flags: review?(ehsan) → review+
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.
Attachment #768291 - Flags: review?(ehsan) → review-
(Assignee)

Comment 13

4 years ago
Created attachment 768370 [details] [diff] [review]
patch 1 - URLUtils interface updated
Attachment #768289 - Attachment is obsolete: true
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!
Attachment #768292 - Flags: review?(ehsan) → feedback+
Keywords: dev-doc-needed
(Assignee)

Comment 15

4 years ago
Created attachment 769011 [details] [diff] [review]
patch 2 - URL
Attachment #768291 - Attachment is obsolete: true
Attachment #769011 - Flags: review?(ehsan)
(Assignee)

Comment 16

4 years ago
Created attachment 769014 [details] [diff] [review]
patch 3 - URL in workers
Attachment #768292 - Attachment is obsolete: true
Attachment #769014 - Flags: review?(bent.mozilla)
(Assignee)

Comment 17

4 years ago
Would be nice to have nsIOService and nsIURI thread-safe: bug 888604
Depends on: 888604
(Assignee)

Comment 18

4 years ago
Created attachment 769756 [details] [diff] [review]
patch 2 - URL
Attachment #769011 - Attachment is obsolete: true
Attachment #769011 - Flags: review?(ehsan)
Attachment #769756 - Flags: review?(ehsan)
(Assignee)

Comment 19

4 years ago
Created attachment 769757 [details] [diff] [review]
patch 3 - URL in workers
Attachment #769014 - Attachment is obsolete: true
Attachment #769014 - Flags: review?(bent.mozilla)
Attachment #769757 - Flags: review?(bent.mozilla)
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.
Attachment #769756 - Flags: review?(ehsan) → review+
(Assignee)

Comment 21

4 years ago
Created attachment 770404 [details] [diff] [review]
patch 2 - URL
Attachment #769756 - Attachment is obsolete: true
Comment on attachment 769757 [details] [diff] [review]
patch 3 - URL in workers

Hopefully khuey can grab this.
Attachment #769757 - Flags: review?(bent.mozilla) → review?(khuey)
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.
Attachment #769757 - Flags: review?(khuey) → review-

Updated

4 years ago
Blocks: 906714
(Assignee)

Comment 24

4 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.
Flags: needinfo?(khuey)
(Assignee)

Comment 25

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

Cycle Collection for URL.cpp
Attachment #770404 - Attachment is obsolete: true
Attachment #792709 - Flags: review?(ehsan)
Answered over IRC.
Flags: needinfo?(khuey)
(Assignee)

Comment 27

4 years ago
Created attachment 792904 [details] [diff] [review]
patch 3 - URL in workers
Attachment #769757 - Attachment is obsolete: true
Attachment #792904 - Flags: review?(khuey)
Attachment #792709 - Flags: review?(ehsan) → review+
Why do we still need trace and finalize hooks?
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.
Attachment #792904 - Flags: review?(khuey) → review+
(Assignee)

Comment 30

4 years ago
Created attachment 799442 [details] [diff] [review]
patch 2 - URL
Attachment #792709 - Attachment is obsolete: true
(Assignee)

Comment 31

4 years ago
Created attachment 799443 [details] [diff] [review]
patch 3 - URL in workers
Attachment #792904 - Attachment is obsolete: true
(Assignee)

Comment 32

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=62dc9a2b0a6e
(Assignee)

Comment 33

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

green on try.
Attachment #799442 - Attachment is obsolete: true
(Assignee)

Comment 34

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=0808ccf1d57e
Keywords: checkin-needed
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
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/907989350527
https://hg.mozilla.org/mozilla-central/rev/f2199d73aef6
https://hg.mozilla.org/mozilla-central/rev/c5fda7e3b3f4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.