[webkit-reviews] review requested: [Bug 16538] KURL should use String instead of DeprecatedString : [Attachment 18785] Patch

[bugzilla-daemon at webkit.org]

Marvin Decker <marv.decker at gmail.com> has asked  for review:
Bug 16538: KURL should use String instead of DeprecatedString
http://bugs.webkit.org/show_bug.cgi?id=16538

Attachment 18785: Patch
http://bugs.webkit.org/attachment.cgi?id=18785&action=edit

------- Additional Comments from Marvin Decker <marv.decker at gmail.com>
This replaces String with DeprecatedString in KURL's interface.

The internal representation is still a DeprecatedString. I tried two different
timer to do the change the internals, but it is challenging because the parsing
code relies on the utf-16/utf-8 duality of the string. This might be more
doable with a fresh look. MacDome was also expressing interest in doing this
task. This interface patch is designed to be the first step in fixing KURL;
clearly the internals will need to be fixed, but these are both complicated
patches which I think are better separated.

There are more conversions which will make the code slower, although I am
pretty sure it will not be measurable: About half the callers already had a
String, and this patch does not change the number of conversions in those
cases. The other half (which had a DeprecatedString) will need an extra string
conversion. This also breaks the optimization where the string inside KURL
would just reference the argument without a malloc in some cases. I left this
in with some FIXMEs for the arguments, so it will be easy to fix when the
internals are also converted to String.

-----

I made a number of changes to KURL aside from just String conversions:

Added a lot of comments to the header file

I made the constructor explicit. I kept finding places where there was a
String, and it was being converted to a KURL when being passed to a function
without the code realizing that this was a potentially slow operation. This
highlights some really stupid usage, often involving resolution of a relative
URL with a string base. For example, in CSSParser::parseBorderImage:
  context.commitImage(new CSSImageValue(
      KURL(KURL(styleElement->baseURL()), uri).string(), styleElement));
Now the fact that baseURL should probably return a KURL is more obvious, or, if
not, at least tells you how expensive the operation you are doing is. I would
say everybody should use KURL and the above code should be:
  context.commitImage(new CSSImageValue(
      KURL(styleElement->baseURL(), uri), styleElement));
A lot of the diff relates to this change.

I added a new function, protocolIs(const char*). A lot of users were doing this
pattern:
  if (url.protocol() == "http");
This is bad for several reasons. First, it does a case-sensitive compare, which
is incorrect (KURL does not canonicalize the scheme). To work around this, some
callers do something like this:
  if (url.protocol().lower() == "http")
This makes the second problem even worse, which is that lots of string
temporaries are getting created for an operation which doesn't need it.  My new
function handles different cases and does not create new objects, while making
the usage more clear.

I fixed some signed/unsigned types in findHostnamesInMailToURL and other places
which are suspicious and generate warnings.

-----

I made two changes outside of KURL relating to bug 16538, which this patch
fixes. Some code expects the string in a KURL to be non-NULL, despite there
being many ways to make a NULL KURL. This code is not tripped because URLs
oftem go through string conversions, i.e. KURL->String->KURL, and this process
will lose the isNull-ness of the string and it will report empty instead. I
made a fix in IconDatabase.cpp, and one in DocLoader.cpp (which I previously
filed bug 16485 for).

In loader.cpp I removed some dead code.