696078 - Move filter attribute processing to frame classes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

This finishes off the removal of all DidAnimateXXX and DidChangeXXX overrides. It does not change the nsSVGElement methods to non-virtual as I'm going to do that in a follow-up bug.

Comment 2

[Robert Longson [:longsonr]]

Attachment: updated

Comment 3

[Robert Longson [:longsonr]]

An alternative to what I've done would be to have the SVG filter child elements (feFunc, feMergeNode, feSpotLight etc) inherit from some new nsSVGFEChild class. This class would have a virtual IsDependentAttribute method just like the nsSVGFE class and we could then have a single nsSVGFELeafFrame class for all the child elements.

Comment 4

[Robert Longson [:longsonr]]

Attachment: update to tip

Comment 5

[Jonathan Watt [:jwatt]]

Still working on this, but one thing you can do while waiting is drop the 'ns' prefix from new classes and file names.

Comment 6

[Robert Longson [:longsonr]]

No worries. Even if you completed your review today and had no comments I wouldn't land this before November 9th now anyway.

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 570479 [details] [diff] [review]
update to tip

Review of attachment 570479 [details] [diff] [review]:
-----------------------------------------------------------------

Can we get rid of the five virtually identical classes SVGFEDistantLightFrame, SVGFEPointLightFrame, SVGFESpotLightFrame, SVGFEFuncFrame and SVGFEMergeNodeFrame? How about having one SVGFEUnstyledLeafFrame, and have an element class SVGFEUnstyledElement for the filter primitive elements that don't inherit nsSVGStylableElement to inherit. That class will be there purely to have it's own IsDependentAttribute (or whatever we call it). The class definition could be put right before the definition of nsSVGFE.


Can you also add a doxygen comment to class nsSVGFE noting that it is not inherited by all filter primitive classes?

::: content/svg/content/src/nsSVGFilters.cpp
@@ +249,5 @@
>  }
>  
> +bool
> +nsSVGFE::IsDependentAttribute(PRInt32 aNameSpaceID,
> +                              nsIAtom* aAttribute) const

I don't think "IsDependentAttribute" is a name that provides much insight into what information it is that this method provides. After reading the code I realize it's basically saying that the attribute is recognizes as having special meaning on this element (isn't some random attribute), and more specifically that it could affect the visual output of the SVG. I think we could come up with a different name that would better convey this. Some suggestions: IsRenderingAttribute, IsRecognizedAttribute, AttrAffectsOutput, AttrAffectsPaint or AttrAffectsRendering. Thoughts?

@@ +5470,5 @@
>                                    const nsAString* aValue, bool aNotify)
>  {
>    if (aNamespaceID == kNameSpaceID_XLink && aName == nsGkAtoms::href) {
> +
> +    // If there's a frame it will deal

Please expand this comment to let people know why we bother to do this when there is *not* a frame.

@@ +5607,5 @@
> +                                          nsIAtom* aAttribute) const
> +{
> +  return nsSVGFEImageElementBase::IsDependentAttribute(aNameSpaceID, aAttribute) ||
> +         (aNameSpaceID == kNameSpaceID_None &&
> +          aAttribute == nsGkAtoms::preserveAspectRatio);

Please add a comment here noting that you're not including nsGkAtoms::href and explaining why.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +4856,5 @@
> +    SIMPLE_SVG_CREATE(feDisplacementMap, NS_NewSVGFELeafFrame),
> +    SIMPLE_SVG_CREATE(feFlood, NS_NewSVGFELeafFrame),
> +    SIMPLE_SVG_CREATE(feGaussianBlur, NS_NewSVGFELeafFrame),
> +    SIMPLE_SVG_CREATE(feImage, NS_NewSVGFEImageFrame),
> +    SIMPLE_SVG_CREATE(feMerge, NS_NewSVGFEContainerFrame),

Weird that we previously gave feMergeNode a frame when we didn't give feMerge a frame.

@@ +4862,5 @@
> +    SIMPLE_SVG_CREATE(feMorphology, NS_NewSVGFELeafFrame), 
> +    SIMPLE_SVG_CREATE(feOffset, NS_NewSVGFELeafFrame), 
> +    SIMPLE_SVG_CREATE(feSpecularLighting, NS_NewSVGFEContainerFrame),
> +    SIMPLE_SVG_CREATE(feTile, NS_NewSVGFELeafFrame), 
> +    SIMPLE_SVG_CREATE(feTurbulence, NS_NewSVGFELeafFrame) 

So did you run into problems due to frames not inheriting nsContainerFrame, or do you have some logic for making filter primitive frames now inherit nsContainerFrame (something more specific than "well now they're containers")?

::: layout/svg/base/src/nsSVGFEContainerFrame.cpp
@@ +52,5 @@
> +
> +  virtual bool IsFrameOfType(PRUint32 aFlags) const
> +  {
> +    return nsSVGFEContainerFrameBase::IsFrameOfType(
> +            aFlags & ~(nsIFrame::eSVG | nsIFrame::eSVGContainer));

It's not clear to me that we should be claiming eSVGContainer here, since that seems like an "implements nsSVGContainerFrame" thing, I think. (Maybe a better name for eSVGContainer would be eSVGGraphicContainer?)

::: layout/svg/base/src/nsSVGLeafFrame.cpp
@@ +40,3 @@
>  
> +typedef nsFrame nsSVGFELeafFrameBase;
> +

Please add a doxygen comment here explaining what "leaf" means in the context of a filter primitive. Specifically, noting that this class is used for filter primitives that do NOT have special child elements that provide them with parameters.

Comment 8

[Jonathan Watt [:jwatt]]

An updated patch taking into account the feedback in the previous bugzilla comments would be welcome at this point.

Comment 9

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #7)
> 
> So did you run into problems due to frames not inheriting nsContainerFrame,
> or do you have some logic for making filter primitive frames now inherit
> nsContainerFrame (something more specific than "well now they're
> containers")?

If an element doesn't get something derived from nsContainerFrame then any child elements never have frames created.

Comment 10

[Jonathan Watt [:jwatt]]

That's probably a good enough reason. :)

Comment 11

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to Jonathan Watt [:jwatt] from comment #7)
> 
> It's not clear to me that we should be claiming eSVGContainer here, since
> that seems like an "implements nsSVGContainerFrame" thing, I think. (Maybe a
> better name for eSVGContainer would be eSVGGraphicContainer?)

nsSVGEffects::InvalidateRenderingObservers requires that container frames are nsIFrame::eSVGContainer otherwise invalidation doesn't work.

http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGEffects.cpp#615

So I've left that as it is. I've addressed the other review comments.

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 574927 [details] [diff] [review]
address review comments

Great comment additions, thanks Robert. Drop the "ns" prefixes from the new classes and file names as requested in comment 5 and r=jwatt. (The easiest way to do that is to have your patch as a mercurial patch, then qpop it, edit the patch file in .hg/patches/... with search and replace to fix the class and file names, and then qpush it back on again.)

Comment 13

[Robert Longson [:longsonr]]

Attachment: updated to pass reftests

Previous version crashed on nesting-invalid-01.svg I've added code in nsCSSFrameConstructor to validate that we only create filter primitive children when they are children of a filter element.

Comment 14

[Mozilla RelEng Bot]

Try run for c52dd94aaf10 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c52dd94aaf10
Results (out of 92 total builds):
    success: 70
    warnings: 20
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c52dd94aaf10

Comment 15

[Robert Longson [:longsonr]]

Attachment: hg changeset patch

had to change nsCSSFrameConstructor so that it passed nesting-invalid-01.svg. The change is to introduce extra validation so that filter primitives (e.g. feMerge) only get frames if they are direct children of a filter and the various filter primitive children must be children of a filter primitive.

Comment 16

[Jonathan Watt [:jwatt]]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed397fc162a

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5ed397fc162a