[webkit-reviews] review granted: [Bug 16214] rename KURL to URL : [Attachment 17837] newer URL -> url patch (now with Windows support and changelogs)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 11 16:00:06 PST 2007


Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 16214: rename KURL to URL
http://bugs.webkit.org/show_bug.cgi?id=16214

Attachment 17837: newer URL -> url patch (now with Windows support and
changelogs)
http://bugs.webkit.org/attachment.cgi?id=17837&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+	 Scrub URL out of the tree in preparation of ranameing KURL to URL.

Renaming is spelled wrong here. Also "in preparation of" is not very good
English.

+	 * WebCore.base.exp:
+	 * bindings/js/kjs_events.cpp:
+	 (WebCore::JSLazyEventListener::parseCode):

I don't think it is all that helpful to list all the files and functions when
the only change is renaming. It would be neat to list only files that have
interesting changes; something other than just renaming. Or we could just omit
the file list entirely. Or leave it as is, not sure.

1637	      DeprecatedString dstUrl =
activeFrame->loader()->completeURL(DeprecatedString(args[0]->toString(exec))).u
rl();
 1637	      String dstUrl =
activeFrame->loader()->completeURL(args[0]->toString(exec)).deprecatedString();


This is strange. It's changed to call deprecatedString(), but the type of the
variable was changed to String!

48	return m_url.url().isNull();
 48	return m_url.deprecatedString().isNull();

We should probably add a KURL.isNull() for this. Not sure why I didn't.

What about platforms other than Mac and Windows.

r=me if you fix the things I mentioned above.


More information about the webkit-reviews mailing list