Implement URL API

[Erik Arvidsson]

http://dvcs.w3.org/hg/url/raw-file/tip/Overview.html

[Erik Arvidsson]

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.

[Kaustubh Atrawalkar]

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

[Erik Arvidsson]

(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

[Kaustubh Atrawalkar]

(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?

[Kaustubh Atrawalkar]

Created attachment 114916 [details]
Working Progress Patch

Added IDL attributes.

[Kaustubh Atrawalkar]

Created attachment 115121 [details]
Patch

[WebKit Review Bot]

Comment on attachment 115121 [details]
Patch

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

[Kaustubh Atrawalkar]

Created attachment 115138 [details]
Progress Patch 2

Added empty functions. Fixed idl file.

[WebKit Review Bot]

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

[Gustavo Noronha (kov)]

Comment on attachment 115138 [details]
Progress Patch 2

Attachment 115138 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10483620

[Early Warning System Bot]

Comment on attachment 115138 [details]
Progress Patch 2

Attachment 115138 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10486672

[Kaustubh Atrawalkar]

Created attachment 115532 [details]
Updated patch

[Ryosuke Niwa]

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!

[Kaustubh Atrawalkar]

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.

[WebKit Review Bot]

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

[Kaustubh Atrawalkar]

(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.

[Adam Barth]

> 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.

[Erik Arvidsson]

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()

[Erik Arvidsson]

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');

[Kaustubh Atrawalkar]

(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()

[Erik Arvidsson]

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.

[Kaustubh Atrawalkar]

(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?

[Erik Arvidsson]

(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");

[Kaustubh Atrawalkar]

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.

[Kaustubh Atrawalkar]

Created attachment 116327 [details]
Working Progress Patch

[Erik Arvidsson]

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>

[Kaustubh Atrawalkar]

Created attachment 116565 [details]
Update Patch

Updated patch after review comments.

[Erik Arvidsson]

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.

[Kaustubh Atrawalkar]

(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?

[Erik Arvidsson]

(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)

[Kaustubh Atrawalkar]

(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.

[Erik Arvidsson]

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);

[Kaustubh Atrawalkar]

(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)

[Erik Arvidsson]

(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?

[Kaustubh Atrawalkar]

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.

[WebKit Review Bot]

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.

[Gyuyoung Kim]

Comment on attachment 118024 [details]
Patch

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

[WebKit Review Bot]

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

[Erik Arvidsson]

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?

[Erik Arvidsson]

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

[Erik Arvidsson]

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.

[Erik Arvidsson]

Created attachment 118145 [details]
WIP

This puts pack the security origin code. createObjectURL works but I have yet to add the eviction code

[Adam Barth]

Comment on attachment 118145 [details]
WIP

Are you looking for feedback, or is this just a checkpoint?

[Erik Arvidsson]

(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

[Collabora GTK+ EWS bot]

Comment on attachment 118145 [details]
WIP

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

[Kaustubh Atrawalkar]

(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.

[Erik Arvidsson]

(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.

[Erik Arvidsson]

Created attachment 118290 [details]
WIP

[Erik Arvidsson]

Comment on attachment 118290 [details]
WIP

This has the expected behavior regarding ContextDestructionObserver and the URLs gets cleaned up when the ScriptExecutionContext gets destroyed.

[Kaustubh Atrawalkar]

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.

[WebKit Review Bot]

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

[Kaustubh Atrawalkar]

Created attachment 118687 [details]
Updated patch

Fixed test cases.

[Kaustubh Atrawalkar]

Erik can you review this patch?  thanks.

[Erik Arvidsson]

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.

[David Levin]

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.

[Kaustubh Atrawalkar]

(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.

[Erik Arvidsson]

I'm closing this since we have a meta bug

*** This bug has been marked as a duplicate of bug 74385 ***