94002 - Implement xsl:apply-imports

Status: VERIFIED FIXED

(Core :: XSLT, defect, P3)

Description

[Nisheeth Ranjan]

Now that xsl:import and xsl:include work properly now.  We should now implement
xsl:apply-imports.

Comment 1

[Nisheeth Ranjan]

Based on discussion with Peter, setting target milestone to 0.9.4

Comment 2

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

For the record, xsl:import still dosn't work (isn't implemented). Patch is 
avalible in bug 78068 though.

Comment 3

[Peter Van der Beken [:peterv]]

This is blocked by xsl:import, and I won't be able to finish it for this milestone.

Comment 4

[Peter Van der Beken [:peterv]]

All these missed the bus/train/plane/boat/whatever. Sad.

Comment 5

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

stealing this from peterv...

Comment 6

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

First attempt coming up...

I don't very much like having 6 arguments to the function 
XSLTProcessor::processMatchedTemplate, but I think it's better then inlining 
the function at the four places it's needed.

Comment 7

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

Attachment: ver 1

Comment 8

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

oh, impincl15 is a pretty good testcase

Comment 9

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

Attachment: sorry, forgot to nullify current-template during xsl:for-each

Comment 10

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

upps, found an error that made apply-imports instantiate the default template 
if no imported template was found. This is wrong since the default templates 
are not imported by any stylesheet. Changed

                processMatchedTemplate(xslTemplate, node, curr->mParams,
                                       *curr->mMode, frame, ps);

to

                if (xslTemplate) {
                    processMatchedTemplate(xslTemplate, node, curr->mParams,
                                           *curr->mMode, frame, ps);
                }

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 54027 [details] [diff] [review]
sorry, forgot to nullify current-template during xsl:for-each

> Node* ProcessorState::findTemplate(Node* aNode,
>-                                   Node* aContext,
>-                                   const String& aMode)
>+                                   const String& aMode,
>+                                   ImportFrame** aImportFrame)
> {
>+    return findTemplate(aNode, aMode, 0, aImportFrame);
>+}
Couldn't we just convert the callers?

>+    NS_ASSERTION(aImportFrame, "missing ImportFrame pointer");
>+    NS_ASSERTION(aNode, "missing node");
>+
>     if (!aNode)
Need !aImportFrame too?

>+    if (aImportedBy) {
>+        while (frameIter.hasNext() &&
>+               (ImportFrame*)frameIter.next() != aImportedBy);
I need to check if this gives a warning. Remind me ;).

>                     tmpPriority = templ->mMatch->getDefaultPriority(aNode,
>-                                                                    aContext,
>+                                                                    0,
Huh?

>+        // ImportFrame which is the first one *not* imported by this frame
>+        ImportFrame* mFirstNotImported;
> 
>+
Nononono.

>+                    ProcessorState::ImportFrame *nextFrame, *newFrame;
>+                    nextFrame =
>+                        (ProcessorState::ImportFrame*)importFrame->next();
>+                    importFrame->previous();
What if there's no next? I might be misunderstanding, so enlighten me.

>+                    // Set current template to null
>+                    ProcessorState::CurrentTemplate *oldRule;
oldTemplate?

>+                processMatchedTemplate(xslTemplate, currNode, 0, mode, frame,
>+                                       ps);
>             }
>+
>+
You got newlines for free in the gas-station?

I think I mostly understand it. Let's hope I still do tomorrow morning. It looks pretty good imho.

Comment 12

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

> > Node* ProcessorState::findTemplate(Node* aNode,
> >-                                   Node* aContext,
> >-                                   const String& aMode)
> >+                                   const String& aMode,
> >+                                   ImportFrame** aImportFrame)
> > {
> >+    return findTemplate(aNode, aMode, 0, aImportFrame);
> >+}
> Couldn't we just convert the callers?

I thought it looked neater this way, rather then requiering an argument that 
was almost never (next to never runwise) used.

> >+    NS_ASSERTION(aImportFrame, "missing ImportFrame pointer");
> >+    NS_ASSERTION(aNode, "missing node");
> >+
> >     if (!aNode)
> Need !aImportFrame too?

Didn't want to add more wallpaper, I'd actually like to remove that |!aNode| 
too. 

> >                 tmpPriority = templ->mMatch->getDefaultPriority(aNode,
> >-                                                                aContext,
> >+                                                                0,
> Huh?

That argument is never ever used (bug 95059), and since i added another 
argument to findTemplate i took the opportunity to remove one.

> >+        // ImportFrame which is the first one *not* imported by this frame
> >+        ImportFrame* mFirstNotImported;
> > 
> >+
> Nononono.

?

> >+                    ProcessorState::ImportFrame *nextFrame, *newFrame;
> >+                    nextFrame =
> >+                        (ProcessorState::ImportFrame*)importFrame->next();
> >+                    importFrame->previous();
> What if there's no next? I might be misunderstanding, so enlighten me.

That's ok, findTemplate should handle that just fine.

> >+                    // Set current template to null
> >+                    ProcessorState::CurrentTemplate *oldRule;
> oldTemplate?

Done, did newRule->newTemplate too

> You got newlines for free in the gas-station?

lol, yep :)
Consider them gone

Comment 13

[Axel Hecht]

Comment on attachment 54027 [details] [diff] [review]
sorry, forgot to nullify current-template during xsl:for-each

>Index: source/xslt/ProcessorState.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xslt/ProcessorState.cpp,v
>retrieving revision 1.48
>diff -u -r1.48 ProcessorState.cpp
>--- ProcessorState.cpp	2001/10/16 11:57:51	1.48
>+++ ProcessorState.cpp	2001/10/18 04:49:48
>@@ -440,18 +440,46 @@
>  * a mode attribute equal to the given mode will be searched.
>  */
> Node* ProcessorState::findTemplate(Node* aNode,
>-                                   Node* aContext,
>-                                   const String& aMode)
>+                                   const String& aMode,
>+                                   ImportFrame** aImportFrame)
<...>
>+Node* ProcessorState::findTemplate(Node* aNode,
>+                                   const String& aMode,
>+                                   ImportFrame* aImportedBy,
>+                                   ImportFrame** aImportFrame)

why not

>+Node* ProcessorState::findTemplate(Node* aNode,
>+                                   const String& aMode,
>+                                   ImportFrame** aImportFrame,
>+                                   ImportFrame* aImportedBy = 0)

?



>+    /*
>+     * Struct holding information about a current template rule
>+     */
>+    struct CurrentTemplate {
>+        ImportFrame* mFrame;
>+        const String* mMode;
>+        NamedMap* mParams;
>+    };
>+

'Current' as part of a type? I don't like that.

So far for now, didn't understand apply-templates yet, need to read the spec
'til I understand what the patch does. Just wanted to raise those two opinions.

Comment 14

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

Attachment: sync with tip

synced with tip and renamed CurrentTemplate to TemplateRule (from the specs
"current template rule").

I don't like default-values and |ImportFrame** aImportFrame| should IMHO be the
last argument since it's an out-argument

Comment 15

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

pushing.. still waiting for review on these

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 57589 [details] [diff] [review]
sync with tip

>+/*
>+ * Finds a template for the given Node. Only templates with
>+ * a mode attribute equal to the given mode will be searched.
>+ */
>+Node* ProcessorState::findTemplate(Node* aNode,
>+                                   const String& aMode,
>+                                   ImportFrame* aImportedBy,
>+                                   ImportFrame** aImportFrame)
>+{
The comment needs to talk about the importedby frame. (change it in the header
too)

>+        while (frameIter.hasNext() &&
>+               (ImportFrame*)frameIter.next() != aImportedBy);
Rewrite this as discussed on IRC.

>+        ImportFrame* mFirstNotImported;
> 
>+
>         // The following stuff is missing here:
Drop the extra newline.

>+                    // Create a new ImportFrame with correct firstNotImported
>+                    ProcessorState::ImportFrame *nextFrame, *newFrame;
>+                    nextFrame =
>+                        (ProcessorState::ImportFrame*)importFrame->next();
>+                    importFrame->previous();
>+                    newFrame = new ProcessorState::ImportFrame(nextFrame);
>+                    if (!newFrame) {
>+                        // XXX ErrorReport: out of memory
>+                        break;
>+                    }
>+
>+                    // Insert frame and process stylesheet
>+                    importFrame->addAfter(newFrame);
Change this as discussed on IRC (next(), addBefore(), previous()).

>+                curr = ps->getCurrentTemplateRule();
>+                if (!curr) {
>+                    String err("apply-imports not allowed here");
>+                    ps->recieveError(err);
>+                    break;
>+
>+                }
Drop the newline after the break.

r=peterv.

Comment 17

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

Attachment: with petervs comments

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 64562 [details] [diff] [review]
with petervs comments

sr=jst

Comment 19

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

checked in

thanks for reviews and comments

Comment 20

[Axel Hecht]

we didn't verify for a long time.
I really checked, so VERIFIED.