409383 - Switch systemLanguage: transient error again

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

The patch for bug 375173 is not working, perhaps it never did properly.

<switch> elements are currently modelled as svgGFrames so we can't use frame->GetType() to identify them.

The patch tests for a svgGFrame, is that too fragile? If we choose to give switch its own frame in the future it will break.

Comment 1

[Robert Longson [:longsonr]]

Attachment: probably better if it doesn't leak

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why don't you make a new switchframe type right now to prevent that fragility?

Comment 3

[Robert Longson [:longsonr]]

We certainly could. It could inherit from an svgGFrame and just override GetType(). I'd like tor's opinion on that ideally.

Comment 4

[Robert Longson [:longsonr]]

Attachment: switch frame

Comment 5

[Robert Longson [:longsonr]]

Tor was OK with the idea.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 294292 [details] [diff] [review]
switch frame

a=beltzner for 1.9

Comment 7

[Robert Longson [:longsonr]]

Checked in, although this is still not completely fixed. The machine I originally developed this patch on now displays the testcase correctly all the time but another slower machine still does not occasionally.

Putting a breakpoint in the nsCSSFrameConstructor check code now shows we are rejecting multiple switch children at least some of the time. Unfortunately it is still possible occasionally to get multiple children clicking on refresh or shift-refresh lots of times on a slow machine. 

GetFirstChild is clearly returning 0 even though a child frame has been created. I've tried a number of alternatives to GetFirstChild and all suffer from the same issue. I guess the child frames are not completely set up when we are doing the check sometimes. Suggestions appreciated.

Comment 8

[Boris Zbarsky [:bzbarsky]]

The problem, presumably, is that we're ending up in ContentAppended with the switch as the parent.  Then we do the loop over all the new kids, construct frames for them all, and _then_ append them to the parent.  When constructing the switch to start with, this problem is avoided by using SVGSwitchProcessChildren instead of ProcessChildren.  But in this case the switch frame has been constructed.

In other words, <switch> has been broken in the face of dynamic DOM changes pretty much all along.  I'm surprised no one bothered to test that when creating the original switch implementation...

The right fix would seem to be to write separate versions of all the child frame processing for switch (including in ContentInserted/ContentAppended).  Or something along those lines.

Comment 9

[Dave Townsend [:mossop]]

I've backed this patch out since it seems to be causing a test failure on fxdbug-win32-tb

Comment 10

[Dave Townsend [:mossop]]

I should add that I left the file nsSVGSwitchFrame.cpp in tree rather than deleting it.

Comment 11

[Robert Longson [:longsonr]]

Although this looks like it wasn't the cause of the test failure, please don't reland it. I want to think about comment 8 first.

Comment 12

[Jonathan Watt [:jwatt]]

It would be really good to get this fixed for 1.9. I'm actually wondering if it shouldn't block...

Comment 13

[Robert Longson [:longsonr]]

The approved patch doesn't do much good which is why I never tried checking it in again even though I doubt it was the cause of any failures.

Basically we need a switch frame class that knows that it should display one and only one child. Most of the code for switch processing in nsCSSFrameConstructor would come out into either the switch frame class or nsSVGElement. This would be a substantial patch.

Comment 14

[Damon Sicore (:damons)]

Need to have a stronger feeling that "wondering if this shouldn't block" to actually block at this point.  If it doesn't dramatically impact user experience, then it's not blocking.

wanted1.9.0.x+.

Comment 15

[Robert Longson [:longsonr]]

Attachment: patch

Comment 16

[Robert Longson [:longsonr]]

(In reply to comment #12)
> It would be really good to get this fixed for 1.9. I'm actually wondering if it
> shouldn't block...
> 

I wonder if you'll still think the same way once you review the patch?

Comment 17

[Boris Zbarsky [:bzbarsky]]

The nsSVGUtils change looks fishy to me, given all the people using xbl:extends="svg:g" out there.

Comment 18

[Robert Longson [:longsonr]]

We have casts to nsSVGElement all over the svg frame code, assuming that's what you mean. We try very hard not to create SVG frames except for SVG content.

The first thing that method does is check that the frame implements nsISVGChildFrame. Does that help reassure you?

I don't really know how xbl:extends works. Is there an example anywhere?

Would an IsFrameOfType(nsIFrame::eSVG) test round the new code make you any happier?

Comment 19

[Jonathan Watt [:jwatt]]

(In reply to comment #18)
> I don't really know how xbl:extends works. Is there an example anywhere?

It essentially means that the bound element gets given the same frame as the element specified in the 'extends' attribute would get, as I understand it. I think our frame Init() implementations prevent this for SVG though. (They QI their mContent and return failure if mContent isn't the type of content the frame expects.)

Comment 20

[Robert Longson [:longsonr]]

The bound element would not implement nsISVGChildFrame though would it? If so it wouldn't get very far in PaintChildWithEffects.

Comment 21

[Boris Zbarsky [:bzbarsky]]

The first part of comment 19 is spot on.

The part about Init() may be true for a lot of SVG frames, but I'm not seeing anything on the parent chain of nsSVGGFrame that enforces that.  In particular, nsSVGGFrame doesn't implement Init(), its parent class is nsSVGDisplayContainerFrame, which doesn't do any QI checks, and calls into non-SVG Init() code.

Of course a simple test of the whole thing should lead to crashes when that code was hit, so I'm not sure why we're trying to tell this by code inspection.  And as I said, people use this stuff.  Some of our SVG reftests use it, in fact, reduced from testcases in bugs.

All the stuff in comment 18 doesn't help, because in this case there would really be an nsSVGGFrame for a non-SVG content node.

If we have other casts, it might be worth checking whether they're actually safe.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> The bound element would not implement nsISVGChildFrame though would it? 

Its frame sure would.

Comment 23

[Robert Longson [:longsonr]]

(In reply to comment #21)
> The part about Init() may be true for a lot of SVG frames, but I'm not seeing
> anything on the parent chain of nsSVGGFrame that enforces that.  In particular,
> nsSVGGFrame doesn't implement Init(), its parent class is
> nsSVGDisplayContainerFrame, which doesn't do any QI checks, and calls into
> non-SVG Init() code.

NS_NewSVGGFrame checks that its content implements nsIDOMSVGTransformable.

> 
> Of course a simple test of the whole thing should lead to crashes when that
> code was hit, so I'm not sure why we're trying to tell this by code inspection.
>  And as I said, people use this stuff.  Some of our SVG reftests use it, in
> fact, reduced from testcases in bugs.

I can't find anything with extends svg:g. If you can point me to anything I'm happy to test and fix any issues. Maybe bug 293704 has something that could be a starting point if I knew what to do with it.

> If we have other casts, it might be worth checking whether they're actually
> safe.
> 

We have 20 or so casts to nsSVGElement, more to types derived from that.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Huh.  Odd.  I know I've seen tests go by that used extends="svg:g" or some such because they were binding SVG to raw XML and otherwise things didn't show up at all...

Comment 25

[Boris Zbarsky [:bzbarsky]]

But yeah, if we're 100% sure this can never happen the cast is safe, I guess.  It would be nice if there were a check like this in the frame constructor instead of scattering it over all the frame classes.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Perhaps I'm just remembering something that used to work and no longer does.  In which case, you may want to tell the author of the patch in bug 378518.

Comment 27

[Robert Longson [:longsonr]]

(In reply to comment #26)
> Perhaps I'm just remembering something that used to work and no longer does. 
> In which case, you may want to tell the author of the patch in bug 378518.
> 

Done.

Comment 28

[Robert Longson [:longsonr]]

Notes:

Will not work if you add/remove switch elements which did't work before.
Will still not work if you change switch elements SatisfiesConditions attributes (although it may well work if you change them and the element is not in a switch).

The dynamic thing switch child attribute could presumably be done as a separate patch since nobody seems to have noticed it doesn't work up to now.

Comment 29

[Robert Longson [:longsonr]]

That first note should have said.

Will work if you add/remove switch elements which did't work before.

Comment 30

[Boris Zbarsky [:bzbarsky]]

So...  As sicking points out in bug 378518 the fact that NS_NewSVGGFrame fails here means we'll fall through to 

7424     newFrame = NS_NewSVGGenericContainerFrame(mPresShell, aContent, aStyleContext);

in ConstructSVGFrame.

That seems to construct a frame that is in fact nsISVGChildFrame, as far as I can tell.

Comment 31

[Robert Longson [:longsonr]]

Attachment: address review comment

The croczilla xbl examples http://croczilla.com/svg/samples/ show that Boris is absolutely correct as usual ;-).

I've put in a guard to prevent the call unless we are working with SVG content. We do need to audit nsSVGDisplayContainerFrame in some other bug as its existing use of SVG state bits seems dodgy.

Comment 32

[Robert Longson [:longsonr]]

Comment on attachment 315055 [details] [diff] [review]
address review comment

will need a reviewer for the non-svg changes

Comment 33

[Boris Zbarsky [:bzbarsky]]

Which non-svg changes?  The frame constructor ones?

Are we trying to get this into rc1?

Comment 34

[Robert Longson [:longsonr]]

Attachment: updated patch

Changed to a static member function to avoid casts altogether.

I suspect we're too late for rc1.

Yes I meant the frame constructor changes. They're really only xode removal though.

Comment 35

[Boris Zbarsky [:bzbarsky]]

If this isn't a "must get into rc1" kind of thing, it'll likely take me a week or so to get to it.

Comment 36

[Robert Longson [:longsonr]]

jwatt and roc are the drivers for SVG so one of them should decide. Unless they do I think you are right to default to assume not in rc1.

Comment 37

[Robert Longson [:longsonr]]

Attachment: reftests

Comment 38

[Robert Longson [:longsonr]]

Attachment: fix text display

Comment 39

[Robert Longson [:longsonr]]

Attachment: fix switch element child test

Sorry for all the review requests.

Comment 40

[Robert Longson [:longsonr]]

Comment on attachment 315480 [details] [diff] [review]
fix switch element child test

I guess this won't make 1.9. I'll try to make something smaller and easier to review. I think I can split it into a switch part and a features part.

Comment 41

[Robert Longson [:longsonr]]

Attachment: reduced patch

This version makes no attempt to fix conditional processing outside the switch element. Non-switch conditional processing needs much more work, building on the changes in this patch.

Comment 42

[Joe Drew (not getting mail)]

Comment on attachment 294292 [details] [diff] [review]
switch frame

Remove approval, since this was backed out.

Comment 43

[Jonathan Watt [:jwatt]]

Huh? What's the point of removing approval flags? It just creates noise and hides history.

Comment 44

[:Gavin Sharp [email: [e-mail]]]

We were trying to make sure that no patches were forgotten for 1.9, and having lots of unresolved bugs with approved patches on them makes that difficult.

Comment 45

[Jonathan Watt [:jwatt]]

I'd have thought it better to use some whiteboard flag to exclude those you've audited from your search would be better, but okay.

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Yes, that's what we settled on after some initial confusion.

Comment 47

[Jonathan Watt [:jwatt]]

Comment on attachment 316683 [details] [diff] [review]
reduced patch

Can you rip out all the comments from nsSVGElementList.cpp and just put something along the lines of the following at the top of the file:

/**
 * This file is used to help create a mapping from a specified SVG element to
 * attributes supported by that element. This mapping can be used to help
 * ensure that we don't accidentally implement support for attributes like
 * requiredFeatures on elements for which the SVG specification does not
 * define support.
 *
 * To use, include this file into another file after defining the SVG_ELEMENT
 * C preprocessor macro as appropriate.
 *
 * The following constants represent the following attributes:
 *
 * SUPPORTS_TEST
 *   The requiredFeatures, requiredExtensions, and systemLanguage attributes
 *
 * SUPPORTS_EXTERNAL
 *   The externalResourcesRequired attribute
 *
 * SUPPORTS_ALL
 *   A convenience value indicating support for all of the above
 *
 * SUPPORTS_NONE
 *   A convenience value indicating support for none of the above
 */

It looks to me like SUPPORTS_LANG should die. I don't see anywhere in the specification where one of requiredFeatures, requiredExtensions, or systemLanguage is valid on an element without the others being valid too. In the SVG 1.1 DTD they only appear as a group. I've no idea why requiredFeatures and requiredExtensions were separated from systemLanguage in our code in this way.

I'd be quite happy if you changed SUPPORTS_TEST to SUPPORTS_CONDITIONAL since that makes it align more closely with the DTD entity for that attribute group. In fact it would make more sense to change the SUPPORTS_ to ATTRS_ IMO since these are a group of attributes (see also my comment below regarding "SupportsConditional").


Can you put a comment along the lines of the following at the top of nsSVGFeatures.cpp, just after the license block:

/**
 * This file contains code to help implement the Conditional Processing
 * section of the SVG specification (i.e. the <switch> element and the
 * requiredFeatures, requiredExtensions and systemLanguage attributes).
 *
 *   http://www.w3.org/TR/SVG/struct.html#ConditionalProcessing
 */


>+/**
>+ * Test to see if a feature is implemented
>+ *
>+ * @param fstr the feature to test
>+ */

Can you make that:

/**
 * Check whether we support the given feature string.
 *
 * @param fstr one of the feature strings specified at
 *    http://www.w3.org/TR/SVG/feature.html
 */

> PRBool
>-NS_SVG_TestFeature(const nsAString& fstr) {
>+NS_SVG_TestFeature(const nsAString& fstr)

It would be nice if this was called "NS_SVG_HaveFeature" while you're touching this code. Using the word "test" makes the functions purpose a little less clear.

>+/**
>+ * Test to see if a list of features are implemented

Can you make that:

/**
 * Check whether we support the given list of feature strings.
 *
 * @param fstr a white space separated list containing one or more of the
 *   feature strings specified at http://www.w3.org/TR/SVG/feature.html
 */

>+static PRBool
>+TestFeatures(const nsAString& fstr)

Same "Test" -> "Have" comment as above.



>+/**
>+ * Test to see if this element supports a specific conditional
>+ *
>+ * @param atom the tag for the element
>+ * @param cond the conditional to test for, one of
>+ *             SUPPORTS_TEST, SUPPORTS_LANG or SUPPORTS_EXTERNAL
>+ */
> static PRBool
>+SupportsConditional(const nsIAtom *atom, PRUint16 cond)

I think the word "conditional" should be restricted to the Conditional Processing attributes requiredFeatures, requiredExtensions, and systemLanguage. The attribute externalResourcesRequired is something else. As a result, I'd suggest naming this function "ElementSupportsAttributes" or something, and changing the comment to "Check whether the specified element supports the specified attributes (i.e. whether the SVG specification defines the elements for the specified element)." (Yes, I know we only pass SUPPORTS_TEST/SUPPORTS_LANG in this file, but it makes the function's functionality clearer anyway.) Can you also change "atom" to "tagName"? Oh, and it would also be nice if you could move it down to just above NS_SVG_SatisfiesConditions so as to keep TestFeatures and TestLanguage together.

That said, I'd prefer renaming TestLanguage to MatchesLanguagePreference to make it clearer what it does.


>+NS_SVG_SatisfiesConditions(nsIContent *aContent, const nsIAtom *aTag)

If you make the changes regarding merging SUPPORTS_TEST and SUPPORTS_LANG I suggested above, you can just return true up front if SupportsConditional (ElementSupportsAttributes) returns false.

Can you move the requiredFeatures check before the requiredExtensions check?

I think you should also feel free to remove the DEBUG_scooter section.


More to come, but an updated patch would be appreciated.

Comment 48

[Robert Longson [:longsonr]]

Attachment: patch

review comments addressed.
fixes bug 441488 too.
includes reftests as part of patch

Comment 49

[Jonathan Watt [:jwatt]]

Comment on attachment 328254 [details] [diff] [review]
patch

>+    if (parent && parent->Tag() == nsGkAtoms::svgSwitch) {
>+      (static_cast <nsSVGSwitchElement *>(parent))->MaybeInvalidate();
>+    }

Can you use parent->NodeInfo()->Equals(nsGkAtoms::svgSwitch, kNameSpaceID_SVG) so we also check the namespace please?

Also I think general style is not to bother with the parentheses around the static_cast and not have the spaces in there, but I have no strong preference.


The links I gave you to add to nsSVGFeatures.cpp are to http://www.w3.org/TR/SVG/... - can you actually change them to http://www.w3.org/TR/SVG11/... to make it clear which version of the spec the code is/was intending to implement at the time it was written?

>+NS_SVG_HaveFeature(const nsAString& aFeature)
>+{
>   if (!NS_SVGEnabled()) {
>     return PR_FALSE;
>   }
>-  nsAutoString lstr(fstr);
>-  lstr.StripWhitespace();

I don't agree with this change exactly. I think leading and trailing white space should be allowed. While that isn't exactly what the code is currently doing, I think your change to not allow leading and trailing white space restricts input formating for no good reason.

>+static PRBool
>+HaveFeatures(const nsAString& aFeatures)

The specification for requiredFeatures says that features can be separated by "white space", but it looks like we only support the space character, and furthermore, only allow a single space character. I don't see a definition of "white space" in the relevant chapter, but in other places it is http://www.w3.org/TR/SVG/animate.html#TimingAttributes . If you feel like fixing that now feel free, otherwise let me know and I'll file a follow-up bug. IMO white space should be allowed before and after the feature strings as well as between them (to allow maximum flexibility in source formatting, and because I see no good reason not to), and of course care should be taken about passing an empty string or a string of whitespace to NS_SVG_HaveFeature (IMO it should also allow white space before and after the feature string).

I guess the white space thing also applies to systemLanguage, so probably better as a follow up bug.

>+/**
>+ * Compare list to attribute value, which may be a list
>+ * According to the SVG 1.1 Spec (at least as I read it), we should take
>+ * the first attribute value and check it for any matches in the users
>+ * preferences, including any prefix matches.
>+ * This algorithm is O(M*N)

Can you change this comment to:

/**
 * Compare the language name(s) in a systemLanguage attribute to the
 * user's language preferences, as defined in
 * http://www.w3.org/TR/SVG11/struct.html#SystemLanguageAttribute
 * We have a match if a language name in the users language preferences
 * exactly equals one of the language names or exactly equals a prefix of
 * one of the language names in the systemLanguage attribute.
 * XXX This algorithm is O(M*N).
 */

>+/**
>+ * Check to see if the element satisfies the conditions required for rendering

Can you make that:

/**
 * Check whether the conditional processing attributes requiredFeatures,
 * requiredExtensions and systemLanguage all "return true" if they apply to
 * and are specified on the given element. Returns true if this element
 * should be rendered, false if it should not.
 */

>+NS_SVG_SupportsAttributes(nsIContent *aContent, const nsIAtom *aTagName)

I'm not sure why you changed your previous name of NS_SVG_SatisfiesConditions to NS_SVG_SupportsAttributes. We really want something that at the call points makes clear what the function is doing (checking that all conditional processing attributes "return true"). NS_SVG_SatisfiesConditions sort of conveyed that to my mind, but NS_SVG_SupportsAttributes doesn't at all. Perhaps a better name would be NS_SVG_PassesConditionalProcessingTests - it's rather verbose, but much more likely to save "call point" readers from having to look up and see what NS_SVG_SatisfiesConditions is actually for (they'll immediately know the "conditions" here are conditional processing conditions).

Comment 50

[Jonathan Watt [:jwatt]]

I also think NS_SVG_PassesConditionalProcessingTests should just take a single nsIContent argument and loose the nsIAtom tag argument. It's so cheap to get the tag, and it makes the code cleaner. You can then just pass the nsIContent to ElementSupportsAttributes and let it get the tag.

If you make the name change to NS_SVG_PassesConditionalProcessingTests, you can then delete this comment:

>   // See if this element supports conditionals & if it does,
>   // handle it

and replace it with something like:

  // Reduce the number of frames we create unnecessarily. Note that this is not
  // where we select which frame in a <switch> to render! That happens in
  // nsSVGSwitchFrame::PaintSVG.

Other than that, the patch looks pretty good. I would like one more pass before giving it r+ though.

Comment 51

[Jonathan Watt [:jwatt]]

Comment on attachment 328254 [details] [diff] [review]
patch

> nsSVGElement::AfterSetAttr(PRInt32 aNamespaceID, nsIAtom* aName,
>                            const nsAString* aValue, PRBool aNotify)

This will be called again and again as the document is being constructed and each attribute is set on each child of the <switch>. I wonder if there is some way to reduce the amount of work we do during document construction while still fixing the bug.


>+      nsIFrame *frame = GetPrimaryFrame();
>+      nsISVGChildFrame* svgFrame = nsnull;
>+
>+      CallQueryInterface(frame, &svgFrame);
>+      nsSVGUtils::UpdateGraphic(svgFrame);
>+      return;

The child may not have a frame due to |display:none|. CallQueryInterface, unlike do_QueryInterface, is not null safe.

Comment 52

[Jonathan Watt [:jwatt]]

Comment on attachment 328254 [details] [diff] [review]
patch

Also, why not reuse UpdateActiveChild() in MaybeInvalidate()?

nsSVGSwitchElement::MaybeInvalidate()
{
  nsIContent *oldActiveChild = mActiveChild.get();
  UpdateActiveChild()
  if (mActiveChild != oldActiveChild) {
    nsIFrame *frame = GetPrimaryFrame();
    if (frame) {
      nsISVGChildFrame* svgFrame = nsnull;
      CallQueryInterface(frame, &svgFrame);
      if (svgFrame) {
        nsSVGUtils::UpdateGraphic(svgFrame);
      }
    }
  }
}

Comment 53

[Jonathan Watt [:jwatt]]

And the |child->IsNodeOfType(nsINode::eELEMENT)| check should probably just move to be inside NS_SVG_SupportsAttributes.

Comment 54

[Boris Zbarsky [:bzbarsky]]

You could add calls to DoneCreatingElement and skip this stuff in those cases... but that might actually be slower than just doing this for these three attributes.

Comment 55

[Robert Longson [:longsonr]]

(In reply to comment #49)
> 
> Can you use parent->NodeInfo()->Equals(nsGkAtoms::svgSwitch, kNameSpaceID_SVG)
> so we also check the namespace please?

Done.

> 
> Also I think general style is not to bother with the parentheses around the
> static_cast and not have the spaces in there, but I have no strong preference.
> 

Done.

> 
> The links I gave you to add to nsSVGFeatures.cpp are to
> http://www.w3.org/TR/SVG/... - can you actually change them to
> http://www.w3.org/TR/SVG11/... to make it clear which version of the spec the
> code is/was intending to implement at the time it was written?

Done.

> The specification for requiredFeatures says that features can be separated by
> "white space", but it looks like we only support the space character, and
> furthermore, only allow a single space character. I don't see a definition of
> "white space" in the relevant chapter, but in other places it is
> http://www.w3.org/TR/SVG/animate.html#TimingAttributes . If you feel like
> fixing that now feel free, otherwise let me know and I'll file a follow-up bug.
> IMO white space should be allowed before and after the feature strings as well
> as between them (to allow maximum flexibility in source formatting, and because
> I see no good reason not to), and of course care should be taken about passing
> an empty string or a string of whitespace to NS_SVG_HaveFeature (IMO it should
> also allow white space before and after the feature string).

Leading, trailing and embedded whitespace allowed now.

> 
> I guess the white space thing also applies to systemLanguage, so probably
> better as a follow up bug.

I don't think it does as the systemLanguage processing calls StripWhitespace.

> Can you change this comment to:
> 
> /**
>  * Compare the language name(s) in a systemLanguage attribute to the
>  * user's language preferences, as defined in
>  * http://www.w3.org/TR/SVG11/struct.html#SystemLanguageAttribute
>  * We have a match if a language name in the users language preferences
>  * exactly equals one of the language names or exactly equals a prefix of
>  * one of the language names in the systemLanguage attribute.
>  * XXX This algorithm is O(M*N).
>  */

Done.

> Can you make that:
> 
> /**
>  * Check whether the conditional processing attributes requiredFeatures,
>  * requiredExtensions and systemLanguage all "return true" if they apply to
>  * and are specified on the given element. Returns true if this element
>  * should be rendered, false if it should not.
>  */

Done.

> I'm not sure why you changed your previous name of NS_SVG_SatisfiesConditions
> to NS_SVG_SupportsAttributes. We really want something that at the call points
> makes clear what the function is doing (checking that all conditional
> processing attributes "return true"). NS_SVG_SatisfiesConditions sort of
> conveyed that to my mind, but NS_SVG_SupportsAttributes doesn't at all. Perhaps
> a better name would be NS_SVG_PassesConditionalProcessingTests - it's rather
> verbose, but much more likely to save "call point" readers from having to look
> up and see what NS_SVG_SatisfiesConditions is actually for (they'll immediately
> know the "conditions" here are conditional processing conditions).
> 

Done.

(In reply to comment #52)
> (From update of attachment 328254 [details] [diff] [review])
> Also, why not reuse UpdateActiveChild() in MaybeInvalidate()?
> 
> nsSVGSwitchElement::MaybeInvalidate()
> {
>   nsIContent *oldActiveChild = mActiveChild.get();
>   UpdateActiveChild()
>   if (mActiveChild != oldActiveChild) {
>     nsIFrame *frame = GetPrimaryFrame();
>     if (frame) {
>       nsISVGChildFrame* svgFrame = nsnull;
>       CallQueryInterface(frame, &svgFrame);
>       if (svgFrame) {
>         nsSVGUtils::UpdateGraphic(svgFrame);
>       }
>     }
>   }
> }
> 

This I can't do as MaybeInvalidate must NOT update mActiveChild. nsSVGUtils::UpdateGraphic does that via a call to UpdateCoveredRegion.

Imagine a switch with 2 children in different places. I make the first child inactive by changing the systemLanguage to "foo" say. So the second child should display and the first child must disappear. We must invalidate the old covered region the first child covered and therefore we must not update the old active child till we have invalidated the old covered region which is what nsSVGUtils::UpdateGraphic does. If mActiveChild is changed anywhere else then we'll get animation artifacts.

(In reply to comment #53)
> And the |child->IsNodeOfType(nsINode::eELEMENT)| check should probably just
> move to be inside NS_SVG_SupportsAttributes.
> 

I'd rather not do that yet. I have a follow up in mind to support dynamic changes to non-switch conditional attributes which would probably mean I'd have to take it out again. All will be revealed in my next exciting installment to conditional processing. If this comment is still relevant after you review that bug I'll do the change there.

Comment 56

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 57

[Jonathan Watt [:jwatt]]

(In reply to comment #54)
> You could add calls to DoneCreatingElement and skip this stuff in those
> cases... but that might actually be slower than just doing this for these three
> attributes.

Well "doing this" means each time one of the three attributes is set, calling NS_SVG_PassesConditionalProcessingTests, which could iterates over all children of the switch examining the value of those three attributes. Anyway, it looks like using DoneCreatingElement involves touching the parser, and I've held Robert up long enough. I'll file a follow-up bug fir investigating that change.

Comment 58

[Jonathan Watt [:jwatt]]

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

r=jwatt with the following addressed. Thanks!!

>+      nsIFrame *frame = GetPrimaryFrame();
>+      if (frame) {
>+        nsISVGChildFrame* svgFrame = nsnull;
>+
>+        CallQueryInterface(frame, &svgFrame);
>+        nsSVGUtils::UpdateGraphic(svgFrame);
>+      }

If it's worth doing a QI instead of casting (it certainly is), then it's worth checking if the QI succeeded. Currently the first thing that nsSVGUtils::UpdateGraphic does is call CallQueryInterface, and that will crash if the QI failed here.

>+nsSVGSwitchFrame::GetFrameForPointSVG(float x, float y, nsIFrame** hit)
>+{
>+  nsIFrame *kid = GetActiveChildFrame();
>+  if (kid) {
>+    nsSVGUtils::HitTestChildren(kid, x, y, hit);
>+  }

nsSVGUtils::HitTestChildren literally only checks the children. We actually want to hit test "kid" too.

(In reply to comment #55)
> This I can't do as MaybeInvalidate must NOT update mActiveChild.
> nsSVGUtils::UpdateGraphic does that via a call to UpdateCoveredRegion.

Ah-hah! Good point. Can you add a comment at the beginning of the function something like this:

  // We don't reuse UpdateActiveChild() and check if mActiveChild has changed
  // to determine if we should call nsSVGUtils::UpdateGraphic. If we did that,
  // nsSVGUtils::UpdateGraphic would not invalidate the old mActiveChild area!

> I'd rather not do that yet.

Sure thing. It was more of a nit anyway. ;-)

Comment 59

[Robert Longson [:longsonr]]

Attachment: address final review comments

(In reply to comment #58)
> nsSVGUtils::HitTestChildren literally only checks the children. We actually
> want to hit test "kid" too.
> 

Good point. Changed to a direct call to GetFrameForPointSVG on the child.

Other comments addressed too.

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Seems like HaveFeatures could use nsWhitespaceTokenizer.

+HaveFeatures(const nsAString& aFeatures)
+{
+  nsAutoString features(aFeatures); // in order to use FindCharInSet()

Make aFeatures const nsSubstring& instead

+        nsISVGChildFrame* svgFrame = nsnull;
+
+        if (svgFrame) {
+          CallQueryInterface(frame, &svgFrame);
+          nsSVGUtils::UpdateGraphic(svgFrame);
+        }

Clearly UpdateGraphic is never called because svgFrame is always null when tested...

Shouldn't MaybeInvalidate actually change mActiveChild? Otherwise, how does mActiveChild get updated from nsSVGElement::AfterSetAttr?

+  // Reduce the number of frames we create unnecessarily. Note that this is not
+  // where we select which frame in a <switch> to render! That happens in
+  // nsSVGSwitchFrame::PaintSVG.
+  if (!NS_SVG_PassesConditionalProcessingTests(aContent)) {

Actually I don't think we can do this. The spec says that
"conditional processing attributes only affect the direct rendering of elements and do not prevent elements from being successfully referenced by other elements (such as via a 'use')."
And I assume this means that patterns, gradients, markers, clip-paths, filters and masks in an inactive switch branch should be referenceable, and therefore we need frames for them.

+        *aRect = box;
+        NS_ADDREF(*aRect);
+        return NS_OK;

You can just use box.swap(*aRect) or *aRect = box.forget();

Comment 61

[Robert Longson [:longsonr]]

Attachment: address sr comments

(In reply to comment #60)
> Seems like HaveFeatures could use nsWhitespaceTokenizer.

Easy when you know how :-)

> Clearly UpdateGraphic is never called because svgFrame is always null when
> tested...

My fault for not retesting the final review changes sufficiently.

> 
> Shouldn't MaybeInvalidate actually change mActiveChild? Otherwise, how does
> mActiveChild get updated from nsSVGElement::AfterSetAttr?

There is a comment in MaybeInvalidate explaining why not.

To reiterate: nsSVGUtils::UpdateGraphic invalidates the old child area, updates the active child via a call to UpdateCoveredRegion then invalidates the new child area. If we update the active child directly in MaybeInvalidate then UpdateGraphic won't be able to invalidate the old active child area and you'll get DOM animation artifacts.

> Actually I don't think we can do this. The spec says that
> "conditional processing attributes only affect the direct rendering of elements
> and do not prevent elements from being successfully referenced by other
> elements (such as via a 'use')."
> And I assume this means that patterns, gradients, markers, clip-paths, filters
> and masks in an inactive switch branch should be referenceable, and therefore
> we need frames for them.
> 

You are absolutely right, however this is the case with the current code base so I'm not making things any worse. Earlier versions of this patch attempted to fix that too but it became complicated and I thought it best to proceed one step at a time. I will be creating a follow up bug to address this issue.

> You can just use box.swap(*aRect) or *aRect = box.forget();
> 

Done.

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #61)
> > Shouldn't MaybeInvalidate actually change mActiveChild? Otherwise, how does
> > mActiveChild get updated from nsSVGElement::AfterSetAttr?
> 
> There is a comment in MaybeInvalidate explaining why not.
> 
> To reiterate: nsSVGUtils::UpdateGraphic invalidates the old child area, updates
> the active child via a call to UpdateCoveredRegion then invalidates the new
> child area. If we update the active child directly in MaybeInvalidate then
> UpdateGraphic won't be able to invalidate the old active child area and you'll
> get DOM animation artifacts.

OK the key thing I missed is that nsSVGSwitchFrame::UpdateCoveredRegion updates the active child.

> You are absolutely right, however this is the case with the current code base
> so I'm not making things any worse. Earlier versions of this patch attempted to
> fix that too but it became complicated and I thought it best to proceed one
> step at a time. I will be creating a follow up bug to address this issue.

I don't understand. If you just take out that "don't create frames" "optimization", doesn't that just fix the problem?

Comment 63

[Robert Longson [:longsonr]]

(In reply to comment #62)
> > You are absolutely right, however this is the case with the current code base
> > so I'm not making things any worse. Earlier versions of this patch attempted to
> > fix that too but it became complicated and I thought it best to proceed one
> > step at a time. I will be creating a follow up bug to address this issue.
> 
> I don't understand. If you just take out that "don't create frames"
> "optimization", doesn't that just fix the problem?
> 

If you had elements which don't meet processing conditions but are not children of switch elements then they must not be displayed. If you take this line out and don't fix things elsewhere to deal with that then they will be displayed.

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I see.

Comment 65

[Robert Longson [:longsonr]]

checked in.

Comment 66

[Jonathan Watt [:jwatt]]

(In reply to comment #57)
> I'll file a follow-up bug fir investigating that change.

I filed bug 445018.

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 329038 [details] [diff] [review]
address sr comments

> nsresult
> nsSVGElement::AfterSetAttr(PRInt32 aNamespaceID, nsIAtom* aName,
>                            const nsAString* aValue, PRBool aNotify)
> {  
>   if (IsEventName(aName) && aValue) {
>     nsresult rv = AddScriptEventListener(GetEventNameForAttr(aName), *aValue);
>     NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+
>+  if (aNamespaceID == kNameSpaceID_None &&
>+      (aName == nsGkAtoms::requiredFeatures ||
>+       aName == nsGkAtoms::requiredExtensions ||
>+       aName == nsGkAtoms::systemLanguage)) {
>+
>+    nsIContent* parent = nsnull;
>+  
>+    nsIContent* bindingParent = GetBindingParent();
>+    if (bindingParent) {
>+      nsIDocument* doc = bindingParent->GetOwnerDoc();
>+      if (doc) {
>+        parent = doc->BindingManager()->GetInsertionParent(bindingParent);
>+      }
>+    }
What is this code trying to do, or why? Would be great to add some comment.

Comment 68

[Robert Longson [:longsonr]]

If the element's parent is a switch and the attributes of the element that might cause a change in the switch's active child change then reevaluate any maybe redraw the switch.

e.g. rect4a in layout/reftests/svg/dynamic-switch-01.svg

Comment 69

[Olli Pettay [:smaug][[e-mail]]]

But why the bindingParent/insertionParent thing?  Why we need to update the insertionParent of the bindingParent?

Comment 70

[Robert Longson [:longsonr]]

I just blindly copied that. That kind of thing is everywhere in SVG content when the code tries to get a parent.

Comment 71

[Olli Pettay [:smaug][[e-mail]]]

I couldn't find any other place where insertionParent of bindingParent is used.
insertionParent (of some element) is used in many places, and that is ok.