Bug 71968

There is currently a webkitURL which is implemented as DOMURL. It has 2 static methods createObjectURL revokeObjectURL Gecko also implements these 2 but they do not use a prefix. I'm suggesting we start adding features from the URL API draft to DOMURL but we keep the 2 static methods.

I wish to implement this. Should I provide the working progress patch ?

(In reply to comment #2) > I wish to implement this. Should I provide the working progress patch ? That sounds reasonable. The spec is pretty much work in progress so expect things to change. For example, it is a known spec bug that the spec uses a DOMTokenList http://www.w3.org/Bugs/Public/show_bug.cgi?id=14569

(In reply to comment #3) > (In reply to comment #2) > > I wish to implement this. Should I provide the working progress patch ? > > That sounds reasonable. > > The spec is pretty much work in progress so expect things to change. For example, it is a known spec bug that the spec uses a DOMTokenList > > http://www.w3.org/Bugs/Public/show_bug.cgi?id=14569 Yes, thats right. Is it fine to add different patches for the progress?

Created attachment 114916 [details] Working Progress Patch Added IDL attributes.

Created attachment 115121 [details] Patch

Comment on attachment 115121 [details] Patch Attachment 115121 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10372465

Created attachment 115138 [details] Progress Patch 2 Added empty functions. Fixed idl file.

Comment on attachment 115138 [details] Progress Patch 2 Attachment 115138 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10486580

Comment on attachment 115138 [details] Progress Patch 2 Attachment 115138 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10483620

Comment on attachment 115138 [details] Progress Patch 2 Attachment 115138 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10486672

Created attachment 115532 [details] Updated patch

Comment on attachment 115532 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=115532&action=review > Source/WebCore/ChangeLog:11 > + No new tests. We most certainly need tests!

Created attachment 115780 [details] IDL Attributes implementation This is initial patch implementing IDL attributes. IDL functions are still empty and in queue for implementation as there is blocking bug for not having DOMTokenList in DOMURL object. I would like to rename this bug to "Implementing DOMURL IDL Attributes" and make a meta bug to track other function implementation bug in a master bug "[Meta] DOMURL Implementation". Let me know your views on this.

Comment on attachment 115780 [details] IDL Attributes implementation Attachment 115780 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10374628 New failing tests: fast/dom/DOMURL/set-domurl-attribute-hash.html fast/dom/DOMURL/set-domurl-attribute-search.html fast/dom/DOMURL/set-domurl-attribute-port.html fast/dom/DOMURL/set-domurl-attribute-hostname.html fast/dom/DOMURL/set-domurl-attribute-pathname.html fast/dom/DOMURL/set-domurl-attribute-host.html

(In reply to comment #15) > (From update of attachment 115780 [details]) > Attachment 115780 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/10374628 > > New failing tests: > fast/dom/DOMURL/set-domurl-attribute-hash.html > fast/dom/DOMURL/set-domurl-attribute-search.html > fast/dom/DOMURL/set-domurl-attribute-port.html > fast/dom/DOMURL/set-domurl-attribute-hostname.html > fast/dom/DOMURL/set-domurl-attribute-pathname.html > fast/dom/DOMURL/set-domurl-attribute-host.html Do I have to add anything for V8 bindings? I am not sure why these are failing here.

> Do I have to add anything for V8 bindings? I am not sure why these are failing here. Dunno. You can build and test Chromium by adding --chromium to update-webkit, build-webkit, and run-webkit-tests.

Comment on attachment 115780 [details] IDL Attributes implementation View in context: https://bugs.webkit.org/attachment.cgi?id=115780&action=review Can you add tests that ensures that: webkitURL instancof Function new webkitURL(...) instanceof webkitURL webkitURL.createObjectURL instanceof function new webkitURL(...).createObjectURL === undefined new webkitURL(...).revokeObjectURL === undefined webkitURL.prototype.getParameter instanceof Function new webkitURL(...).hasOwnProperty('getParameter') === false etc > Source/WebCore/html/DOMURL.idl:30 > Conditional=BLOB, The conditional should be moved to createObjectURL and revokeObjectURL > Source/WebCore/html/DOMURL.idl:56 > [ConvertNullStringTo=Undefined] DOMString createObjectURL(in MediaStream stream); This and revokeObjectURL are class (static) methods. > LayoutTests/fast/dom/DOMURL/get-domurl-attribute-port.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Don't use script-tests. It just doubles the number of files in the repo. http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#WritingJavaScript-basedDOM-onlyTestCases <!DOCTYPE html> <script src="../../js/resources/js-test-pre.js"></script> <script> // your tests go here </script> <script src="../../js/resources/js-test-post.js"></script> > LayoutTests/fast/dom/DOMURL/script-tests/get-domurl-attribute-port.js:3 > +var domurl = document.createElement("DOMURL"); What? This is not an element. Can you add an assert that this does NOT work? The correct usage of this should be: var url = new webkitURL(urlString, baseUrlString); > LayoutTests/fast/dom/DOMURL/script-tests/set-domurl-attribute-hash.js:8 > +debug("Hash value does not start with '#'"); > +domurl.href = "https://www.mydomain.com:8080/path/testurl.html#middle"; > +domurl.hash = "hash-value"; > +shouldBe("domurl.href", "'https://www.mydomain.com:8080/path/testurl.html#hash-value'"); This is also wrong. The constructor should not have any properties except for the two "static" methods, createObjectURL and revokeObjectURL. var url = new URL('http://www.example.com/#hash-value'); shouldBeEqualToString('url.hash', 'hash-value'); Can you add tests that asserts that the constructor has the right static methods and that the instance has the expected properties and methods? > LayoutTests/fast/dom/DOMURL/script-tests/set-domurl-attribute-host.js:17 > +try { > +domurl.href = "https://www.mydomain.com:8080/path/?key=value"; > +domurl.host = "www.other?domain.com:8080"; > +shouldBe("domurl.href", "'https://www.other/?domain.com:8080/path/?key=value'"); > +} catch(e) { > +debug("Exception: " + e.description); > +} Use shouldThrow()

You also need to update DOMWindow.idl to remove/change the conditional for webkitURL. We should also test that we have a stringifier for the href property. In other words toString should return href var url = new webkitURL('http://www.example.com/'); shouldBe('url.toString()', 'url.href');

(In reply to comment #18) > (From update of attachment 115780 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115780&action=review > > Can you add tests that ensures that: > > webkitURL instancof Function > > new webkitURL(...) instanceof webkitURL > > webkitURL.createObjectURL instanceof function > > new webkitURL(...).createObjectURL === undefined > new webkitURL(...).revokeObjectURL === undefined > > webkitURL.prototype.getParameter instanceof Function > new webkitURL(...).hasOwnProperty('getParameter') === false > > etc > Surely, I will add them all. > > Source/WebCore/html/DOMURL.idl:30 > > Conditional=BLOB, > > The conditional should be moved to createObjectURL and revokeObjectURL > Yes as the blob conditional is only applicable to these two methods now. > > Source/WebCore/html/DOMURL.idl:56 > > [ConvertNullStringTo=Undefined] DOMString createObjectURL(in MediaStream stream); > > This and revokeObjectURL are class (static) methods. > > > LayoutTests/fast/dom/DOMURL/get-domurl-attribute-port.html:1 > > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > Don't use script-tests. It just doubles the number of files in the repo. > > http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#WritingJavaScript-basedDOM-onlyTestCases > > <!DOCTYPE html> > <script src="../../js/resources/js-test-pre.js"></script> > <script> > > // your tests go here > > </script> > <script src="../../js/resources/js-test-post.js"></script> > No worries. I will change them all. > > LayoutTests/fast/dom/DOMURL/script-tests/get-domurl-attribute-port.js:3 > > +var domurl = document.createElement("DOMURL"); > > What? This is not an element. Can you add an assert that this does NOT work? > > The correct usage of this should be: > > var url = new webkitURL(urlString, baseUrlString); > Opps. Sorry for that. Think i did some foolish mistake there. > > LayoutTests/fast/dom/DOMURL/script-tests/set-domurl-attribute-hash.js:8 > > +debug("Hash value does not start with '#'"); > > +domurl.href = "https://www.mydomain.com:8080/path/testurl.html#middle"; > > +domurl.hash = "hash-value"; > > +shouldBe("domurl.href", "'https://www.mydomain.com:8080/path/testurl.html#hash-value'"); > > This is also wrong. The constructor should not have any properties except for the two "static" methods, createObjectURL and revokeObjectURL. > > var url = new URL('http://www.example.com/#hash-value'); > shouldBeEqualToString('url.hash', 'hash-value'); > Right. I will change them all. > Can you add tests that asserts that the constructor has the right static methods and that the instance has the expected properties and methods? > > > LayoutTests/fast/dom/DOMURL/script-tests/set-domurl-attribute-host.js:17 > > +try { > > +domurl.href = "https://www.mydomain.com:8080/path/?key=value"; > > +domurl.host = "www.other?domain.com:8080"; > > +shouldBe("domurl.href", "'https://www.other/?domain.com:8080/path/?key=value'"); > > +} catch(e) { > > +debug("Exception: " + e.description); > > +} > > Use shouldThrow()

I implemented this in JS+DOM https://gist.github.com/1384398 It might be useful for you to better understand how this is supposed to work.

(In reply to comment #21) > I implemented this in JS+DOM > > https://gist.github.com/1384398 > > It might be useful for you to better understand how this is supposed to work. Thanx Erik. It is much useful. Just one question it should workin both ways var url = new webkitURL() var url = window.webkitURL Right?

(In reply to comment #22) > (In reply to comment #21) > > I implemented this in JS+DOM > > > > https://gist.github.com/1384398 > > > > It might be useful for you to better understand how this is supposed to work. > > Thanx Erik. It is much useful. Just one question it should workin both ways > var url = new webkitURL() > var url = window.webkitURL > > Right? var url = window.webkitURL; works but it does not give you a URL instance. Instead it should give you a function object that can be used to construct new URL instances. shouldBeFalse("'protocol' in webkitURL"); shouldBeFalse("'username' in webkitURL"); shouldBeFalse("'password' in webkitURL"); shouldBeFalse("'host' in webkitURL"); shouldBeFalse("'hostname' in webkitURL"); shouldBeFalse("'port' in webkitURL"); shouldBeFalse("'pathname' in webkitURL"); shouldBeFalse("'search' in webkitURL"); shouldBeFalse("'hash' in webkitURL"); shouldBeFalse("'filename' in webkitURL"); shouldBeFalse("'origin' in webkitURL"); shouldBeFalse("'parameterNames' in webkitURL"); shouldBeFalse("'getParametername' in webkitURL"); shouldBeFalse("'getParameterAllname' in webkitURL"); shouldBeFalse("'appendParameter' in webkitURL"); shouldBeFalse("'clearParametername' in webkitURL"); shouldBeFalse("'href' in webkitURL"); shouldBeTrue("createObjectURL' in webkitURL"); shouldBeTrue("'revokeObjectURL' in webkitURL");

Thanks Erik for adding bug for static support in IDL. I was trying to fix window.webkitURL but was not working. Will try to work on that.

Created attachment 116327 [details] Working Progress Patch

Comment on attachment 116327 [details] Working Progress Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116327&action=review > Source/WebCore/html/DOMURL.cpp:145 > + return m_href; Move all the single statement functions to the .h file > Source/WebCore/html/DOMURL.cpp:161 > + if (value == "null") > + return; Why are you comparing to the string "null"? Did you want to do? if (value.isNull()) return; But the question is, do we really need to do this? Doesn't KURL::setProtocol work correctly with this in mind already? > Source/WebCore/html/DOMURL.cpp:165 > + KURL url = href(); > + url.setProtocol(value); > + setHref(url.string()); m_href is mutable so there should be no need to make a copy of it, serialize it and reparse it. void DOMURL::setProtocol(const String& value) { m_href.setProtocol(value); } > Source/WebCore/html/DOMURL.cpp:190 > +void DOMURL::setPassword(const String& value) > +{ > + KURL url = href(); > + url.setPass(value); > + setHref(url.string()); > +} void DOMURL::setPassword(const String& value) { m_href.setPass(value); } > Source/WebCore/html/DOMURL.cpp:257 > + KURL url = href(); > + if (!url.canSetHostOrPort()) > + return; Maybe do this before the loop? > LayoutTests/fast/dom/DOMURL/check-instanceof-domurl-functions.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> <!DOCTYPE html>

Created attachment 116565 [details] Update Patch Updated patch after review comments.

Comment on attachment 116565 [details] Update Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116565&action=review > Source/WebCore/html/DOMURL.cpp:154 > + if (value.isEmpty() || value == "null") Remove"null" case > Source/WebCore/html/DOMURL.cpp:239 > + m_href.setQuery(newSearch.replace('#', "%23")); This looks suspicious.

(In reply to comment #28) > (From update of attachment 116565 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116565&action=review > > > Source/WebCore/html/DOMURL.cpp:154 > > + if (value.isEmpty() || value == "null") > > Remove"null" case > Fixed that. > > Source/WebCore/html/DOMURL.cpp:239 > > + m_href.setQuery(newSearch.replace('#', "%23")); > > This looks suspicious. This was to make sure that "#" in the search does not leak to hash. Do u think should be removed?

(In reply to comment #29) > > > Source/WebCore/html/DOMURL.cpp:239 > > > + m_href.setQuery(newSearch.replace('#', "%23")); > > > > This looks suspicious. > > This was to make sure that "#" in the search does not leak to hash. Do u think should be removed? I thought that KURL would do the right thing but I see that it doesn't. I kind of wonder if it would not be cleaner to fix KURL here? (and remove this code from HTMLAnchorElement etc)

(In reply to comment #30) > (In reply to comment #29) > > > > Source/WebCore/html/DOMURL.cpp:239 > > > > + m_href.setQuery(newSearch.replace('#', "%23")); > > > > > > This looks suspicious. > > > > This was to make sure that "#" in the search does not leak to hash. Do u think should be removed? > > I thought that KURL would do the right thing but I see that it doesn't. I kind of wonder if it would not be cleaner to fix KURL here? (and remove this code from HTMLAnchorElement etc) Yes, agree. We can mark it as FIXME as of now may be if you say.

Comment on attachment 116565 [details] Update Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116565&action=review > Source/WebCore/html/DOMURL.idl:60 > + [Conditional=BLOB] [ConvertNullStringTo=Undefined] static DOMString createObjectURL(in Blob blob); > + [Conditional=BLOB] static void revokeObjectURL(in DOMString url); I landed the support for static. The static keyword needs to come before the extended attributes. static [Conditional=BLOB, ConvertNullStringTo=Undefined] DOMString createObjectURL(in Blob blob); static [Conditional=BLOB] void revokeObjectURL(in DOMString url);

(In reply to comment #32) > (From update of attachment 116565 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116565&action=review > > > Source/WebCore/html/DOMURL.idl:60 > > + [Conditional=BLOB] [ConvertNullStringTo=Undefined] static DOMString createObjectURL(in Blob blob); > > + [Conditional=BLOB] static void revokeObjectURL(in DOMString url); > > I landed the support for static. > > The static keyword needs to come before the extended attributes. > > static [Conditional=BLOB, ConvertNullStringTo=Undefined] DOMString createObjectURL(in Blob blob); > static [Conditional=BLOB] void revokeObjectURL(in DOMString url); Its not working somehow. I get compilation errors for DerievedSources as the conversion becomes like this - static #ifdef BLOB createObjectURL(Blob* blob)

(In reply to comment #33) > Its not working somehow. I get compilation errors for DerievedSources as the conversion becomes like this - > > static #ifdef BLOB createObjectURL(Blob* blob) I don't see this problem. Can you upload the patch that gives you this?

Created attachment 118024 [details] Patch Tried to get the patch working somehow. Not sure if all correct yet. Erik, need some assistance in the patch. May be u can review and let me know missing parts. I found the problem in having "static" keyword before "[Conditional]". Its because there is function overloading in the idl file. When i removed one of them (MediaStream parameter API) it was building fine. But in normal case i get errors - DerivedSources/WebCore/JSDOMURL.cpp:554:1: error: stray ‘#’ in program DerivedSources/WebCore/JSDOMURL.cpp:567:2: error: #endif without #if DerivedSources/WebCore/JSDOMURL.cpp:569:1: error: stray ‘#’ in program DerivedSources/WebCore/JSDOMURL.cpp:593:2: error: #endif without #if This is bcoz it creates two separate static functions in JSDOMURL.cpp as - static EncodedJSValue JSC_HOST_CALL jsDOMURLConstructorFunctionCreateObjectURL1(ExecState* exec) static EncodedJSValue JSC_HOST_CALL jsDOMURLConstructorFunctionCreateObjectURL2(ExecState* exec) one for MediaStream & one for Blob.

Attachment 118024 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/html/DOMURL.cpp:92: Extra space after ( in if [whitespace/parens] [5] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 118024 [details] Patch Attachment 118024 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10739098

Comment on attachment 118024 [details] Patch Attachment 118024 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10734959 New failing tests: http/tests/fileapi/create-blob-url-from-data-url.html fast/files/apply-blob-url-to-xhr.html fast/dom/HTMLAnchorElement/anchor-nodownload-set.html editing/pasteboard/data-transfer-items-image-png.html fast/dom/HTMLAnchorElement/anchor-nodownload.html fast/dom/DOMURL/domurl-attribute-hostname.html fast/dom/DOMURL/domurl-attribute-hash.html fast/files/apply-blob-url-to-img.html fast/dom/DOMURL/domurl-attribute-search.html fast/dom/HTMLAnchorElement/anchor-download-unset.html fast/dom/HTMLAnchorElement/anchor-download.html

Comment on attachment 118024 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118024&action=review > Source/WebCore/html/DOMURL.idl:59 > #if defined(ENABLE_MEDIA_STREAM) && ENABLE_MEDIA_STREAM > - [ConvertNullStringTo=Undefined] DOMString createObjectURL(in MediaStream stream); > + [Conditional=BLOB] static DOMString createObjectURL(in MediaStream stream); > #endif > - [ConvertNullStringTo=Undefined] DOMString createObjectURL(in Blob blob); > - void revokeObjectURL(in DOMString url); > + [Conditional=BLOB] static DOMString createObjectURL(in Blob blob); > + static [Conditional=BLOB] void revokeObjectURL(in DOMString url); These needs to be static [Conditional=BLOB] type name(params); However, beware that you will need to keep the #if here since overloaded methods cannot have different Conditional. Maybe a comment is due?

Created attachment 118104 [details] Fixed idl and removed static initializers

Kaustubh: I uploaded a patch that compiles. I had to fix the IDL and fix the static initializers. One thing that we need to do is to clean up the URLs created by createObjectURL when the context is destroyed. Before this change window.webkitURL returned an instance of DOMURL (which extends ContextDestructionObserver) which cause contextDestroyed to get called when the context was destroyed. In our case we might not have any ContextDestructionObserver but I think we can create one in createObjectURL.

Created attachment 118145 [details] WIP This puts pack the security origin code. createObjectURL works but I have yet to add the eviction code

Comment on attachment 118145 [details] WIP Are you looking for feedback, or is this just a checkpoint?

(In reply to comment #43) > (From update of attachment 118145 [details]) > Are you looking for feedback, or is this just a checkpoint? Not yet. I'm helping Kaustubh getting things closer to working. I'll try to remember to do --no-review in the future. Thanks

Comment on attachment 118145 [details] WIP Attachment 118145 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10752006

(In reply to comment #41) > Kaustubh: I uploaded a patch that compiles. I had to fix the IDL and fix the static initializers. > > One thing that we need to do is to clean up the URLs created by createObjectURL when the context is destroyed. Before this change window.webkitURL returned an instance of DOMURL (which extends ContextDestructionObserver) which cause contextDestroyed to get called when the context was destroyed. In our case we might not have any ContextDestructionObserver but I think we can create one in createObjectURL. Thanks a lot Erik. I got where I needed fixes. But somehow, the patch is still failing for gtk port for the same errors "static" for overloaded functions. I am trying to fix those and also implement ContextDestructionObvserver.

(In reply to comment #46) > Thanks a lot Erik. I got where I needed fixes. But somehow, the patch is still failing for gtk port for the same errors "static" for overloaded functions. I am trying to fix those and also implement ContextDestructionObvserver. Let me finish the ContextDestructionObserver part. I'm already heads down into it.

Created attachment 118290 [details] WIP

Comment on attachment 118290 [details] WIP This has the expected behavior regarding ContextDestructionObserver and the URLs gets cleaned up when the ScriptExecutionContext gets destroyed.

Created attachment 118365 [details] Updated patch Will be submitting for the r? & cq? after bug - 74068 is fixed. Till then this will fail to build on gtk port.

Comment on attachment 118365 [details] Updated patch Attachment 118365 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10797328 New failing tests: fast/dom/DOMURL/domurl-attribute-hash.html fast/dom/DOMURL/domurl-attribute-hostname.html fast/dom/DOMURL/domurl-attribute-search.html

Created attachment 118687 [details] Updated patch Fixed test cases.

Erik can you review this patch? thanks.

Comment on attachment 118687 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118687&action=review I'm not a webkit reviewer but I'm happy to give you feedback anyway. This needs more tests. You need to test every property getter and setter and make sure that all the code paths are tested. For example, in DOMURL::setHost there are a lot of branches. You need to have about 10 different tests to cover all the possible branches in there. I also feel like there is a bit too much logic in here. It seems like we could move more code into KURL. > Source/WebCore/html/DOMURL.cpp:257 > + unsigned port = value.toUInt(); How do we handle non numbers in value? > Source/WebCore/html/DOMURL.cpp:282 > + m_href.setQuery(newSearch); I didn't see any change in KURL::setQuery to handle '#' in the search string. Please add a test that ensures that setting search with '#' works as expected. > Source/WebCore/html/DOMURL.cpp:301 > +void DOMURL::setFilename(const String&) > +{ > + // FIXME: Implement me. > + notImplemented(); > +} These should be implemented before we expose these to JS. > Source/WebCore/html/DOMURL.idl:52 > + DOMString getParameter(in DOMString name); > + void appendParameter(in DOMString name, in DOMString values); > + void appendParameter(in DOMString name, in DOMString[] values); > + void clearParameter(in DOMString name); I would remove these if you do not plan to implement them in the first patch. > Source/WebCore/html/DOMURL.idl:53 > + Missing DOMString[] getParameterAll(in DOMString name); > Source/WebCore/page/DOMWindow.idl:816 > + attribute DOMURLConstructor webkitURL; Can you file a bug that this should be renamed to URLConstructor? We should never expose DOMURL to script. > LayoutTests/fast/dom/DOMURL/check-instanceof-domurl-functions.html:36 > +shouldBeFalse("url.hasOwnProperty('clearParameter')") The opposite case is also interesting. But in general it is better to actually test their functionality than their presence since the functionality will implicitly cover the presence. shouldBeTrue("'getParameter' in url"); > LayoutTests/fast/dom/DOMURL/domurl-attribute-hash.html:12 > +debug("Hash value start with #"); > +var domurl = new webkitURL("https://www.mydomain.com:8080/path/testurl.html#middle"); > +shouldBeEqualToString("domurl.hash", "#middle"); You need to test the setter too.

Comment on attachment 118687 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118687&action=review A few quick comments which are in no way comprehensive. > Source/WebCore/html/DOMURL.cpp:36 > +#if ENABLE(BLOB) Don't exclude headers using an"#if" (unless absolutely necessary in order to keep some platforms building). I don't think that is the case here. > Source/WebCore/html/DOMURL.cpp:58 > + : ContextDestructionObserver(scriptExecutionContext) { } {} should be on the next lines. > Source/WebCore/html/DOMURL.cpp:60 > + virtual void contextDestroyed() Should not be inlined in the class since we don't want this function inlined. Note that this leaks in the same way the current class does. contextDestroyed isn't guaranteed to be called before the destructor. If the destructor is called first, then this clean up (for the registeries) should run still. > Source/WebCore/html/DOMURL.cpp:74 > + publicURLManagerMap().remove(context); not threadsafe. > Source/WebCore/html/DOMURL.cpp:92 > + return staticPublicURLManagers; Not threadsafe (which is bad because this can be used from workers). > Source/WebCore/html/DOMURL.cpp:98 > + OwnPtr<PublicURLManager>& manager = map.add(scriptExecutionContext, nullptr).first->second; not threadsafe. > Source/WebCore/html/DOMURL.h:45 > + static PassRefPtr<DOMURL> create(const String& url) { return adoptRef(new DOMURL(url, emptyString())); } Where are these called? > Source/WebCore/html/DOMURL.idl:-32 > - NoStaticTables Why is this being removed? afaik, It needs to be here since this is exposed in Web Workers.

(In reply to comment #55) Hi David, thanks for the review. Please find my comments inline. This patch has been divided into two parts to make review/committing easy. One part of making createObjectURL & revokeObjectURL static has been already landed. I will be updating this patch soon and will update new one. > (From update of attachment 118687 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118687&action=review > > A few quick comments which are in no way comprehensive. > > > Source/WebCore/html/DOMURL.cpp:36 > > +#if ENABLE(BLOB) > > Don't exclude headers using an"#if" (unless absolutely necessary in order to keep some platforms building). I don't think that is the case here. > I agree with this. But in this scenario, the header under #if will be required only in that case. Unless otherwise it will get built but the only prob will be it will unnecessary increase code segment AFAIK. Please guide if wrong. > > Source/WebCore/html/DOMURL.cpp:58 > > + : ContextDestructionObserver(scriptExecutionContext) { } > > {} should be on the next lines. > Corrected. > > Source/WebCore/html/DOMURL.cpp:60 > > + virtual void contextDestroyed() > > Should not be inlined in the class since we don't want this function inlined. > > Note that this leaks in the same way the current class does. > > contextDestroyed isn't guaranteed to be called before the destructor. > > If the destructor is called first, then this clean up (for the registeries) should run still. > contextDestroyed will get called first whenever ContextDestructionObserver::contextDestroyed gets called. This will ensure the cleanup of data related to PublicURLManager class. As this class is inline completely I tried to keep functions as well inline. > > Source/WebCore/html/DOMURL.cpp:74 > > + publicURLManagerMap().remove(context); > > not threadsafe. > > > Source/WebCore/html/DOMURL.cpp:92 > > + return staticPublicURLManagers; > > Not threadsafe (which is bad because this can be used from workers). > > > Source/WebCore/html/DOMURL.cpp:98 > > + OwnPtr<PublicURLManager>& manager = map.add(scriptExecutionContext, nullptr).first->second; > > not threadsafe. > Should I be using ThreadGlobalData for this? I'm not sure. > > Source/WebCore/html/DOMURL.h:45 > > + static PassRefPtr<DOMURL> create(const String& url) { return adoptRef(new DOMURL(url, emptyString())); } > > Where are these called? > Updated with new URL specs. > > Source/WebCore/html/DOMURL.idl:-32 > > - NoStaticTables > > Why is this being removed? > > afaik, It needs to be here since this is exposed in Web Workers. Agree. Will need to put it back.

I'm closing this since we have a meta bug *** This bug has been marked as a duplicate of bug 74385 ***