Bug 78214 – Rename DOMURL to URL in the bindings

• Component: DOM
• Status: RESOLVED FIXED

[Kaustubh Atrawalkar]

Need to rename DOMURL to URL as per specs.

[Kaustubh Atrawalkar]

This will fix the failing test cases in global-constructors.html

[Kaustubh Atrawalkar]

Created attachment 126289 [details]
Patch

[Erik Arvidsson]

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]

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

Created attachment 126450 [details]
Updated Patch

[Kaustubh Atrawalkar]

Created attachment 126458 [details]
Updated Patch

I had missed new changes in IDL files from Haraken. Fixed those.

[Eric Seidel (no email)]

Based on Arv's comments, lets do this later.

[Eric Seidel (no email)]

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]

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

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]

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]

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]

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

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]

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

Created attachment 128672 [details]
Updated Patch

Updated patch for Renaming DOMURL to URL using InterfaceName. Can you review once?

[Erik Arvidsson]

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]

Created attachment 129003 [details]
Updated Patch

Fixed test cases. Rebaslined. Eric/Adam can you review once?

[WebKit Review Bot]

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]

Created attachment 129841 [details]
Rebaseline Patch

Patch need to be rebaselined. Updated.

[Kentaro Hara]

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 on attachment 129841 [details]
Rebaseline Patch

Clearing flags on attachment: 129841

Committed r109574: <http://trac.webkit.org/changeset/109574>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.