|Summary:||Rename DOMURL to URL in the bindings|
|Product:||WebKit||Reporter:||Kaustubh Atrawalkar <kaustubh.ra>|
|Severity:||Normal||CC:||abarth, arv, eric, haraken, kaustubh.ra, levin, ojan, ossy, rakuco, vestbo, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:||79384|
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 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
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.