Bug 78214 - Rename DOMURL to URL in the bindings
Summary: Rename DOMURL to URL in the bindings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://dvcs.w3.org/hg/url/raw-file/ti...
Keywords:
Depends on: 79384
Blocks: 74385
  Show dependency treegraph
 
Reported: 2012-02-09 01:18 PST by Kaustubh Atrawalkar
Modified: 2012-03-02 07:15 PST (History)
11 users (show)

See Also:


Attachments
Patch (68.80 KB, patch)
2012-02-09 05:39 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Updated Patch (40.48 KB, patch)
2012-02-09 22:02 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Updated Patch (39.90 KB, patch)
2012-02-09 22:46 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Updated Patch (13.25 KB, patch)
2012-02-24 00:21 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Updated Patch (11.12 KB, patch)
2012-02-27 03:07 PST, Kaustubh Atrawalkar
abarth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Rebaseline Patch (11.20 KB, patch)
2012-03-02 00:54 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kaustubh Atrawalkar 2012-02-09 01:18:20 PST
Need to rename DOMURL to URL as per specs.
Comment 1 Kaustubh Atrawalkar 2012-02-09 01:19:44 PST
This will fix the failing test cases in global-constructors.html
Comment 2 Kaustubh Atrawalkar 2012-02-09 05:39:02 PST
Created attachment 126289 [details]
Patch
Comment 3 Erik Arvidsson 2012-02-09 10:26:57 PST
Comment on attachment 126289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126289&action=review

> Source/WebCore/page/DOMWindow.idl:785
> +        attribute [Conditional=BLOB] URLConstructor URL;

I think we should keep this as webkitURL for a little longer. We are not even close to implementing the whole thing so and there is no rush to rename this.

> Source/WebCore/workers/WorkerContext.idl:104
> +        attribute [Conditional=BLOB] URLConstructor URL;

Keep as webkitURL for now
Comment 4 Kaustubh Atrawalkar 2012-02-09 21:55:09 PST
(In reply to comment #3)
> (From update of attachment 126289 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126289&action=review
> 
> > Source/WebCore/page/DOMWindow.idl:785
> > +        attribute [Conditional=BLOB] URLConstructor URL;
> 
> I think we should keep this as webkitURL for a little longer. We are not even close to implementing the whole thing so and there is no rush to rename this.
> 
I agree, but just to make it compatible with other browsers. Currently, I saw all sites use - "window.webkitURL || window.URL"
To avoid this I thought to remove prefix webkit and made it as per specs requirement. 
> > Source/WebCore/workers/WorkerContext.idl:104
> > +        attribute [Conditional=BLOB] URLConstructor URL;
> 
> Keep as webkitURL for now

Surely, guess we can make it later. Will upload it with webkitURL
Comment 5 Kaustubh Atrawalkar 2012-02-09 22:02:07 PST
Created attachment 126450 [details]
Updated Patch
Comment 6 Kaustubh Atrawalkar 2012-02-09 22:46:45 PST
Created attachment 126458 [details]
Updated Patch

I had missed new changes in IDL files from Haraken. Fixed those.
Comment 7 Eric Seidel (no email) 2012-02-10 10:25:34 PST
Based on Arv's comments, lets do this later.
Comment 8 Eric Seidel (no email) 2012-02-10 10:57:47 PST
Comment on attachment 126458 [details]
Updated Patch

Cleared review? from attachment 126458 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment 9 Kaustubh Atrawalkar 2012-02-10 19:14:21 PST
(In reply to comment #7)
> Based on Arv's comments, lets do this later.

I think what Arv meant here was to keep java script binding side name as webkitURL and change the DOMURLConstructor to URLConstructor. So that when implementation is ready just changing the two lines in DOMWindow.idl & WorkerContext.idl would migrate from JavaScript side later. I need to push big chunk of URL decomposition attributes. But for that it would be better if this DOMURL to URL change goes in. correct me if wrong. Thanks.
Comment 10 Adam Barth 2012-02-10 19:45:02 PST
We likely want to continue to call the implementation DOMURL so as to be distinct from URL, which is what we plan to rename KURL to.
Comment 11 Erik Arvidsson 2012-02-15 14:28:16 PST
Sorry for letting this fall off my radar.

Keeping the C++ name DOMURL seems better but we should make sure that we only expose the name "URL" to the web page.

I think we should name the constructor webkitURL until we at least have all the methods implemented.
Comment 12 Kaustubh Atrawalkar 2012-02-16 02:45:52 PST
Reopening as per Erik's comment. Should I provide the patch only to rename DOMURLConstructor to URLConstructor and migrate DOMURL to URL in WebIDL files?
Comment 13 Erik Arvidsson 2012-02-16 08:38:00 PST
(In reply to comment #12)
> Reopening as per Erik's comment. Should I provide the patch only to rename DOMURLConstructor to URLConstructor and migrate DOMURL to URL in WebIDL files?

Yes please.
Comment 14 Erik Arvidsson 2012-02-21 17:22:47 PST
I'm working on the renaming of the bindings in bug 78721. Once that is done it should be trivial to fix this bug.
Comment 15 Kaustubh Atrawalkar 2012-02-21 22:50:28 PST
(In reply to comment #14)
> I'm working on the renaming of the bindings in bug 78721. Once that is done it should be trivial to fix this bug.

Great. That would really help to fix this. I was wondering since last week how to fix this without changing file names. Now I hope it will be easy after your patch :)
Comment 16 Kaustubh Atrawalkar 2012-02-24 00:21:58 PST
Created attachment 128672 [details]
Updated Patch

Updated patch for Renaming DOMURL to URL using InterfaceName. Can you review once?
Comment 17 Erik Arvidsson 2012-02-24 09:22:23 PST
Comment on attachment 128672 [details]
Updated Patch

Can you see if some of the tests can be updated?

View in context: https://bugs.webkit.org/attachment.cgi?id=128672&action=review

> LayoutTests/platform/gtk/fast/dom/prototype-inheritance-2-expected.txt:234
> +PASS Window from inner
> +PASS WindowConstructor from inner.document.forms.testForm.0.ownerDocument.defaultView.constructor
> +PASS WindowPrototype from inner.document.forms.testForm.0.ownerDocument.defaultView.__proto__

Please rebaseline

> LayoutTests/platform/mac/fast/js/constructor-length-expected.txt:31
> +FAIL URL.length should be 0. Threw exception ReferenceError: Can't find variable: URL

Please fix this

> LayoutTests/platform/mac/fast/js/global-constructors-expected.txt:340
> +FAIL webkitURL.toString() should be [object webkitURLConstructor]. Was [object URLConstructor].

Not much to do about this one until we strip the prefix.
Comment 18 Kaustubh Atrawalkar 2012-02-27 03:07:02 PST
Created attachment 129003 [details]
Updated Patch

Fixed test cases. Rebaslined. Eric/Adam can you review once?
Comment 19 WebKit Review Bot 2012-03-02 00:13:12 PST
Comment on attachment 129003 [details]
Updated Patch

Rejecting attachment 129003 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
saving rejects to file LayoutTests/platform/mac/fast/js/constructor-length-expected.txt.rej
patching file LayoutTests/platform/mac/fast/js/global-constructors-expected.txt
patching file LayoutTests/platform/qt/fast/dom/prototype-inheritance-2-expected.txt
patching file LayoutTests/platform/qt/fast/js/global-constructors-expected.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11757822
Comment 20 Kaustubh Atrawalkar 2012-03-02 00:54:34 PST
Created attachment 129841 [details]
Rebaseline Patch

Patch need to be rebaselined. Updated.
Comment 21 Kentaro Hara 2012-03-02 01:27:29 PST
Comment on attachment 129841 [details]
Rebaseline Patch

The rebaselining looks OK. Based on abarth's previous r+, let me r+ it.
Comment 22 WebKit Review Bot 2012-03-02 07:15:06 PST
Comment on attachment 129841 [details]
Rebaseline Patch

Clearing flags on attachment: 129841

Committed r109574: <http://trac.webkit.org/changeset/109574>
Comment 23 WebKit Review Bot 2012-03-02 07:15:12 PST
All reviewed patches have been landed.  Closing bug.