RESOLVED FIXED 78214
Rename DOMURL to URL in the bindings
https://bugs.webkit.org/show_bug.cgi?id=78214
Summary Rename DOMURL to URL in the bindings
Kaustubh Atrawalkar
Reported 2012-02-09 01:18:20 PST
Need to rename DOMURL to URL as per specs.
Attachments
Patch (68.80 KB, patch)
2012-02-09 05:39 PST, Kaustubh Atrawalkar
no flags
Updated Patch (40.48 KB, patch)
2012-02-09 22:02 PST, Kaustubh Atrawalkar
no flags
Updated Patch (39.90 KB, patch)
2012-02-09 22:46 PST, Kaustubh Atrawalkar
no flags
Updated Patch (13.25 KB, patch)
2012-02-24 00:21 PST, Kaustubh Atrawalkar
no flags
Updated Patch (11.12 KB, patch)
2012-02-27 03:07 PST, Kaustubh Atrawalkar
abarth: review+
webkit.review.bot: commit-queue-
Rebaseline Patch (11.20 KB, patch)
2012-03-02 00:54 PST, Kaustubh Atrawalkar
no flags
Kaustubh Atrawalkar
Comment 1 2012-02-09 01:19:44 PST
This will fix the failing test cases in global-constructors.html
Kaustubh Atrawalkar
Comment 2 2012-02-09 05:39:02 PST
Erik Arvidsson
Comment 3 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
Kaustubh Atrawalkar
Comment 4 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
Kaustubh Atrawalkar
Comment 5 2012-02-09 22:02:07 PST
Created attachment 126450 [details] Updated Patch
Kaustubh Atrawalkar
Comment 6 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.
Eric Seidel (no email)
Comment 7 2012-02-10 10:25:34 PST
Based on Arv's comments, lets do this later.
Eric Seidel (no email)
Comment 8 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).
Kaustubh Atrawalkar
Comment 9 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.
Adam Barth
Comment 10 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.
Erik Arvidsson
Comment 11 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.
Kaustubh Atrawalkar
Comment 12 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?
Erik Arvidsson
Comment 13 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.
Erik Arvidsson
Comment 14 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.
Kaustubh Atrawalkar
Comment 15 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 :)
Kaustubh Atrawalkar
Comment 16 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?
Erik Arvidsson
Comment 17 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.
Kaustubh Atrawalkar
Comment 18 2012-02-27 03:07:02 PST
Created attachment 129003 [details] Updated Patch Fixed test cases. Rebaslined. Eric/Adam can you review once?
WebKit Review Bot
Comment 19 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
Kaustubh Atrawalkar
Comment 20 2012-03-02 00:54:34 PST
Created attachment 129841 [details] Rebaseline Patch Patch need to be rebaselined. Updated.
Kentaro Hara
Comment 21 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.
WebKit Review Bot
Comment 22 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>
WebKit Review Bot
Comment 23 2012-03-02 07:15:12 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.