[webkit-reviews] review denied: [Bug 12499] External <use> xlink:href references do not work : [Attachment 21102] Initial patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Rob Buis <rwlbuis at gmail.com>'s request
for review:
Bug 12499: External <use> xlink:href references do not work
http://bugs.webkit.org/show_bug.cgi?id=12499

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

------- Additional Comments from Darin Adler <darin at apple.com>
+    This file is part of the KDE libraries

I don't think it's helpful to include this in WebKit source files.

 #include "CachedXSLStyleSheet.h"
+#if ENABLE(SVG)
+#include "CachedSVGDocument.h"
+#endif
 #include "CString.h"

We normally put includes that require an #if in a separate paragraph, after all
the unconditional includes.

 class CachedXSLStyleSheet;
+#if ENABLE(SVG)
+class CachedSVGDocument;
+#endif
 class Document;

Same with class forward declarations.

+    CachedSVGDocument* requestSVGDocument(const String &url);

Please put the & next to the typename.

 #include "XLinkNames.h"
+#include "DocLoader.h"
+#include "CachedSVGDocument.h"

Please keep includes sorted alphabetically.

+    , m_cachedDoc(0)

Why abbreviate? Why not "m_cachedDocument"?

+    if (url.find('#') > -1) { // format is #target
+	 unsigned int end = url.find('#');

It doesn't make sense to call find twice here. Since this is URL parsing, I
think this should be done with a helper function in KURL.h. It would go along
with the others, like equalIgnoringRef and protocolIs. No need to do that in
this patch, but a good idea in the end. I really don't think that getDocUrl is
a good name for this function. The abbreviation is unclear, for one thing.

+    if (docUrl.isEmpty())
+	 return document();
+    else

We normally don't do else after return.

+    const Document* doc = referencedDocument();

Why const?

You need a test case for this new feature! I'm setting review- because of the
lack of a test case mainly.