Bug 3275 - Support Mozilla's XSLTProcessor JS object
Summary: Support Mozilla's XSLTProcessor JS object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 412
Hardware: All All
: P1 Normal
Assignee: Eric Seidel (no email)
URL: http://www.mozilla.org/projects/xslt/...
Keywords:
Depends on: 5404
Blocks: 5446 5492
  Show dependency treegraph
 
Reported: 2005-06-04 20:39 PDT by Dave Hyatt
Modified: 2006-08-13 19:10 PDT (History)
7 users (show)

See Also:


Attachments
First stab at a test document. (6.38 KB, text/html)
2005-10-18 02:47 PDT, Eric Seidel (no email)
no flags Details
My preliminary patch (minus xcode changes) for XSLTProcessor support (58.75 KB, patch)
2005-10-18 03:14 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for XSLTProcessor support (38.27 KB, patch)
2005-10-20 13:40 PDT, Eric Seidel (no email)
darin: review-
Details | Formatted Diff | Diff
XLSTProcessor test cases (15.12 KB, patch)
2005-10-20 13:40 PDT, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff
Patch fixing darin's complaints. (38.01 KB, patch)
2005-10-26 23:47 PDT, Eric Seidel (no email)
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2005-06-04 20:39:19 PDT
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.
Comment 1 Stuart Morgan 2005-06-11 15:17:22 PDT
Apple Bug: rdar://3642402
Comment 2 Eric Seidel (no email) 2005-09-27 16:39:34 PDT
Major sites depend on this functionality, bumping priority.
Comment 3 Bert JW Regeer 2005-10-02 09:10:46 PDT
Adding CC. Very much interested in seeing this go through. Would be nice to take
advantage of it.
Comment 4 Eric Seidel (no email) 2005-10-18 00:55:31 PDT
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
Comment 5 Eric Seidel (no email) 2005-10-18 02:47:32 PDT
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.
Comment 6 Eric Seidel (no email) 2005-10-18 03:14:31 PDT
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.
Comment 7 Eric Seidel (no email) 2005-10-20 13:40:19 PDT
Created attachment 4427 [details]
Patch for XSLTProcessor support
Comment 8 Eric Seidel (no email) 2005-10-20 13:40:59 PDT
Created attachment 4428 [details]
XLSTProcessor test cases

Note, we fail some of these tests.  This is known and will be tracked by other
bugs.
Comment 9 Darin Adler 2005-10-22 08:43:38 PDT
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.
Comment 10 Darin Adler 2005-10-22 08:46:15 PDT
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
Comment 11 Timothy Hatcher 2005-10-24 14:30:24 PDT
Eric said this weekend he was going to land this after he made some tweaks.
Comment 12 Timothy Hatcher 2005-10-24 14:31:21 PDT
Didn't mean to change the resolution. 
Comment 13 Eric Seidel (no email) 2005-10-24 21:55:29 PDT
(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.

Comment 14 Eric Seidel (no email) 2005-10-24 22:01:27 PDT
(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.
Comment 15 Eric Seidel (no email) 2005-10-24 22:03:03 PDT
(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.
Comment 16 Darin Adler 2005-10-25 10:26:23 PDT
(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.
Comment 17 Eric Seidel (no email) 2005-10-26 23:47:32 PDT
Created attachment 4486 [details]
Patch fixing darin's complaints.
Comment 18 Geoffrey Garen 2005-10-27 14:23:20 PDT
Darin, did you mean to r+ the patch instead of the layout test?
Comment 19 Dave Hyatt 2005-10-27 14:48:16 PDT
Comment on attachment 4486 [details]
Patch fixing darin's complaints.

r=me
Comment 20 Eric Seidel (no email) 2005-10-27 15:41:59 PDT
Done!
Comment 21 j_mckitrick 2006-08-12 08:41:35 PDT
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.
Comment 22 Alexey Proskuryakov 2006-08-13 02:46:21 PDT
(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.
Comment 23 Eric Seidel (no email) 2006-08-13 19:10:19 PDT
(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.