Closed Bug 696078 Opened 8 years ago Closed 8 years ago

Move filter attribute processing to frame classes

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

Attachments

(1 file, 5 obsolete files)

No description provided.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → longsonr
Flags: in-testsuite-
Attachment #568394 - Flags: review?(jwatt)
Blocks: 696210
Attachment #568394 - Flags: review?(jwatt)
Attached patch updated (obsolete) — Splinter Review
Attachment #568394 - Attachment is obsolete: true
Attachment #568876 - Flags: review?(jwatt)
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.
Summary: Move attribute processing to frame classes → Move filter attribute processing to frame classes
Attached patch update to tip (obsolete) — Splinter Review
Attachment #568876 - Attachment is obsolete: true
Attachment #568876 - Flags: review?(jwatt)
Attachment #570479 - Flags: review?(jwatt)
Still working on this, but one thing you can do while waiting is drop the 'ns' prefix from new classes and file names.
No worries. Even if you completed your review today and had no comments I wouldn't land this before November 9th now anyway.
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.
An updated patch taking into account the feedback in the previous bugzilla comments would be welcome at this point.
(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.
That's probably a good enough reason. :)
Attached patch address review comments (obsolete) — Splinter Review
(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.
Attachment #570479 - Attachment is obsolete: true
Attachment #570479 - Flags: review?(jwatt)
Attachment #574927 - Flags: review?(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.)
Attachment #574927 - Flags: review?(jwatt) → review+
Attached patch updated to pass reftests (obsolete) — Splinter Review
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.
Attachment #574927 - Attachment is obsolete: true
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/longsonr@gmail.com-c52dd94aaf10
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.
Attachment #575471 - Attachment is obsolete: true
Attachment #575688 - Flags: review?(jwatt)
Attachment #575688 - Attachment is patch: true
Attachment #575688 - Flags: review?(jwatt) → review+
https://hg.mozilla.org/mozilla-central/rev/5ed397fc162a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Depends on: 704706
You need to log in before you can comment on or make changes to this bug.