Closed
Bug 887364
Opened 12 years ago
Closed 12 years ago
Implement URL API
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: baku, Assigned: baku)
References
(Blocks 1 open bug, )
Details
(Keywords: dev-doc-complete)
Attachments
(3 files, 11 obsolete files)
10.07 KB,
patch
|
Details | Diff | Splinter Review | |
31.08 KB,
patch
|
Details | Diff | Splinter Review | |
34.60 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•12 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.
Comment 2•12 years ago
|
||
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•12 years ago
|
||
This first patch updates the URLUtils interface.
Attachment #768289 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #768291 -
Flags: review?(ehsan)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #768292 -
Flags: review?(ehsan)
Assignee | ||
Comment 6•12 years ago
|
||
I would like to implement |attribute URLQuery? query| in a follow-up.
Comment 7•12 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.
Assignee | ||
Comment 8•12 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.
Comment 10•12 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #6)
> I would like to implement |attribute URLQuery? query| in a follow-up.
Sure.
Comment 11•12 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.
Attachment #768289 -
Flags: review?(ehsan) → review+
Comment 12•12 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.
Attachment #768291 -
Flags: review?(ehsan) → review-
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #768289 -
Attachment is obsolete: true
Comment 14•12 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!
Attachment #768292 -
Flags: review?(ehsan) → feedback+
Updated•12 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 15•12 years ago
|
||
Attachment #768291 -
Attachment is obsolete: true
Attachment #769011 -
Flags: review?(ehsan)
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #768292 -
Attachment is obsolete: true
Attachment #769014 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 17•12 years ago
|
||
Would be nice to have nsIOService and nsIURI thread-safe: bug 888604
Depends on: 888604
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #769011 -
Attachment is obsolete: true
Attachment #769011 -
Flags: review?(ehsan)
Attachment #769756 -
Flags: review?(ehsan)
Assignee | ||
Comment 19•12 years ago
|
||
Attachment #769014 -
Attachment is obsolete: true
Attachment #769014 -
Flags: review?(bent.mozilla)
Attachment #769757 -
Flags: review?(bent.mozilla)
Comment 20•12 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.
Attachment #769756 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 21•12 years ago
|
||
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-
Assignee | ||
Comment 24•12 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•12 years ago
|
||
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•12 years ago
|
||
Attachment #769757 -
Attachment is obsolete: true
Attachment #792904 -
Flags: review?(khuey)
Updated•12 years ago
|
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•12 years ago
|
||
Attachment #792709 -
Attachment is obsolete: true
Assignee | ||
Comment 31•12 years ago
|
||
Attachment #792904 -
Attachment is obsolete: true
Assignee | ||
Comment 32•12 years ago
|
||
Assignee | ||
Comment 34•12 years ago
|
||
Keywords: checkin-needed
Comment 35•12 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
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 37•12 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
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•