78068 - xsl:import not supported

Status: VERIFIED FIXED

(Core :: XSLT, defect, P2)

Description

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

xsl:import is not supported in Transformiix. Will attach an patch that
implements it by doing the following:

* Adds some code to the current xsl:import to handle import precidence
   Added the following functions
    ProcessorState::getImportPrecedence gets current import precedence
    ProcessorState::setImportPrecedence sets current import precedence
    ProcessorState::newImportPrecedence sets current import precedence to that
                                        of a new imported stylesheet
   Changed code in
    XSLTProcessor::processTopLevel

* Revese the order in which top-level elements are process (ie do it lastChild 
  to firstChild). I had to do this for the importprecedence calculations.
   Changed code in
    XSLTProcessor::processTopLevel

* Made ProcessorState able to load documents. This code is basically moved here
  from DocumentFunctionCall. Now document() and xsl:import/include uses the same
  mechanism to load documents.
   Moved the following function
    DocumentFunctionCall::retrieveDocument to ProcessorState::retrieveDocument
   Changed code in
    DocumentFunctionCall::evaluate
    ProcessorState::~ProcessorState
   Removed the following functions
    ProcessorState::getLoadedDocument
    ProcessorState::addLoadedDocument

* Added made correct recursion-loop-protection for xsl:import/include.
   Added the following functions
    ProcessorState::enterStylesheet   notifies the processorstate that a new
                                      stylesheet is entered
    ProcessorState::leaveStylesheet   notifies the processorstate that a
                                      stylesheet is left

* The bottom-to-top processing didn't go well with how default templates worked
  which actually is since findTemplate dosn't support importprecidence. However
  I didn't want to fix this in findTemplate since our defaulttemplate handling
  is pretty bad anyway. We currently do it by adding the templates to the
  xslt-DOM. This is first of all a BadThing, and second, not all default
  templates can be done this way.
  Now I instead implement them by adding some code to ::processTemplate
   Removed code from
    ProcessorState::initialize
    ProcessorState::~ProcessorState
   Added code in
    XSLTProcessor::processTemplate

* Some small changes in findTemplates to account for the top-to-bottom search
  When two templates have the same priority I select the first one instead of
  the last one.
   Changed code in
    ProcessorState::findTemplate

I'm not entierly happy with how import states work (don't like adding three
functions and two variables to ProcessorState), but lets take that over IRC.

Ok, with a description like this I expect a fast review ;-)

Comment 1

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

Attachment: patch ver 1

Comment 2

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

Just noticed some not-so-nice if nesting in the new default template code, but 
I'll fix that when we've decided about how to handle import precedence

Comment 3

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

Attachment: ver 2

Comment 4

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

removed get/setImportPrecedence from ProcessorState and made import precedence 
an argument to XSLTProcessor::processTopLevel instead. So the flow of the code 
is exactly the same as before...

Comment 5

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

I split up the patch for your reviewing pleasure :)

The patch that actually implements xsl:import is very dependent on the other 
patches, but the other ones should be pretty independent...

Comment 6

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

Attachment: implement default templates using code instead modifying DOM

Comment 7

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

Attachment: Move retreaveDocument from DocumentFunctionCall to ProcessorState

Comment 8

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

Attachment: small change (and small fix) to ProcessorState::findTemplate to account bottom-to-top processing in processTopLevel

Comment 9

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

Attachment: actually implement xsl:import

Comment 10

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

Attachment: Merged with tip. Only doing the "total" file for now as there are no functional differences...

Comment 11

[Axel Hecht]

two things, enterStylesheet doesnt push the URI to enteredStylesheets, and
there's a TAB in the line impPrec = importPrecedence;

Apart from this the singled patches looked fine.

Gonna check the global patch later.

Axel

Comment 12

[Axel Hecht]

about implementing the import precedence.
all we needed was moving ProcessorState.templates from a NodeList to a list of
NodeLists, and we would get the right behaviour in findTemplate rather easy.
Having just a list of precedence yields the idea of dropping the numeric value
alltogether, and it seems like a feasable task.
Then we could do processTopLevel forward again, which is a good investment into
the future, where we might end up doing at least processTopLevel async.

Axel

Comment 13

[Axel Hecht]

spawned bug 83651 for making use of import precedence.
I have two major points.
First, we shouldn't implement import as part of the processTopLevel children
loop.
It's only allowed as first elements of xsl:stylesheet, and so we should impl
import in a extra loop.
Going with the idea of frames, adding the current frame when entering the 
toplevel loop and after processing the imports should get us to an easy (forward
looping) implementation of import precedence.
I'm rather religious on both points, do have only forward loops, and don't allow
xsl:import mixed with other loplevel elements. As far as I read the spec, we
are in worse trouble than we have to, if we don't keep those two loops apart. 

Axel

Comment 14

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

ok, I've implemented the frames (though I call them containers) and made a 
separate loop for imports. Since I now traverse the document top-to-bottom I'm 
no longed dependant on default templates being implemented in code so I broke 
out bug 86270 on that.

However this is now dependant on a bugfix in the List class which is avalible 
in bug 85189. If that is not landed first the list of containers will be broken 
(which currently "only" causes leaks)

Comment 15

[Axel Hecht]

adding dep on loading documents at all for module

Comment 16

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

Attachment: With importContainers

Comment 17

[Axel Hecht]

mental note, ProcessorState::loadedDocuments

Comment 18

[Axel Hecht]

setting milestone on the bugs that nisheeth wanted. Setting Priority up for
landing indication

Axel

Comment 19

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

Attachment: fixed comments from peterv and Pike

Comment 20

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

got r=peterv on attachment 46069 [details] [diff] [review] on irc

Comment 21

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

pushing and cc'ing shaver for sr

Comment 22

[Mike Shaver (:shaver emeritus)]

Comment on attachment 46069 [details] [diff] [review]
fixed comments from peterv and Pike

sr=shaver (also marking has-review based on previous comment)

Comment 23

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

Small crashlanding, but now it's in...

Comment 24

[Axel Hecht]

bitching buttons, verfication spam