[webkit-reviews] review granted: [Bug 16538] KURL should use String instead of DeprecatedString : [Attachment 19095] patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 16538: KURL should use String instead of DeprecatedString
http://bugs.webkit.org/show_bug.cgi?id=16538

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
I survived!

Seems odd to have this outside of setPort:
	  case Port: {
180	      int port = str.toInt();
181	      if (port < 0 || port > 0xFFFF)
182		  port = 0;
183	      url.setPort(port);
184	      break;
185	  }

I think you should s/string/url/ on these comments:
KJS::JSValue* jsStringOrNull(const KURL&); // null if the string is null

These could be one line ifs:
102		if (HTMLAnchorElement* anchor = [self anchorElement]) {
1103		     NSURL *href = anchor->href();
1104		     return href;
1105		 }
} else if (m_renderer->isImage() && m_renderer->element() &&
m_renderer->element()->hasTagName(imgTag)) {
1107		 NSURL *src =
static_cast<HTMLImageElement*>(m_renderer->element())->src();
1108		 return src;
1109	     }

Better if you could file a bug:
2185		     // FIXME: What's the point of changing the value of a
local variable
2186		     // (title) that's not looked at afterward? Obviously
there's no test
2187		     // case for this!
2188		     if (!n->isHTMLElement()) {
2189			 String modifiedTitle = title.domString();
2190			 title = modifiedTitle.replace('&', "&&");
2191		     }
21872192 #endif

Sigh.  Sadly KURL Node::baseURI() const is yet-another recursive walk through
your parent chain using virtual methods. :(

We might want to add mailtoProtocol, httpProtocol, ftpProtocol AtomicStrings
some day.

Missing space before ':'
page->chrome()->addMessageToConsole(HTMLMessageSource,
1481	     isWarning(errorCode) ? WarningMessageLevel: ErrorMessageLevel,
1482	     message, lineNumber, document->url().string());


Those would be different depending on how it's exposed to the DOM bindings:
      if (m_frame->tree() && m_frame->tree()->parent())
-	   return "";	    
+	  return KURL();

Seems like something we should test:
	 KURL r = dl->doc()->url();
-	   if (r.protocol().startsWith("http") && r.path().isEmpty())
+	  if ((r.protocolIs("http") || r.protocolIs("https")) &&
r.path().isEmpty())
	      r.setPath("/");

Why?
-	   DeprecatedString domain = r.host();
-	   if (dl->doc()->isHTMLDocument())
-	       domain =
static_cast<HTMLDocument*>(dl->doc())->domain().deprecatedString();

Looks good to me!