Last Comment Bug 696078 - Move filter attribute processing to frame classes
: Move filter attribute processing to frame classes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Robert Longson
:
Mentors:
Depends on: 704706
Blocks: 696210 740945
  Show dependency treegraph
 
Reported: 2011-10-20 07:52 PDT by Robert Longson
Modified: 2012-03-30 13:28 PDT (History)
2 users (show)
longsonr: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (81.97 KB, patch)
2011-10-20 07:55 PDT, Robert Longson
no flags Details | Diff | Splinter Review
updated (86.97 KB, patch)
2011-10-22 08:17 PDT, Robert Longson
no flags Details | Diff | Splinter Review
update to tip (84.42 KB, patch)
2011-10-29 08:08 PDT, Robert Longson
no flags Details | Diff | Splinter Review
address review comments (84.01 KB, patch)
2011-11-16 10:03 PST, Robert Longson
jwatt: review+
Details | Diff | Splinter Review
updated to pass reftests (86.21 KB, patch)
2011-11-18 08:25 PST, Robert Longson
no flags Details | Diff | Splinter Review
hg changeset patch (66.47 KB, patch)
2011-11-19 11:41 PST, Robert Longson
jwatt: review+
Details | Diff | Splinter Review

Description Robert Longson 2011-10-20 07:52:52 PDT

    
Comment 1 Robert Longson 2011-10-20 07:55:24 PDT
Created attachment 568394 [details] [diff] [review]
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 2011-10-22 08:17:00 PDT
Created attachment 568876 [details] [diff] [review]
updated
Comment 3 Robert Longson 2011-10-24 07:28:11 PDT
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 2011-10-29 08:08:07 PDT
Created attachment 570479 [details] [diff] [review]
update to tip
Comment 5 Jonathan Watt [:jwatt] 2011-11-02 08:41:41 PDT
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 2011-11-02 11:58:50 PDT
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] 2011-11-12 17:16:21 PST
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] 2011-11-12 17:17:51 PST
An updated patch taking into account the feedback in the previous bugzilla comments would be welcome at this point.
Comment 9 Robert Longson 2011-11-12 17:36:33 PST
(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] 2011-11-16 03:37:30 PST
That's probably a good enough reason. :)
Comment 11 Robert Longson 2011-11-16 10:03:58 PST
Created attachment 574927 [details] [diff] [review]
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] 2011-11-17 17:06:52 PST
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 2011-11-18 08:25:47 PST
Created attachment 575471 [details] [diff] [review]
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 2011-11-18 10:00:37 PST
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
Comment 15 Robert Longson 2011-11-19 11:41:01 PST
Created attachment 575688 [details] [diff] [review]
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] 2011-11-21 14:17:45 PST
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed397fc162a
Comment 17 Ed Morley [:emorley] 2011-11-21 19:06:29 PST
https://hg.mozilla.org/mozilla-central/rev/5ed397fc162a

Note You need to log in before you can comment on or make changes to this bug.