Integrate improvements from james experimental to trunk about svgweb HOT 3 CLOSED

James:
Here are the comments I have so far on the review:

core/
    SVGGradient.as: nice cleanup separating out getStopData and getMatrix
        getMatrix():
            I do not understand the getMatrix calculation changes. My understanding
of the gradient in SVG is that the object width and height does not factor into 
the
gradient placement, orientation, scaling, or repeat boundaries. I do not see 
how your
calculations using those factors can be correct. I'd be suprised if this works 
with
all the demo files. I am hopeful that the code that is valiantly trying to 
determine
the object width and heights can be removed.

            I had the calculations separate for linear and radial gradients because
radial gradients are only partially implemented and I suspected the remaining 
work
would require different implementations, but I agree with unifying them for now 
to
avoid dual maintenance in the mean time.

    SVGNode.as:
        fill = getColor(..)  is getColorAndAlpha(..) on the trunk
        The reason is that based on my testing with firefox, all these opacities are
cumulative in SVG and so they all need to be factored in. The node_alpha code 
needs
to be removed, though. Not sure where I was going with that. Note that rgba 
syntax is
implemented here using getColorAndAlpha, but not anywhere else. I only 
encountered
rgba on one occasion, and it was on a fill and so that is why it is implemented 
here.
I think full rgba support will take some work but probably needs to be done at 
some
point.

        Nice cleanup separating out parseNodes. By triggering this in the
constructor, from the setting of the xml, doesn't this result in a parse of the
entire tree all at once as opposed to the staggered asynchronous approach that 
waited
for frame rendering? I'm not saying that is bad necessarily, just that this 
seems
pretty significant and want to understand. I thought there might have been 
issues I
ran into before with initializing the child before the parent had rendered, but 
I do
not remember what it was. Perhaps the setAttribute / style cleanup fixed those
issues. I still wonder if there is a gotcha here, but if testing reveals no 
problems,
maybe not.

        TODO:
            Attribute / style changes
            Changes to transforms, viewBox
            Cloning changes
            Node parsing quirks of masks and blurs
            Scripting impacts


utils/
    SVGColors.as:
        getColor and getColorAndAlpha
            the trim call is partially redundant with the replace call 3 lines down.
            proposed solution: remove trim and fix the replace three lines down to be
/\s/g
            reasoning: performance is a concern here and /\s/g is probably the
simplest one pass regex, while the trim approach avoids excessive replacements. 
My
hunch is that one pass is faster (unless /g is a perf killer). The function 
call has
a null check which becomes redundant, and once removed the whole function call 
has
minimal value and so I'd leave the whitespace strip inline. Performance testing
should guide us on what to do on issues like these, once the code settles down.


svg/
    TODO:
        second pass review of SVGViewer separations

Testing:
    TODO:
        Copy james-experimental code into copy of trunk in order to test demos /
scripting files.

core/
    SVGNode.as:
        Attribute / style changes:
            setAttribute():
               For the the TO DO: regarding detecting when to redraw, I'd look at
handleAttrChange() on the trunk which attempts to solve the same problem.  Note
however that the trunk has a unified transformNode and so handleAttrChange will 
not
work the same on this branch with respect to transforms

        Changes to transforms, viewBox:
            parseTransform():
                A change in order of operations appears to have taken place in this
branch with respect to applyViewBox versus transformNode (the attribute). Hmm. 
Perhaps this helps remove another of my hacks (the viewbox undo matrix) but the
applying of the viewbox transform first ordering was done for correctness after 
a
bunch of analysis and testing and so this change will need extra attention. 
Believe
me, I'd like to avoid using that undo matrix and I am open to another approach 
that
works as well as the trunk.

        Cloning changes:
           clone():
               James, you are a genius with these esoteric details of actionscript.

           For <use> tags,
               The design appears to be to copy referenced nodes as a child when
rendering and keep the overrides in the parents to preserve local changes.

               I like the way ids are handled by not registering the duplicates
instead of renumbering like the trunk.  Yours is more in line with the spec.

               The thing that concerns me is that the spec appears to say that <use>
nodes are copied into separate, non-exposed DOM tree but it does not appear to 
be
explicit about whether changes to the original should effect the clone, which 
is what
I sort of assumed but now I'm not so sure.  I thought I had this understood, 
but I'll
have to test this in Firefox to be sure. If we don't have to update those 
clones that
would be great.

           For xlink:href handling, your implementation appears to be reserved just
for the common usage of gradients and has an on-demand recursive traversal to 
the
stops. I think the trunk implementation works for all nodes and I think that is
legitimate syntax for other node types. Realistically, I've only seen 
xlink:href on
<a>, gradients, <use>, and <pattern>.

           With respect to gradients specifically, the implementation does not
account for potential attribute overrides and children across different nodes 
like
the trunk attempts to. Not that I can point to any existing example that does 
that,
but I think I could create examples that would not work with these 
simplifications,
so there would be a cost for the simplicity gained.

           The idea of leaving the xml inheritence that xlink:href calls for to the
moment of retrieval is an interesting idea. The trunk makes a recursive copy of 
the
data all at once at the start of parsing. The recursive copy is a bit messy but
dealing with href on demand for more than a few scenarios could start to get 
messy
too. Its not really clear which is better. Something I'll keep thinking about.


        Node parsing quirks of masks and blurs:
           *see note below

        Scripting impacts:
           Some code for inline html retrieval appears missing on your branch but
they could be adapted to the new SVGViewerWeb.

        *In some of the above code areas, certain ugly hacks of mine for clipping and
blurs are not present in your branch.  The resulting code simplifications are 
nice,
but correctness remains a top priority for integrations to the trunk. I am 
eager to
find solutions to the problems the hacks solve that are more elegent and the
simplifications in your code represents an ideal I'm hoping to move the trunk 
towards.

svg/
        Second pass review of SVGViewer separations:
           Some code for inline html retrieval appears missing on your branch but
they could be adapted to SVGViewerWeb.

    SVGSVGNode.as:
        This is good stuff, I think. My preference is to use the Firefox
implementation as a guide as to what is correct with respect to Id scopes for 
inline
<svg> nodes. So, this will take some testing. But assuming each svg node has a
separate Id table, this will be quite handy work. Getting rid of SVGRoot is 
probably
a good thing too.

TODO:
Testing:
    Copy james-experimental code into copy of trunk in order to test demos /
scripting files.
        LOADED event dispatching:
           TODO: test that ONLOAD is always tiggerring at the end

There are a number of improvements that are going to be worked on independently 
for
merging to the trunk. I am going to open an issue for each improvement so I can 
keep
track of them and I will close this Issue since it is too broad.