[webkit-reviews] review denied: [Bug 3275] Support Mozilla's XSLTProcessor JS object : [Attachment 4427] Patch for XSLTProcessor support

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Eric Seidel <macdome at opendarwin.org>'s
request for review:
Bug 3275: Support Mozilla's XSLTProcessor JS object
http://bugzilla.opendarwin.org/show_bug.cgi?id=3275

Attachment 4427: Patch for XSLTProcessor support
http://bugzilla.opendarwin.org/attachment.cgi?id=4427&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
On this line:

+    XSLTProcessorConstructorImp(ExecState *) : ObjectImp() { }

there's no need to mention ObjectImp explicitly. The default is to
default-construct the base classes.

In this line:

     , m_transformSourceDocument(0)

there's no need to explicitly initialize a SharedPtr. They all get
ininitialized to 0.

In this line:

+    SharedPtr<XSLTProcessorImpl> processor = new XSLTProcessorImpl();

there's no need to have the () after the class name.

Here:

+void *xmlDocPtrForString(const QString &source, QString url)

the URL parameter should also be "const QString &".

In parseErrorFunc you need to free the errorMessage allocated by vasprintf,
this line:

+    ERROR(errorMessage);

should be:

+    ERROR("%s", errorMessage);

and the entire thing should be inside #if !ERROR_DISABLED to avoid creating the
error message and doing nothing with it in production builds.

This code:

+    if (retval < 0)
+	 return false;
+    return true;

should just be:

    return retval >= 0;

This line:

+    if(parameters.count())

is missing a space after the if.

And this line:

+    for(it.toFirst(); it.current(); ++it) {

is missing a space after the for.

And this line:

+    parameterArray[index] = NULL;

should use 0, not NULL (yes, I know these are not new lines of code you wrote).


Same here:

+    if(!params)

and here:

+    while(*temp) {

In this line:

+    bool sourceIsDocument = (sourceNode == ownerDocument.get());

There's no need for the "get()". You can just compare raw pointers and
SharedPtrs directly.

In this line:

+    if (sourceMIMEType == QString::fromLatin1("text/html"))
+    if (sourceMIMEType == QString::fromLatin1("text/plain"))

there's no need to call fromLatin1, and in fact it makes the code smaller. You
can just compare a QString and C-style literal directly.

Also, I suggest the "text/plain" code go inside the else, since the MIME type
can't be both text/html and text/plain.

Here:

+SharedPtr<DocumentImpl> XSLTProcessorImpl::createDocumentFromSource(QString
documentSource, QString sourceMIMEType, NodeImpl *sourceNode, KHTMLView *view)
+static inline SharedPtr<DocumentFragmentImpl> createFragmentFromSource(QString
sourceString, QString sourceMIMEType, NodeImpl *sourceNode, DocumentImpl
*ouputDoc)

the parameters should be const QString&, not QString.

Are there bug reports about getParameter, setParameter, and removeParameter
ignoring the namespace?

I really think that m_parameters should be using a KXMLCore hash table, and not
a QDict. Use of QDict is deprecated.

Otherwise, looks good.