Bug 3275 – Support Mozilla's XSLTProcessor JS object

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Dave Hyatt]

Right now the only way XSLT can be used in Web Kit is through the use of client-side processing 
instructions in the source XML.  Mozilla offers a programmatic API for performing transformations from JS 
through an <tt>XSLTProcessor</tt> object.  The following <a href="http://www.mozilla.org/projects/
xslt/js-interface.html">document</a> on the Mozilla Web site describes this JS API.  We would like to 
match this API exactly.

[Stuart Morgan]

Apple Bug: rdar://3642402

[Eric Seidel (no email)]

Major sites depend on this functionality, bumping priority.

[Bert JW Regeer]

Adding CC. Very much interested in seeing this go through. Would be nice to take
advantage of it.

[Eric Seidel (no email)]

Ok, so I have this all coded up, and working.  Although my testing up until now has been *extremely 
limited*.  I took a moment tonight and thought up some potential test cases for this new API.  Hopefully 
I will get a chance to code all of these tests up.  However I also would not complain if someone took the 
liberty of making some of their own tests.  Particularly any of you who are skilled web designers 
desiring of this feature.

Things I thought might be useful to test:

void importStylesheet(in DOMNode style);
- Import two different stylesheets (last one wins)
- Import same stylesheet twice
- Import undefined stylesheet
- Import invalid stylesheet (not wellformed xlst)
- Don't import stylesheet

DOMDocumentFragment transformToFragment(in DOMNode source, in DOMDocument output);
- use undefined source
- use non-DOMDocument output parameter
- transform twice

DOMDocument transformToDocument(in DOMNode source);
- use undefined source
- transform twice

void setParameter(in DOMString namespaceURI, in DOMString localName, in Value value);
- pass same localname, different namespaces
- pass undefined namespace
- pass undefined name
- pass undefined value
- pass unsupported value (object, for instance?)

Value getParameter(in DOMString namespaceURI, in DOMString localName);
- pass same localname, different namespaces
- pass undefined namespace
- pass undefined name
- pass name which has not been set

void removeParameter(in DOMString namespaceURI, in DOMString localName);
- pass same localname, different namespaces
- pass undefined namespace
- pass undefined name
- pass name which has not been set

void clearParameters();
- call, verify that parameters have been cleared.

void reset();
- call, verify that parameters have been cleared.
- call, verify that stylesheet has been cleared.

I've been using the very minimalist docs found here:
http://www.mozilla.org/projects/xslt/js-interface.html
http://lxr.mozilla.org/seamonkey/source/content/xsl/public/nsIXSLTProcessor.idl

[Eric Seidel (no email)]

Created attachment 4395 [details]
First stab at a test document.

This needs to be placed in LayoutTests/fast/xsl to work (as it currently
depends on resources there.

This document is not quite done.  Particularly, I'm not yet testing any
parameter based transforms.

Also, I'm not yet testing "null" values for most things.  I figure I probably
need to test both null and undefined...

Several tests "pass" under Safari, but throw exceptions under FireFox, not sure
why yet, not sure what exceptions we should be throwing.  Comments welcome.

[Eric Seidel (no email)]

Created attachment 4396 [details]
My preliminary patch (minus xcode changes) for XSLTProcessor support

This is a pretty function patch... including a bunch of fixes to our
serialization code... I'll probably clean it up a bit before landing.  And I
still need to have better exception throwing and possibly namespace support.

[Eric Seidel (no email)]

Created attachment 4427 [details]
Patch for XSLTProcessor support

[Eric Seidel (no email)]

Created attachment 4428 [details]
XLSTProcessor test cases

Note, we fail some of these tests.  This is known and will be tracked by other
bugs.

[Darin Adler]

Comment on attachment 4427 [details]
Patch for XSLTProcessor support

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.

[Darin Adler]

Comment on attachment 4428 [details]
XLSTProcessor test cases

I think that this:

+if (resultDocument != undefined)

isn't great code. You'd normally do if (resultDocument) or if (resultDocument
!= null).

In general I think we are using undefined in some places we should be using
null (in our code, not our test cases).

I'd like to see test cases that use namespace URIs, even if the test results
are "wrong", so we can detect changes to our behavior in those cases.

But this seems like a good start. r=me

[Timothy Hatcher]

Eric said this weekend he was going to land this after he made some tweaks.

[Timothy Hatcher]

Didn't mean to change the resolution. 

[Eric Seidel (no email)]

(In reply to comment #9)
> I really think that m_parameters should be using a KXMLCore hash table, and not
> a QDict. Use of QDict is deprecated.

Agreed.  I fought with HashSet for quite a while.   Unfortunately it's not ready to support 
SharedPtr<DOMStringImpl> as key/values correctly yet.

[Eric Seidel (no email)]

(In reply to comment #9)
> In this line:
> 
> +    bool sourceIsDocument = (sourceNode == ownerDocument.get());
> 
> There's no need for the "get()". You can just compare raw pointers and
> SharedPtrs directly.

The compile fails if I don't include .get() in that line.  Something about the compare being ambigious.

[Eric Seidel (no email)]

(In reply to comment #9)
> +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.

I intentionally have left QString documentSource not passed const &, as I change that parameter locally.  
I guess it might be slightly more efficient only to cause the copy *if* I'm about to change the parameter 
locally, however that would complicate the code a bit, I think.

[Darin Adler]

(In reply to comment #15)
> I intentionally have left QString documentSource not passed const &, as I change that parameter locally.  

In my opinion that's not a compelling argument. You should not determine the types of the parameters 
based on this issue. Generally it's bad style to modify your parameters, although the language does allow 
it.

[Eric Seidel (no email)]

Created attachment 4486 [details]
Patch fixing darin's complaints.

[Geoffrey Garen]

Darin, did you mean to r+ the patch instead of the layout test?

[Dave Hyatt]

Comment on attachment 4486 [details]
Patch fixing darin's complaints.

r=me

[Eric Seidel (no email)]

Done!

I'm having problems with transformToFragment().  Here's a snippet:

	var t = new XSLTProcessor(); alert(t);
	t.importStylesheet(x);

	var f = t.transformToFragment(s, document); alert(f);
	d.innerHTML = "";
	d.appendChild(f);

where:
 s = source node to be transformed
x = xslt node
d = destination for result

The XSLTProcessor object instantiates, and importStylesheet() works.  But the resulting fragment is null.

[Alexey Proskuryakov]

(In reply to comment #21)
> I'm having problems with transformToFragment().

Could you please file a new bug for this, preferably with a test case? That would greatly simplify tracking and, ultimately, resolving the issue.

[Eric Seidel (no email)]

(In reply to comment #21)
> I'm having problems with transformToFragment().  Here's a snippet:
> 
>         var t = new XSLTProcessor(); alert(t);
>         t.importStylesheet(x);
> 
>         var f = t.transformToFragment(s, document); alert(f);
>         d.innerHTML = "";
>         d.appendChild(f);
> 
> where:
>  s = source node to be transformed
> x = xslt node
> d = destination for result
> 
> The XSLTProcessor object instantiates, and importStylesheet() works.  But the
> resulting fragment is null.
> 

Yes, please post a new bug.  There are certainly issues in our first-pass implementation.