199329 - embedded stylesheets no longer work

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

with compiled stylesheets we regressed embedded stylesheet. We should fix that ASAP

Comment 1

[Axel Hecht]

Attachment: fix embedded stylesheets

Fix embedded stylesheets for both module and standalone.
Standalone has been more seriously broken in this respect, so the fixes over
there
are more involved. Esp. we need a parsedURL object to avoid scanning URLs for
parts over and over.

Comment 2

[Axel Hecht]

taking over. testcase checked in, gonna stage in a while.

I did not fix namespace bootstrapping for the js interface, that's a different
bug, IMHO. At least a different patch.

Comment 3

[Axel Hecht]

Comment on attachment 119337 [details] [diff] [review]
fix embedded stylesheets

requesting reviews.
Found one issue, consider the txParsedURL implementations #ifdef'ed, too

Comment 4

[Axel Hecht]

Comment on attachment 119337 [details] [diff] [review]
fix embedded stylesheets

new patch reqested by Jonas

Comment 5

[Axel Hecht]

Attachment: fix embedding with extra handler

changes to previous patch:
- added a handler for embedding, forwarding to gTxRoot stuff. The embedding
status is held in txStylesheetCompilerState now.
- got rid of the success return value, that is in the embed handler, now, too.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 119668 [details] [diff] [review]
fix embedding with extra handler

You need to handle embedded included/imported stylesheets too.

>+txParsedURL::init(const nsAFlatString& aSpec)
>+        else if (*c == '/') {
>+            // no #ref, filename found
>+            break;
>+        }

Remove this, '/' is a valid character in 

>+    if (c == start) {
>+        // we just have a filename
>+        if (specLength && *c != '#') {
>+            // don't do anything for "#"
>+            mName.Append(start, specLength);
>+        }
>+        return;
>+    }

Remove this, this case should be handled by the code below.

>+    if (start < end) {
>+        mName.Append(start, end - start);
>+    }

Remove the if-test, .Append should handle a zero-length string.

>+void
>+txParsedURL::resolve(const txParsedURL& aRef, txParsedURL& aDest)
>+{
>+    aDest.mPath = mPath + aRef.mPath;

You need to handle aRef being an absolute url.

>+    if (aRef.mName.Length()) {
>+        aDest.mName = aRef.mName;
>+    }
>+    else {
>+        aDest.mName = mName;
>+    }

What if aRef is a path like "foo/bar/"? that leaves mName empty.
And use .IsEmpty()

>+    if (aRef.mRef.Length()) {
>+        aDest.mRef = aRef.mRef; // base ref dies
>+    }

You need to handle the case when aRef is an empty url. According to the spec
resolving "" against the base "foo/bar#baz" will give "foo/bar#baz"

>Index: xslt/txMozillaStylesheetCompiler.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xslt/txMozillaStylesheetCompiler.cpp,v
>retrieving revision 1.2
>diff -u -r1.2 txMozillaStylesheetCompiler.cpp
>--- xslt/txMozillaStylesheetCompiler.cpp	26 Mar 2003 01:09:57 -0000	1.2
>+++ xslt/txMozillaStylesheetCompiler.cpp	7 Apr 2003 12:37:06 -0000
>@@ -47,9 +47,10 @@
> #include "nsINodeInfo.h"
> #include "nsIParser.h"
> #include "nsISyncLoadDOMService.h"
>-#include "nsIURI.h"
>+#include "nsIURL.h"
> #include "nsIXMLContentSink.h"
> #include "nsNetUtil.h"
>+#include "nsEscape.h"
> #include "nsParserCIID.h"
> #include "txAtoms.h"
> #include "txMozillaXSLTProcessor.h"
>@@ -103,7 +104,17 @@
>                                      PRUint32 aIndex,
>                                      PRUint32 aLineNumber)
> {
>-    nsresult rv = mCompiler->startElement(aName, aAtts, aAttsCount);
>+    // XXX aIndex should be a signed int, that's a bug in the 
>+    // Mozilla content sink api. Work around this, as I'm not sure
>+    // the conversion works both ways.
>+    PRInt32 idIndex;
>+    if (aIndex == (PRUint32)-1) {
>+        idIndex = -1;
>+    }
>+    else {
>+        idIndex = (PRInt32)aIndex;
>+    }
>+    nsresult rv = mCompiler->startElement(aName, aAtts, aAttsCount, idIndex);

You can make this just |(PRInt32)aIndex;| the conversion works both ways as
long as both types are the same size (I'm 100% sure). But feel free to leave
the first part of the comment.

>@@ -323,8 +334,13 @@
> TX_LoadSheet(nsIURI* aUri, txMozillaXSLTProcessor* aProcessor,
>              nsILoadGroup* aLoadGroup, nsIURI* aReferrerUri)
> {
>-    nsCAutoString uri;
>+    nsCAutoString uri, ref;
>     aUri->GetSpec(uri);
>+    nsCOMPtr<nsIURL> url = do_QueryInterface(aUri);
>+    if (url) {
>+        url->GetRef(ref);
>+        NS_UnescapeURL(ref);

Are you sure the ref should be unescaped?

>@@ -337,6 +353,9 @@
>     nsRefPtr<txStylesheetSink> sink = new txStylesheetSink(compiler);
>     NS_ENSURE_TRUE(sink, NS_ERROR_OUT_OF_MEMORY);
> 
>+    if (ref.Length()) {
>+        compiler->setTarget(NS_ConvertASCIItoUCS2(ref));
>+    }

Use .IsEmpty (assuming this should be kept at all, see comment further down).
Same thing everywhere where setTarget is called

>Index: xslt/txStandaloneStylesheetCompiler.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xslt/txStandaloneStylesheetCompiler.cpp,v
>retrieving revision 1.2
>diff -u -r1.2 txStandaloneStylesheetCompiler.cpp
>--- xslt/txStandaloneStylesheetCompiler.cpp	26 Mar 2003 01:09:58 -0000	1.2
>+++ xslt/txStandaloneStylesheetCompiler.cpp	7 Apr 2003 12:37:07 -0000
>@@ -72,11 +72,13 @@
> };
> 
> nsresult
>-TX_CompileStylesheetPath(const nsAString& aHref, txStylesheet** aResult)
>+TX_CompileStylesheetPath(const txParsedURL& aURL, txStylesheet** aResult)
> {
>     *aResult = nsnull;
>-    nsAutoString errMsg;
>-    istream* xslInput = URIUtils::getInputStream(aHref, errMsg);
>+    nsAutoString errMsg, filePath;
>+
>+    filePath = aURL.getFile();
>+    istream* xslInput = URIUtils::getInputStream(filePath, errMsg);

Can't you do |...getInputStream(aURL.getFile(), ...|? Maybe you need to make
.getFile() return a const to make that compile cross platform.

>-    nsresult rv = TX_CompileStylesheetPath(stylePath, getter_AddRefs(style));
>+    nsresult rv = TX_CompileStylesheetPath(resolved, getter_AddRefs(style));
>+    if (NS_FAILED(rv)) {
>+        return rv;
>+    }

Please use NS_ENSURE_SUCCESS

>Index: xslt/txStandaloneXSLTProcessor.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xslt/txStandaloneXSLTProcessor.h,v
>retrieving revision 1.4
>diff -u -r1.4 txStandaloneXSLTProcessor.h
>--- xslt/txStandaloneXSLTProcessor.h	26 Mar 2003 01:09:59 -0000	1.4
>+++ xslt/txStandaloneXSLTProcessor.h	7 Apr 2003 12:37:08 -0000
>@@ -122,7 +122,8 @@
>      * @param aErr    error observer
>      * @result NS_OK if transformation was successful
>      */
>-    nsresult transform(Document* aXMLDoc, ostream& aOut, ErrorObserver& aErr);
>+    nsresult transformDocument(Document* aXMLDoc, const nsAString& aPath,
>+                               ostream& aOut, ErrorObserver& aErr);

Can't you get aPath from aXMLDoc.getBaseURI? It was very nice cleanup when we
stopped passing around paths and documents as separate entities (way back when)
and i'd hate to see us to back to that.

>Index: xslt/txStylesheetCompileHandlers.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xslt/txStylesheetCompileHandlers.cpp,v
>retrieving revision 1.2
>diff -u -r1.2 txStylesheetCompileHandlers.cpp
>--- xslt/txStylesheetCompileHandlers.cpp	26 Mar 2003 01:10:01 -0000	1.2
>+++ xslt/txStylesheetCompileHandlers.cpp	7 Apr 2003 12:37:13 -0000
>@@ -52,6 +52,7 @@
> 
> txHandlerTable* gTxIgnoreHandler = 0;
> txHandlerTable* gTxRootHandler = 0;
>+txHandlerTable* gTxEmbedHandler = 0;
> txHandlerTable* gTxTopHandler = 0;
> txHandlerTable* gTxTemplateHandler = 0;
> txHandlerTable* gTxTextHandler = 0;
>@@ -516,6 +517,32 @@
>     return NS_OK;
> }
> 
>+nsresult
>+txFnStartEmbed(PRInt32 aNamespaceID,
>+                  nsIAtom* aLocalName,
>+                  nsIAtom* aPrefix,
>+                  txStylesheetAttr* aAttributes,
>+                  PRInt32 aAttrCount,
>+                  txStylesheetCompilerState& aState)
>+{
>+    if (aState.getEmbedStatus() != txStylesheetCompilerState::eInEmbed) {
>+        return NS_OK;
>+    }
>+    return txFnStartStylesheet(aNamespaceID, aLocalName, aPrefix,
>+                               aAttributes, aAttrCount, aState);
>+}
>+
>+nsresult
>+txFnEndEmbed(txStylesheetCompilerState& aState)
>+{
>+    if (aState.getEmbedStatus() != txStylesheetCompilerState::eInEmbed) {
>+        return NS_OK;
>+    }
>+    nsresult rv = txFnEndStylesheet(aState);
>+    aState.doneEmbedding();
>+    return rv;
>+}
>+
> 
> /**
>  * Top handlers
>@@ -2614,6 +2641,17 @@
>   txFnTextError
> };
> 
>+txHandlerTableData gTxEmbedTableData = {
>+  // Handlers
>+  { { 0, 0, 0, 0 } },
>+  // Other
>+  { 0, 0, txFnStartEmbed, txFnEndEmbed },
>+  // LRE
>+  { 0, 0, txFnStartElementIgnore, txFnEndElementIgnore },
>+  // Text
>+  txFnTextIgnore
>+};
>+
> txHandlerTableData gTxTopTableData = {
>   // Handlers
>   { { kNameSpaceID_XSLT, "attribute-set", txFnStartAttributeSet, txFnEndAttributeSet },
>@@ -2858,6 +2896,7 @@
>     nsresult rv = NS_OK;
> 
>     INIT_HANDLER(Root);
>+    INIT_HANDLER(Embed);
>     INIT_HANDLER(Top);
>     INIT_HANDLER(Ignore);
>     INIT_HANDLER(Template);
>Index: xslt/txStylesheetCompileHandlers.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xslt/txStylesheetCompileHandlers.h,v
>retrieving revision 1.2
>diff -u -r1.2 txStylesheetCompileHandlers.h
>--- xslt/txStylesheetCompileHandlers.h	26 Mar 2003 01:10:02 -0000	1.2
>+++ xslt/txStylesheetCompileHandlers.h	7 Apr 2003 12:37:16 -0000
>@@ -89,5 +89,6 @@
> };
> 
> extern txHandlerTable* gTxRootHandler;
>+extern txHandlerTable* gTxEmbedHandler;
> 
> #endif //TRANSFRMX_TXSTYLESHEETCOMPILEHANDLERS_H
>Index: xslt/txStylesheetCompiler.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xslt/txStylesheetCompiler.cpp,v
>retrieving revision 1.2
>diff -u -r1.2 txStylesheetCompiler.cpp
>--- xslt/txStylesheetCompiler.cpp	26 Mar 2003 01:10:02 -0000	1.2
>+++ xslt/txStylesheetCompiler.cpp	7 Apr 2003 12:37:16 -0000
>@@ -130,7 +130,7 @@
> nsresult
> txStylesheetCompiler::startElement(const PRUnichar *aName,
>                                    const PRUnichar **aAttrs,
>-                                   PRInt32 aAttrCount)
>+                                   PRInt32 aAttrCount, PRInt32 aIDOffset)
> {
>     if (NS_FAILED(mStatus)) {
>         return mStatus;
>@@ -207,8 +207,11 @@
> 
>     NS_ENSURE_TRUE(namespaceID != kNameSpaceID_Unknown, NS_ERROR_FAILURE);
> 
>+    if (aIDOffset > 0) {
>+        aIDOffset /= 2;
>+    }

In general I try to avoid modifying arguments, feel free to create a new
|PRInt32 idOffset|.

>@@ -216,7 +219,8 @@
>                                            nsIAtom* aLocalName,
>                                            nsIAtom* aPrefix,
>                                            txStylesheetAttr* aAttributes,
>-                                           PRInt32 aAttrCount)
>+                                           PRInt32 aAttrCount,
>+                                           PRInt32 aIDOffset)
> {
>     nsresult rv = NS_OK;
>     PRInt32 i;
>@@ -316,6 +320,14 @@
>         }
>     }
> 
>+    if (mEmbedStatus == eNeedEmbed) {
>+        // handle embedded stylesheets
>+        if (aIDOffset >= 0 && aAttributes[aIDOffset].mValue.Equals(mTarget)) {
>+            // We found the right ID, signal to compile the 
>+            // embedded stylesheet.
>+            mEmbedStatus = eInEmbed;
>+        }
>+    }
>     txElementHandler* handler;
>     do {
>         handler = isInstruction ?
>@@ -413,6 +425,15 @@
>     }
> }
> 
>+void
>+txStylesheetCompiler::setTarget(const nsAString& aTarget)
>+{
>+    mTarget = aTarget;
>+    mEmbedStatus = eNeedEmbed;
>+    mHandlerTable = gTxEmbedHandler;
>+}

How about removing this function and adding another argument to the ctor
instead? It should only ever be called right after creating the compiler
anyway.

>+    // Embedded stylesheets state
>+    enum EmbedStatus
>+    {
>+        eNoEmbed,
>+        eNeedEmbed,
>+        eInEmbed,
>+        eHasEmbed
>+    };
>+    EmbedStatus getEmbedStatus()
>+    {
>+        return mEmbedStatus;
>+    }
>+    void doneEmbedding()
>+    {
>+        mEmbedStatus = eHasEmbed;
>+    }

IMHO you could just make mEmbedStatus public. That's how most other members are
done anyway.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

oh, and you need to check that the found "stylesheet element" is named
xsl:stylesheet/xsl:transform

Comment 8

[Axel Hecht]

I wonder if I move the fragment handling into txStylesheetCompiler::init.
That would take away the possible use of netwerk for module, though.
(I'm tempted to move the init call out of the constructor, too.)
Comments?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm ok with moving ::init out of the ctor if that helps.

But I still think it seems easier to just add a URIUtils::getFragmentIdentifier.
If we only call that once (or twice or three times) for every fileload it won't
make a difference. Another alternative is to go all the way and use parsed
uri-objects everywhere and let that just wrap an nsIURI for module (though that
might complicate the hashing we do in the documenthash)

Comment 10

[Axel Hecht]

Comment on attachment 119668 [details] [diff] [review]
fix embedding with extra handler

new patch in the works

Comment 11

[Axel Hecht]

Attachment: comments addressed

Fixed comments by sicking.
I moved the embed target a bit to handle it in ::init.
I also made the enum purely protected and just exposed the test and a done 
from txStylesheetCompileState to the handlers.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 121458 [details] [diff] [review]
comments addressed

>Index: base/txURIUtils.cpp
>+void
>+txParsedURL::init(const nsAFlatString& aSpec)
>+{

Nit: You could assert that all the components of the url is empty just so that
noone calls init twice on the same object.

>+    PRUint32 specLength = aSpec.Length();
>+    if (!specLength) {
>+        return;
>+    }
>+    const PRUnichar* start = aSpec.get();
>+    const PRUnichar* end = start + specLength;
>+    const PRUnichar* c = end - 1;
>+
>+    // check for #ref
>+    while (c >= start) {

Nit: These two while-loos could easily be turned into for-loops.

>+        if (*c == '#') {
>+            // we could eventually unescape this, too.
>+            mRef.Append(c + 1, end - c -1 );
>+            end = c;
>+            --c;
>+            if (c == start) {
>+                // we're done,
>+                return;
>+            }

You could remove the |if| and the --c. The code below would deal.

r=sicking

Comment 13

[Axel Hecht]

while fixing sicking's comments, I realised that
  mRef = Substring(c + 1, end);
is much closer to what we want then
  mRef.Append(c + 1, end - c -1);
and the Substring is constructed anyway. So I moved the assignments over to
assigning to a Substring in txParsedURL::init.
I also moved to for loops. To not end up funny when calling init() more than once,
I Truncate all three members to start with. (All three being empty doesn't meen
this txParsedURL wasn't init()ed, so I allow init() being called more than once)

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 121458 [details] [diff] [review]
comments addressed

>Index: main/txXSLTMarkDriver.cpp
>===================================================================

>@@ -59,8 +59,10 @@
>     int loadStylesheet (char * filename)
>     {
>         NS_ConvertASCIItoUCS2 path(filename);
>+        txParsedURL url;
>+        url.init(path);

Just make this

	url.init(NS_ConvertASCIItoUCS2(filename));

Btw, you always seem to use txParsedURL as construct and init. Since init
doesn't return an error code, I wonder why we can't just have a constructor
taking a string.

>Index: xslt/txStandaloneStylesheetCompiler.cpp
>===================================================================

> nsresult
>-TX_CompileStylesheetPath(const nsAString& aHref, txStylesheet** aResult)
>+TX_CompileStylesheetPath(const txParsedURL& aURL, txStylesheet** aResult)
> {
>+    *aResult = nsnull;
>+    nsAutoString errMsg, filePath, spec;

I prefer the declaration of variables close to where they're being used

...
>+    spec = filePath;

so make this

    nsAutoString spec = filePath;

>Index: xslt/txStandaloneXSLTProcessor.cpp
>===================================================================

>@@ -251,11 +258,10 @@
>                 type.Truncate();
>                 tmpHref.Truncate();
>                 parseStylesheetPI(data, type, tmpHref);
>-                if (type.Equals(NS_LITERAL_STRING("text/xsl"))) {
>-                    href.Truncate();
>-                    nsAutoString baseURI;
>-                    node->getBaseURI(baseURI);
>-                    URIUtils::resolveHref(tmpHref, baseURI, href);
>+                if (type.Equals(NS_LITERAL_STRING("text/xsl")) ||
>+                    type.Equals(NS_LITERAL_STRING("text/xml"))) {
>+                    href = tmpHref;
>+                    return;

Do you want to change the comment for this function? We no longer use the last
PI but the first one.

>Index: xslt/txStylesheetCompileHandlers.cpp
>===================================================================

>+nsresult
>+txFnStartEmbed(PRInt32 aNamespaceID,
>+                  nsIAtom* aLocalName,
>+                  nsIAtom* aPrefix,
>+                  txStylesheetAttr* aAttributes,
>+                  PRInt32 aAttrCount,
>+                  txStylesheetCompilerState& aState)

Line this up correctly.

>Index: xslt/txStylesheetCompiler.cpp
>===================================================================

>+    // Check for fragment identifier of an embedded stylesheet.
>+    PRInt32 fragment = aBaseURI.FindChar('#');
>+    if (fragment >= 0) {
>+        PRInt32 fragmentLength = aBaseURI.Length() - fragment - 1;
>+        if (fragmentLength > 0) {
>+            // This is really an embedded stylesheet, not just a
>+            // "url#". We may want to unescape the fragment.
>+            mTarget = Substring(aBaseURI, (PRUint32)fragment + 1,
>+                                fragmentLength);

I'd just do

    PRInt32 fragment = aBaseURI.FindChar('#') + 1;
    if (fragment > 0) {
	PRInt32 fragmentLength = aBaseURI.Length() - fragment;
	if (fragmentLength > 0) {
	    // This is really an embedded stylesheet, not just a
	    // "url#". We may want to unescape the fragment.
	    mTarget = Substring(aBaseURI, (PRUint32)fragment,

Comment 15

[Axel Hecht]

Attachment: patch with comments by peterv and sicking

final patch for approval

Comment 16

[Axel Hecht]

Comment on attachment 122095 [details] [diff] [review]
patch with comments by peterv and sicking

carrying r=bugmail, sr=peterv from attachement 121458.
requesting approval for 1.4b.
This patch fixes embedded stylesheets which have been regressed since the
landing of stylesheet compilation. The patch is XSLT only and well tested on a
variety of testcases.

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 122095 [details] [diff] [review]
patch with comments by peterv and sicking

a=asa (on behalf of drivers) for checkin to 1.4b

Comment 18

[Axel Hecht]

checked in. Hey, I made the milestone :-)

Comment 19

[Axel Hecht]

mass verifying