Bug 71968 - Implement URL API
Summary: Implement URL API
Status: RESOLVED DUPLICATE of bug 74385
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Erik Arvidsson
URL: http://dvcs.w3.org/hg/url/raw-file/ti...
Keywords:
Depends on: 76816 72998 74068
Blocks: 74385
  Show dependency treegraph
 
Reported: 2011-11-09 16:38 PST by Erik Arvidsson
Modified: 2012-05-04 15:49 PDT (History)
14 users (show)

See Also:


Attachments
Working Progress Patch (7.10 KB, text/plain)
2011-11-14 04:11 PST, Kaustubh Atrawalkar
no flags Details
Patch (9.58 KB, patch)
2011-11-15 01:29 PST, Kaustubh Atrawalkar
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Progress Patch 2 (10.75 KB, patch)
2011-11-15 04:18 PST, Kaustubh Atrawalkar
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (10.88 KB, patch)
2011-11-16 23:40 PST, Kaustubh Atrawalkar
rniwa: review-
Details | Formatted Diff | Diff
IDL Attributes implementation (46.72 KB, patch)
2011-11-18 03:34 PST, Kaustubh Atrawalkar
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Working Progress Patch (34.45 KB, patch)
2011-11-23 01:34 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Update Patch (32.86 KB, patch)
2011-11-24 23:02 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Patch (35.88 KB, patch)
2011-12-06 04:35 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Fixed idl and removed static initializers (19.30 KB, patch)
2011-12-06 13:45 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
WIP (23.68 KB, patch)
2011-12-06 17:00 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
WIP (42.61 KB, patch)
2011-12-07 16:59 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Updated patch (51.41 KB, patch)
2011-12-08 05:06 PST, Kaustubh Atrawalkar
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (40.62 KB, patch)
2011-12-10 00:28 PST, Kaustubh Atrawalkar
levin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2011-11-09 16:38:35 PST
http://dvcs.w3.org/hg/url/raw-file/tip/Overview.html
Comment 1 Erik Arvidsson 2011-11-09 16:41:58 PST
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.
Comment 2 Kaustubh Atrawalkar 2011-11-11 01:48:24 PST
I wish to implement this. Should I provide the working progress patch ?
Comment 3 Erik Arvidsson 2011-11-11 14:21:15 PST
(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
Comment 4 Kaustubh Atrawalkar 2011-11-14 04:11:11 PST
(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?
Comment 5 Kaustubh Atrawalkar 2011-11-14 04:11:53 PST
Created attachment 114916 [details]
Working Progress Patch

Added IDL attributes.
Comment 6 Kaustubh Atrawalkar 2011-11-15 01:29:40 PST
Created attachment 115121 [details]
Patch
Comment 7 WebKit Review Bot 2011-11-15 01:39:51 PST
Comment on attachment 115121 [details]
Patch

Attachment 115121 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10372465
Comment 8 Kaustubh Atrawalkar 2011-11-15 04:18:03 PST
Created attachment 115138 [details]
Progress Patch 2

Added empty functions. Fixed idl file.
Comment 9 WebKit Review Bot 2011-11-15 19:52:15 PST
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 10 Gustavo Noronha (kov) 2011-11-15 22:20:32 PST
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 11 Early Warning System Bot 2011-11-16 00:26:09 PST
Comment on attachment 115138 [details]
Progress Patch 2

Attachment 115138 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10486672
Comment 12 Kaustubh Atrawalkar 2011-11-16 23:40:07 PST
Created attachment 115532 [details]
Updated patch
Comment 13 Ryosuke Niwa 2011-11-17 01:18:52 PST
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!
Comment 14 Kaustubh Atrawalkar 2011-11-18 03:34:25 PST
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 15 WebKit Review Bot 2011-11-18 09:36:30 PST
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
Comment 16 Kaustubh Atrawalkar 2011-11-18 23:05:42 PST
(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.
Comment 17 Adam Barth 2011-11-18 23:32:46 PST
> 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 18 Erik Arvidsson 2011-11-19 09:05:49 PST
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()
Comment 19 Erik Arvidsson 2011-11-19 10:19:25 PST
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');
Comment 20 Kaustubh Atrawalkar 2011-11-20 23:24:11 PST
(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()
Comment 21 Erik Arvidsson 2011-11-21 16:07:36 PST
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.
Comment 22 Kaustubh Atrawalkar 2011-11-21 20:08:30 PST
(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?
Comment 23 Erik Arvidsson 2011-11-22 09:54:05 PST
(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");
Comment 24 Kaustubh Atrawalkar 2011-11-22 19:39:28 PST
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.
Comment 25 Kaustubh Atrawalkar 2011-11-23 01:34:10 PST
Created attachment 116327 [details]
Working Progress Patch
Comment 26 Erik Arvidsson 2011-11-23 10:21:19 PST
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>
Comment 27 Kaustubh Atrawalkar 2011-11-24 23:02:52 PST
Created attachment 116565 [details]
Update Patch

Updated patch after review comments.
Comment 28 Erik Arvidsson 2011-11-26 14:12:02 PST
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.
Comment 29 Kaustubh Atrawalkar 2011-11-28 03:26:01 PST
(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?
Comment 30 Erik Arvidsson 2011-11-28 09:51:39 PST
(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)
Comment 31 Kaustubh Atrawalkar 2011-11-29 05:02:47 PST
(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 32 Erik Arvidsson 2011-11-29 17:00:30 PST
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);
Comment 33 Kaustubh Atrawalkar 2011-12-02 00:22:15 PST
(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)
Comment 34 Erik Arvidsson 2011-12-02 09:17:35 PST
(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?
Comment 35 Kaustubh Atrawalkar 2011-12-06 04:35:30 PST
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.
Comment 36 WebKit Review Bot 2011-12-06 04:38:02 PST
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 37 Gyuyoung Kim 2011-12-06 04:56:37 PST
Comment on attachment 118024 [details]
Patch

Attachment 118024 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10739098
Comment 38 WebKit Review Bot 2011-12-06 08:11:36 PST
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 39 Erik Arvidsson 2011-12-06 10:33:27 PST
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?
Comment 40 Erik Arvidsson 2011-12-06 13:45:26 PST
Created attachment 118104 [details]
Fixed idl and removed static initializers
Comment 41 Erik Arvidsson 2011-12-06 13:57:41 PST
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.
Comment 42 Erik Arvidsson 2011-12-06 17:00:02 PST
Created attachment 118145 [details]
WIP

This puts pack the security origin code. createObjectURL works but I have yet to add the eviction code
Comment 43 Adam Barth 2011-12-06 17:11:08 PST
Comment on attachment 118145 [details]
WIP

Are you looking for feedback, or is this just a checkpoint?
Comment 44 Erik Arvidsson 2011-12-06 17:44:43 PST
(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 45 Collabora GTK+ EWS bot 2011-12-06 19:28:21 PST
Comment on attachment 118145 [details]
WIP

Attachment 118145 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10752006
Comment 46 Kaustubh Atrawalkar 2011-12-07 03:04:58 PST
(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.
Comment 47 Erik Arvidsson 2011-12-07 08:15:30 PST
(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.
Comment 48 Erik Arvidsson 2011-12-07 16:59:12 PST
Created attachment 118290 [details]
WIP
Comment 49 Erik Arvidsson 2011-12-07 17:00:36 PST
Comment on attachment 118290 [details]
WIP

This has the expected behavior regarding ContextDestructionObserver and the URLs gets cleaned up when the ScriptExecutionContext gets destroyed.
Comment 50 Kaustubh Atrawalkar 2011-12-08 05:06:36 PST
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 51 WebKit Review Bot 2011-12-08 12:00:26 PST
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
Comment 52 Kaustubh Atrawalkar 2011-12-10 00:28:16 PST
Created attachment 118687 [details]
Updated patch

Fixed test cases.
Comment 53 Kaustubh Atrawalkar 2011-12-12 09:49:48 PST
Erik can you review this patch?  thanks.
Comment 54 Erik Arvidsson 2011-12-12 10:45:33 PST
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 55 David Levin 2012-01-20 01:55:17 PST
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.
Comment 56 Kaustubh Atrawalkar 2012-01-20 02:52:15 PST
(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.
Comment 57 Erik Arvidsson 2012-05-04 15:49:39 PDT
I'm closing this since we have a meta bug

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